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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> 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.

> > > +    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.
Cheers,

--
Julien Grall

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