Message-ID: <7519429.lvqk35OSZv@diego>
Date:   Sun, 29 May 2022 19:49:41 +0200
From:   Heiko Stübner <heiko@...ech.de>
To:     palmer@...belt.com, paul.walmsley@...ive.com,
        Samuel Holland <samuel@...lland.org>
Cc:     linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        wefu@...hat.com, guoren@...nel.org, mick@....forth.gr,
        cmuellner@...ux.com, philipp.tomsich@...ll.eu, hch@....de
Subject: Re: [PATCH v2 5/5] riscv: remove usage of function-pointers from cpufeatures and t-head errata

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@...ech.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@...lland.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;
> >  }
> > 
> 
> 

Powered by blists - more mailing lists