| Message ID | 20240420-dev-charlie-support_thead_vector_6_9-v3-8-67cff4271d1d@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: Support vendor extensions and xtheadvector | expand |
On Sat, Apr 20, 2024 at 06:04:40PM -0700, Charlie Jenkins wrote: > Vendor extensions are maintained in per-vendor structs (separate from > standard extensions which live in riscv_isa). Create vendor variants for > the existing extension helpers to interface with the riscv_isa_vendor > bitmaps. > There is a good amount of overlap between these functions, so > the alternative checking code can be factored out. Can you please split this out? > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > --- > arch/riscv/errata/sifive/errata.c | 3 ++ > arch/riscv/errata/thead/errata.c | 3 ++ > arch/riscv/include/asm/cpufeature.h | 86 +++++++++++++++++------------- > arch/riscv/include/asm/vendor_extensions.h | 56 +++++++++++++++++++ > arch/riscv/kernel/cpufeature.c | 20 ++++--- > arch/riscv/kernel/vendor_extensions.c | 40 ++++++++++++++ > 6 files changed, 164 insertions(+), 44 deletions(-) > > diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c > index 3d9a32d791f7..b29b6e405ff2 100644 > --- a/arch/riscv/errata/sifive/errata.c > +++ b/arch/riscv/errata/sifive/errata.c > @@ -12,6 +12,7 @@ > #include <asm/alternative.h> > #include <asm/vendorid_list.h> > #include <asm/errata_list.h> > +#include <asm/vendor_extensions.h> > > struct errata_info_t { > char name[32]; > @@ -99,6 +100,8 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, > for (alt = begin; alt < end; alt++) { > if (alt->vendor_id != SIFIVE_VENDOR_ID) > continue; > + if (alt->patch_id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) > + continue; > if (alt->patch_id >= ERRATA_SIFIVE_NUMBER) { > WARN(1, "This errata id:%d is not in kernel errata list", alt->patch_id); > continue; > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > index b1c410bbc1ae..d8e78cc687bc 100644 > --- a/arch/riscv/errata/thead/errata.c > +++ b/arch/riscv/errata/thead/errata.c > @@ -18,6 +18,7 @@ > #include <asm/io.h> > #include <asm/patch.h> > #include <asm/vendorid_list.h> > +#include <asm/vendor_extensions.h> > > static bool errata_probe_pbmt(unsigned int stage, > unsigned long arch_id, unsigned long impid) > @@ -163,6 +164,8 @@ void thead_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, > for (alt = begin; alt < end; alt++) { > if (alt->vendor_id != THEAD_VENDOR_ID) > continue; > + if (alt->patch_id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) > + continue; > if (alt->patch_id >= ERRATA_THEAD_NUMBER) This number is 2, how does the patching actually work for vendor stuff when the base is always greater than 2? > continue; > > diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h > index db6a6d7d6b2e..83e1143db9ad 100644 > --- a/arch/riscv/include/asm/cpufeature.h > +++ b/arch/riscv/include/asm/cpufeature.h > @@ -103,22 +103,13 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i > __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext) > > static __always_inline bool > -riscv_has_extension_likely(const unsigned long ext) > +__riscv_has_extension_likely_alternatives(const unsigned long vendor, const unsigned long ext) > { > - compiletime_assert(ext < RISCV_ISA_EXT_MAX, > - "ext must be < RISCV_ISA_EXT_MAX"); > - > - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > - asm goto( > - ALTERNATIVE("j %l[l_no]", "nop", 0, %[ext], 1) > - : > - : [ext] "i" (ext) > - : > - : l_no); > - } else { > - if (!__riscv_isa_extension_available(NULL, ext)) > - goto l_no; > - } > + asm goto(ALTERNATIVE("j %l[l_no]", "nop", %[vendor], %[ext], 1) > + : > + : [vendor] "i" (vendor), [ext] "i" (ext) > + : > + : l_no); > > return true; > l_no: > @@ -126,42 +117,65 @@ riscv_has_extension_likely(const unsigned long ext) > } > > static __always_inline bool > -riscv_has_extension_unlikely(const unsigned long ext) > +__riscv_has_extension_unlikely_alternatives(const unsigned long vendor, const unsigned long ext) ngl, I think you could drop the _alternatives from these - the likely/unlikely is only actually a thing because of the alternatives in the first place & just retain the __ as a differentiator. That'd help you with some of the long-line wrangling you've been doing below. > { > - compiletime_assert(ext < RISCV_ISA_EXT_MAX, > - "ext must be < RISCV_ISA_EXT_MAX"); > - > - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > - asm goto( > - ALTERNATIVE("nop", "j %l[l_yes]", 0, %[ext], 1) > - : > - : [ext] "i" (ext) > - : > - : l_yes); > - } else { > - if (__riscv_isa_extension_available(NULL, ext)) > - goto l_yes; > - } > + asm goto(ALTERNATIVE("nop", "j %l[l_yes]", %[vendor], %[ext], 1) > + : > + : [vendor] "i" (vendor), [ext] "i" (ext) > + : > + : l_yes); > > return false; > l_yes: > return true; > } > > +static __always_inline bool > +riscv_has_extension_likely(const unsigned long ext) Can you format this so that its on one line & wrap the arguments if needs be? > +{ > + compiletime_assert(ext < RISCV_ISA_EXT_MAX, > + "ext must be < RISCV_ISA_EXT_MAX"); > + > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) > + return __riscv_has_extension_likely_alternatives(0, ext); > + else I'm almost certain I said this before, but none of the else branches are needed here, there's a return in the if branch, so the remainder of the function becomes unconditionally executed. > + return __riscv_isa_extension_available(NULL, ext); > +} > + > +static __always_inline bool > +riscv_has_extension_unlikely(const unsigned long ext) > +{ > + compiletime_assert(ext < RISCV_ISA_EXT_MAX, > + "ext must be < RISCV_ISA_EXT_MAX"); > + > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) > + return __riscv_has_extension_unlikely_alternatives(0, ext); > + else > + return __riscv_isa_extension_available(NULL, ext); > +} > + > static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext) > { > - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext)) > - return true; > + compiletime_assert(ext < RISCV_ISA_EXT_MAX, > + "ext must be < RISCV_ISA_EXT_MAX"); > > - return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && > + __riscv_has_extension_likely_alternatives(0, ext)) 0 is meaningless, please make this more understandable using a define of some sort. > + return true; > + else > + return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); > } > > static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext) > { > - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext)) > - return true; > + compiletime_assert(ext < RISCV_ISA_EXT_MAX, > + "ext must be < RISCV_ISA_EXT_MAX"); > > - return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && > + __riscv_has_extension_unlikely_alternatives(0, ext)) > + return true; > + else > + return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); > } > > #endif > diff --git a/arch/riscv/include/asm/vendor_extensions.h b/arch/riscv/include/asm/vendor_extensions.h > index 0af1ddd0af70..3e676a96016e 100644 > --- a/arch/riscv/include/asm/vendor_extensions.h > +++ b/arch/riscv/include/asm/vendor_extensions.h > @@ -23,4 +23,60 @@ extern const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[]; > > extern const size_t riscv_isa_vendor_ext_list_size; > > +/* > + * The alternatives need some way of distinguishing between vendor extensions > + * and errata. Incrementing all of the vendor extension keys so they are at > + * least 0x8000 accomplishes that. > + */ > +#define RISCV_VENDOR_EXT_ALTERNATIVES_BASE 0x8000 > + > +bool __riscv_isa_vendor_extension_available(int cpu, unsigned long vendor, unsigned int bit); > +#define riscv_cpu_isa_vendor_extension_available(cpu, vendor, ext) \ > + __riscv_isa_vendor_extension_available(cpu, vendor, RISCV_ISA_VENDOR_EXT_##ext) > +#define riscv_isa_vendor_extension_available(vendor, ext) \ > + __riscv_isa_vendor_extension_available(-1, vendor, RISCV_ISA_VENDOR_EXT_##ext) > + > +static __always_inline bool > +riscv_has_vendor_extension_likely(const unsigned long vendor, const unsigned long ext) > +{ > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) > + return __riscv_has_extension_likely_alternatives(vendor, > + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE); > + else > + return __riscv_isa_vendor_extension_available(-1, vendor, ext); > +} > + > +static __always_inline bool > +riscv_has_vendor_extension_unlikely(const unsigned long vendor, const unsigned long ext) > +{ > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) > + return __riscv_has_extension_unlikely_alternatives(vendor, > + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE); > + else > + return __riscv_isa_vendor_extension_available(-1, vendor, ext); > +} > + > +static __always_inline bool riscv_cpu_has_vendor_extension_likely(const unsigned long vendor, > + int cpu, const unsigned long ext) > +{ > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && > + __riscv_has_extension_likely_alternatives(vendor, > + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE)) > + return true; > + else > + return __riscv_isa_vendor_extension_available(cpu, vendor, ext); > +} > + > +static __always_inline bool riscv_cpu_has_vendor_extension_unlikely(const unsigned long vendor, > + int cpu, > + const unsigned long ext) > +{ > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && > + __riscv_has_extension_unlikely_alternatives(vendor, > + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE)) > + return true; > + else > + return __riscv_isa_vendor_extension_available(cpu, vendor, ext); > +} > + > #endif /* _ASM_VENDOR_EXTENSIONS_H */ > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index c9f36822e337..17371887abcc 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -833,25 +833,29 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, > { > struct alt_entry *alt; > void *oldptr, *altptr; > - u16 id, value; > + u16 id, value, vendor; > > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > return; > > for (alt = begin; alt < end; alt++) { > - if (alt->vendor_id != 0) > - continue; > - > id = PATCH_ID_CPUFEATURE_ID(alt->patch_id); > + vendor = PATCH_ID_CPUFEATURE_ID(alt->vendor_id); > > - if (id >= RISCV_ISA_EXT_MAX) { > + if (id < RISCV_ISA_EXT_MAX) { I think any reliance on the standard ext max requires a comment explaining what the interaction is between it and the vendor stuff. > + if (alt->vendor_id != 0) > + continue; If this happens, it's a bug, should we be continuing silently? Cheers, Conor. > + > + if (!__riscv_isa_extension_available(NULL, id)) > + continue; > + } else if (id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) { > + if (!__riscv_isa_vendor_extension_available(-1, vendor, id)) > + continue; > + } else { > WARN(1, "This extension id:%d is not in ISA extension list", id); > continue; > } > > - if (!__riscv_isa_extension_available(NULL, id)) > - continue; > - > value = PATCH_ID_CPUFEATURE_VALUE(alt->patch_id); > if (!riscv_cpufeature_patch_check(id, value)) > continue; > diff --git a/arch/riscv/kernel/vendor_extensions.c b/arch/riscv/kernel/vendor_extensions.c > index f76cb3013c2d..eced93eec5a6 100644 > --- a/arch/riscv/kernel/vendor_extensions.c > +++ b/arch/riscv/kernel/vendor_extensions.c > @@ -3,6 +3,7 @@ > * Copyright 2024 Rivos, Inc > */ > > +#include <asm/vendorid_list.h> > #include <asm/vendor_extensions.h> > #include <asm/vendor_extensions/thead.h> > > @@ -16,3 +17,42 @@ const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[] = { > }; > > const size_t riscv_isa_vendor_ext_list_size = ARRAY_SIZE(riscv_isa_vendor_ext_list); > + > +/** > + * __riscv_isa_vendor_extension_available() - Check whether given vendor > + * extension is available or not. > + * > + * @cpu: check if extension is available on this cpu > + * @vendor: vendor that the extension is a member of > + * @bit: bit position of the desired extension > + * Return: true or false > + * > + * NOTE: When cpu is -1, will check if extension is available on all cpus > + */ > +bool __riscv_isa_vendor_extension_available(int cpu, unsigned long vendor, unsigned int bit) > +{ > + unsigned long *bmap; > + struct riscv_isainfo *cpu_bmap; > + size_t bmap_size; > + > + switch (vendor) { > +#ifdef CONFIG_RISCV_ISA_VENDOR_EXT_THEAD > + case THEAD_VENDOR_ID: > + bmap = riscv_isa_vendor_ext_list_thead.vendor_bitmap; > + cpu_bmap = riscv_isa_vendor_ext_list_thead.per_hart_vendor_bitmap; > + bmap_size = riscv_isa_vendor_ext_list_thead.bitmap_size; > + break; > +#endif > + default: > + return false; > + } > + > + if (cpu != -1) > + bmap = cpu_bmap[cpu].isa; > + > + if (bit >= bmap_size) > + return false; > + > + return test_bit(bit, bmap) ? true : false; > +} > +EXPORT_SYMBOL_GPL(__riscv_isa_vendor_extension_available); > > -- > 2.44.0 >
On Fri, Apr 26, 2024 at 05:19:59PM +0100, Conor Dooley wrote: > On Sat, Apr 20, 2024 at 06:04:40PM -0700, Charlie Jenkins wrote: > > Vendor extensions are maintained in per-vendor structs (separate from > > standard extensions which live in riscv_isa). Create vendor variants for > > the existing extension helpers to interface with the riscv_isa_vendor > > bitmaps. > > > There is a good amount of overlap between these functions, so > > the alternative checking code can be factored out. > > Can you please split this out? Sure. > > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > --- > > arch/riscv/errata/sifive/errata.c | 3 ++ > > arch/riscv/errata/thead/errata.c | 3 ++ > > arch/riscv/include/asm/cpufeature.h | 86 +++++++++++++++++------------- > > arch/riscv/include/asm/vendor_extensions.h | 56 +++++++++++++++++++ > > arch/riscv/kernel/cpufeature.c | 20 ++++--- > > arch/riscv/kernel/vendor_extensions.c | 40 ++++++++++++++ > > 6 files changed, 164 insertions(+), 44 deletions(-) > > > > diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c > > index 3d9a32d791f7..b29b6e405ff2 100644 > > --- a/arch/riscv/errata/sifive/errata.c > > +++ b/arch/riscv/errata/sifive/errata.c > > @@ -12,6 +12,7 @@ > > #include <asm/alternative.h> > > #include <asm/vendorid_list.h> > > #include <asm/errata_list.h> > > +#include <asm/vendor_extensions.h> > > > > struct errata_info_t { > > char name[32]; > > @@ -99,6 +100,8 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, > > for (alt = begin; alt < end; alt++) { > > if (alt->vendor_id != SIFIVE_VENDOR_ID) > > continue; > > + if (alt->patch_id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) > > + continue; > > if (alt->patch_id >= ERRATA_SIFIVE_NUMBER) { > > WARN(1, "This errata id:%d is not in kernel errata list", alt->patch_id); > > continue; > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > index b1c410bbc1ae..d8e78cc687bc 100644 > > --- a/arch/riscv/errata/thead/errata.c > > +++ b/arch/riscv/errata/thead/errata.c > > @@ -18,6 +18,7 @@ > > #include <asm/io.h> > > #include <asm/patch.h> > > #include <asm/vendorid_list.h> > > +#include <asm/vendor_extensions.h> > > > > static bool errata_probe_pbmt(unsigned int stage, > > unsigned long arch_id, unsigned long impid) > > @@ -163,6 +164,8 @@ void thead_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, > > for (alt = begin; alt < end; alt++) { > > if (alt->vendor_id != THEAD_VENDOR_ID) > > continue; > > + if (alt->patch_id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) > > + continue; > > > if (alt->patch_id >= ERRATA_THEAD_NUMBER) > > This number is 2, how does the patching actually work for vendor stuff > when the base is always greater than 2? > Patching is handled through the patcher at the end of cpufeature.c. The vendor_id field is set in the alternatives for errata and now also for vendor extensions. The vendor extension patching is all handled generically. This is distinguished by the patch_id being greater than RISCV_VENDOR_EXT_ALTERNATIVES_BASE, which should leave way more than enough room for errata ids. Since the code already checks if the patch_id is greater than the errata number, I can drop the "if (alt->patch_id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE)" check. > > continue; > > > > diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h > > index db6a6d7d6b2e..83e1143db9ad 100644 > > --- a/arch/riscv/include/asm/cpufeature.h > > +++ b/arch/riscv/include/asm/cpufeature.h > > @@ -103,22 +103,13 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i > > __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext) > > > > static __always_inline bool > > -riscv_has_extension_likely(const unsigned long ext) > > +__riscv_has_extension_likely_alternatives(const unsigned long vendor, const unsigned long ext) > > { > > - compiletime_assert(ext < RISCV_ISA_EXT_MAX, > > - "ext must be < RISCV_ISA_EXT_MAX"); > > - > > - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > > - asm goto( > > - ALTERNATIVE("j %l[l_no]", "nop", 0, %[ext], 1) > > - : > > - : [ext] "i" (ext) > > - : > > - : l_no); > > - } else { > > - if (!__riscv_isa_extension_available(NULL, ext)) > > - goto l_no; > > - } > > + asm goto(ALTERNATIVE("j %l[l_no]", "nop", %[vendor], %[ext], 1) > > + : > > + : [vendor] "i" (vendor), [ext] "i" (ext) > > + : > > + : l_no); > > > > return true; > > l_no: > > @@ -126,42 +117,65 @@ riscv_has_extension_likely(const unsigned long ext) > > } > > > > static __always_inline bool > > -riscv_has_extension_unlikely(const unsigned long ext) > > +__riscv_has_extension_unlikely_alternatives(const unsigned long vendor, const unsigned long ext) > > ngl, I think you could drop the _alternatives from these - the > likely/unlikely is only actually a thing because of the alternatives in > the first place & just retain the __ as a differentiator. That'd help > you with some of the long-line wrangling you've been doing below. > Sounds good. > > { > > - compiletime_assert(ext < RISCV_ISA_EXT_MAX, > > - "ext must be < RISCV_ISA_EXT_MAX"); > > - > > - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > > - asm goto( > > - ALTERNATIVE("nop", "j %l[l_yes]", 0, %[ext], 1) > > - : > > - : [ext] "i" (ext) > > - : > > - : l_yes); > > - } else { > > - if (__riscv_isa_extension_available(NULL, ext)) > > - goto l_yes; > > - } > > + asm goto(ALTERNATIVE("nop", "j %l[l_yes]", %[vendor], %[ext], 1) > > + : > > + : [vendor] "i" (vendor), [ext] "i" (ext) > > + : > > + : l_yes); > > > > return false; > > l_yes: > > return true; > > } > > > > +static __always_inline bool > > +riscv_has_extension_likely(const unsigned long ext) > > Can you format this so that its on one line & wrap the arguments if > needs be? > > > +{ > > + compiletime_assert(ext < RISCV_ISA_EXT_MAX, > > + "ext must be < RISCV_ISA_EXT_MAX"); > > + > > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) > > + return __riscv_has_extension_likely_alternatives(0, ext); > > + else > > I'm almost certain I said this before, but none of the else branches are > needed here, there's a return in the if branch, so the remainder of the > function becomes unconditionally executed. Will fix. > > > + return __riscv_isa_extension_available(NULL, ext); > > +} > > + > > +static __always_inline bool > > +riscv_has_extension_unlikely(const unsigned long ext) > > +{ > > + compiletime_assert(ext < RISCV_ISA_EXT_MAX, > > + "ext must be < RISCV_ISA_EXT_MAX"); > > + > > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) > > + return __riscv_has_extension_unlikely_alternatives(0, ext); > > + else > > + return __riscv_isa_extension_available(NULL, ext); > > +} > > + > > static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext) > > { > > - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext)) > > - return true; > > + compiletime_assert(ext < RISCV_ISA_EXT_MAX, > > + "ext must be < RISCV_ISA_EXT_MAX"); > > > > - return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); > > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && > > + __riscv_has_extension_likely_alternatives(0, ext)) > > 0 is meaningless, please make this more understandable using a define of > some sort. I'll add a define. > > > + return true; > > + else > > + return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); > > } > > > > static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext) > > { > > - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext)) > > - return true; > > + compiletime_assert(ext < RISCV_ISA_EXT_MAX, > > + "ext must be < RISCV_ISA_EXT_MAX"); > > > > - return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); > > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && > > + __riscv_has_extension_unlikely_alternatives(0, ext)) > > + return true; > > + else > > + return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); > > } > > > > #endif > > diff --git a/arch/riscv/include/asm/vendor_extensions.h b/arch/riscv/include/asm/vendor_extensions.h > > index 0af1ddd0af70..3e676a96016e 100644 > > --- a/arch/riscv/include/asm/vendor_extensions.h > > +++ b/arch/riscv/include/asm/vendor_extensions.h > > @@ -23,4 +23,60 @@ extern const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[]; > > > > extern const size_t riscv_isa_vendor_ext_list_size; > > > > +/* > > + * The alternatives need some way of distinguishing between vendor extensions > > + * and errata. Incrementing all of the vendor extension keys so they are at > > + * least 0x8000 accomplishes that. > > + */ > > +#define RISCV_VENDOR_EXT_ALTERNATIVES_BASE 0x8000 > > + > > +bool __riscv_isa_vendor_extension_available(int cpu, unsigned long vendor, unsigned int bit); > > +#define riscv_cpu_isa_vendor_extension_available(cpu, vendor, ext) \ > > + __riscv_isa_vendor_extension_available(cpu, vendor, RISCV_ISA_VENDOR_EXT_##ext) > > +#define riscv_isa_vendor_extension_available(vendor, ext) \ > > + __riscv_isa_vendor_extension_available(-1, vendor, RISCV_ISA_VENDOR_EXT_##ext) > > + > > +static __always_inline bool > > +riscv_has_vendor_extension_likely(const unsigned long vendor, const unsigned long ext) > > +{ > > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) > > + return __riscv_has_extension_likely_alternatives(vendor, > > + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE); > > + else > > + return __riscv_isa_vendor_extension_available(-1, vendor, ext); > > +} > > + > > +static __always_inline bool > > +riscv_has_vendor_extension_unlikely(const unsigned long vendor, const unsigned long ext) > > +{ > > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) > > + return __riscv_has_extension_unlikely_alternatives(vendor, > > + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE); > > + else > > + return __riscv_isa_vendor_extension_available(-1, vendor, ext); > > +} > > + > > +static __always_inline bool riscv_cpu_has_vendor_extension_likely(const unsigned long vendor, > > + int cpu, const unsigned long ext) > > +{ > > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && > > + __riscv_has_extension_likely_alternatives(vendor, > > + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE)) > > + return true; > > + else > > + return __riscv_isa_vendor_extension_available(cpu, vendor, ext); > > +} > > + > > +static __always_inline bool riscv_cpu_has_vendor_extension_unlikely(const unsigned long vendor, > > + int cpu, > > + const unsigned long ext) > > +{ > > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && > > + __riscv_has_extension_unlikely_alternatives(vendor, > > + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE)) > > + return true; > > + else > > + return __riscv_isa_vendor_extension_available(cpu, vendor, ext); > > +} > > + > > #endif /* _ASM_VENDOR_EXTENSIONS_H */ > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index c9f36822e337..17371887abcc 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -833,25 +833,29 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, > > { > > struct alt_entry *alt; > > void *oldptr, *altptr; > > - u16 id, value; > > + u16 id, value, vendor; > > > > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > return; > > > > for (alt = begin; alt < end; alt++) { > > - if (alt->vendor_id != 0) > > - continue; > > - > > id = PATCH_ID_CPUFEATURE_ID(alt->patch_id); > > + vendor = PATCH_ID_CPUFEATURE_ID(alt->vendor_id); > > > > - if (id >= RISCV_ISA_EXT_MAX) { > > + if (id < RISCV_ISA_EXT_MAX) { > > I think any reliance on the standard ext max requires a comment > explaining what the interaction is between it and the vendor stuff. > I'll add a comment describing that anything with an id below RISCV_ISA_EXT_MAX is interpreted as a standard extension and anything with an id above RISCV_VENDOR_EXT_ALTERNATIVES_BASE will be interpreted as a vendor extension. > > + if (alt->vendor_id != 0) > > + continue; > > If this happens, it's a bug, should we be continuing silently? > This indicates that the patch should be treated as errata and that's why it silently continues. I can add a comment that says that. - Charlie > Cheers, > Conor. > > > + > > + if (!__riscv_isa_extension_available(NULL, id)) > > + continue; > > + } else if (id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) { > > + if (!__riscv_isa_vendor_extension_available(-1, vendor, id)) > > + continue; > > + } else { > > WARN(1, "This extension id:%d is not in ISA extension list", id); > > continue; > > } > > > > - if (!__riscv_isa_extension_available(NULL, id)) > > - continue; > > - > > value = PATCH_ID_CPUFEATURE_VALUE(alt->patch_id); > > if (!riscv_cpufeature_patch_check(id, value)) > > continue; > > diff --git a/arch/riscv/kernel/vendor_extensions.c b/arch/riscv/kernel/vendor_extensions.c > > index f76cb3013c2d..eced93eec5a6 100644 > > --- a/arch/riscv/kernel/vendor_extensions.c > > +++ b/arch/riscv/kernel/vendor_extensions.c > > @@ -3,6 +3,7 @@ > > * Copyright 2024 Rivos, Inc > > */ > > > > +#include <asm/vendorid_list.h> > > #include <asm/vendor_extensions.h> > > #include <asm/vendor_extensions/thead.h> > > > > @@ -16,3 +17,42 @@ const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[] = { > > }; > > > > const size_t riscv_isa_vendor_ext_list_size = ARRAY_SIZE(riscv_isa_vendor_ext_list); > > + > > +/** > > + * __riscv_isa_vendor_extension_available() - Check whether given vendor > > + * extension is available or not. > > + * > > + * @cpu: check if extension is available on this cpu > > + * @vendor: vendor that the extension is a member of > > + * @bit: bit position of the desired extension > > + * Return: true or false > > + * > > + * NOTE: When cpu is -1, will check if extension is available on all cpus > > + */ > > +bool __riscv_isa_vendor_extension_available(int cpu, unsigned long vendor, unsigned int bit) > > +{ > > + unsigned long *bmap; > > + struct riscv_isainfo *cpu_bmap; > > + size_t bmap_size; > > + > > + switch (vendor) { > > +#ifdef CONFIG_RISCV_ISA_VENDOR_EXT_THEAD > > + case THEAD_VENDOR_ID: > > + bmap = riscv_isa_vendor_ext_list_thead.vendor_bitmap; > > + cpu_bmap = riscv_isa_vendor_ext_list_thead.per_hart_vendor_bitmap; > > + bmap_size = riscv_isa_vendor_ext_list_thead.bitmap_size; > > + break; > > +#endif > > + default: > > + return false; > > + } > > + > > + if (cpu != -1) > > + bmap = cpu_bmap[cpu].isa; > > + > > + if (bit >= bmap_size) > > + return false; > > + > > + return test_bit(bit, bmap) ? true : false; > > +} > > +EXPORT_SYMBOL_GPL(__riscv_isa_vendor_extension_available); > > > > -- > > 2.44.0 > >
On Fri, Apr 26, 2024 at 01:01:10PM -0700, Charlie Jenkins wrote: > On Fri, Apr 26, 2024 at 05:19:59PM +0100, Conor Dooley wrote: > > On Sat, Apr 20, 2024 at 06:04:40PM -0700, Charlie Jenkins wrote: > > > @@ -163,6 +164,8 @@ void thead_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, > > > for (alt = begin; alt < end; alt++) { > > > if (alt->vendor_id != THEAD_VENDOR_ID) > > > continue; > > > + if (alt->patch_id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) > > > + continue; > > > > > if (alt->patch_id >= ERRATA_THEAD_NUMBER) > > > > This number is 2, how does the patching actually work for vendor stuff > > when the base is always greater than 2? > > > > Patching is handled through the patcher at the end of cpufeature.c. The > vendor_id field is set in the alternatives for errata and now also for > vendor extensions. The vendor extension patching is all handled > generically. > > This is distinguished by the patch_id being greater than > RISCV_VENDOR_EXT_ALTERNATIVES_BASE, which should leave way more than > enough room for errata ids. Since the code already checks if the > patch_id is greater than the errata number, I can drop the > "if (alt->patch_id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE)" check. Ah, ye, I was being dumb - I was somehow thinking that you were using the errata patch function to patch this, which obviously wouldn't be a good design. If that had been the case, you'd never patch anything, but you patch the extensions using the cpufeature patch function so it all works out. If anything, I guess there could be an assert here somewhere that enforces ERRATA_VENDOR_NUMBER < VENDOR_EXT_BASE.
diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c index 3d9a32d791f7..b29b6e405ff2 100644 --- a/arch/riscv/errata/sifive/errata.c +++ b/arch/riscv/errata/sifive/errata.c @@ -12,6 +12,7 @@ #include <asm/alternative.h> #include <asm/vendorid_list.h> #include <asm/errata_list.h> +#include <asm/vendor_extensions.h> struct errata_info_t { char name[32]; @@ -99,6 +100,8 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, for (alt = begin; alt < end; alt++) { if (alt->vendor_id != SIFIVE_VENDOR_ID) continue; + if (alt->patch_id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) + continue; if (alt->patch_id >= ERRATA_SIFIVE_NUMBER) { WARN(1, "This errata id:%d is not in kernel errata list", alt->patch_id); continue; diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c index b1c410bbc1ae..d8e78cc687bc 100644 --- a/arch/riscv/errata/thead/errata.c +++ b/arch/riscv/errata/thead/errata.c @@ -18,6 +18,7 @@ #include <asm/io.h> #include <asm/patch.h> #include <asm/vendorid_list.h> +#include <asm/vendor_extensions.h> static bool errata_probe_pbmt(unsigned int stage, unsigned long arch_id, unsigned long impid) @@ -163,6 +164,8 @@ void thead_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, for (alt = begin; alt < end; alt++) { if (alt->vendor_id != THEAD_VENDOR_ID) continue; + if (alt->patch_id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) + continue; if (alt->patch_id >= ERRATA_THEAD_NUMBER) continue; diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h index db6a6d7d6b2e..83e1143db9ad 100644 --- a/arch/riscv/include/asm/cpufeature.h +++ b/arch/riscv/include/asm/cpufeature.h @@ -103,22 +103,13 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext) static __always_inline bool -riscv_has_extension_likely(const unsigned long ext) +__riscv_has_extension_likely_alternatives(const unsigned long vendor, const unsigned long ext) { - compiletime_assert(ext < RISCV_ISA_EXT_MAX, - "ext must be < RISCV_ISA_EXT_MAX"); - - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { - asm goto( - ALTERNATIVE("j %l[l_no]", "nop", 0, %[ext], 1) - : - : [ext] "i" (ext) - : - : l_no); - } else { - if (!__riscv_isa_extension_available(NULL, ext)) - goto l_no; - } + asm goto(ALTERNATIVE("j %l[l_no]", "nop", %[vendor], %[ext], 1) + : + : [vendor] "i" (vendor), [ext] "i" (ext) + : + : l_no); return true; l_no: @@ -126,42 +117,65 @@ riscv_has_extension_likely(const unsigned long ext) } static __always_inline bool -riscv_has_extension_unlikely(const unsigned long ext) +__riscv_has_extension_unlikely_alternatives(const unsigned long vendor, const unsigned long ext) { - compiletime_assert(ext < RISCV_ISA_EXT_MAX, - "ext must be < RISCV_ISA_EXT_MAX"); - - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { - asm goto( - ALTERNATIVE("nop", "j %l[l_yes]", 0, %[ext], 1) - : - : [ext] "i" (ext) - : - : l_yes); - } else { - if (__riscv_isa_extension_available(NULL, ext)) - goto l_yes; - } + asm goto(ALTERNATIVE("nop", "j %l[l_yes]", %[vendor], %[ext], 1) + : + : [vendor] "i" (vendor), [ext] "i" (ext) + : + : l_yes); return false; l_yes: return true; } +static __always_inline bool +riscv_has_extension_likely(const unsigned long ext) +{ + compiletime_assert(ext < RISCV_ISA_EXT_MAX, + "ext must be < RISCV_ISA_EXT_MAX"); + + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) + return __riscv_has_extension_likely_alternatives(0, ext); + else + return __riscv_isa_extension_available(NULL, ext); +} + +static __always_inline bool +riscv_has_extension_unlikely(const unsigned long ext) +{ + compiletime_assert(ext < RISCV_ISA_EXT_MAX, + "ext must be < RISCV_ISA_EXT_MAX"); + + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) + return __riscv_has_extension_unlikely_alternatives(0, ext); + else + return __riscv_isa_extension_available(NULL, ext); +} + static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext) { - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext)) - return true; + compiletime_assert(ext < RISCV_ISA_EXT_MAX, + "ext must be < RISCV_ISA_EXT_MAX"); - return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && + __riscv_has_extension_likely_alternatives(0, ext)) + return true; + else + return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); } static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext) { - if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext)) - return true; + compiletime_assert(ext < RISCV_ISA_EXT_MAX, + "ext must be < RISCV_ISA_EXT_MAX"); - return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && + __riscv_has_extension_unlikely_alternatives(0, ext)) + return true; + else + return __riscv_isa_extension_available(hart_isa[cpu].isa, ext); } #endif diff --git a/arch/riscv/include/asm/vendor_extensions.h b/arch/riscv/include/asm/vendor_extensions.h index 0af1ddd0af70..3e676a96016e 100644 --- a/arch/riscv/include/asm/vendor_extensions.h +++ b/arch/riscv/include/asm/vendor_extensions.h @@ -23,4 +23,60 @@ extern const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[]; extern const size_t riscv_isa_vendor_ext_list_size; +/* + * The alternatives need some way of distinguishing between vendor extensions + * and errata. Incrementing all of the vendor extension keys so they are at + * least 0x8000 accomplishes that. + */ +#define RISCV_VENDOR_EXT_ALTERNATIVES_BASE 0x8000 + +bool __riscv_isa_vendor_extension_available(int cpu, unsigned long vendor, unsigned int bit); +#define riscv_cpu_isa_vendor_extension_available(cpu, vendor, ext) \ + __riscv_isa_vendor_extension_available(cpu, vendor, RISCV_ISA_VENDOR_EXT_##ext) +#define riscv_isa_vendor_extension_available(vendor, ext) \ + __riscv_isa_vendor_extension_available(-1, vendor, RISCV_ISA_VENDOR_EXT_##ext) + +static __always_inline bool +riscv_has_vendor_extension_likely(const unsigned long vendor, const unsigned long ext) +{ + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) + return __riscv_has_extension_likely_alternatives(vendor, + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE); + else + return __riscv_isa_vendor_extension_available(-1, vendor, ext); +} + +static __always_inline bool +riscv_has_vendor_extension_unlikely(const unsigned long vendor, const unsigned long ext) +{ + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) + return __riscv_has_extension_unlikely_alternatives(vendor, + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE); + else + return __riscv_isa_vendor_extension_available(-1, vendor, ext); +} + +static __always_inline bool riscv_cpu_has_vendor_extension_likely(const unsigned long vendor, + int cpu, const unsigned long ext) +{ + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && + __riscv_has_extension_likely_alternatives(vendor, + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE)) + return true; + else + return __riscv_isa_vendor_extension_available(cpu, vendor, ext); +} + +static __always_inline bool riscv_cpu_has_vendor_extension_unlikely(const unsigned long vendor, + int cpu, + const unsigned long ext) +{ + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && + __riscv_has_extension_unlikely_alternatives(vendor, + ext + RISCV_VENDOR_EXT_ALTERNATIVES_BASE)) + return true; + else + return __riscv_isa_vendor_extension_available(cpu, vendor, ext); +} + #endif /* _ASM_VENDOR_EXTENSIONS_H */ diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index c9f36822e337..17371887abcc 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -833,25 +833,29 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, { struct alt_entry *alt; void *oldptr, *altptr; - u16 id, value; + u16 id, value, vendor; if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) return; for (alt = begin; alt < end; alt++) { - if (alt->vendor_id != 0) - continue; - id = PATCH_ID_CPUFEATURE_ID(alt->patch_id); + vendor = PATCH_ID_CPUFEATURE_ID(alt->vendor_id); - if (id >= RISCV_ISA_EXT_MAX) { + if (id < RISCV_ISA_EXT_MAX) { + if (alt->vendor_id != 0) + continue; + + if (!__riscv_isa_extension_available(NULL, id)) + continue; + } else if (id >= RISCV_VENDOR_EXT_ALTERNATIVES_BASE) { + if (!__riscv_isa_vendor_extension_available(-1, vendor, id)) + continue; + } else { WARN(1, "This extension id:%d is not in ISA extension list", id); continue; } - if (!__riscv_isa_extension_available(NULL, id)) - continue; - value = PATCH_ID_CPUFEATURE_VALUE(alt->patch_id); if (!riscv_cpufeature_patch_check(id, value)) continue; diff --git a/arch/riscv/kernel/vendor_extensions.c b/arch/riscv/kernel/vendor_extensions.c index f76cb3013c2d..eced93eec5a6 100644 --- a/arch/riscv/kernel/vendor_extensions.c +++ b/arch/riscv/kernel/vendor_extensions.c @@ -3,6 +3,7 @@ * Copyright 2024 Rivos, Inc */ +#include <asm/vendorid_list.h> #include <asm/vendor_extensions.h> #include <asm/vendor_extensions/thead.h> @@ -16,3 +17,42 @@ const struct riscv_isa_vendor_ext_data_list *riscv_isa_vendor_ext_list[] = { }; const size_t riscv_isa_vendor_ext_list_size = ARRAY_SIZE(riscv_isa_vendor_ext_list); + +/** + * __riscv_isa_vendor_extension_available() - Check whether given vendor + * extension is available or not. + * + * @cpu: check if extension is available on this cpu + * @vendor: vendor that the extension is a member of + * @bit: bit position of the desired extension + * Return: true or false + * + * NOTE: When cpu is -1, will check if extension is available on all cpus + */ +bool __riscv_isa_vendor_extension_available(int cpu, unsigned long vendor, unsigned int bit) +{ + unsigned long *bmap; + struct riscv_isainfo *cpu_bmap; + size_t bmap_size; + + switch (vendor) { +#ifdef CONFIG_RISCV_ISA_VENDOR_EXT_THEAD + case THEAD_VENDOR_ID: + bmap = riscv_isa_vendor_ext_list_thead.vendor_bitmap; + cpu_bmap = riscv_isa_vendor_ext_list_thead.per_hart_vendor_bitmap; + bmap_size = riscv_isa_vendor_ext_list_thead.bitmap_size; + break; +#endif + default: + return false; + } + + if (cpu != -1) + bmap = cpu_bmap[cpu].isa; + + if (bit >= bmap_size) + return false; + + return test_bit(bit, bmap) ? true : false; +} +EXPORT_SYMBOL_GPL(__riscv_isa_vendor_extension_available);
Vendor extensions are maintained in per-vendor structs (separate from standard extensions which live in riscv_isa). Create vendor variants for the existing extension helpers to interface with the riscv_isa_vendor bitmaps. There is a good amount of overlap between these functions, so the alternative checking code can be factored out. Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> --- arch/riscv/errata/sifive/errata.c | 3 ++ arch/riscv/errata/thead/errata.c | 3 ++ arch/riscv/include/asm/cpufeature.h | 86 +++++++++++++++++------------- arch/riscv/include/asm/vendor_extensions.h | 56 +++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 20 ++++--- arch/riscv/kernel/vendor_extensions.c | 40 ++++++++++++++ 6 files changed, 164 insertions(+), 44 deletions(-)