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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 28/10/2020 08:43, Bertrand Marquis wrote:
> > On 27 Oct 2020, at 22:44, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> >
> > On Mon, 26 Oct 2020, 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.
> > >
> > > Also taint the hypervisor as unsecure when this errata applies and
> > > mention Cortex A57 r0p0 - r1p2 as not security supported in SUPPORT.md
> > >
> > > Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > > ---
> > > SUPPORT.md               |  1 +
> > > xen/arch/arm/cpuerrata.c | 13 +++++++++++++
> > > 2 files changed, 14 insertions(+)
> > >
> > > diff --git a/SUPPORT.md b/SUPPORT.md
> > > index 5fbe5fc444..f7a3b046b0 100644
> > > --- a/SUPPORT.md
> > > +++ b/SUPPORT.md
> > > @@ -38,6 +38,7 @@ supported in this document.
> > > ### ARM v8
> > >
> > >      Status: Supported
> > > +    Status, Cortex A57 r0p0 - r1p2, not security supported (Errata 832075)
> > >
> > > ## Host hardware support
> > >
> > > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> > > index 0430069a84..b35e8cd0b9 100644
> > > --- a/xen/arch/arm/cpuerrata.c
> > > +++ b/xen/arch/arm/cpuerrata.c
> > > @@ -503,6 +503,19 @@ void check_local_cpu_errata(void)
> > > void __init enable_errata_workarounds(void)
> > > {
> > >      enable_cpu_capabilities(arm_errata);
> > > +
> > > +#ifdef CONFIG_ARM64_ERRATUM_832075
> > > +    if ( cpus_have_cap(ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE) )
> > > +    {
> > > +        printk_once("**** This CPU is affected by the errata 832075. ****\n"
> > > +                    "**** Guests without CPU erratum workarounds     ****\n"
> > > +                    "**** can deadlock the system!                   ****\n"
> > > +                    "**** Only trusted guests should be used.        ****\n");
> > These can be on 2 lines, no need to be on 4 lines.
> I can fix that in a v3.
>
> > I know that Julien wrote about printing the warning from
> > enable_errata_workarounds but to me it looks more natural if we did it
> > from the .enable function specific to ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE.
> I have no preference either here but i kind of like this way because if we had 
> more warnings
> they would allow be at the same place.
So I add this placement in mind because the previous version was using 
warning_add() (It can't be called from non-init helper). As we are using 
printk_once() now, I don't really have a preference.
So I would stick with what you wrote.

Cheers,

--
Julien Grall