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

From: Jisheng Zhang <jszhang@kernel.org>
To: Atish Patra <atishp@atishpatra.org>
Cc: Anup Patel <apatel@ventanamicro.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Andrey Ryabinin <ryabinin.a.a@gmail.com>,
	Alexander Potapenko <glider@google.com>,
	Andrey Konovalov <andreyknvl@gmail.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Alexandre Ghiti <alexandre.ghiti@canonical.com>,
	linux-riscv <linux-riscv@lists.infradead.org>,
	"linux-kernel@vger.kernel.org List"
	<linux-kernel@vger.kernel.org>,
	kasan-dev@googlegroups.com
Subject: Re: [PATCH v2 2/4] riscv: introduce unified static key mechanism for CPU features
Date: Sun, 15 May 2022 15:15:34 +0800	[thread overview]
Message-ID: <YoCollqhS93NJZjL@xhacker> (raw)
In-Reply-To: <CAOnJCU+XR5mtqKBQLMj3JgsTPgvAQdO_jj2FWqcu7f9MezNCKA@mail.gmail.com>

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.

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.

In summary, I can see riscv_isa doesn't cover features which need static
keys, and vice vesa.

Could you please comment?

Thanks in advance,
Jisheng