[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] xen/arm: Warn user on cpu errata 832075



On Thu, 15 Oct 2020, Bertrand Marquis wrote:
> > On 14 Oct 2020, at 22:15, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> > 
> > On Wed, 14 Oct 2020, Julien Grall wrote:
> >> On 14/10/2020 17:03, Bertrand Marquis wrote:
> >>>> On 14 Oct 2020, at 12:35, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> >>>> wrote:
> >>>> 
> >>>> On 14/10/2020 11:41, Bertrand Marquis wrote:
> >>>>> When a Cortex A57 processor is affected by CPU errata 832075, a guest
> >>>>> not implementing the workaround for it could deadlock the system.
> >>>>> Add a warning during boot informing the user that only trusted guests
> >>>>> should be executed on the system.
> >>>>> An equivalent warning is already given to the user by KVM on cores
> >>>>> affected by this errata.
> >>>>> 
> >>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> >>>>> ---
> >>>>> xen/arch/arm/cpuerrata.c | 21 +++++++++++++++++++++
> >>>>> 1 file changed, 21 insertions(+)
> >>>>> 
> >>>>> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> >>>>> index 6c09017515..8f9ab6dde1 100644
> >>>>> --- a/xen/arch/arm/cpuerrata.c
> >>>>> +++ b/xen/arch/arm/cpuerrata.c
> >>>>> @@ -240,6 +240,26 @@ static int enable_ic_inv_hardening(void *data)
> >>>>> 
> >>>>> #endif
> >>>>> 
> >>>>> +#ifdef CONFIG_ARM64_ERRATUM_832075
> >>>>> +
> >>>>> +static int warn_device_load_acquire_errata(void *data)
> >>>>> +{
> >>>>> +    static bool warned = false;
> >>>>> +
> >>>>> +    if ( !warned )
> >>>>> +    {
> >>>>> +        warning_add("This CPU is affected by the errata 832075.\n"
> >>>>> +                    "Guests without required CPU erratum workarounds\n"
> >>>>> +                    "can deadlock the system!\n"
> >>>>> +                    "Only trusted guests should be used on this
> >>>>> system.\n");
> >>>>> +        warned = true;
> >>>> 
> >>>> This is an antipattern, which probably wants fixing elsewhere as well.
> >>>> 
> >>>> warning_add() is __init.  It's not legitimate to call from a non-init
> >>>> function, and a less useless build system would have modpost to object.
> >>>> 
> >>>> The ARM_SMCCC_ARCH_WORKAROUND_1 instance asserts based on system state,
> >>>> but this provides no safety at all.
> >>>> 
> >>>> 
> >>>> What warning_add() actually does is queue messages for some point near
> >>>> the end of boot.  It's not clear that this is even a clever thing to do.
> >>>> 
> >>>> I'm very tempted to suggest a blanket change to printk_once().
> >>> 
> >>> If this is needed then this could be done in an other serie ?
> >> 
> >> The callback ->enable() will be called when a CPU is onlined/offlined. So 
> >> this
> >> is going to require if you plan to support CPU hotplugs or suspend resume.
> >> 
> >>> Would be good to keep this patch as purely handling the errata.
> > 
> > My preference would be to keep this patch small with just the errata,
> > maybe using a simple printk_once as Andrew and Julien discussed.
> > 
> > There is another instance of warning_add potentially being called
> > outside __init in xen/arch/arm/cpuerrata.c:
> > enable_smccc_arch_workaround_1. So if you are up for it, it would be
> > good to produce a patch to fix that too.
> > 
> > 
> >> In the case of this patch, how about moving the warning_add() in
> >> enable_errata_workarounds()?
> >> 
> >> By then we should now all the errata present on your platform. All CPUs
> >> onlined afterwards (i.e. runtime) should always abide to the set discover
> >> during boot.
> > 
> > If I understand your suggestion correctly, it would work for
> > warn_device_load_acquire_errata, because it is just a warning, but it
> > would not work for enable_smccc_arch_workaround_1, because there is
> > actually a call to be made there.
> > 
> > Maybe it would be simpler to use printk_once in both cases? I don't have
> > a strong preference either way.
> 
> I could do the following (in a serie of 2 patches):
> - modify enable_smccc_arch_workaround_1 to use printk_once with a 
>   prefix/suffix “****” on each line printed (and maybe adapting print to fit 
> a 
>   line length of 80)
> - modify my patch to do the print in enable_errata_workarounds using also
>   the prefix/suffix and printk_once
> 
> Please confirm that this strategy would fit everyone.

I think it is OK but if you are going to use printk_once in your patch
you might as well leave it in the .enable implementation.

Julien, what do you think?

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.