Messages in this thread | | | Date | Tue, 17 May 2022 01:24:15 +0800 | From | Jisheng Zhang <> | Subject | Re: [PATCH v2 2/4] riscv: introduce unified static key mechanism for CPU features |
| |
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.
> > 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.
Thanks
| |