Message-ID: <CAK9=C2VJ-+bu20+QOfKrq6cEBE93Yi21U=zU9AKOSQi1GGHWiA@mail.gmail.com>
Date:   Tue, 17 May 2022 09:31:50 +0530
From:   Anup Patel <apatel@...tanamicro.com>
To:     Jisheng Zhang <jszhang@...nel.org>
Cc:     Anup Patel <anup@...infault.org>,
        Atish Patra <atishp@...shpatra.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Andrey Ryabinin <ryabinin.a.a@...il.com>,
        Alexander Potapenko <glider@...gle.com>,
        Andrey Konovalov <andreyknvl@...il.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        Alexandre Ghiti <alexandre.ghiti@...onical.com>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        kasan-dev@...glegroups.com
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@...nel.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@...nel.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@...nel.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@...nel.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

Powered by blists - more mailing lists