Re: [Xen-devel] [PATCH 06/13] xen/arm: Add ARCH_WORKAROUND_2 support for guests

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 24 May 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 24/05/18 01:40, Stefano Stabellini wrote:
> > On Wed, 23 May 2018, Stefano Stabellini wrote:
> > > On Tue, 22 May 2018, Julien Grall wrote:
> > > > In order to offer ARCH_WORKAROUND_2 support to guests, we need to track
> > > > the
> > > > state of the workaround per-vCPU. The field 'pad' in cpu_info is now
> > > > repurposed to store flags easily accessible in assembly.
> > > > 
> > > > As the hypervisor will always run with the workaround enabled, we may
> > > > need to enable (on guest exit) or disable (on guest entry) the
> > > > workaround.
> > > > 
> > > > A follow-up patch will add fastpath for the workaround for arm64 guests.
> > > > 
> > > > This is part of XSA-263.
> > > > 
> > > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > > ---
> > > >   xen/arch/arm/domain.c         |  8 ++++++++
> > > >   xen/arch/arm/traps.c          | 20 ++++++++++++++++++++
> > > >   xen/arch/arm/vsmc.c           | 37
> > > > +++++++++++++++++++++++++++++++++++++
> > > >   xen/include/asm-arm/current.h |  6 +++++-
> > > >   4 files changed, 70 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > > index e7b33e92fb..9168195a9c 100644
> > > > --- a/xen/arch/arm/domain.c
> > > > +++ b/xen/arch/arm/domain.c
> > > > @@ -21,6 +21,7 @@
> > > >   #include <xen/wait.h>
> > > >     #include <asm/alternative.h>
> > > > +#include <asm/cpuerrata.h>
> > > >   #include <asm/cpufeature.h>
> > > >   #include <asm/current.h>
> > > >   #include <asm/event.h>
> > > > @@ -575,6 +576,13 @@ int vcpu_initialise(struct vcpu *v)
> > > >       if ( (rc = vcpu_vtimer_init(v)) != 0 )
> > > >           goto fail;
> > > >   +    /*
> > > > +     * The workaround 2 (i.e SSBD mitigation) is enabled by default if
> > > > +     * supported.
> > > > +     */
> > > > +    if ( get_ssbd_state() == ARM_SSBD_RUNTIME )
> > > > +        v->arch.cpu_info->flags |= CPUINFO_WORKAROUND_2_FLAG;
> > > > +
> > > >       return rc;
> > > >     fail:
> > > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > > index 5c18e918b0..020b0b8eef 100644
> > > > --- a/xen/arch/arm/traps.c
> > > > +++ b/xen/arch/arm/traps.c
> > > > @@ -2011,10 +2011,23 @@ inject_abt:
> > > >           inject_iabt_exception(regs, gva, hsr.len);
> > > >   }
> > > >   +static inline bool needs_ssbd_flip(struct vcpu *v)
> > > > +{
> > > > +    if ( !check_workaround_ssbd() )
> > > > +        return false;
> > > 
> > > Why not check on get_ssbd_state() == ARM_SSBD_RUNTIME? 
> 
> get_ssbd_state() would introduce an overhead for each entry/exit even on
> platform not affected. check_workaround_ssbd() remove this overhead by using
> an alternative.

Ah yes, great idea.


> > I am confused on
> > > when is the right time to use the cpu capability check
> > > (check_workaround_ssbd), when is the right time to call get_ssbd_state()
> > > and when is the right time to call cpu_require_ssbd_mitigation().
> 
> See my answer in the previous patches.

I understand now, I didn't realize we wanted to go into the details of
improving big.LITTLE scenarios with different erratas on the differnt
CPUs.


> > > 
> > > 
> > > > +    return !((v->arch.cpu_info->flags & CPUINFO_WORKAROUND_2_FLAG) &&
> > > > +             cpu_require_ssbd_mitigation());
> > > 
> > > It looks like this won't do as intended when v->arch.cpu_info->flags = 0
> > > and cpu_require_ssbd_mitigation() returns false, am I right?
> > > 
> > > Maybe needs_ssbd_flip() should be implemented as follows:
> > > 
> > >    return get_ssbd_state() == ARM_SSBD_RUNTIME &&
> > >      !(v->arch.cpu_info->flags & CPUINFO_WORKAROUND_2_FLAG)
> > 
> > With the intention of supporting systems where not all CPUs need/have
> > the workaround, then it should be:
> > 
> >     return cpu_require_ssbd_mitigation() &&
> >       !(v->arch.cpu_info->flags & CPUINFO_WORKAROUND_2_FLAG)
> 
> Yes, I did the exact same error I reported to Marc on the KVM side :/. I will
> update the patch.
 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel