On Wed, Mar 27, 2024 at 09:06:39PM -0700, Ron Economos wrote: > On 3/27/24 4:48 PM, Conor Dooley wrote: > > On Wed, Mar 27, 2024 at 12:00:27PM -0700, Ron Economos wrote: > > > On 3/27/24 7:40 AM, Greg KH wrote: > > > > On Tue, Mar 05, 2024 at 08:05:30AM +0000, Conor.Dooley@xxxxxxxxxxxxx wrote: > > > > > On 05/03/2024 07:44, gregkh@xxxxxxxxxxxxxxxxxxx wrote: > > > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > > > > > > > > > This is a note to let you know that I've just added the patch titled > > > > > > > > > > > > riscv: Add a custom ISA extension for the [ms]envcfg CSR > > > > > > > > > > > > to the 6.7-stable tree which can be found at: > > > > > > http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary > > > > > > > > > > > > The filename of the patch is: > > > > > > riscv-add-a-custom-isa-extension-for-the-envcfg-csr.patch > > > > > > and it can be found in the queue-6.7 subdirectory. > > > > > > > > > > > > If you, or anyone else, feels it should not be added to the stable tree, > > > > > > please let <stable@xxxxxxxxxxxxxxx> know about it. > > > > > > > > > > > > > > > > > > From 4774848fef6041716a4883217eb75f6b10eb183b Mon Sep 17 00:00:00 2001 > > > > > > From: Samuel Holland <samuel.holland@xxxxxxxxxx> > > > > > > Date: Tue, 27 Feb 2024 22:55:34 -0800 > > > > > > Subject: riscv: Add a custom ISA extension for the [ms]envcfg CSR > > > > > > > > > > > > From: Samuel Holland <samuel.holland@xxxxxxxxxx> > > > > > > > > > > > > commit 4774848fef6041716a4883217eb75f6b10eb183b upstream. > > > > > > > > > > > > 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. > > > > > > > > > > > > Cc: <stable@xxxxxxxxxxxxxxx> # v6.7+ > > > > > > Signed-off-by: Samuel Holland <samuel.holland@xxxxxxxxxx> > > > > > > Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx> > > > > > > Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx> > > > > > > Link: https://lore.kernel.org/r/20240228065559.3434837-3-samuel.holland@xxxxxxxxxx > > > > > > Signed-off-by: Palmer Dabbelt <palmer@xxxxxxxxxxxx> > > > > > > Cc: Ron Economos <re@xxxxxxxx> > > > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > > > > > > --- > > > > > > arch/riscv/include/asm/hwcap.h | 2 ++ > > > > > > arch/riscv/kernel/cpufeature.c | 14 ++++++++++++-- > > > > > > 2 files changed, 14 insertions(+), 2 deletions(-) > > > > > > > > > > > > --- a/arch/riscv/include/asm/hwcap.h > > > > > > +++ b/arch/riscv/include/asm/hwcap.h > > > > > > @@ -58,6 +58,8 @@ > > > > > > #define RISCV_ISA_EXT_SMSTATEEN 43 > > > > > > #define RISCV_ISA_EXT_ZICOND 44 > > > > > > > > > > > > +#define RISCV_ISA_EXT_XLINUXENVCFG 127 > > > > > > + > > > > > > #define RISCV_ISA_EXT_MAX 64 > > > > > These defines here need to be lower than RISCV_ISA_EXT_MAX. > > > > > I think adjusting the value of XLINUXENVCFG to 63 will > > > > > suffice here, the max got bumped to 128 in 6.8. > > > > Can you send a patch for this? > > > > > > > > thanks, > > > > > > > > greg k-h > > > This patch was subsequently reverted, so there's nothing to do here. > > The former might be true, but I don't think the latter is, the patch > > was CCed to stable for a reason. I'll add it to my todo list Greg, but > > no promises for when I will actually get around to it. It's not my patch > > so I don't have a usecase that needs this or w/e. > > > > Cheers, > > Conor. > > I was the one that requested this patch. Here's the history. > > The upstream commit 05ab803d1ad8ac505ade77c6bd3f86b1b4ea0dc4 "riscv: > Save/restore envcfg CSR during CPU suspend" was added to 6.7.9-rc1 > because it has a Fixes tag. This caused a build failure because > RISCV_ISA_EXT_XLINUXENVCFG wasn't defined. > > I asked Greg to apply this upstream commit > 4774848fef6041716a4883217eb75f6b10eb183b "riscv: Add a custom ISA > extension for the [ms]envcfg CSR" to resolve the issue (since it defines > RISCV_ISA_EXT_XLINUXENVCFG). However, I neglected to test the patch and > it failed with zicbom and zicboz being undefined. Something that seems missing from your history is that the author of the patch put "Cc: <stable@xxxxxxxxxxxxxxx> # v6.7+" into the commit message, so this should have gone to stable regardless. However, it failed to get applied: https://lore.kernel.org/all/2024030445-rants-grading-f698@gregkh/ but Greg picked it up when you asked for it, which I think is where this ties into your recollection: https://lore.kernel.org/all/2024030538-affair-bristle-25d8@gregkh/ > At this point I realized that there was a whole series of patches for > this code going pretty far back in the commit history. Instead of trying > to sort things out with possibly many patches, I requested that Greg > drop both "riscv: Save/restore envcfg CSR during CPU suspend" and this > patch "riscv: Add a custom ISA extension for the [ms]envcfg CSR". This > solved the build issues and 6.7.9 was shipped. > > So the real culprit was "riscv: Save/restore envcfg CSR during CPU > suspend". This seems like a pretty complex backport for 6.7.x which is > going away soon, so that's why I said there's nothing to be done. If 6.7 is going away soon, then sure, probably a waste of our time doing anything here, but I think all that's required would be to do something like: commit 4be775992c7aa9d5633a31184f352c0837d4feb7 Author: Conor Dooley <conor.dooley@xxxxxxxxxxxxx> Date: Thu Mar 28 08:15:55 2024 +0000 riscv: add support for "superset" extensions Based on upstream commits 0d8295ed975b ("riscv: add ISA extension parsing for scalar crypto") and aec3353963b8 ("riscv: add ISA extension parsing for vector crypto") riscv_isa_ext_data grows a pair of new members, to permit setting the relevant bits for "superset" extensions, both while parsing the ISA string and the new dedicated extension properties. Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h index aa6548b46a8a..cb7e793de5b9 100644 --- a/arch/riscv/include/asm/cpufeature.h +++ b/arch/riscv/include/asm/cpufeature.h @@ -59,6 +59,8 @@ struct riscv_isa_ext_data { const unsigned int id; const char *name; const char *property; + const unsigned int *subset_ext_ids; + const unsigned int subset_ext_size; }; extern const struct riscv_isa_ext_data riscv_isa_ext[]; diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 867778813353..f33f5b11e468 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -108,12 +108,20 @@ static bool riscv_isa_extension_check(int id) return true; } -#define __RISCV_ISA_EXT_DATA(_name, _id) { \ - .name = #_name, \ - .property = #_name, \ - .id = _id, \ +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) { \ + .name = #_name, \ + .property = #_name, \ + .id = _id, \ + .subset_ext_ids = _subset_exts, \ + .subset_ext_size = _subset_exts_size \ } +#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0) + +/* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */ +#define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \ + _RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts)) + /* * The canonical order of ISA extension names in the ISA string is defined in * chapter 27 of the unprivileged specification. and follow that up with: commit 5e60461bceb153bcabb4e1237ea92513835a6c41 Author: Samuel Holland <samuel.holland@xxxxxxxxxx> Date: Tue Feb 27 22:55:34 2024 -0800 riscv: Add a custom ISA extension for the [ms]envcfg CSR 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. Cc: <stable@xxxxxxxxxxxxxxx> # v6.7+ Signed-off-by: Samuel Holland <samuel.holland@xxxxxxxxxx> Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx> Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx> Link: https://lore.kernel.org/r/20240228065559.3434837-3-samuel.holland@xxxxxxxxxx Signed-off-by: Palmer Dabbelt <palmer@xxxxxxxxxxxx> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h index 06d30526ef3b..f1a175fb08aa 100644 --- a/arch/riscv/include/asm/hwcap.h +++ b/arch/riscv/include/asm/hwcap.h @@ -58,6 +58,7 @@ #define RISCV_ISA_EXT_SMSTATEEN 43 #define RISCV_ISA_EXT_ZICOND 44 +#define RISCV_ISA_EXT_XLINUXENVCFG 63 #define RISCV_ISA_EXT_MAX 64 #ifdef CONFIG_RISCV_M_MODE diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index f33f5b11e468..b5b47d5ccaf1 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -122,6 +122,16 @@ static bool riscv_isa_extension_check(int id) #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \ _RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts)) +/* + * 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. @@ -175,8 +185,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { __RISCV_ISA_EXT_DATA(p, RISCV_ISA_EXT_p), __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), And with that, you can cherry-pick "riscv: Save/restore envcfg CSR during CPU suspend" and have it build correctly. That builds for my basic config and boots. Nothing there should be conditional on any config options, so I think the build bots won't mind. Cheers, Conor.
Attachment:
signature.asc
Description: PGP signature
