| Message ID | 20220210214018.55739-5-atishp@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Provide a fraemework for RISC-V ISA extensions | expand |
Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb Atish Patra: > Multi-letter extensions can be probed using exising > riscv_isa_extension_available API now. It doesn't support versioning > right now as there is no use case for it. > Individual extension specific implementation will be added during > each extension support. > > Signed-off-by: Atish Patra <atishp@rivosinc.com> Tested-by: Heiko Stuebner <heiko@sntech.de> By the way, does a similar parsing exist for opensbi as well? Things like svpbmt as well as zicbom have CSR bits controlling how these functions should behave (enabling them, etc), so I guess opensbi also needs to parse the extensions from the ISA string? Heiko > --- > arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++ > arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++--- > 2 files changed, 42 insertions(+), 3 deletions(-) > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > index 5ce50468aff1..170bd80da520 100644 > --- a/arch/riscv/include/asm/hwcap.h > +++ b/arch/riscv/include/asm/hwcap.h > @@ -34,7 +34,25 @@ extern unsigned long elf_hwcap; > #define RISCV_ISA_EXT_s ('s' - 'a') > #define RISCV_ISA_EXT_u ('u' - 'a') > > +/* > + * Increse this to higher value as kernel support more ISA extensions. > + */ > #define RISCV_ISA_EXT_MAX 64 > +#define RISCV_ISA_EXT_NAME_LEN_MAX 32 > + > +/* The base ID for multi-letter ISA extensions */ > +#define RISCV_ISA_EXT_BASE 26 > + > +/* > + * This enum represent the logical ID for each multi-letter RISC-V ISA extension. > + * The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed > + * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter > + * extensions while all the multi-letter extensions should define the next > + * available logical extension id. > + */ > +enum riscv_isa_ext_id { > + RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX, > +}; > > unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index e9e3b0693d16..469b9739faf7 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -83,7 +83,7 @@ void __init riscv_fill_hwcap(void) > > for_each_of_cpu_node(node) { > unsigned long this_hwcap = 0; > - unsigned long this_isa = 0; > + uint64_t this_isa = 0; > > if (riscv_of_processor_hartid(node) < 0) > continue; > @@ -169,12 +169,22 @@ void __init riscv_fill_hwcap(void) > if (*isa != '_') > --isa; > > +#define SET_ISA_EXT_MAP(name, bit) \ > + do { \ > + if ((ext_end - ext == sizeof(name) - 1) && \ > + !memcmp(ext, name, sizeof(name) - 1)) { \ > + this_isa |= (1UL << bit); \ > + pr_info("Found ISA extension %s", name);\ > + } \ > + } while (false) \ > + > if (unlikely(ext_err)) > continue; > if (!ext_long) { > this_hwcap |= isa2hwcap[(unsigned char)(*ext)]; > this_isa |= (1UL << (*ext - 'a')); > } > +#undef SET_ISA_EXT_MAP > } > > /* > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void) > else > elf_hwcap = this_hwcap; > > - if (riscv_isa[0]) > + if (riscv_isa[0]) { > +#if IS_ENABLED(CONFIG_32BIT) > + riscv_isa[0] &= this_isa & 0xFFFFFFFF; > + riscv_isa[1] &= this_isa >> 32; > +#else > riscv_isa[0] &= this_isa; > - else > +#endif > + } else { > +#if IS_ENABLED(CONFIG_32BIT) > + riscv_isa[0] = this_isa & 0xFFFFFFFF; > + riscv_isa[1] = this_isa >> 32; > +#else > riscv_isa[0] = this_isa; > +#endif > + } > } > > /* We don't support systems with F but without D, so mask those out >
On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote: > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb Atish Patra: > > Multi-letter extensions can be probed using exising > > riscv_isa_extension_available API now. It doesn't support versioning > > right now as there is no use case for it. > > Individual extension specific implementation will be added during > > each extension support. > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > > By the way, does a similar parsing exist for opensbi as well? > Things like svpbmt as well as zicbom have CSR bits controlling how > these functions should behave (enabling them, etc), so I guess > opensbi also needs to parse the extensions from the ISA string? > > No. Currently, OpenSBI relies on the CSR read/write & trap method to identify the extensions [1]. https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404 In the future, zicbom can be detected in the same manner. However, svpbmt is a bit tricky as it doesn't define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ? > Heiko > > > --- > > arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++ > > arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++--- > > 2 files changed, 42 insertions(+), 3 deletions(-) > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > index 5ce50468aff1..170bd80da520 100644 > > --- a/arch/riscv/include/asm/hwcap.h > > +++ b/arch/riscv/include/asm/hwcap.h > > @@ -34,7 +34,25 @@ extern unsigned long elf_hwcap; > > #define RISCV_ISA_EXT_s ('s' - 'a') > > #define RISCV_ISA_EXT_u ('u' - 'a') > > > > +/* > > + * Increse this to higher value as kernel support more ISA extensions. > > + */ > > #define RISCV_ISA_EXT_MAX 64 > > +#define RISCV_ISA_EXT_NAME_LEN_MAX 32 > > + > > +/* The base ID for multi-letter ISA extensions */ > > +#define RISCV_ISA_EXT_BASE 26 > > + > > +/* > > + * This enum represent the logical ID for each multi-letter RISC-V ISA extension. > > + * The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed > > + * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter > > + * extensions while all the multi-letter extensions should define the next > > + * available logical extension id. > > + */ > > +enum riscv_isa_ext_id { > > + RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX, > > +}; > > > > unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index e9e3b0693d16..469b9739faf7 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -83,7 +83,7 @@ void __init riscv_fill_hwcap(void) > > > > for_each_of_cpu_node(node) { > > unsigned long this_hwcap = 0; > > - unsigned long this_isa = 0; > > + uint64_t this_isa = 0; > > > > if (riscv_of_processor_hartid(node) < 0) > > continue; > > @@ -169,12 +169,22 @@ void __init riscv_fill_hwcap(void) > > if (*isa != '_') > > --isa; > > > > +#define SET_ISA_EXT_MAP(name, bit) \ > > + do { \ > > + if ((ext_end - ext == sizeof(name) - 1) && \ > > + !memcmp(ext, name, sizeof(name) - 1)) { \ > > + this_isa |= (1UL << bit); \ > > + pr_info("Found ISA extension %s", name);\ > > + } \ > > + } while (false) \ > > + > > if (unlikely(ext_err)) > > continue; > > if (!ext_long) { > > this_hwcap |= isa2hwcap[(unsigned char)(*ext)]; > > this_isa |= (1UL << (*ext - 'a')); > > } > > +#undef SET_ISA_EXT_MAP > > } > > > > /* > > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void) > > else > > elf_hwcap = this_hwcap; > > > > - if (riscv_isa[0]) > > + if (riscv_isa[0]) { > > +#if IS_ENABLED(CONFIG_32BIT) > > + riscv_isa[0] &= this_isa & 0xFFFFFFFF; > > + riscv_isa[1] &= this_isa >> 32; > > +#else > > riscv_isa[0] &= this_isa; > > - else > > +#endif > > + } else { > > +#if IS_ENABLED(CONFIG_32BIT) > > + riscv_isa[0] = this_isa & 0xFFFFFFFF; > > + riscv_isa[1] = this_isa >> 32; > > +#else > > riscv_isa[0] = this_isa; > > +#endif > > + } > > } > > > > /* We don't support systems with F but without D, so mask those out > > > > > >
Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra: > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb Atish Patra: > > > Multi-letter extensions can be probed using exising > > > riscv_isa_extension_available API now. It doesn't support versioning > > > right now as there is no use case for it. > > > Individual extension specific implementation will be added during > > > each extension support. > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > > > > > By the way, does a similar parsing exist for opensbi as well? > > Things like svpbmt as well as zicbom have CSR bits controlling how > > these functions should behave (enabling them, etc), so I guess > > opensbi also needs to parse the extensions from the ISA string? > > > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to > identify the extensions [1]. > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404 I guess my question is more, who is supposed to set CBIE, CBCFE bits in the ENVCFG CSR. I.e. at it's default settings CMO instructions will cause illegal instructions until the level above does allow them. When the kernel wants to call a cache-invalidate, from my reading menvcfg needs to be modified accordingly - which would fall in SBI's court? > In the future, zicbom can be detected in the same manner. However, > svpbmt is a bit tricky as it doesn't > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ? There is the PBMTE bit in MENVCFG, which I found while looking through the zicbom-parts, which is supposed to "control wheter svpbmt is available for use". So I guess the question is the same as above :-) Heiko > > Heiko > > > > > --- > > > arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++ > > > arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++--- > > > 2 files changed, 42 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > index 5ce50468aff1..170bd80da520 100644 > > > --- a/arch/riscv/include/asm/hwcap.h > > > +++ b/arch/riscv/include/asm/hwcap.h > > > @@ -34,7 +34,25 @@ extern unsigned long elf_hwcap; > > > #define RISCV_ISA_EXT_s ('s' - 'a') > > > #define RISCV_ISA_EXT_u ('u' - 'a') > > > > > > +/* > > > + * Increse this to higher value as kernel support more ISA extensions. > > > + */ > > > #define RISCV_ISA_EXT_MAX 64 > > > +#define RISCV_ISA_EXT_NAME_LEN_MAX 32 > > > + > > > +/* The base ID for multi-letter ISA extensions */ > > > +#define RISCV_ISA_EXT_BASE 26 > > > + > > > +/* > > > + * This enum represent the logical ID for each multi-letter RISC-V ISA extension. > > > + * The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed > > > + * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter > > > + * extensions while all the multi-letter extensions should define the next > > > + * available logical extension id. > > > + */ > > > +enum riscv_isa_ext_id { > > > + RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX, > > > +}; > > > > > > unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); > > > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > index e9e3b0693d16..469b9739faf7 100644 > > > --- a/arch/riscv/kernel/cpufeature.c > > > +++ b/arch/riscv/kernel/cpufeature.c > > > @@ -83,7 +83,7 @@ void __init riscv_fill_hwcap(void) > > > > > > for_each_of_cpu_node(node) { > > > unsigned long this_hwcap = 0; > > > - unsigned long this_isa = 0; > > > + uint64_t this_isa = 0; > > > > > > if (riscv_of_processor_hartid(node) < 0) > > > continue; > > > @@ -169,12 +169,22 @@ void __init riscv_fill_hwcap(void) > > > if (*isa != '_') > > > --isa; > > > > > > +#define SET_ISA_EXT_MAP(name, bit) \ > > > + do { \ > > > + if ((ext_end - ext == sizeof(name) - 1) && \ > > > + !memcmp(ext, name, sizeof(name) - 1)) { \ > > > + this_isa |= (1UL << bit); \ > > > + pr_info("Found ISA extension %s", name);\ > > > + } \ > > > + } while (false) \ > > > + > > > if (unlikely(ext_err)) > > > continue; > > > if (!ext_long) { > > > this_hwcap |= isa2hwcap[(unsigned char)(*ext)]; > > > this_isa |= (1UL << (*ext - 'a')); > > > } > > > +#undef SET_ISA_EXT_MAP > > > } > > > > > > /* > > > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void) > > > else > > > elf_hwcap = this_hwcap; > > > > > > - if (riscv_isa[0]) > > > + if (riscv_isa[0]) { > > > +#if IS_ENABLED(CONFIG_32BIT) > > > + riscv_isa[0] &= this_isa & 0xFFFFFFFF; > > > + riscv_isa[1] &= this_isa >> 32; > > > +#else > > > riscv_isa[0] &= this_isa; > > > - else > > > +#endif > > > + } else { > > > +#if IS_ENABLED(CONFIG_32BIT) > > > + riscv_isa[0] = this_isa & 0xFFFFFFFF; > > > + riscv_isa[1] = this_isa >> 32; > > > +#else > > > riscv_isa[0] = this_isa; > > > +#endif > > > + } > > > } > > > > > > /* We don't support systems with F but without D, so mask those out > > > > > > > > > > > > > >
On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote: > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra: > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb Atish Patra: > > > > Multi-letter extensions can be probed using exising > > > > riscv_isa_extension_available API now. It doesn't support versioning > > > > right now as there is no use case for it. > > > > Individual extension specific implementation will be added during > > > > each extension support. > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > > > > > > > > By the way, does a similar parsing exist for opensbi as well? > > > Things like svpbmt as well as zicbom have CSR bits controlling how > > > these functions should behave (enabling them, etc), so I guess > > > opensbi also needs to parse the extensions from the ISA string? > > > > > > > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to > > identify the extensions [1]. > > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404 > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause > illegal instructions until the level above does allow them. > > When the kernel wants to call a cache-invalidate, from my reading menvcfg > needs to be modified accordingly - which would fall in SBI's court? > I think so. I had the same question for the SSTC extension as well. This is what I currently do: 1. Detect menvcfg first, detect stimecmp 2. Enable SSTC feature only if both are available 3. Set the STCE bit in menvcfg if SSTC is available Here is the patch https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889 > > > In the future, zicbom can be detected in the same manner. However, > > svpbmt is a bit tricky as it doesn't > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ? > > There is the PBMTE bit in MENVCFG, which I found while looking through the > zicbom-parts, which is supposed to "control wheter svpbmt is available for > use". So I guess the question is the same as above :-) > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI needs other way to detect if PBMTE is available. That's why, I think MENVCFG should be set correctly by the hardware upon reset. What do you think about that ? I couldn't find anything related to the reset state for menvcfg. > > Heiko > > > > > Heiko > > > > > > > --- > > > > arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++ > > > > arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++--- > > > > 2 files changed, 42 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > > index 5ce50468aff1..170bd80da520 100644 > > > > --- a/arch/riscv/include/asm/hwcap.h > > > > +++ b/arch/riscv/include/asm/hwcap.h > > > > @@ -34,7 +34,25 @@ extern unsigned long elf_hwcap; > > > > #define RISCV_ISA_EXT_s ('s' - 'a') > > > > #define RISCV_ISA_EXT_u ('u' - 'a') > > > > > > > > +/* > > > > + * Increse this to higher value as kernel support more ISA extensions. > > > > + */ > > > > #define RISCV_ISA_EXT_MAX 64 > > > > +#define RISCV_ISA_EXT_NAME_LEN_MAX 32 > > > > + > > > > +/* The base ID for multi-letter ISA extensions */ > > > > +#define RISCV_ISA_EXT_BASE 26 > > > > + > > > > +/* > > > > + * This enum represent the logical ID for each multi-letter RISC-V ISA extension. > > > > + * The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed > > > > + * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter > > > > + * extensions while all the multi-letter extensions should define the next > > > > + * available logical extension id. > > > > + */ > > > > +enum riscv_isa_ext_id { > > > > + RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX, > > > > +}; > > > > > > > > unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); > > > > > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > > index e9e3b0693d16..469b9739faf7 100644 > > > > --- a/arch/riscv/kernel/cpufeature.c > > > > +++ b/arch/riscv/kernel/cpufeature.c > > > > @@ -83,7 +83,7 @@ void __init riscv_fill_hwcap(void) > > > > > > > > for_each_of_cpu_node(node) { > > > > unsigned long this_hwcap = 0; > > > > - unsigned long this_isa = 0; > > > > + uint64_t this_isa = 0; > > > > > > > > if (riscv_of_processor_hartid(node) < 0) > > > > continue; > > > > @@ -169,12 +169,22 @@ void __init riscv_fill_hwcap(void) > > > > if (*isa != '_') > > > > --isa; > > > > > > > > +#define SET_ISA_EXT_MAP(name, bit) \ > > > > + do { \ > > > > + if ((ext_end - ext == sizeof(name) - 1) && \ > > > > + !memcmp(ext, name, sizeof(name) - 1)) { \ > > > > + this_isa |= (1UL << bit); \ > > > > + pr_info("Found ISA extension %s", name);\ > > > > + } \ > > > > + } while (false) \ > > > > + > > > > if (unlikely(ext_err)) > > > > continue; > > > > if (!ext_long) { > > > > this_hwcap |= isa2hwcap[(unsigned char)(*ext)]; > > > > this_isa |= (1UL << (*ext - 'a')); > > > > } > > > > +#undef SET_ISA_EXT_MAP > > > > } > > > > > > > > /* > > > > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void) > > > > else > > > > elf_hwcap = this_hwcap; > > > > > > > > - if (riscv_isa[0]) > > > > + if (riscv_isa[0]) { > > > > +#if IS_ENABLED(CONFIG_32BIT) > > > > + riscv_isa[0] &= this_isa & 0xFFFFFFFF; > > > > + riscv_isa[1] &= this_isa >> 32; > > > > +#else > > > > riscv_isa[0] &= this_isa; > > > > - else > > > > +#endif > > > > + } else { > > > > +#if IS_ENABLED(CONFIG_32BIT) > > > > + riscv_isa[0] = this_isa & 0xFFFFFFFF; > > > > + riscv_isa[1] = this_isa >> 32; > > > > +#else > > > > riscv_isa[0] = this_isa; > > > > +#endif > > > > + } > > > > } > > > > > > > > /* We don't support systems with F but without D, so mask those out > > > > > > > > > > > > > > > > > > > > > > > > > >
Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra: > On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra: > > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb Atish Patra: > > > > > Multi-letter extensions can be probed using exising > > > > > riscv_isa_extension_available API now. It doesn't support versioning > > > > > right now as there is no use case for it. > > > > > Individual extension specific implementation will be added during > > > > > each extension support. > > > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > > > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > > > > > > > > > > > By the way, does a similar parsing exist for opensbi as well? > > > > Things like svpbmt as well as zicbom have CSR bits controlling how > > > > these functions should behave (enabling them, etc), so I guess > > > > opensbi also needs to parse the extensions from the ISA string? > > > > > > > > > > > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to > > > identify the extensions [1]. > > > > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404 > > > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the > > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause > > illegal instructions until the level above does allow them. > > > > When the kernel wants to call a cache-invalidate, from my reading menvcfg > > needs to be modified accordingly - which would fall in SBI's court? > > > > I think so. I had the same question for the SSTC extension as well. > This is what I currently do: > > 1. Detect menvcfg first, detect stimecmp > 2. Enable SSTC feature only if both are available > 3. Set the STCE bit in menvcfg if SSTC is available > > Here is the patch > https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889 Hmm, the CBO fields are defined as WARL (write any, read legal), so I guess some sort of trap won't work here. The priv-spec only points to the cmo-spec for these bits and the cmo-spec does not specifiy what the value should be when cmo is not present. > > > In the future, zicbom can be detected in the same manner. However, > > > svpbmt is a bit tricky as it doesn't > > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ? > > > > There is the PBMTE bit in MENVCFG, which I found while looking through the > > zicbom-parts, which is supposed to "control wheter svpbmt is available for > > use". So I guess the question is the same as above :-) > > > > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI > needs other way to > detect if PBMTE is available. > > That's why, I think MENVCFG should be set correctly by the hardware > upon reset. What do you think > about that ? I couldn't find anything related to the reset state for menvcfg. me neither. Both the priv-spec as well as the cmobase spec do not specifiy any reset-values it seems. So I guess in the Qemu case, Qemu needs to set that bit when its svpbmt extension is enabled? Heiko > > > > > --- > > > > > arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++ > > > > > arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++--- > > > > > 2 files changed, 42 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > > > index 5ce50468aff1..170bd80da520 100644 > > > > > --- a/arch/riscv/include/asm/hwcap.h > > > > > +++ b/arch/riscv/include/asm/hwcap.h > > > > > @@ -34,7 +34,25 @@ extern unsigned long elf_hwcap; > > > > > #define RISCV_ISA_EXT_s ('s' - 'a') > > > > > #define RISCV_ISA_EXT_u ('u' - 'a') > > > > > > > > > > +/* > > > > > + * Increse this to higher value as kernel support more ISA extensions. > > > > > + */ > > > > > #define RISCV_ISA_EXT_MAX 64 > > > > > +#define RISCV_ISA_EXT_NAME_LEN_MAX 32 > > > > > + > > > > > +/* The base ID for multi-letter ISA extensions */ > > > > > +#define RISCV_ISA_EXT_BASE 26 > > > > > + > > > > > +/* > > > > > + * This enum represent the logical ID for each multi-letter RISC-V ISA extension. > > > > > + * The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed > > > > > + * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter > > > > > + * extensions while all the multi-letter extensions should define the next > > > > > + * available logical extension id. > > > > > + */ > > > > > +enum riscv_isa_ext_id { > > > > > + RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX, > > > > > +}; > > > > > > > > > > unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); > > > > > > > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > > > index e9e3b0693d16..469b9739faf7 100644 > > > > > --- a/arch/riscv/kernel/cpufeature.c > > > > > +++ b/arch/riscv/kernel/cpufeature.c > > > > > @@ -83,7 +83,7 @@ void __init riscv_fill_hwcap(void) > > > > > > > > > > for_each_of_cpu_node(node) { > > > > > unsigned long this_hwcap = 0; > > > > > - unsigned long this_isa = 0; > > > > > + uint64_t this_isa = 0; > > > > > > > > > > if (riscv_of_processor_hartid(node) < 0) > > > > > continue; > > > > > @@ -169,12 +169,22 @@ void __init riscv_fill_hwcap(void) > > > > > if (*isa != '_') > > > > > --isa; > > > > > > > > > > +#define SET_ISA_EXT_MAP(name, bit) \ > > > > > + do { \ > > > > > + if ((ext_end - ext == sizeof(name) - 1) && \ > > > > > + !memcmp(ext, name, sizeof(name) - 1)) { \ > > > > > + this_isa |= (1UL << bit); \ > > > > > + pr_info("Found ISA extension %s", name);\ > > > > > + } \ > > > > > + } while (false) \ > > > > > + > > > > > if (unlikely(ext_err)) > > > > > continue; > > > > > if (!ext_long) { > > > > > this_hwcap |= isa2hwcap[(unsigned char)(*ext)]; > > > > > this_isa |= (1UL << (*ext - 'a')); > > > > > } > > > > > +#undef SET_ISA_EXT_MAP > > > > > } > > > > > > > > > > /* > > > > > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void) > > > > > else > > > > > elf_hwcap = this_hwcap; > > > > > > > > > > - if (riscv_isa[0]) > > > > > + if (riscv_isa[0]) { > > > > > +#if IS_ENABLED(CONFIG_32BIT) > > > > > + riscv_isa[0] &= this_isa & 0xFFFFFFFF; > > > > > + riscv_isa[1] &= this_isa >> 32; > > > > > +#else > > > > > riscv_isa[0] &= this_isa; > > > > > - else > > > > > +#endif > > > > > + } else { > > > > > +#if IS_ENABLED(CONFIG_32BIT) > > > > > + riscv_isa[0] = this_isa & 0xFFFFFFFF; > > > > > + riscv_isa[1] = this_isa >> 32; > > > > > +#else > > > > > riscv_isa[0] = this_isa; > > > > > +#endif > > > > > + } > > > > > } > > > > > > > > > > /* We don't support systems with F but without D, so mask those out > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Mon, Feb 14, 2022 at 2:22 PM Heiko Stübner <heiko@sntech.de> wrote: > > Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra: > > On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra: > > > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb Atish Patra: > > > > > > Multi-letter extensions can be probed using exising > > > > > > riscv_isa_extension_available API now. It doesn't support versioning > > > > > > right now as there is no use case for it. > > > > > > Individual extension specific implementation will be added during > > > > > > each extension support. > > > > > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > > > > > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > > > > > > > > > > > > > > By the way, does a similar parsing exist for opensbi as well? > > > > > Things like svpbmt as well as zicbom have CSR bits controlling how > > > > > these functions should behave (enabling them, etc), so I guess > > > > > opensbi also needs to parse the extensions from the ISA string? > > > > > > > > > > > > > > > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to > > > > identify the extensions [1]. > > > > > > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404 > > > > > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the > > > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause > > > illegal instructions until the level above does allow them. > > > > > > When the kernel wants to call a cache-invalidate, from my reading menvcfg > > > needs to be modified accordingly - which would fall in SBI's court? > > > > > > > I think so. I had the same question for the SSTC extension as well. > > This is what I currently do: > > > > 1. Detect menvcfg first, detect stimecmp > > 2. Enable SSTC feature only if both are available > > 3. Set the STCE bit in menvcfg if SSTC is available > > > > Here is the patch > > https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889 > > Hmm, the CBO fields are defined as WARL (write any, read legal), > so I guess some sort of trap won't work here. > Correct. Traps for extensions that introduce new CSRs. I was suggesting setting the corresponding bits in MENVCFG and reading it again to check if it sticks. > The priv-spec only points to the cmo-spec for these bits and the cmo-spec > does not specifiy what the value should be when cmo is not present. > > > > > > In the future, zicbom can be detected in the same manner. However, > > > > svpbmt is a bit tricky as it doesn't > > > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ? > > > > > > There is the PBMTE bit in MENVCFG, which I found while looking through the > > > zicbom-parts, which is supposed to "control wheter svpbmt is available for > > > use". So I guess the question is the same as above :-) > > > > > > > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI > > needs other way to > > detect if PBMTE is available. > > > > That's why, I think MENVCFG should be set correctly by the hardware > > upon reset. What do you think > > about that ? I couldn't find anything related to the reset state for menvcfg. > > me neither. Both the priv-spec as well as the cmobase spec do not > specifiy any reset-values it seems. > I have raised an issue in the ISA spec. https://github.com/riscv/riscv-isa-manual/issues/820 > So I guess in the Qemu case, Qemu needs to set that bit when > its svpbmt extension is enabled? > We can do that if the priv spec is modified to allow that. > > Heiko > > > > > > > > --- > > > > > > arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++ > > > > > > arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++--- > > > > > > 2 files changed, 42 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > > > > index 5ce50468aff1..170bd80da520 100644 > > > > > > --- a/arch/riscv/include/asm/hwcap.h > > > > > > +++ b/arch/riscv/include/asm/hwcap.h > > > > > > @@ -34,7 +34,25 @@ extern unsigned long elf_hwcap; > > > > > > #define RISCV_ISA_EXT_s ('s' - 'a') > > > > > > #define RISCV_ISA_EXT_u ('u' - 'a') > > > > > > > > > > > > +/* > > > > > > + * Increse this to higher value as kernel support more ISA extensions. > > > > > > + */ > > > > > > #define RISCV_ISA_EXT_MAX 64 > > > > > > +#define RISCV_ISA_EXT_NAME_LEN_MAX 32 > > > > > > + > > > > > > +/* The base ID for multi-letter ISA extensions */ > > > > > > +#define RISCV_ISA_EXT_BASE 26 > > > > > > + > > > > > > +/* > > > > > > + * This enum represent the logical ID for each multi-letter RISC-V ISA extension. > > > > > > + * The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed > > > > > > + * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter > > > > > > + * extensions while all the multi-letter extensions should define the next > > > > > > + * available logical extension id. > > > > > > + */ > > > > > > +enum riscv_isa_ext_id { > > > > > > + RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX, > > > > > > +}; > > > > > > > > > > > > unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); > > > > > > > > > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > > > > index e9e3b0693d16..469b9739faf7 100644 > > > > > > --- a/arch/riscv/kernel/cpufeature.c > > > > > > +++ b/arch/riscv/kernel/cpufeature.c > > > > > > @@ -83,7 +83,7 @@ void __init riscv_fill_hwcap(void) > > > > > > > > > > > > for_each_of_cpu_node(node) { > > > > > > unsigned long this_hwcap = 0; > > > > > > - unsigned long this_isa = 0; > > > > > > + uint64_t this_isa = 0; > > > > > > > > > > > > if (riscv_of_processor_hartid(node) < 0) > > > > > > continue; > > > > > > @@ -169,12 +169,22 @@ void __init riscv_fill_hwcap(void) > > > > > > if (*isa != '_') > > > > > > --isa; > > > > > > > > > > > > +#define SET_ISA_EXT_MAP(name, bit) \ > > > > > > + do { \ > > > > > > + if ((ext_end - ext == sizeof(name) - 1) && \ > > > > > > + !memcmp(ext, name, sizeof(name) - 1)) { \ > > > > > > + this_isa |= (1UL << bit); \ > > > > > > + pr_info("Found ISA extension %s", name);\ > > > > > > + } \ > > > > > > + } while (false) \ > > > > > > + > > > > > > if (unlikely(ext_err)) > > > > > > continue; > > > > > > if (!ext_long) { > > > > > > this_hwcap |= isa2hwcap[(unsigned char)(*ext)]; > > > > > > this_isa |= (1UL << (*ext - 'a')); > > > > > > } > > > > > > +#undef SET_ISA_EXT_MAP > > > > > > } > > > > > > > > > > > > /* > > > > > > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void) > > > > > > else > > > > > > elf_hwcap = this_hwcap; > > > > > > > > > > > > - if (riscv_isa[0]) > > > > > > + if (riscv_isa[0]) { > > > > > > +#if IS_ENABLED(CONFIG_32BIT) > > > > > > + riscv_isa[0] &= this_isa & 0xFFFFFFFF; > > > > > > + riscv_isa[1] &= this_isa >> 32; > > > > > > +#else > > > > > > riscv_isa[0] &= this_isa; > > > > > > - else > > > > > > +#endif > > > > > > + } else { > > > > > > +#if IS_ENABLED(CONFIG_32BIT) > > > > > > + riscv_isa[0] = this_isa & 0xFFFFFFFF; > > > > > > + riscv_isa[1] = this_isa >> 32; > > > > > > +#else > > > > > > riscv_isa[0] = this_isa; > > > > > > +#endif > > > > > > + } > > > > > > } > > > > > > > > > > > > /* We don't support systems with F but without D, so mask those out > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Mon, Feb 14, 2022 at 3:22 PM Atish Kumar Patra <atishp@rivosinc.com> wrote: > > On Mon, Feb 14, 2022 at 2:22 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra: > > > On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra: > > > > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb Atish Patra: > > > > > > > Multi-letter extensions can be probed using exising > > > > > > > riscv_isa_extension_available API now. It doesn't support versioning > > > > > > > right now as there is no use case for it. > > > > > > > Individual extension specific implementation will be added during > > > > > > > each extension support. > > > > > > > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > > > > > > > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > > > > > > > > > > > > > > > > > By the way, does a similar parsing exist for opensbi as well? > > > > > > Things like svpbmt as well as zicbom have CSR bits controlling how > > > > > > these functions should behave (enabling them, etc), so I guess > > > > > > opensbi also needs to parse the extensions from the ISA string? > > > > > > > > > > > > > > > > > > > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to > > > > > identify the extensions [1]. > > > > > > > > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404 > > > > > > > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the > > > > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause > > > > illegal instructions until the level above does allow them. > > > > > > > > When the kernel wants to call a cache-invalidate, from my reading menvcfg > > > > needs to be modified accordingly - which would fall in SBI's court? > > > > > > > > > > I think so. I had the same question for the SSTC extension as well. > > > This is what I currently do: > > > > > > 1. Detect menvcfg first, detect stimecmp > > > 2. Enable SSTC feature only if both are available > > > 3. Set the STCE bit in menvcfg if SSTC is available > > > > > > Here is the patch > > > https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889 > > > > Hmm, the CBO fields are defined as WARL (write any, read legal), > > so I guess some sort of trap won't work here. > > > > Correct. Traps for extensions that introduce new CSRs. > I was suggesting setting the corresponding bits in MENVCFG and reading > it again to check if it sticks. > > > The priv-spec only points to the cmo-spec for these bits and the cmo-spec > > does not specifiy what the value should be when cmo is not present. > > > > > > > > > In the future, zicbom can be detected in the same manner. However, > > > > > svpbmt is a bit tricky as it doesn't > > > > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ? > > > > > > > > There is the PBMTE bit in MENVCFG, which I found while looking through the > > > > zicbom-parts, which is supposed to "control wheter svpbmt is available for > > > > use". So I guess the question is the same as above :-) > > > > > > > > > > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI > > > needs other way to > > > detect if PBMTE is available. > > > > > > That's why, I think MENVCFG should be set correctly by the hardware > > > upon reset. What do you think > > > about that ? I couldn't find anything related to the reset state for menvcfg. > > > > me neither. Both the priv-spec as well as the cmobase spec do not > > specifiy any reset-values it seems. > > > I have raised an issue in the ISA spec. > https://github.com/riscv/riscv-isa-manual/issues/820 > > > So I guess in the Qemu case, Qemu needs to set that bit when > > its svpbmt extension is enabled? > > > > We can do that if the priv spec is modified to allow that. > As per Greg's response, hardware is not expected to do that. So we have to dynamically detect the extensions in OpenSBI and write to menvcfg. I am not sure what needs to be done for CBIE bits as it both flush(01) or invalidate(11) are valid values > > > > Heiko > > > > > > > > > > > --- > > > > > > > arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++ > > > > > > > arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++--- > > > > > > > 2 files changed, 42 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > > > > > index 5ce50468aff1..170bd80da520 100644 > > > > > > > --- a/arch/riscv/include/asm/hwcap.h > > > > > > > +++ b/arch/riscv/include/asm/hwcap.h > > > > > > > @@ -34,7 +34,25 @@ extern unsigned long elf_hwcap; > > > > > > > #define RISCV_ISA_EXT_s ('s' - 'a') > > > > > > > #define RISCV_ISA_EXT_u ('u' - 'a') > > > > > > > > > > > > > > +/* > > > > > > > + * Increse this to higher value as kernel support more ISA extensions. > > > > > > > + */ > > > > > > > #define RISCV_ISA_EXT_MAX 64 > > > > > > > +#define RISCV_ISA_EXT_NAME_LEN_MAX 32 > > > > > > > + > > > > > > > +/* The base ID for multi-letter ISA extensions */ > > > > > > > +#define RISCV_ISA_EXT_BASE 26 > > > > > > > + > > > > > > > +/* > > > > > > > + * This enum represent the logical ID for each multi-letter RISC-V ISA extension. > > > > > > > + * The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed > > > > > > > + * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter > > > > > > > + * extensions while all the multi-letter extensions should define the next > > > > > > > + * available logical extension id. > > > > > > > + */ > > > > > > > +enum riscv_isa_ext_id { > > > > > > > + RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX, > > > > > > > +}; > > > > > > > > > > > > > > unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); > > > > > > > > > > > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > > > > > index e9e3b0693d16..469b9739faf7 100644 > > > > > > > --- a/arch/riscv/kernel/cpufeature.c > > > > > > > +++ b/arch/riscv/kernel/cpufeature.c > > > > > > > @@ -83,7 +83,7 @@ void __init riscv_fill_hwcap(void) > > > > > > > > > > > > > > for_each_of_cpu_node(node) { > > > > > > > unsigned long this_hwcap = 0; > > > > > > > - unsigned long this_isa = 0; > > > > > > > + uint64_t this_isa = 0; > > > > > > > > > > > > > > if (riscv_of_processor_hartid(node) < 0) > > > > > > > continue; > > > > > > > @@ -169,12 +169,22 @@ void __init riscv_fill_hwcap(void) > > > > > > > if (*isa != '_') > > > > > > > --isa; > > > > > > > > > > > > > > +#define SET_ISA_EXT_MAP(name, bit) \ > > > > > > > + do { \ > > > > > > > + if ((ext_end - ext == sizeof(name) - 1) && \ > > > > > > > + !memcmp(ext, name, sizeof(name) - 1)) { \ > > > > > > > + this_isa |= (1UL << bit); \ > > > > > > > + pr_info("Found ISA extension %s", name);\ > > > > > > > + } \ > > > > > > > + } while (false) \ > > > > > > > + > > > > > > > if (unlikely(ext_err)) > > > > > > > continue; > > > > > > > if (!ext_long) { > > > > > > > this_hwcap |= isa2hwcap[(unsigned char)(*ext)]; > > > > > > > this_isa |= (1UL << (*ext - 'a')); > > > > > > > } > > > > > > > +#undef SET_ISA_EXT_MAP > > > > > > > } > > > > > > > > > > > > > > /* > > > > > > > @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void) > > > > > > > else > > > > > > > elf_hwcap = this_hwcap; > > > > > > > > > > > > > > - if (riscv_isa[0]) > > > > > > > + if (riscv_isa[0]) { > > > > > > > +#if IS_ENABLED(CONFIG_32BIT) > > > > > > > + riscv_isa[0] &= this_isa & 0xFFFFFFFF; > > > > > > > + riscv_isa[1] &= this_isa >> 32; > > > > > > > +#else > > > > > > > riscv_isa[0] &= this_isa; > > > > > > > - else > > > > > > > +#endif > > > > > > > + } else { > > > > > > > +#if IS_ENABLED(CONFIG_32BIT) > > > > > > > + riscv_isa[0] = this_isa & 0xFFFFFFFF; > > > > > > > + riscv_isa[1] = this_isa >> 32; > > > > > > > +#else > > > > > > > riscv_isa[0] = this_isa; > > > > > > > +#endif > > > > > > > + } > > > > > > > } > > > > > > > > > > > > > > /* We don't support systems with F but without D, so mask those out > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Am Dienstag, 15. Februar 2022, 10:12:53 CET schrieb Atish Kumar Patra: > On Mon, Feb 14, 2022 at 3:22 PM Atish Kumar Patra <atishp@rivosinc.com> wrote: > > > > On Mon, Feb 14, 2022 at 2:22 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra: > > > > On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra: > > > > > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > > > > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb Atish Patra: > > > > > > > > Multi-letter extensions can be probed using exising > > > > > > > > riscv_isa_extension_available API now. It doesn't support versioning > > > > > > > > right now as there is no use case for it. > > > > > > > > Individual extension specific implementation will be added during > > > > > > > > each extension support. > > > > > > > > > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > > > > > > > > > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > > > > > > > > > > > > > > > > > > > > By the way, does a similar parsing exist for opensbi as well? > > > > > > > Things like svpbmt as well as zicbom have CSR bits controlling how > > > > > > > these functions should behave (enabling them, etc), so I guess > > > > > > > opensbi also needs to parse the extensions from the ISA string? > > > > > > > > > > > > > > > > > > > > > > > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to > > > > > > identify the extensions [1]. > > > > > > > > > > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404 > > > > > > > > > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the > > > > > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause > > > > > illegal instructions until the level above does allow them. > > > > > > > > > > When the kernel wants to call a cache-invalidate, from my reading menvcfg > > > > > needs to be modified accordingly - which would fall in SBI's court? > > > > > > > > > > > > > I think so. I had the same question for the SSTC extension as well. > > > > This is what I currently do: > > > > > > > > 1. Detect menvcfg first, detect stimecmp > > > > 2. Enable SSTC feature only if both are available > > > > 3. Set the STCE bit in menvcfg if SSTC is available > > > > > > > > Here is the patch > > > > https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889 > > > > > > Hmm, the CBO fields are defined as WARL (write any, read legal), > > > so I guess some sort of trap won't work here. > > > > > > > Correct. Traps for extensions that introduce new CSRs. > > I was suggesting setting the corresponding bits in MENVCFG and reading > > it again to check if it sticks. > > > > > The priv-spec only points to the cmo-spec for these bits and the cmo-spec > > > does not specifiy what the value should be when cmo is not present. > > > > > > > > > > > > In the future, zicbom can be detected in the same manner. However, > > > > > > svpbmt is a bit tricky as it doesn't > > > > > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ? > > > > > > > > > > There is the PBMTE bit in MENVCFG, which I found while looking through the > > > > > zicbom-parts, which is supposed to "control wheter svpbmt is available for > > > > > use". So I guess the question is the same as above :-) > > > > > > > > > > > > > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI > > > > needs other way to > > > > detect if PBMTE is available. > > > > > > > > That's why, I think MENVCFG should be set correctly by the hardware > > > > upon reset. What do you think > > > > about that ? I couldn't find anything related to the reset state for menvcfg. > > > > > > me neither. Both the priv-spec as well as the cmobase spec do not > > > specifiy any reset-values it seems. > > > > > I have raised an issue in the ISA spec. > > https://github.com/riscv/riscv-isa-manual/issues/820 > > > > > So I guess in the Qemu case, Qemu needs to set that bit when > > > its svpbmt extension is enabled? > > > > > > > We can do that if the priv spec is modified to allow that. > > > > As per Greg's response, hardware is not expected to do that. > So we have to dynamically detect the extensions in OpenSBI and write to menvcfg. > > I am not sure what needs to be done for CBIE bits as it both flush(01) > or invalidate(11) are valid values From looking at the security remark in the cmo-spec, I guess flush would be the appropriate thing to do? "Until a modified cache block has updated memory, a CBO.INVAL instruction may expose stale data values in memory if the CSRs are programmed to perform an invalidate operation. This behavior may result in a security hole if lower privileged level software performs an invalidate operation and accesses sensitive information in memory." But also do we actually _want_ to enable cmo always ... Greg was talking about backwards compatiblity in his response as well.
Am Dienstag, 15. Februar 2022, 10:48:16 CET schrieb Heiko Stübner: > Am Dienstag, 15. Februar 2022, 10:12:53 CET schrieb Atish Kumar Patra: > > On Mon, Feb 14, 2022 at 3:22 PM Atish Kumar Patra <atishp@rivosinc.com> wrote: > > > > > > On Mon, Feb 14, 2022 at 2:22 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra: > > > > > On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > > > > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra: > > > > > > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > > > > > > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb Atish Patra: > > > > > > > > > Multi-letter extensions can be probed using exising > > > > > > > > > riscv_isa_extension_available API now. It doesn't support versioning > > > > > > > > > right now as there is no use case for it. > > > > > > > > > Individual extension specific implementation will be added during > > > > > > > > > each extension support. > > > > > > > > > > > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > > > > > > > > > > > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > > > > > > > > > > > > > > > > > > > > > > > By the way, does a similar parsing exist for opensbi as well? > > > > > > > > Things like svpbmt as well as zicbom have CSR bits controlling how > > > > > > > > these functions should behave (enabling them, etc), so I guess > > > > > > > > opensbi also needs to parse the extensions from the ISA string? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to > > > > > > > identify the extensions [1]. > > > > > > > > > > > > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404 > > > > > > > > > > > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the > > > > > > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause > > > > > > illegal instructions until the level above does allow them. > > > > > > > > > > > > When the kernel wants to call a cache-invalidate, from my reading menvcfg > > > > > > needs to be modified accordingly - which would fall in SBI's court? > > > > > > > > > > > > > > > > I think so. I had the same question for the SSTC extension as well. > > > > > This is what I currently do: > > > > > > > > > > 1. Detect menvcfg first, detect stimecmp > > > > > 2. Enable SSTC feature only if both are available > > > > > 3. Set the STCE bit in menvcfg if SSTC is available > > > > > > > > > > Here is the patch > > > > > https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889 > > > > > > > > Hmm, the CBO fields are defined as WARL (write any, read legal), > > > > so I guess some sort of trap won't work here. > > > > > > > > > > Correct. Traps for extensions that introduce new CSRs. > > > I was suggesting setting the corresponding bits in MENVCFG and reading > > > it again to check if it sticks. > > > > > > > The priv-spec only points to the cmo-spec for these bits and the cmo-spec > > > > does not specifiy what the value should be when cmo is not present. > > > > > > > > > > > > > > > In the future, zicbom can be detected in the same manner. However, > > > > > > > svpbmt is a bit tricky as it doesn't > > > > > > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ? > > > > > > > > > > > > There is the PBMTE bit in MENVCFG, which I found while looking through the > > > > > > zicbom-parts, which is supposed to "control wheter svpbmt is available for > > > > > > use". So I guess the question is the same as above :-) > > > > > > > > > > > > > > > > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI > > > > > needs other way to > > > > > detect if PBMTE is available. > > > > > > > > > > That's why, I think MENVCFG should be set correctly by the hardware > > > > > upon reset. What do you think > > > > > about that ? I couldn't find anything related to the reset state for menvcfg. > > > > > > > > me neither. Both the priv-spec as well as the cmobase spec do not > > > > specifiy any reset-values it seems. > > > > > > > I have raised an issue in the ISA spec. > > > https://github.com/riscv/riscv-isa-manual/issues/820 > > > > > > > So I guess in the Qemu case, Qemu needs to set that bit when > > > > its svpbmt extension is enabled? > > > > > > > > > > We can do that if the priv spec is modified to allow that. > > > > > > > As per Greg's response, hardware is not expected to do that. > > So we have to dynamically detect the extensions in OpenSBI and write to menvcfg. Doesn't SBI also get the devicetree and could therefore parse the ISA string for extensions? Might be less volatile and would have both Kernel and SBI do the same thing for detection. > > I am not sure what needs to be done for CBIE bits as it both flush(01) > > or invalidate(11) are valid values > > From looking at the security remark in the cmo-spec, I guess flush would be > the appropriate thing to do? > > "Until a modified cache block has updated memory, a CBO.INVAL instruction may expose stale data values > in memory if the CSRs are programmed to perform an invalidate operation. This behavior may result in a > security hole if lower privileged level software performs an invalidate operation and accesses sensitive > information in memory." > > But also do we actually _want_ to enable cmo always ... Greg was talking > about backwards compatiblity in his response as well. >
On Tue, Feb 15, 2022 at 1:50 AM Heiko Stübner <heiko@sntech.de> wrote: > > Am Dienstag, 15. Februar 2022, 10:48:16 CET schrieb Heiko Stübner: > > Am Dienstag, 15. Februar 2022, 10:12:53 CET schrieb Atish Kumar Patra: > > > On Mon, Feb 14, 2022 at 3:22 PM Atish Kumar Patra <atishp@rivosinc.com> wrote: > > > > > > > > On Mon, Feb 14, 2022 at 2:22 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > > > Am Montag, 14. Februar 2022, 21:42:32 CET schrieb Atish Patra: > > > > > > On Mon, Feb 14, 2022 at 12:24 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > > > > > > > Am Montag, 14. Februar 2022, 21:14:13 CET schrieb Atish Patra: > > > > > > > > On Mon, Feb 14, 2022 at 12:06 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > > > > > > > > > > > > > > > Am Donnerstag, 10. Februar 2022, 22:40:16 CET schrieb Atish Patra: > > > > > > > > > > Multi-letter extensions can be probed using exising > > > > > > > > > > riscv_isa_extension_available API now. It doesn't support versioning > > > > > > > > > > right now as there is no use case for it. > > > > > > > > > > Individual extension specific implementation will be added during > > > > > > > > > > each extension support. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > > > > > > > > > > > > > > > > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > > > > > > > > > > > > > > > > > > > > > > > > > > By the way, does a similar parsing exist for opensbi as well? > > > > > > > > > Things like svpbmt as well as zicbom have CSR bits controlling how > > > > > > > > > these functions should behave (enabling them, etc), so I guess > > > > > > > > > opensbi also needs to parse the extensions from the ISA string? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > No. Currently, OpenSBI relies on the CSR read/write & trap method to > > > > > > > > identify the extensions [1]. > > > > > > > > > > > > > > > > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L404 > > > > > > > > > > > > > > I guess my question is more, who is supposed to set CBIE, CBCFE bits in the > > > > > > > ENVCFG CSR. I.e. at it's default settings CMO instructions will cause > > > > > > > illegal instructions until the level above does allow them. > > > > > > > > > > > > > > When the kernel wants to call a cache-invalidate, from my reading menvcfg > > > > > > > needs to be modified accordingly - which would fall in SBI's court? > > > > > > > > > > > > > > > > > > > I think so. I had the same question for the SSTC extension as well. > > > > > > This is what I currently do: > > > > > > > > > > > > 1. Detect menvcfg first, detect stimecmp > > > > > > 2. Enable SSTC feature only if both are available > > > > > > 3. Set the STCE bit in menvcfg if SSTC is available > > > > > > > > > > > > Here is the patch > > > > > > https://github.com/atishp04/opensbi/commit/e6b185821e8302bffdceb4633b413252e0de4889 > > > > > > > > > > Hmm, the CBO fields are defined as WARL (write any, read legal), > > > > > so I guess some sort of trap won't work here. > > > > > > > > > > > > > Correct. Traps for extensions that introduce new CSRs. > > > > I was suggesting setting the corresponding bits in MENVCFG and reading > > > > it again to check if it sticks. > > > > > > > > > The priv-spec only points to the cmo-spec for these bits and the cmo-spec > > > > > does not specifiy what the value should be when cmo is not present. > > > > > > > > > > > > > > > > > > In the future, zicbom can be detected in the same manner. However, > > > > > > > > svpbmt is a bit tricky as it doesn't > > > > > > > > define any new CSR. Do you think OpenSBI needs to detect svpbmt for any reason ? > > > > > > > > > > > > > > There is the PBMTE bit in MENVCFG, which I found while looking through the > > > > > > > zicbom-parts, which is supposed to "control wheter svpbmt is available for > > > > > > > use". So I guess the question is the same as above :-) > > > > > > > > > > > > > > > > > > > PBMTE bit in MENVCFG says if PBMTE bit is available or not. OpenSBI > > > > > > needs other way to > > > > > > detect if PBMTE is available. > > > > > > > > > > > > That's why, I think MENVCFG should be set correctly by the hardware > > > > > > upon reset. What do you think > > > > > > about that ? I couldn't find anything related to the reset state for menvcfg. > > > > > > > > > > me neither. Both the priv-spec as well as the cmobase spec do not > > > > > specifiy any reset-values it seems. > > > > > > > > > I have raised an issue in the ISA spec. > > > > https://github.com/riscv/riscv-isa-manual/issues/820 > > > > > > > > > So I guess in the Qemu case, Qemu needs to set that bit when > > > > > its svpbmt extension is enabled? > > > > > > > > > > > > > We can do that if the priv spec is modified to allow that. > > > > > > > > > > As per Greg's response, hardware is not expected to do that. > > > So we have to dynamically detect the extensions in OpenSBI and write to menvcfg. > > Doesn't SBI also get the devicetree and could therefore parse > the ISA string for extensions? Might be less volatile and would > have both Kernel and SBI do the same thing for detection. > It does. But the later stage boot loader can replace the DT as well. A incorrect DT passed to OpenSBI may lead to crash the system but that's probably okay because the "riscv,isa" properties shouldn't be incorrect at the first place. We can set the hart features based on DT parsing as well (similar to Linux kernel) I suggested the earlier method because that infra already exists in OpenSBI and will continue to exist because not all hart features are ISA extensions. So we can leverage that or add dt parsing as well. I am fine with either approach. > > > > I am not sure what needs to be done for CBIE bits as it both flush(01) > > > or invalidate(11) are valid values > > > > From looking at the security remark in the cmo-spec, I guess flush would be > > the appropriate thing to do? > > Looks like that. But how does a supervisor/usermode use the invalidate functionality ? > > "Until a modified cache block has updated memory, a CBO.INVAL instruction may expose stale data values > > in memory if the CSRs are programmed to perform an invalidate operation. This behavior may result in a > > security hole if lower privileged level software performs an invalidate operation and accesses sensitive > > information in memory." > > > > But also do we actually _want_ to enable cmo always ... Greg was talking > > about backwards compatiblity in his response as well. > > > > > >
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h index 5ce50468aff1..170bd80da520 100644 --- a/arch/riscv/include/asm/hwcap.h +++ b/arch/riscv/include/asm/hwcap.h @@ -34,7 +34,25 @@ extern unsigned long elf_hwcap; #define RISCV_ISA_EXT_s ('s' - 'a') #define RISCV_ISA_EXT_u ('u' - 'a') +/* + * Increse this to higher value as kernel support more ISA extensions. + */ #define RISCV_ISA_EXT_MAX 64 +#define RISCV_ISA_EXT_NAME_LEN_MAX 32 + +/* The base ID for multi-letter ISA extensions */ +#define RISCV_ISA_EXT_BASE 26 + +/* + * This enum represent the logical ID for each multi-letter RISC-V ISA extension. + * The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed + * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter + * extensions while all the multi-letter extensions should define the next + * available logical extension id. + */ +enum riscv_isa_ext_id { + RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX, +}; unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index e9e3b0693d16..469b9739faf7 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -83,7 +83,7 @@ void __init riscv_fill_hwcap(void) for_each_of_cpu_node(node) { unsigned long this_hwcap = 0; - unsigned long this_isa = 0; + uint64_t this_isa = 0; if (riscv_of_processor_hartid(node) < 0) continue; @@ -169,12 +169,22 @@ void __init riscv_fill_hwcap(void) if (*isa != '_') --isa; +#define SET_ISA_EXT_MAP(name, bit) \ + do { \ + if ((ext_end - ext == sizeof(name) - 1) && \ + !memcmp(ext, name, sizeof(name) - 1)) { \ + this_isa |= (1UL << bit); \ + pr_info("Found ISA extension %s", name);\ + } \ + } while (false) \ + if (unlikely(ext_err)) continue; if (!ext_long) { this_hwcap |= isa2hwcap[(unsigned char)(*ext)]; this_isa |= (1UL << (*ext - 'a')); } +#undef SET_ISA_EXT_MAP } /* @@ -187,10 +197,21 @@ void __init riscv_fill_hwcap(void) else elf_hwcap = this_hwcap; - if (riscv_isa[0]) + if (riscv_isa[0]) { +#if IS_ENABLED(CONFIG_32BIT) + riscv_isa[0] &= this_isa & 0xFFFFFFFF; + riscv_isa[1] &= this_isa >> 32; +#else riscv_isa[0] &= this_isa; - else +#endif + } else { +#if IS_ENABLED(CONFIG_32BIT) + riscv_isa[0] = this_isa & 0xFFFFFFFF; + riscv_isa[1] = this_isa >> 32; +#else riscv_isa[0] = this_isa; +#endif + } } /* We don't support systems with F but without D, so mask those out
Multi-letter extensions can be probed using exising riscv_isa_extension_available API now. It doesn't support versioning right now as there is no use case for it. Individual extension specific implementation will be added during each extension support. Signed-off-by: Atish Patra <atishp@rivosinc.com> --- arch/riscv/include/asm/hwcap.h | 18 ++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 27 ++++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 3 deletions(-)