| Message ID | 20230424194911.264850-5-heiko.stuebner@vrull.eu (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Expose the isa-string via the AT_BASE_PLATFORM aux vector | expand |
On Mon, Apr 24, 2023 at 09:49:11PM +0200, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > T-Head cores support a number of own ISA extensions that also include > optimized instructions which could benefit userspace to improve > performance. > > Extensions supported by current T-Head cores are: > * XTheadBa - bitmanipulation instructions for address calculation > * XTheadBb - conditional basic bit-manipulation instructions > * XTheadBs - instructions to access a single bit in a register > * XTheadCmo - cache management operations > * XTheadCondMov - conditional move instructions > * XTheadFMemIdx - indexed memory operations for floating-point registers > * XTheadFmv - double-precision floating-point high-bit data transmission > intructions for RV32 > * XTheadInt - instructions to reduce the code size of ISRs and/or the > interrupt latencies that are caused by ISR entry/exit code > * XTheadMac - multiply-accumulate instructions > * XTheadMemIdx - indexed memory operations for GP registers > * XTheadMemPair - two-GPR memory operations > * XTheadSync - multi-core synchronization instructions > > In-depth descriptions of these extensions can be found on > https://github.com/T-head-Semi/thead-extension-spec > > Support for those extensions was merged into the relevant toolchains > so userspace programs can select necessary optimizations when needed. > > So a mechanism to the isa-string generation to export vendor-extension > lists via the errata mechanism and implement it for T-Head C9xx cores. > > This exposes these vendor extensions then both in AT_BASE_PLATFORM > and /proc/cpuinfo. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > --- > arch/riscv/errata/thead/errata.c | 43 ++++++++++++++++++++++++++++ > arch/riscv/include/asm/alternative.h | 4 +++ > arch/riscv/kernel/alternative.c | 21 ++++++++++++++ > arch/riscv/kernel/cpu.c | 12 ++++++++ > 4 files changed, 80 insertions(+) > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > index 1036b8f933ec..eb635bf80737 100644 > --- a/arch/riscv/errata/thead/errata.c > +++ b/arch/riscv/errata/thead/errata.c > @@ -15,6 +15,7 @@ > #include <asm/errata_list.h> > #include <asm/hwprobe.h> > #include <asm/patch.h> > +#include <asm/switch_to.h> > #include <asm/vendorid_list.h> > > static bool errata_probe_pbmt(unsigned int stage, > @@ -125,3 +126,45 @@ void __init_or_module thead_feature_probe_func(unsigned int cpu, > if ((archid == 0) && (impid == 0)) > per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST; > } > + > + > +char *thead_extension_list_func(unsigned long archid, > + unsigned long impid) > +{ > + if ((archid == 0) && (impid == 0)) { > + const char *xbase1 = "xtheadba_xtheadbb_xtheadbs_xtheadcmo_xtheadcondmov"; > + const char *xbase2 = "_xtheadint_xtheadmac_xtheadmemidx_xtheadmempair_xtheadsync"; > + const char *xfpu = "_xtheadfmemIdx"; > +#ifdef CONFIG_32BIT > + const char *xfpu32 = "_xtheadfmv"; > +#endif > + int len = strlen(xbase1) + strlen(xbase2); > + char *str; > + > + if (has_fpu()) { > + len += strlen(xfpu); > +#ifdef CONFIG_32BIT > + len+= strlen(xfpu32); > +#endif > + } > + > + str = kzalloc(len, GFP_KERNEL); > + if (!str) > + return str; > + > + strcpy(str, xbase1); > + > + if (has_fpu()) { > + strcat(str, xfpu); > +#ifdef CONFIG_32BIT > + strcat(str, xfpu32); > +#endif > + } > + > + strcat(str, xbase2); > + > + return str; > + } > + > + return NULL; > +} > diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h > index a8f5cf6694a1..8c9aec196649 100644 > --- a/arch/riscv/include/asm/alternative.h > +++ b/arch/riscv/include/asm/alternative.h > @@ -31,6 +31,7 @@ > #define ALT_ALT_PTR(a) __ALT_PTR(a, alt_offset) > > void __init probe_vendor_features(unsigned int cpu); > +char *list_vendor_extensions(void); > void __init apply_boot_alternatives(void); > void __init apply_early_boot_alternatives(void); > void apply_module_alternatives(void *start, size_t length); > @@ -55,6 +56,8 @@ void thead_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, > > void thead_feature_probe_func(unsigned int cpu, unsigned long archid, > unsigned long impid); > +char *thead_extension_list_func(unsigned long archid, > + unsigned long impid); > > void riscv_cpufeature_patch_func(struct alt_entry *begin, struct alt_entry *end, > unsigned int stage); > @@ -62,6 +65,7 @@ void riscv_cpufeature_patch_func(struct alt_entry *begin, struct alt_entry *end, > #else /* CONFIG_RISCV_ALTERNATIVE */ > > static inline void probe_vendor_features(unsigned int cpu) { } > +static inline char *list_vendor_extensions(void) { return NULL; } > static inline void apply_boot_alternatives(void) { } > static inline void apply_early_boot_alternatives(void) { } > static inline void apply_module_alternatives(void *start, size_t length) { } > diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c > index fc65c9293ac5..18913fd1809f 100644 > --- a/arch/riscv/kernel/alternative.c > +++ b/arch/riscv/kernel/alternative.c > @@ -29,6 +29,8 @@ struct cpu_manufacturer_info_t { > unsigned int stage); > void (*feature_probe_func)(unsigned int cpu, unsigned long archid, > unsigned long impid); > + char *(*extension_list_func)(unsigned long archid, > + unsigned long impid); > }; > > static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_info_t *cpu_mfr_info) > @@ -54,6 +56,7 @@ static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_inf > case THEAD_VENDOR_ID: > cpu_mfr_info->patch_func = thead_errata_patch_func; > cpu_mfr_info->feature_probe_func = thead_feature_probe_func; > + cpu_mfr_info->extension_list_func = thead_extension_list_func; > break; > #endif > default: > @@ -157,6 +160,24 @@ void __init_or_module probe_vendor_features(unsigned int cpu) > cpu_mfr_info.imp_id); > } > > +/* > + * Lists the vendor-specific extensions common to all cores. > + * Returns a new underscore "_" concatenated string that the > + * caller is supposed to free after use. > + */ > +char *list_vendor_extensions(void) > +{ > + struct cpu_manufacturer_info_t cpu_mfr_info; > + > + riscv_fill_cpu_mfr_info(&cpu_mfr_info); > + if (!cpu_mfr_info.extension_list_func) > + return NULL; > + > + return cpu_mfr_info.extension_list_func(cpu_mfr_info.arch_id, > + cpu_mfr_info.imp_id); > + > +} > + > /* > * This is called very early in the boot process (directly after we run > * a feature detect on the boot CPU). No need to worry about other CPUs > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c > index 71770563199f..6a0a45b2eb20 100644 > --- a/arch/riscv/kernel/cpu.c > +++ b/arch/riscv/kernel/cpu.c > @@ -7,6 +7,7 @@ > #include <linux/init.h> > #include <linux/seq_file.h> > #include <linux/of.h> > +#include <asm/alternative.h> > #include <asm/cpufeature.h> > #include <asm/csr.h> > #include <asm/hwcap.h> > @@ -260,6 +261,7 @@ static char *riscv_create_isa_string(void) > { > int maxlen = 4; > char *isa_str; > + char *vendor_isa; > int i; > > /* calculate the needed string length */ > @@ -268,6 +270,10 @@ static char *riscv_create_isa_string(void) > maxlen++; > maxlen += strlen_isa_ext(); > > + vendor_isa = list_vendor_extensions(); > + if (vendor_isa) > + maxlen += strlen(vendor_isa) + 1; > + > isa_str = kzalloc(maxlen, GFP_KERNEL); > if (!isa_str) > return ERR_PTR(-ENOMEM); > @@ -287,6 +293,12 @@ static char *riscv_create_isa_string(void) > > strcat_isa_ext(isa_str); > > + if(vendor_isa) { ^ need a space > + strcat(isa_str, "_"); > + strcat(isa_str, vendor_isa); > + kfree(vendor_isa); > + } > + > return isa_str; > } > > -- > 2.39.0 > For the extension of riscv_create_isa_string to support vendor lists, Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Hey Heiko, On Mon, Apr 24, 2023 at 09:49:11PM +0200, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > T-Head cores support a number of own ISA extensions that also include > optimized instructions which could benefit userspace to improve > performance. > > Extensions supported by current T-Head cores are: > * XTheadBa - bitmanipulation instructions for address calculation > * XTheadBb - conditional basic bit-manipulation instructions > * XTheadBs - instructions to access a single bit in a register > * XTheadCmo - cache management operations > * XTheadCondMov - conditional move instructions > * XTheadFMemIdx - indexed memory operations for floating-point registers > * XTheadFmv - double-precision floating-point high-bit data transmission > intructions for RV32 > * XTheadInt - instructions to reduce the code size of ISRs and/or the > interrupt latencies that are caused by ISR entry/exit code > * XTheadMac - multiply-accumulate instructions > * XTheadMemIdx - indexed memory operations for GP registers > * XTheadMemPair - two-GPR memory operations > * XTheadSync - multi-core synchronization instructions > > In-depth descriptions of these extensions can be found on > https://github.com/T-head-Semi/thead-extension-spec > > Support for those extensions was merged into the relevant toolchains > so userspace programs can select necessary optimizations when needed. > > So a mechanism to the isa-string generation to export vendor-extension > lists via the errata mechanism and implement it for T-Head C9xx cores. > > This exposes these vendor extensions then both in AT_BASE_PLATFORM > and /proc/cpuinfo. I'm not entirely sure if this patch is meant to be a demo, but I don't like the idea of using these registers to determine what extensions are reported. riscv,isa in a devicetree (for as much as I might dislike it at this point in time), or the ACPI equivalent, should be the mechanism for enabling/disabling these kinds of things. Otherwise, we are just going to end up causing problems for ourselves with various lists of this that and the other extension for different combinations of hardware. The open source c906 has the same archid/impid too right? Assuming this is a serious proposal, how would you intend dealing with modified versions of those cores? I am pretty sure that you intended this to be a demo though, particularly given the wording of the below quote from your cover, but in case you did not: NAK to this way of sourcing the information. Anyways, since your cover's considerations section seems partly aimed at me, given my discussion with head-the-ball last week: > Things to still consider: > ------------------------- > Right now both hwprobe and this approach will only pass through > extensions the kernel actually knows about itself. This should not > necessarily be needed (but could be an optional feature for e.g. virtualization). What do you mean by virtualisation here? It's the job of the hypervisor etc to make sure that what it passes to its guest contains only what it wants the guest to see, right? IIUC, that's another point against doing what this patch does. > Most extensions don’t introduce new user-mode state that the kernel needs to > manage (e.g. new registers). Extension that do introduce new user-mode state > are usually disabled by default and have to be enabled by S mode or M mode > (e.g. FS[1:0] for the +floating-point extension). So there should not be a > reason to filter any extensions that are unknown. I think in general this can be safely assumed, but I don't think it is unreasonable to expect someone may make, for example, XConorGigaVector that gets turned on by the same bits as regular old vector but has some extra registers. Not saying that I think that that is a good idea, but it is a distinct possibility that this will happen, and I don't think forwarding it to userspace is a good idea. > So it might make more sense to just pass through a curated list (common > set) created from the core's isa strings and remove state-handling > extensions when they are not enabled in the kernel-side (sort of > blacklisting extensions that need actual kernel support). Yeah, as discussed with Christoph the other day I don't think we can really avoid such a blacklist. I don't think it'd require any sort of vendor specific handling, since, as you point out, a vendor may well implement extensions that were created by other companies. It's easy, right?? "Just" parse the dt, tokenise the extensions & delete whatever is in the blacklist, right? Hyperbole aside, I think that doing something like this increases the need for a system like Evan's, as userspace may need a way to differentiate between what the hardware is capable of (as reported by the isa string in /proc/cpuinfo or the content of 3/4) and what the kernel actually supports. > However, this is a very related, but still independent discussion. Aye, this discussion and the first two patches are relevant whether 3/4 is accepted or not IMO. Cheers, Conor. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > --- > arch/riscv/errata/thead/errata.c | 43 ++++++++++++++++++++++++++++ > arch/riscv/include/asm/alternative.h | 4 +++ > arch/riscv/kernel/alternative.c | 21 ++++++++++++++ > arch/riscv/kernel/cpu.c | 12 ++++++++ > 4 files changed, 80 insertions(+) > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > index 1036b8f933ec..eb635bf80737 100644 > --- a/arch/riscv/errata/thead/errata.c > +++ b/arch/riscv/errata/thead/errata.c > @@ -15,6 +15,7 @@ > #include <asm/errata_list.h> > #include <asm/hwprobe.h> > #include <asm/patch.h> > +#include <asm/switch_to.h> > #include <asm/vendorid_list.h> > > static bool errata_probe_pbmt(unsigned int stage, > @@ -125,3 +126,45 @@ void __init_or_module thead_feature_probe_func(unsigned int cpu, > if ((archid == 0) && (impid == 0)) > per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST; > } > + > + > +char *thead_extension_list_func(unsigned long archid, > + unsigned long impid) > +{ > + if ((archid == 0) && (impid == 0)) { > + const char *xbase1 = "xtheadba_xtheadbb_xtheadbs_xtheadcmo_xtheadcondmov"; > + const char *xbase2 = "_xtheadint_xtheadmac_xtheadmemidx_xtheadmempair_xtheadsync"; > + const char *xfpu = "_xtheadfmemIdx"; > +#ifdef CONFIG_32BIT > + const char *xfpu32 = "_xtheadfmv"; > +#endif > + int len = strlen(xbase1) + strlen(xbase2); > + char *str; > + > + if (has_fpu()) { > + len += strlen(xfpu); > +#ifdef CONFIG_32BIT > + len+= strlen(xfpu32); > +#endif > + } > + > + str = kzalloc(len, GFP_KERNEL); > + if (!str) > + return str; > + > + strcpy(str, xbase1); > + > + if (has_fpu()) { > + strcat(str, xfpu); > +#ifdef CONFIG_32BIT > + strcat(str, xfpu32); > +#endif > + } > + > + strcat(str, xbase2); > + > + return str; > + } > + > + return NULL; > +} > diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h > index a8f5cf6694a1..8c9aec196649 100644 > --- a/arch/riscv/include/asm/alternative.h > +++ b/arch/riscv/include/asm/alternative.h > @@ -31,6 +31,7 @@ > #define ALT_ALT_PTR(a) __ALT_PTR(a, alt_offset) > > void __init probe_vendor_features(unsigned int cpu); > +char *list_vendor_extensions(void); > void __init apply_boot_alternatives(void); > void __init apply_early_boot_alternatives(void); > void apply_module_alternatives(void *start, size_t length); > @@ -55,6 +56,8 @@ void thead_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, > > void thead_feature_probe_func(unsigned int cpu, unsigned long archid, > unsigned long impid); > +char *thead_extension_list_func(unsigned long archid, > + unsigned long impid); > > void riscv_cpufeature_patch_func(struct alt_entry *begin, struct alt_entry *end, > unsigned int stage); > @@ -62,6 +65,7 @@ void riscv_cpufeature_patch_func(struct alt_entry *begin, struct alt_entry *end, > #else /* CONFIG_RISCV_ALTERNATIVE */ > > static inline void probe_vendor_features(unsigned int cpu) { } > +static inline char *list_vendor_extensions(void) { return NULL; } > static inline void apply_boot_alternatives(void) { } > static inline void apply_early_boot_alternatives(void) { } > static inline void apply_module_alternatives(void *start, size_t length) { } > diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c > index fc65c9293ac5..18913fd1809f 100644 > --- a/arch/riscv/kernel/alternative.c > +++ b/arch/riscv/kernel/alternative.c > @@ -29,6 +29,8 @@ struct cpu_manufacturer_info_t { > unsigned int stage); > void (*feature_probe_func)(unsigned int cpu, unsigned long archid, > unsigned long impid); > + char *(*extension_list_func)(unsigned long archid, > + unsigned long impid); > }; > > static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_info_t *cpu_mfr_info) > @@ -54,6 +56,7 @@ static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_inf > case THEAD_VENDOR_ID: > cpu_mfr_info->patch_func = thead_errata_patch_func; > cpu_mfr_info->feature_probe_func = thead_feature_probe_func; > + cpu_mfr_info->extension_list_func = thead_extension_list_func; > break; > #endif > default: > @@ -157,6 +160,24 @@ void __init_or_module probe_vendor_features(unsigned int cpu) > cpu_mfr_info.imp_id); > } > > +/* > + * Lists the vendor-specific extensions common to all cores. > + * Returns a new underscore "_" concatenated string that the > + * caller is supposed to free after use. > + */ > +char *list_vendor_extensions(void) > +{ > + struct cpu_manufacturer_info_t cpu_mfr_info; > + > + riscv_fill_cpu_mfr_info(&cpu_mfr_info); > + if (!cpu_mfr_info.extension_list_func) > + return NULL; > + > + return cpu_mfr_info.extension_list_func(cpu_mfr_info.arch_id, > + cpu_mfr_info.imp_id); > + > +} > + > /* > * This is called very early in the boot process (directly after we run > * a feature detect on the boot CPU). No need to worry about other CPUs > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c > index 71770563199f..6a0a45b2eb20 100644 > --- a/arch/riscv/kernel/cpu.c > +++ b/arch/riscv/kernel/cpu.c > @@ -7,6 +7,7 @@ > #include <linux/init.h> > #include <linux/seq_file.h> > #include <linux/of.h> > +#include <asm/alternative.h> > #include <asm/cpufeature.h> > #include <asm/csr.h> > #include <asm/hwcap.h> > @@ -260,6 +261,7 @@ static char *riscv_create_isa_string(void) > { > int maxlen = 4; > char *isa_str; > + char *vendor_isa; > int i; > > /* calculate the needed string length */ > @@ -268,6 +270,10 @@ static char *riscv_create_isa_string(void) > maxlen++; > maxlen += strlen_isa_ext(); > > + vendor_isa = list_vendor_extensions(); > + if (vendor_isa) > + maxlen += strlen(vendor_isa) + 1; > + > isa_str = kzalloc(maxlen, GFP_KERNEL); > if (!isa_str) > return ERR_PTR(-ENOMEM); > @@ -287,6 +293,12 @@ static char *riscv_create_isa_string(void) > > strcat_isa_ext(isa_str); > > + if(vendor_isa) { > + strcat(isa_str, "_"); > + strcat(isa_str, vendor_isa); > + kfree(vendor_isa); > + } > + > return isa_str; > } > > -- > 2.39.0 >
Hey Conor, Am Mittwoch, 26. April 2023, 14:29:16 CEST schrieb Conor Dooley: > On Mon, Apr 24, 2023 at 09:49:11PM +0200, Heiko Stuebner wrote: > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > T-Head cores support a number of own ISA extensions that also include > > optimized instructions which could benefit userspace to improve > > performance. > > > > Extensions supported by current T-Head cores are: > > * XTheadBa - bitmanipulation instructions for address calculation > > * XTheadBb - conditional basic bit-manipulation instructions > > * XTheadBs - instructions to access a single bit in a register > > * XTheadCmo - cache management operations > > * XTheadCondMov - conditional move instructions > > * XTheadFMemIdx - indexed memory operations for floating-point registers > > * XTheadFmv - double-precision floating-point high-bit data transmission > > intructions for RV32 > > * XTheadInt - instructions to reduce the code size of ISRs and/or the > > interrupt latencies that are caused by ISR entry/exit code > > * XTheadMac - multiply-accumulate instructions > > * XTheadMemIdx - indexed memory operations for GP registers > > * XTheadMemPair - two-GPR memory operations > > * XTheadSync - multi-core synchronization instructions > > > > In-depth descriptions of these extensions can be found on > > https://github.com/T-head-Semi/thead-extension-spec > > > > Support for those extensions was merged into the relevant toolchains > > so userspace programs can select necessary optimizations when needed. > > > > So a mechanism to the isa-string generation to export vendor-extension > > lists via the errata mechanism and implement it for T-Head C9xx cores. > > > > This exposes these vendor extensions then both in AT_BASE_PLATFORM > > and /proc/cpuinfo. > > I'm not entirely sure if this patch is meant to be a demo, but I don't > like the idea of using these registers to determine what extensions are > reported. It took me a while to grasp the above, but I think you mean determining extensions based on mvendor etc, right? > riscv,isa in a devicetree (for as much as I might dislike it at this > point in time), or the ACPI equivalent, should be the mechanism for > enabling/disabling these kinds of things. > Otherwise, we are just going to end up causing problems for ourselves > with various lists of this that and the other extension for different > combinations of hardware. > The open source c906 has the same archid/impid too right? Assuming this is > a serious proposal, how would you intend dealing with modified versions > of those cores? > > I am pretty sure that you intended this to be a demo though, particularly > given the wording of the below quote from your cover, yeah, this one was more following a train of thought. Thinking about the issues, this was more of an addon thought, as I wasn't really sure which way to go. So you're right, vendor isa-extensions should also come from the ISA string from firmware, similar to the base extensions. Not based on the mvendor-id and friends. > but in case you did > not: > > NAK to this way of sourcing the information. > > Anyways, since your cover's considerations section seems partly aimed at > me, given my discussion with head-the-ball last week: > > > Things to still consider: > > ------------------------- > > Right now both hwprobe and this approach will only pass through > > extensions the kernel actually knows about itself. This should not > > necessarily be needed (but could be an optional feature for e.g. virtualization). > > What do you mean by virtualisation here? It's the job of the hypervisor > etc to make sure that what it passes to its guest contains only what it > wants the guest to see, right? > IIUC, that's another point against doing what this patch does. I guess I'm still seeing Zbb and friends - with just computational instructions as always good to have. But I guess you're right that the hypervisor should be able to control itself which extensions. > > Most extensions don’t introduce new user-mode state that the kernel needs to > > manage (e.g. new registers). Extension that do introduce new user-mode state > > are usually disabled by default and have to be enabled by S mode or M mode > > (e.g. FS[1:0] for the +floating-point extension). So there should not be a > > reason to filter any extensions that are unknown. > > I think in general this can be safely assumed, but I don't think it is > unreasonable to expect someone may make, for example, XConorGigaVector > that gets turned on by the same bits as regular old vector but has some > extra registers. > Not saying that I think that that is a good idea, but it is a distinct > possibility that this will happen, and I don't think forwarding it to > userspace is a good idea. The thead-vector (0.7.1) would probably fit this description. Though in that case, userspace definitly needs to know about it, to use it :-) . But of course this should only be forwarded when relevant support is available in the kernel. > > So it might make more sense to just pass through a curated list (common > > set) created from the core's isa strings and remove state-handling > > extensions when they are not enabled in the kernel-side (sort of > > blacklisting extensions that need actual kernel support). > > Yeah, as discussed with Christoph the other day I don't think we can > really avoid such a blacklist. I don't think it'd require any sort of > vendor specific handling, since, as you point out, a vendor may well > implement extensions that were created by other companies. > > It's easy, right?? "Just" parse the dt, tokenise the extensions & delete > whatever is in the blacklist, right? And then reality happens ;-) > Hyperbole aside, I think that doing something like this increases the > need for a system like Evan's, as userspace may need a way to > differentiate between what the hardware is capable of (as reported by > the isa string in /proc/cpuinfo or the content of 3/4) and what the > kernel actually supports. > > > However, this is a very related, but still independent discussion. > > Aye, this discussion and the first two patches are relevant whether 3/4 > is accepted or not IMO. I guess I'll poke this some more in the meantime, taking into account all the comments from above :-) . Thanks Heiko
Hey Heiko, On Thu, Apr 27, 2023 at 07:15:58PM +0200, Heiko Stübner wrote: > Am Mittwoch, 26. April 2023, 14:29:16 CEST schrieb Conor Dooley: > > On Mon, Apr 24, 2023 at 09:49:11PM +0200, Heiko Stuebner wrote: > > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > I'm not entirely sure if this patch is meant to be a demo, but I don't > > like the idea of using these registers to determine what extensions are > > reported. > > It took me a while to grasp the above, but I think you mean determining > extensions based on mvendor etc, right? Yes, sorry. Apologies if that was not clear. I suppose the SBI implementation could (as ours does!) could report something different to the registers themselves, so using that word is probably not a good idea anyway. > > riscv,isa in a devicetree (for as much as I might dislike it at this > > point in time), or the ACPI equivalent, should be the mechanism for > > enabling/disabling these kinds of things. > > > Otherwise, we are just going to end up causing problems for ourselves > > with various lists of this that and the other extension for different > > combinations of hardware. > > The open source c906 has the same archid/impid too right? Assuming this is > > a serious proposal, how would you intend dealing with modified versions > > of those cores? > > > > I am pretty sure that you intended this to be a demo though, particularly > > given the wording of the below quote from your cover, > > yeah, this one was more following a train of thought. Thinking about the > issues, this was more of an addon thought, as I wasn't really sure which > way to go. > > So you're right, vendor isa-extensions should also come from the ISA > string from firmware, similar to the base extensions. Not based on the > mvendor-id and friends. :) > > > Things to still consider: > > > ------------------------- > > > Right now both hwprobe and this approach will only pass through > > > extensions the kernel actually knows about itself. This should not > > > necessarily be needed (but could be an optional feature for e.g. virtualization). > > > > What do you mean by virtualisation here? It's the job of the hypervisor > > etc to make sure that what it passes to its guest contains only what it > > wants the guest to see, right? > > IIUC, that's another point against doing what this patch does. > > I guess I'm still seeing Zbb and friends - with just computational > instructions as always good to have. But I guess you're right that the > hypervisor should be able to control itself which extensions. Yah, there may not be any obvious downsides to something like Zbb, but I think that taking control away from the hypervisors etc isn't a good idea. Having a simple policy of blocking things that are known to misbehave would require less maint. than a list of things that are okay to pass through, but both are probably cans-of-worms. I think we need to think carefully about what policy is chosen here. Allowlist will be slower, but at least we'll not tell userspace something that is not usable. Blocklist will be easier to manage, but can only be reactive. > > > Most extensions don’t introduce new user-mode state that the kernel needs to > > > manage (e.g. new registers). Extension that do introduce new user-mode state > > > are usually disabled by default and have to be enabled by S mode or M mode > > > (e.g. FS[1:0] for the +floating-point extension). So there should not be a > > > reason to filter any extensions that are unknown. > > > > I think in general this can be safely assumed, but I don't think it is > > unreasonable to expect someone may make, for example, XConorGigaVector > > that gets turned on by the same bits as regular old vector but has some > > extra registers. > > Not saying that I think that that is a good idea, but it is a distinct > > possibility that this will happen, and I don't think forwarding it to > > userspace is a good idea. > > The thead-vector (0.7.1) would probably fit this description. Though in > that case, userspace definitly needs to know about it, to use it :-) . > > But of course this should only be forwarded when relevant support > is available in the kernel. Right. IIRC, the plan for that is to add `v` to riscv,isa & alternatives will do the rest as opposed to doing an `_xtheadvector` type thing. Assuming the latter for a moment, we'd have to blacklist `_xheadvector` for kernels compiled without vector support even if the relevant support is added to the kernel. Similarly, we'd have to blacklist it for kernels with vector support, but without the erratum enabled. I think the plan was the former though, so you'd have to block passing `v` to userspace if vector is enabled and the erratum is not supported. Should ERRATA_THEAD_VECTOR be mandatory then for RISCV_ISA_VECTOR && ERRATA_THEAD kernels? What am I missing? Also, in a world where we do do some sort of passing, should we only forward the vendor extensions, or should we forward the standard ones too? What about supervisor mode only stuff? There's a bunch of questions to consider here, even if for some of them the answer may be obvious. As I said, not really bothered about hwprobe, aux vector etc, but this side of things is particularly interesting to me. Cheers, Conor.
On Thu, Apr 27, 2023 at 07:28:49PM +0100, Conor Dooley wrote: > Hey Heiko, > > On Thu, Apr 27, 2023 at 07:15:58PM +0200, Heiko Stübner wrote: > > Am Mittwoch, 26. April 2023, 14:29:16 CEST schrieb Conor Dooley: > > > On Mon, Apr 24, 2023 at 09:49:11PM +0200, Heiko Stuebner wrote: > > > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > I'm not entirely sure if this patch is meant to be a demo, but I don't > > > like the idea of using these registers to determine what extensions are > > > reported. > > > > It took me a while to grasp the above, but I think you mean determining > > extensions based on mvendor etc, right? > > Yes, sorry. Apologies if that was not clear. I suppose the SBI > implementation could (as ours does!) could report something different to > the registers themselves, so using that word is probably not a good idea > anyway. > > > > riscv,isa in a devicetree (for as much as I might dislike it at this > > > point in time), or the ACPI equivalent, should be the mechanism for > > > enabling/disabling these kinds of things. > > > > > Otherwise, we are just going to end up causing problems for ourselves > > > with various lists of this that and the other extension for different > > > combinations of hardware. > > > The open source c906 has the same archid/impid too right? Assuming this is > > > a serious proposal, how would you intend dealing with modified versions > > > of those cores? > > > > > > I am pretty sure that you intended this to be a demo though, particularly > > > given the wording of the below quote from your cover, > > > > yeah, this one was more following a train of thought. Thinking about the > > issues, this was more of an addon thought, as I wasn't really sure which > > way to go. > > > > So you're right, vendor isa-extensions should also come from the ISA > > string from firmware, similar to the base extensions. Not based on the > > mvendor-id and friends. > > :) > > > > > Things to still consider: > > > > ------------------------- > > > > Right now both hwprobe and this approach will only pass through > > > > extensions the kernel actually knows about itself. This should not > > > > necessarily be needed (but could be an optional feature for e.g. virtualization). > > > > > > What do you mean by virtualisation here? It's the job of the hypervisor > > > etc to make sure that what it passes to its guest contains only what it > > > wants the guest to see, right? > > > IIUC, that's another point against doing what this patch does. > > > > I guess I'm still seeing Zbb and friends - with just computational > > instructions as always good to have. But I guess you're right that the > > hypervisor should be able to control itself which extensions. > > Yah, there may not be any obvious downsides to something like Zbb, but I > think that taking control away from the hypervisors etc isn't a good > idea. > Having a simple policy of blocking things that are known to misbehave > would require less maint. than a list of things that are okay to pass > through, but both are probably cans-of-worms. > I think we need to think carefully about what policy is chosen here. > Allowlist will be slower, but at least we'll not tell userspace > something that is not usable. Blocklist will be easier to manage, but > can only be reactive. > > > > > Most extensions don’t introduce new user-mode state that the kernel needs to > > > > manage (e.g. new registers). Extension that do introduce new user-mode state > > > > are usually disabled by default and have to be enabled by S mode or M mode > > > > (e.g. FS[1:0] for the +floating-point extension). So there should not be a > > > > reason to filter any extensions that are unknown. > > > > > > I think in general this can be safely assumed, but I don't think it is > > > unreasonable to expect someone may make, for example, XConorGigaVector > > > that gets turned on by the same bits as regular old vector but has some > > > extra registers. > > > Not saying that I think that that is a good idea, but it is a distinct > > > possibility that this will happen, and I don't think forwarding it to > > > userspace is a good idea. > > > > The thead-vector (0.7.1) would probably fit this description. Though in > > that case, userspace definitly needs to know about it, to use it :-) . > > > > But of course this should only be forwarded when relevant support > > is available in the kernel. > > Right. IIRC, the plan for that is to add `v` to riscv,isa & alternatives > will do the rest as opposed to doing an `_xtheadvector` type thing. > > Assuming the latter for a moment, we'd have to blacklist `_xheadvector` > for kernels compiled without vector support even if the relevant support > is added to the kernel. Similarly, we'd have to blacklist it for kernels > with vector support, but without the erratum enabled. > > I think the plan was the former though, so you'd have to block passing > `v` to userspace if vector is enabled and the erratum is not supported. > Should ERRATA_THEAD_VECTOR be mandatory then for RISCV_ISA_VECTOR && > ERRATA_THEAD kernels? > What am I missing? I think what I missed is that for riscv,isa containing `_xtheadvector` but a kernel without support for vector then we'd hit Andy's trap-on-first-use and that's one of the cases that don't need to be considered here. > Also, in a world where we do do some sort of passing, should we only > forward the vendor extensions, or should we forward the standard ones > too? > What about supervisor mode only stuff? There's a bunch of questions to > consider here, even if for some of them the answer may be obvious. > > As I said, not really bothered about hwprobe, aux vector etc, but this > side of things is particularly interesting to me. > > Cheers, > Conor.
On Thu, Apr 27, 2023 at 07:28:49PM +0100, Conor Dooley wrote: > Hey Heiko, > > On Thu, Apr 27, 2023 at 07:15:58PM +0200, Heiko Stübner wrote: > > Am Mittwoch, 26. April 2023, 14:29:16 CEST schrieb Conor Dooley: > > > On Mon, Apr 24, 2023 at 09:49:11PM +0200, Heiko Stuebner wrote: > > > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> ... > > > What do you mean by virtualisation here? It's the job of the hypervisor > > > etc to make sure that what it passes to its guest contains only what it > > > wants the guest to see, right? > > > IIUC, that's another point against doing what this patch does. > > > > I guess I'm still seeing Zbb and friends - with just computational > > instructions as always good to have. But I guess you're right that the > > hypervisor should be able to control itself which extensions. > > Yah, there may not be any obvious downsides to something like Zbb, but I > think that taking control away from the hypervisors etc isn't a good > idea. If there's any chance that a VM will need to migrate from a host with, e.g. Zbb, to one without it, then the VM will need Zbb disabled from the start. > Having a simple policy of blocking things that are known to misbehave > would require less maint. than a list of things that are okay to pass > through, but both are probably cans-of-worms. > I think we need to think carefully about what policy is chosen here. > Allowlist will be slower, but at least we'll not tell userspace > something that is not usable. Blocklist will be easier to manage, but > can only be reactive. I have experience [trying] to maintain deny-lists for CPU features, both for x86 Xen guests and Arm KVM guests. I don't recommend it. To do it right, you need to be proactive, tracking upcoming CPU features to add the ones that can't be supported by virt or aren't ready to be supported by virt to the deny-list before somebody trips over them. In practice, usually somebody trips over it first, causing fires which have to be put out. If an allow-list is used, then, when a new feature is missed, no fires are started. The worst that can happen is somebody expected the feature and didn't see it, so they complain, at which point you add it. ... > Also, in a world where we do do some sort of passing, should we only > forward the vendor extensions, or should we forward the standard ones > too? I guess we need to forward anything userspace can and should use. > What about supervisor mode only stuff? That's not something userspace can use. If we want to expose which supervisor mode features the CPU has to userspace, for information purposes, then I think proc or sysfs would be sufficient for that. The downside of using an allow-list for what extensions get exposed to userspace is that even extensions the kernel can't/won't use will need a kernel patch before userspace can use them. But, as I stated above, that downside (people complaining a feature they expect is missing), is, IMO, better than the alternative of exposing things that shouldn't be. Thanks, drew
On Fri, Apr 28, 2023 at 12:28:24PM +0200, Andrew Jones wrote: > On Thu, Apr 27, 2023 at 07:28:49PM +0100, Conor Dooley wrote: > > On Thu, Apr 27, 2023 at 07:15:58PM +0200, Heiko Stübner wrote: > > > Am Mittwoch, 26. April 2023, 14:29:16 CEST schrieb Conor Dooley: > > > > On Mon, Apr 24, 2023 at 09:49:11PM +0200, Heiko Stuebner wrote: > > > > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > ... > > > > What do you mean by virtualisation here? It's the job of the hypervisor > > > > etc to make sure that what it passes to its guest contains only what it > > > > wants the guest to see, right? > > > > IIUC, that's another point against doing what this patch does. > > > > > > I guess I'm still seeing Zbb and friends - with just computational > > > instructions as always good to have. But I guess you're right that the > > > hypervisor should be able to control itself which extensions. > > > > Yah, there may not be any obvious downsides to something like Zbb, but I > > think that taking control away from the hypervisors etc isn't a good > > idea. > > If there's any chance that a VM will need to migrate from a host with, > e.g. Zbb, to one without it, then the VM will need Zbb disabled from the > start. (Almost) Everything is obvious to someone :) > > Having a simple policy of blocking things that are known to misbehave > > would require less maint. than a list of things that are okay to pass > > through, but both are probably cans-of-worms. > > I think we need to think carefully about what policy is chosen here. > > Allowlist will be slower, but at least we'll not tell userspace > > something that is not usable. Blocklist will be easier to manage, but > > can only be reactive. > > I have experience [trying] to maintain deny-lists for CPU features, > both for x86 Xen guests and Arm KVM guests. I don't recommend it. To > do it right, you need to be proactive, tracking upcoming CPU features > to add the ones that can't be supported by virt or aren't ready to > be supported by virt to the deny-list before somebody trips over them. > In practice, usually somebody trips over it first, causing fires which > have to be put out. If an allow-list is used, then, when a new feature > is missed, no fires are started. The worst that can happen is somebody > expected the feature and didn't see it, so they complain, at which > point you add it. Right. Blocking-unless-known is what I suggested when canvassed for an opinion last week but the complaint was that the kernel having to maintain a list would be a significant speed-bump for people. With a lighter-weight method of forwarding to userspace extensions that the kernel doesn't need to care about (no integration with .._has_extension[un]likely() etc) hopefully the roadblock would be a speedbump instead. I think I would rather speed-bumps & complaints about things being slow, than having to fight fires. > > Also, in a world where we do do some sort of passing, should we only > > forward the vendor extensions, or should we forward the standard ones > > too? > > I guess we need to forward anything userspace can and should use. That, combined with what we have now, would mean that userspace would get told both what the kernel supports and additional other things that the kernel may not support, but userspace can use without that support being present. I think that is a reasonable thing to do, although it'd muddy the waters a bit with what the output in /proc/cpuinfo means. (I'm kinda taking the particular bit of the series in isolation, as if /proc/cpuinfo is the only place in which this information will be exposed.) > > What about supervisor mode only stuff? > > That's not something userspace can use. If we want to expose which > supervisor mode features the CPU has to userspace, for information > purposes, then I think proc or sysfs would be sufficient for that. Yeah, as above I'm kinda looking at it from a really naive "only /proc/cpuinfo exists" point of view for the sake of simplicity. Depending on implementation, reporting supervisor-only stuff that the kernel supports may make life easier & there's probably some value to someone in passing that information to userspace too. > The downside of using an allow-list for what extensions get exposed > to userspace is that even extensions the kernel can't/won't use > will need a kernel patch before userspace can use them. But, as > I stated above, that downside (people complaining a feature they > expect is missing), is, IMO, better than the alternative of exposing > things that shouldn't be. Yeah, I would be in that camp too, but gotta suggest the various options for the sake of stirring discussion :) Thanks, Conor.
diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c index 1036b8f933ec..eb635bf80737 100644 --- a/arch/riscv/errata/thead/errata.c +++ b/arch/riscv/errata/thead/errata.c @@ -15,6 +15,7 @@ #include <asm/errata_list.h> #include <asm/hwprobe.h> #include <asm/patch.h> +#include <asm/switch_to.h> #include <asm/vendorid_list.h> static bool errata_probe_pbmt(unsigned int stage, @@ -125,3 +126,45 @@ void __init_or_module thead_feature_probe_func(unsigned int cpu, if ((archid == 0) && (impid == 0)) per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST; } + + +char *thead_extension_list_func(unsigned long archid, + unsigned long impid) +{ + if ((archid == 0) && (impid == 0)) { + const char *xbase1 = "xtheadba_xtheadbb_xtheadbs_xtheadcmo_xtheadcondmov"; + const char *xbase2 = "_xtheadint_xtheadmac_xtheadmemidx_xtheadmempair_xtheadsync"; + const char *xfpu = "_xtheadfmemIdx"; +#ifdef CONFIG_32BIT + const char *xfpu32 = "_xtheadfmv"; +#endif + int len = strlen(xbase1) + strlen(xbase2); + char *str; + + if (has_fpu()) { + len += strlen(xfpu); +#ifdef CONFIG_32BIT + len+= strlen(xfpu32); +#endif + } + + str = kzalloc(len, GFP_KERNEL); + if (!str) + return str; + + strcpy(str, xbase1); + + if (has_fpu()) { + strcat(str, xfpu); +#ifdef CONFIG_32BIT + strcat(str, xfpu32); +#endif + } + + strcat(str, xbase2); + + return str; + } + + return NULL; +} diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h index a8f5cf6694a1..8c9aec196649 100644 --- a/arch/riscv/include/asm/alternative.h +++ b/arch/riscv/include/asm/alternative.h @@ -31,6 +31,7 @@ #define ALT_ALT_PTR(a) __ALT_PTR(a, alt_offset) void __init probe_vendor_features(unsigned int cpu); +char *list_vendor_extensions(void); void __init apply_boot_alternatives(void); void __init apply_early_boot_alternatives(void); void apply_module_alternatives(void *start, size_t length); @@ -55,6 +56,8 @@ void thead_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, void thead_feature_probe_func(unsigned int cpu, unsigned long archid, unsigned long impid); +char *thead_extension_list_func(unsigned long archid, + unsigned long impid); void riscv_cpufeature_patch_func(struct alt_entry *begin, struct alt_entry *end, unsigned int stage); @@ -62,6 +65,7 @@ void riscv_cpufeature_patch_func(struct alt_entry *begin, struct alt_entry *end, #else /* CONFIG_RISCV_ALTERNATIVE */ static inline void probe_vendor_features(unsigned int cpu) { } +static inline char *list_vendor_extensions(void) { return NULL; } static inline void apply_boot_alternatives(void) { } static inline void apply_early_boot_alternatives(void) { } static inline void apply_module_alternatives(void *start, size_t length) { } diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c index fc65c9293ac5..18913fd1809f 100644 --- a/arch/riscv/kernel/alternative.c +++ b/arch/riscv/kernel/alternative.c @@ -29,6 +29,8 @@ struct cpu_manufacturer_info_t { unsigned int stage); void (*feature_probe_func)(unsigned int cpu, unsigned long archid, unsigned long impid); + char *(*extension_list_func)(unsigned long archid, + unsigned long impid); }; static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_info_t *cpu_mfr_info) @@ -54,6 +56,7 @@ static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_inf case THEAD_VENDOR_ID: cpu_mfr_info->patch_func = thead_errata_patch_func; cpu_mfr_info->feature_probe_func = thead_feature_probe_func; + cpu_mfr_info->extension_list_func = thead_extension_list_func; break; #endif default: @@ -157,6 +160,24 @@ void __init_or_module probe_vendor_features(unsigned int cpu) cpu_mfr_info.imp_id); } +/* + * Lists the vendor-specific extensions common to all cores. + * Returns a new underscore "_" concatenated string that the + * caller is supposed to free after use. + */ +char *list_vendor_extensions(void) +{ + struct cpu_manufacturer_info_t cpu_mfr_info; + + riscv_fill_cpu_mfr_info(&cpu_mfr_info); + if (!cpu_mfr_info.extension_list_func) + return NULL; + + return cpu_mfr_info.extension_list_func(cpu_mfr_info.arch_id, + cpu_mfr_info.imp_id); + +} + /* * This is called very early in the boot process (directly after we run * a feature detect on the boot CPU). No need to worry about other CPUs diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c index 71770563199f..6a0a45b2eb20 100644 --- a/arch/riscv/kernel/cpu.c +++ b/arch/riscv/kernel/cpu.c @@ -7,6 +7,7 @@ #include <linux/init.h> #include <linux/seq_file.h> #include <linux/of.h> +#include <asm/alternative.h> #include <asm/cpufeature.h> #include <asm/csr.h> #include <asm/hwcap.h> @@ -260,6 +261,7 @@ static char *riscv_create_isa_string(void) { int maxlen = 4; char *isa_str; + char *vendor_isa; int i; /* calculate the needed string length */ @@ -268,6 +270,10 @@ static char *riscv_create_isa_string(void) maxlen++; maxlen += strlen_isa_ext(); + vendor_isa = list_vendor_extensions(); + if (vendor_isa) + maxlen += strlen(vendor_isa) + 1; + isa_str = kzalloc(maxlen, GFP_KERNEL); if (!isa_str) return ERR_PTR(-ENOMEM); @@ -287,6 +293,12 @@ static char *riscv_create_isa_string(void) strcat_isa_ext(isa_str); + if(vendor_isa) { + strcat(isa_str, "_"); + strcat(isa_str, vendor_isa); + kfree(vendor_isa); + } + return isa_str; }