Messages in this thread Patch in this message | | | From | Heiko Stübner <> | Subject | Re: [PATCH v2 5/5] riscv: remove usage of function-pointers from cpufeatures and t-head errata | Date | Sun, 29 May 2022 19:49:41 +0200 |
| |
Hi,
Am Sonntag, 29. Mai 2022, 03:27:48 CEST schrieb Samuel Holland: > On 5/26/22 3:56 PM, Heiko Stuebner wrote: > > Having a list of alternatives to check with a per-entry function pointer > > to a check function is nice style-wise. But in case of early-alternatives > > it can clash with the non-relocated kernel and the function pointer in > > the list pointing to a completely wrong location. > > > > This isn't an issue with one or two list entries, as in that case the > > compiler seems to unroll the loop and even usage of the list structure > > and then only does relative jumps into the check functions based on this. > > > > When adding a third entry to either list though, the issue that was > > hiding there from the beginning is triggered resulting a jump to a > > memory address that isn't part of the kernel at all. > > > > The list of features/erratas only contained an unused name and the > > pointer to the check function, so an easy solution for the problem > > is to just unroll the loop in code, dismantle the whole list structure > > and just call the relevant check functions one by one ourself. > > > > For the T-Head errata this includes moving the stage-check inside > > the check functions. > > > > The issue is only relevant for things that might be called for early- > > alternatives (T-Head and possible future main extensions), so the > > SiFive erratas were not affected from the beginning, as they got > > an early return for early-alternatives in the original patchset. > > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
[...]
> > -static u32 thead_errata_probe(unsigned int stage, unsigned long archid, unsigned long impid) > > +static u32 thead_errata_probe(unsigned int stage, > > + unsigned long archid, unsigned long impid) > > { > > - const struct errata_info *info; > > u32 cpu_req_errata = 0; > > - int idx; > > - > > - for (idx = 0; idx < ERRATA_THEAD_NUMBER; idx++) { > > - info = &errata_list[idx]; > > > > - if ((stage == RISCV_ALTERNATIVES_MODULE || > > - info->stage == stage) && info->check_func(archid, impid)) > > - cpu_req_errata |= (1U << idx); > > - } > > + if (errata_probe_pbmt(stage, archid, impid)) > > + cpu_req_errata |= (1U << ERRATA_THEAD_PBMT); > > > > return cpu_req_errata; > > } > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index b33564df81e1..b63c25c55bf1 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -246,12 +246,7 @@ void __init riscv_fill_hwcap(void) > > } > > > > #ifdef CONFIG_RISCV_ALTERNATIVE > > -struct cpufeature_info { > > - char name[ERRATA_STRING_LENGTH_MAX]; > > - bool (*check_func)(unsigned int stage); > > -}; > > - > > -static bool __init_or_module cpufeature_svpbmt_check_func(unsigned int stage) > > +static bool __init_or_module cpufeature_probe_svpbmt(unsigned int stage) > > Now that this function isn't used as a function pointer anymore, it doesn't need > to be specific to SVPBMT. I think the logic here is the same for ZICBOM. Does it > make sense to move it to the calling function?
we don't know how other extensions need to probe though, so for example in my yet-to-send zicbom-v3 the probe function itself looks like
static u32 __init_or_module cpufeature_probe(unsigned int stage) { u32 cpu_req_feature = 0;
if (cpufeature_probe_svpbmt(stage)) cpu_req_feature |= (1U << CPUFEATURE_SVPBMT);
if (cpufeature_probe_cmo(stage)) cpu_req_feature |= (1U << CPUFEATURE_CMO);
return cpu_req_feature; }
As this might get longer in the future, I actually do like the actual checks being separate for readability.
But I'll just yield to the majority opinion ;-)
Heiko
> > With the conflicts between this and the CMO series manually fixed: > > Tested-by: Samuel Holland <samuel@sholland.org> > > Regards, > Samuel > > > { > > #ifdef CONFIG_RISCV_ISA_SVPBMT > > switch (stage) { > > @@ -265,26 +260,19 @@ static bool __init_or_module cpufeature_svpbmt_check_func(unsigned int stage) > > return false; > > } > > > > -static const struct cpufeature_info __initdata_or_module > > -cpufeature_list[CPUFEATURE_NUMBER] = { > > - { > > - .name = "svpbmt", > > - .check_func = cpufeature_svpbmt_check_func > > - }, > > -}; > > - > > +/* > > + * Probe presence of individual extensions. > > + * > > + * This code may also be executed before kernel relocation, so we cannot use > > + * addresses generated by the address-of operator as they won't be valid in > > + * this context. > > + */ > > static u32 __init_or_module cpufeature_probe(unsigned int stage) > > { > > - const struct cpufeature_info *info; > > u32 cpu_req_feature = 0; > > - int idx; > > - > > - for (idx = 0; idx < CPUFEATURE_NUMBER; idx++) { > > - info = &cpufeature_list[idx]; > > > > - if (info->check_func(stage)) > > - cpu_req_feature |= (1U << idx); > > - } > > + if (cpufeature_probe_svpbmt(stage)) > > + cpu_req_feature |= (1U << CPUFEATURE_SVPBMT); > > > > return cpu_req_feature; > > } > > > >
| |