Re: [PATCH -fixes v4 2/3] riscv: Add a custom ISA extension for the [ms]envcfg CSR

[Palmer Dabbelt <palmer@xxxxxxxxxxx>]

On Wed, 28 Feb 2024 02:12:14 PST (-0800), Conor Dooley wrote:
> On Tue, Feb 27, 2024 at 10:55:34PM -0800, Samuel Holland wrote:
> > The [ms]envcfg CSR was added in version 1.12 of the RISC-V privileged
> > ISA (aka S[ms]1p12). However, bits in this CSR are defined by several
> > other extensions which may be implemented separately from any particular
> > version of the privileged ISA (for example, some unrelated errata may
> > prevent an implementation from claiming conformance with Ss1p12). As a
> > result, Linux cannot simply use the privileged ISA version to determine
> > if the CSR is present. It must also check if any of these other
> > extensions are implemented. It also cannot probe the existence of the
> > CSR at runtime, because Linux does not require Sstrict, so (in the
> > absence of additional information) it cannot know if a CSR at that
> > address is [ms]envcfg or part of some non-conforming vendor extension.
> >
> > Since there are several standard extensions that imply the existence of
> > the [ms]envcfg CSR, it becomes unwieldy to check for all of them
> > wherever the CSR is accessed. Instead, define a custom Xlinuxenvcfg ISA
> > extension bit that is implied by the other extensions and denotes that
> > the CSR exists as defined in the privileged ISA, containing at least one
> > of the fields common between menvcfg and senvcfg.
>
> > This extension does not need to be parsed from the devicetree or ISA
> > string because it can only be implemented as a subset of some other
> > standard extension.
>
> NGL, every time I look at the superset stuff I question whether or not
> it is a good implementation, but it is nice to see that it at least
> makes the creation of quasi-extension flags like this straightforward.

We can always add it to the DT list as a proper extension, but I think 
for this sort of stuff it's good enough for now -- we've already got a 
bunch of complexity for the proper ISA-defined extension dependencies, 
so it's not like we could really get away from it entirely.

> Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>
> Cheers,
> Conor.
>
>
> > Cc: <stable@xxxxxxxxxxxxxxx> # v6.7+
> > Signed-off-by: Samuel Holland <samuel.holland@xxxxxxxxxx>
> > ---
> >
> > Changes in v4:
> >  - New patch for v4
> >
> >  arch/riscv/include/asm/hwcap.h |  2 ++
> >  arch/riscv/kernel/cpufeature.c | 14 ++++++++++++--
> >  2 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index 5340f818746b..1f2d2599c655 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -81,6 +81,8 @@
> >  #define RISCV_ISA_EXT_ZTSO		72
> >  #define RISCV_ISA_EXT_ZACAS		73
> >  
> > +#define RISCV_ISA_EXT_XLINUXENVCFG	127
> > +
> >  #define RISCV_ISA_EXT_MAX		128
> >  #define RISCV_ISA_EXT_INVALID		U32_MAX
> >  
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index c5b13f7dd482..dacffef68ce2 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -201,6 +201,16 @@ static const unsigned int riscv_zvbb_exts[] = {
> >  	RISCV_ISA_EXT_ZVKB
> >  };
> >  
> > +/*
> > + * While the [ms]envcfg CSRs were not defined until version 1.12 of the RISC-V
> > + * privileged ISA, the existence of the CSRs is implied by any extension which
> > + * specifies [ms]envcfg bit(s). Hence, we define a custom ISA extension for the
> > + * existence of the CSR, and treat it as a subset of those other extensions.
> > + */
> > +static const unsigned int riscv_xlinuxenvcfg_exts[] = {
> > +	RISCV_ISA_EXT_XLINUXENVCFG
> > +};
> > +
> >  /*
> >   * The canonical order of ISA extension names in the ISA string is defined in
> >   * chapter 27 of the unprivileged specification.
> > @@ -250,8 +260,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
> >  	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
> >  	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
> >  	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
> > -	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
> > -	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
> > +	__RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts),
> > +	__RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts),
> >  	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
> >  	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
> >  	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> > --
> > 2.43.1