Messages in this thread | | | Date | Thu, 29 Feb 2024 10:23:39 -0800 (PST) | Subject | Re: [PATCH -fixes v4 2/3] riscv: Add a custom ISA extension for the [ms]envcfg CSR | From | Palmer Dabbelt <> |
| |
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@microchip.com> > > Cheers, > Conor. > > >> >> Cc: <stable@vger.kernel.org> # v6.7+ >> Signed-off-by: Samuel Holland <samuel.holland@sifive.com> >> --- >> >> 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 >>
| |