Messages in this thread | | | From | Anup Patel <> | Date | Tue, 17 May 2022 09:31:50 +0530 | Subject | Re: [PATCH v2 2/4] riscv: introduce unified static key mechanism for CPU features |
| |
On Mon, May 16, 2022 at 11:02 PM Jisheng Zhang <jszhang@kernel.org> wrote: > > On Sun, May 15, 2022 at 08:19:37PM +0530, Anup Patel wrote: > > On Sun, May 15, 2022 at 12:54 PM Jisheng Zhang <jszhang@kernel.org> wrote: > > > > > > On Wed, May 11, 2022 at 11:29:32PM -0700, Atish Patra wrote: > > > > On Mon, May 9, 2022 at 7:50 AM Jisheng Zhang <jszhang@kernel.org> wrote: > > > > > > > > > > On Mon, May 09, 2022 at 09:17:10AM +0530, Anup Patel wrote: > > > > > > On Sun, May 8, 2022 at 9:47 PM Jisheng Zhang <jszhang@kernel.org> wrote: > > > > > > > > > > > > > > Currently, riscv has several features why may not be supported on all > > > > > > > riscv platforms, for example, FPU, SV48 and so on. To support unified > > > > > > > kernel Image style, we need to check whether the feature is suportted > > > > > > > or not. If the check sits at hot code path, then performance will be > > > > > > > impacted a lot. static key can be used to solve the issue. In the past > > > > > > > FPU support has been converted to use static key mechanism. I believe > > > > > > > we will have similar cases in the future. > > > > > > > > > > > > It's not just FPU and Sv48. There are several others such as Svinval, > > > > > > Vector, Svnapot, Svpbmt, and many many others. > > > > > > > > > > > > Overall, I agree with the approach of using static key array but I > > > > > > disagree with the semantics and the duplicate stuff being added. > > > > > > > > > > > > Please see more comments below .. > > > > > > > > > > > > > > > > > > > > Similar as arm64 does(in fact, some code is borrowed from arm64), this > > > > > > > patch tries to add an unified mechanism to use static keys for all > > > > > > > the cpu features by implementing an array of default-false static keys > > > > > > > and enabling them when detected. The cpus_have_*_cap() check uses the > > > > > > > static keys if riscv_const_caps_ready is finalized, otherwise the > > > > > > > compiler generates the bitmap test. > > > > > > > > > > > > First of all, we should stop calling this a feature (like ARM does). Rather, > > > > > > we should call these as isa extensions ("isaext") to align with the RISC-V > > > > > > priv spec and RISC-V profiles spec. For all the ISA optionalities which do > > > > > > not have distinct extension name, the RISC-V profiles spec is assigning > > > > > > names to all such optionalities. > > > > > > > > > > Same as the reply a few minutes ago, the key problem here is do all > > > > > CPU features belong to *ISA* extensions? For example, SV48, SV57 etc. > > > > > I agree with Atish's comments here: > > > > > > > > > > "I think the cpu feature is a superset of the ISA extension. > > > > > cpu feature != ISA extension" > > > > > > > > > > > > > It seems to be accurate at that point in time. However, the latest > > > > profile spec seems to > > > > define everything as an extension including sv48. > > > > > > > > https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc#623-rva22s64-supported-optional-extensions > > > > > > > > It may be a redundant effort and confusing to create two sets i.e. > > > > feature and extension in this case. > > > > But this specification is not frozen yet and may change in the future. > > > > We at least know that that is the current intention. > > > > > > > > Array of static keys is definitely useful and should be used for all > > > > well defined ISA extensions by the ratified priv spec. > > > > This will simplify this patch as well. For any feature/extensions > > > > (i.e. sv48/sv57) which was never defined as an extension > > > > in the priv spec but profile seems to define it now, I would leave it > > > > alone for the time being. Converting the existing code > > > > to static key probably has value but please do not include it in the > > > > static key array setup. > > > > > > > > Once the profile spec is frozen, we can decide which direction the > > > > Linux kernel should go. > > > > > > > > > > Hi Atish, Anup, > > > > > > I see your points and thanks for the information of the profile > > > spec. Now, I have other two points about isa VS features: > > > > > > 1. Not all isa extenstions need static key mechanism, so if we > > > make a static key array with 1:1 riscv_isa <-> static key relationship > > > there may be waste. > > > > > > For example, the 'a', 'c', 'i', 'm' and so on don't have static > > > key usage. > > > > Not all isa extensions but a large number of them will need a static > > key. It's better to always have one static key per ISA extension > > defined in cpufeatures.c > > Currently, RISCV_ISA_EXT_MAX equals to 64 while the base ID is 26. > In those 26 base IDs, only F/D and V need static key, it means > we waste at least 24 static keys.
If you want to save space of unused static keys then there are other ways.
For example, you can create a small static key array which has many-to-one relation with the ISA extension numbers. For ISA extension which are always ON or always OFF, we can use fixed FALSE and TRUE keys. Something like below.
enum riscv_isa_ext_key { RISCV_ISA_EXT_KEY_FALSE = 0, RISCV_ISA_EXT_KEY_TRUE, RISCV_ISA_EXT_KEY_FLOAD, /* For 'F' and 'D' */ RISCV_ISA_EXT_KEY_VECTOR, /* For all vector extensions */ RISCV_ISA_EXT_KEY_SVINVAL, RISCV_ISA_EXT_KEY_SSCOFPMT, RISCV_ISA_EXT_KEY_MAX, };
extern unsigned char __riscv_isa_ext_id2key[RISCV_ISA_EXT_ID_MAX]; extern struct static_key_false __riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_MAX];
static __always_inline bool __riscv_isa_extension_keycheck(unsigned int ext) { if (RISCV_ISA_EXT_ID_MAX <= ext) return false; return static_branch_unlikely(&__riscv_isa_ext_keys[__riscv_isa_ext_id2key[ext]]); } #define riscv_isa_extension_keycheck(ext) \ __riscv_isa_extension_keycheck(RISCV_ISA_EXT_##ext)
> > > > > For example, F, D, V, Sstc, Svinval, Ssofpmt, Zb*, AIA, etc. > > > > > > > > 2.We may need riscv architecture static keys for non-isa, this is > > > usually related with the linux os itself, for example > > > a static key for "unmap kernelspace at userspace". > > > static keys for "spectre CVE mitigations" > > > etc. > > > > These things look more like errata or workarounds so better > > to use that framework instead of ISA extensions (or features). > > Currently, the errata workarounds are implemented with ALTERNATIVEs > but I believe sometime we may need static key to implement the > workarounds. However this can be checked later. Now I worried about > the static key waste above.
That's a separate topic and for now what we need is a simple and extensible approach to have static keys for ISA extensions.
Regards, Anup
> > Thanks
| |