On Mon, Apr 29, 2024 at 05:04:55PM +0200, Clément Léger wrote: > Since a few extensions (Zicbom/Zicboz) already needs validation and > future ones will need it as well (Zc*) add a validate() callback to > struct riscv_isa_ext_data. This require to rework the way extensions are > parsed and split it in two phases. First phase is isa string or isa > extension list parsing and consists in enabling all the extensions in a > temporary bitmask without any validation. The second step "resolves" the > final isa bitmap, handling potential missing dependencies. The mechanism > is quite simple and simply validate each extension described in the > temporary bitmap before enabling it in the final isa bitmap. validate() > callbacks can return either 0 for success, -EPROBEDEFER if extension > needs to be validated again at next loop. A previous ISA bitmap is kept > to avoid looping mutliple times if an extension dependencies are never > satisfied until we reach a stable state. In order to avoid any potential > infinite looping, allow looping a maximum of the number of extension we > handle. Zicboz and Zicbom extensions are modified to use this validation > mechanism.
Your reply to my last review only talked about part of my comments, which is usually what you do when you're gonna implement the rest, but you haven't. I like the change you've made to shorten looping, but I'd at least like a response to why a split is not worth doing :) Thanks, Conor. > > Signed-off-by: Clément Léger <[email protected]> > --- > arch/riscv/include/asm/cpufeature.h | 1 + > arch/riscv/kernel/cpufeature.c | 211 ++++++++++++++++------------ > 2 files changed, 126 insertions(+), 86 deletions(-) > > diff --git a/arch/riscv/include/asm/cpufeature.h > b/arch/riscv/include/asm/cpufeature.h > index 347805446151..000796c2d0b1 100644 > --- a/arch/riscv/include/asm/cpufeature.h > +++ b/arch/riscv/include/asm/cpufeature.h > @@ -70,6 +70,7 @@ struct riscv_isa_ext_data { > const char *property; > const unsigned int *subset_ext_ids; > const unsigned int subset_ext_size; > + int (*validate)(const struct riscv_isa_ext_data *data, const unsigned > long *isa_bitmap); > }; > > extern const struct riscv_isa_ext_data riscv_isa_ext[]; > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 115ba001f1bc..cb2ffa6c8c33 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -72,51 +72,58 @@ bool __riscv_isa_extension_available(const unsigned long > *isa_bitmap, unsigned i > } > EXPORT_SYMBOL_GPL(__riscv_isa_extension_available); > > -static bool riscv_isa_extension_check(int id) > +static bool riscv_isa_extension_valid(int id) > { > - switch (id) { > - case RISCV_ISA_EXT_ZICBOM: > - if (!riscv_cbom_block_size) { > - pr_err("Zicbom detected in ISA string, disabling as no > cbom-block-size found\n"); > - return false; > - } else if (!is_power_of_2(riscv_cbom_block_size)) { > - pr_err("Zicbom disabled as cbom-block-size present, but > is not a power-of-2\n"); > - return false; > - } > - return true; > - case RISCV_ISA_EXT_ZICBOZ: > - if (!riscv_cboz_block_size) { > - pr_err("Zicboz detected in ISA string, disabling as no > cboz-block-size found\n"); > - return false; > - } else if (!is_power_of_2(riscv_cboz_block_size)) { > - pr_err("Zicboz disabled as cboz-block-size present, but > is not a power-of-2\n"); > - return false; > - } > - return true; > - case RISCV_ISA_EXT_INVALID: > - return false; > + return id != RISCV_ISA_EXT_INVALID; > +} > + > +static int riscv_ext_zicbom_validate(const struct riscv_isa_ext_data *data, > + const unsigned long *isa_bitmap) > +{ > + if (!riscv_cbom_block_size) { > + pr_err("Zicbom detected in ISA string, disabling as no > cbom-block-size found\n"); > + return -EINVAL; > + } else if (!is_power_of_2(riscv_cbom_block_size)) { > + pr_err("Zicbom disabled as cbom-block-size present, but is not > a power-of-2\n"); > + return -EINVAL; > } > + return 0; > +} > > - return true; > +static int riscv_ext_zicboz_validate(const struct riscv_isa_ext_data *data, > + const unsigned long *isa_bitmap) > +{ > + if (!riscv_cboz_block_size) { > + pr_err("Zicboz detected in ISA string, disabling as no > cboz-block-size found\n"); > + return -EINVAL; > + } else if (!is_power_of_2(riscv_cboz_block_size)) { > + pr_err("Zicboz disabled as cboz-block-size present, but is not > a power-of-2\n"); > + return -EINVAL; > + } > + return 0; > } > > -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) { > \ > - .name = #_name, > \ > - .property = #_name, > \ > - .id = _id, > \ > - .subset_ext_ids = _subset_exts, > \ > - .subset_ext_size = _subset_exts_size > \ > +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, > _validate) { \ > + .name = #_name, > \ > + .property = #_name, > \ > + .id = _id, > \ > + .subset_ext_ids = _subset_exts, > \ > + .subset_ext_size = _subset_exts_size, > \ > + .validate = _validate > \ > } > > -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, > NULL, 0) > +#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, > NULL, 0, NULL) > > /* Used to declare pure "lasso" extension (Zk for instance) */ > #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \ > - _RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, > ARRAY_SIZE(_bundled_exts)) > + _RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, \ > + ARRAY_SIZE(_bundled_exts), NULL) > > /* Used to declare extensions that are a superset of other extensions (Zvbb > for instance) */ > #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \ > - _RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts)) > + _RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), NULL) > +#define __RISCV_ISA_EXT_SUPERSET_VALIDATE(_name, _id, _sub_exts, _validate) \ > + _RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), > _validate) > > static const unsigned int riscv_zk_bundled_exts[] = { > RISCV_ISA_EXT_ZBKB, > @@ -247,8 +254,10 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > __RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c), > __RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v), > __RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h), > - __RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, > riscv_xlinuxenvcfg_exts), > - __RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, > riscv_xlinuxenvcfg_exts), > + __RISCV_ISA_EXT_SUPERSET_VALIDATE(zicbom, RISCV_ISA_EXT_ZICBOM, > riscv_xlinuxenvcfg_exts, > + riscv_ext_zicbom_validate), > + __RISCV_ISA_EXT_SUPERSET_VALIDATE(zicboz, RISCV_ISA_EXT_ZICBOZ, > riscv_xlinuxenvcfg_exts, > + riscv_ext_zicboz_validate), > __RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR), > __RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND), > __RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR), > @@ -310,33 +319,80 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > > const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext); > > -static void __init match_isa_ext(const struct riscv_isa_ext_data *ext, const > char *name, > - const char *name_end, struct riscv_isainfo > *isainfo) > +static void riscv_isa_set_ext(const struct riscv_isa_ext_data *ext, unsigned > long *bitmap) > { > - if ((name_end - name == strlen(ext->name)) && > - !strncasecmp(name, ext->name, name_end - name)) { > - /* > - * If this is a bundle, enable all the ISA extensions that > - * comprise the bundle. > - */ > - if (ext->subset_ext_size) { > - for (int i = 0; i < ext->subset_ext_size; i++) { > - if > (riscv_isa_extension_check(ext->subset_ext_ids[i])) > - set_bit(ext->subset_ext_ids[i], > isainfo->isa); > - } > + /* > + * This is valid even for bundle extensions which uses the > RISCV_ISA_EXT_INVALID id > + * (rejected by riscv_isa_extension_valid()). > + */ > + if (riscv_isa_extension_valid(ext->id)) > + set_bit(ext->id, bitmap); > + > + for (int i = 0; i < ext->subset_ext_size; i++) { > + if (riscv_isa_extension_valid(ext->subset_ext_ids[i])) > + set_bit(ext->subset_ext_ids[i], bitmap); > + } > +} > + > +static void __init riscv_resolve_isa(unsigned long *isa_bitmap, struct > riscv_isainfo *isainfo, > + unsigned long *this_hwcap, unsigned long > *isa2hwcap) > +{ > + bool loop; > + const struct riscv_isa_ext_data *ext; > + DECLARE_BITMAP(prev_bitmap, RISCV_ISA_EXT_MAX); > + int max_loop_count = riscv_isa_ext_count, ret; > + > + do { > + loop = false; > + if (max_loop_count-- < 0) { > + pr_err("Failed to reach a stable ISA state\n"); > + return; > } > + memcpy(prev_bitmap, isainfo->isa, sizeof(prev_bitmap)); > + for (int i = 0; i < riscv_isa_ext_count; i++) { > + ext = &riscv_isa_ext[i]; > + > + /* Bundle extensions ids are invalid*/ > + if (!riscv_isa_extension_valid(ext->id)) > + continue; > + > + if (!test_bit(ext->id, isa_bitmap) || test_bit(ext->id, > isainfo->isa)) > + continue; > + > + if (ext->validate) { > + ret = ext->validate(ext, isainfo->isa); > + if (ret) { > + if (ret == -EPROBE_DEFER) > + loop = true; > + else > + clear_bit(ext->id, isa_bitmap); > + continue; > + } > + } > > - /* > - * This is valid even for bundle extensions which uses the > RISCV_ISA_EXT_INVALID id > - * (rejected by riscv_isa_extension_check()). > - */ > - if (riscv_isa_extension_check(ext->id)) > set_bit(ext->id, isainfo->isa); > + > + /* Only single letter extensions get set in hwcap */ > + if (ext->id < RISCV_ISA_EXT_BASE) > + *this_hwcap |= isa2hwcap[ext->id]; > + } > + } while (loop && memcmp(prev_bitmap, isainfo->isa, > sizeof(prev_bitmap))); > +} > + > +static void __init match_isa_ext(const char *name, const char *name_end, > unsigned long *bitmap) > +{ > + for (int i = 0; i < riscv_isa_ext_count; i++) { > + const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i]; > + > + if ((name_end - name == strlen(ext->name)) && > + !strncasecmp(name, ext->name, name_end - name)) { > + riscv_isa_set_ext(ext, bitmap); > + break; > + } > } > } > > -static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct > riscv_isainfo *isainfo, > - unsigned long *isa2hwcap, const char > *isa) > +static void __init riscv_resolve_isa_string(const char *isa, unsigned long > *bitmap) > { > /* > * For all possible cpus, we have already validated in > @@ -349,7 +405,7 @@ static void __init riscv_parse_isa_string(unsigned long > *this_hwcap, struct risc > while (*isa) { > const char *ext = isa++; > const char *ext_end = isa; > - bool ext_long = false, ext_err = false; > + bool ext_err = false; > > switch (*ext) { > case 's': > @@ -389,7 +445,6 @@ static void __init riscv_parse_isa_string(unsigned long > *this_hwcap, struct risc > * character itself while eliminating the extensions > version number. > * A simple re-increment solves this problem. > */ > - ext_long = true; > for (; *isa && *isa != '_'; ++isa) > if (unlikely(!isalnum(*isa))) > ext_err = true; > @@ -469,17 +524,8 @@ static void __init riscv_parse_isa_string(unsigned long > *this_hwcap, struct risc > > if (unlikely(ext_err)) > continue; > - if (!ext_long) { > - int nr = tolower(*ext) - 'a'; > > - if (riscv_isa_extension_check(nr)) { > - *this_hwcap |= isa2hwcap[nr]; > - set_bit(nr, isainfo->isa); > - } > - } else { > - for (int i = 0; i < riscv_isa_ext_count; i++) > - match_isa_ext(&riscv_isa_ext[i], ext, ext_end, > isainfo); > - } > + match_isa_ext(ext, ext_end, bitmap); > } > } > > @@ -501,6 +547,7 @@ static void __init > riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap) > for_each_possible_cpu(cpu) { > struct riscv_isainfo *isainfo = &hart_isa[cpu]; > unsigned long this_hwcap = 0; > + DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 }; > > if (acpi_disabled) { > node = of_cpu_device_node_get(cpu); > @@ -523,7 +570,7 @@ static void __init > riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap) > } > } > > - riscv_parse_isa_string(&this_hwcap, isainfo, isa2hwcap, isa); > + riscv_resolve_isa_string(isa, isa_bitmap); > > /* > * These ones were as they were part of the base ISA when the > @@ -531,10 +578,10 @@ static void __init > riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap) > * unconditionally where `i` is in riscv,isa on DT systems. > */ > if (acpi_disabled) { > - set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa); > - set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa); > - set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa); > - set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa); > + set_bit(RISCV_ISA_EXT_ZICSR, isa_bitmap); > + set_bit(RISCV_ISA_EXT_ZIFENCEI, isa_bitmap); > + set_bit(RISCV_ISA_EXT_ZICNTR, isa_bitmap); > + set_bit(RISCV_ISA_EXT_ZIHPM, isa_bitmap); > } > > /* > @@ -548,9 +595,11 @@ static void __init > riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap) > if (acpi_disabled && riscv_cached_mvendorid(cpu) == > THEAD_VENDOR_ID && > riscv_cached_marchid(cpu) == 0x0) { > this_hwcap &= ~isa2hwcap[RISCV_ISA_EXT_v]; > - clear_bit(RISCV_ISA_EXT_v, isainfo->isa); > + clear_bit(RISCV_ISA_EXT_v, isa_bitmap); > } > > + riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap); > + > /* > * All "okay" hart should have same isa. Set HWCAP based on > * common capabilities of every "okay" hart, in case they don't > @@ -579,6 +628,7 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned > long *isa2hwcap) > unsigned long this_hwcap = 0; > struct device_node *cpu_node; > struct riscv_isainfo *isainfo = &hart_isa[cpu]; > + DECLARE_BITMAP(isa_bitmap, RISCV_ISA_EXT_MAX) = { 0 }; > > cpu_node = of_cpu_device_node_get(cpu); > if (!cpu_node) { > @@ -598,22 +648,11 @@ static int __init > riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap) > ext->property) < 0) > continue; > > - if (ext->subset_ext_size) { > - for (int j = 0; j < ext->subset_ext_size; j++) { > - if > (riscv_isa_extension_check(ext->subset_ext_ids[i])) > - set_bit(ext->subset_ext_ids[j], > isainfo->isa); > - } > - } > - > - if (riscv_isa_extension_check(ext->id)) { > - set_bit(ext->id, isainfo->isa); > - > - /* Only single letter extensions get set in > hwcap */ > - if (strnlen(riscv_isa_ext[i].name, 2) == 1) > - this_hwcap |= > isa2hwcap[riscv_isa_ext[i].id]; > - } > + riscv_isa_set_ext(ext, isa_bitmap); > } > > + riscv_resolve_isa(isa_bitmap, isainfo, &this_hwcap, isa2hwcap); > + > of_node_put(cpu_node); > > /* > -- > 2.43.0 >
signature.asc
Description: PGP signature
