Re: [PATCH v2 2/4] riscv: introduce unified static key mechanism for CPU features

[Jisheng Zhang <jszhang@kernel.org>]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv