Re: [Xen-devel] [PATCH RFC for-4.13 04/10] xen/arm: Ensure the SSBD workaround is re-enabled right after exiting a guest

[Julien Grall <julien.grall@xxxxxxx>]

On 27/09/2019 12:56, Volodymyr Babchuk wrote:
> Julien,
Hi...

> Julien Grall writes:
>
> > At the moment, SSBD workaround is re-enabled for Xen after interrupts
> > are unmasked. This means we may end up to execute some part of the
> > hypervisor if an interrupt is received before the workaround is
> > re-enabled.
> >
> > As the rest of enter_hypervisor_from_guest() does not require to have
> > interrupts masked, the function is now split in two parts:
> >      1) enter_hypervisor_from_guest_noirq() called with interrupts
> >         masked.
> I'm okay with this approach, but I don't like name for
> enter_hypervisor_from_guest_noirq(). Right now it is doing exactly one
> thing - mitigates SSBD. So, maybe more appropriate name will be
> something like "mitigate_ssbd()" ?
If I wanted to call it mitigate_ssbd() I would have implemented completely 
differently. The reason it is like that is because we may need more code to be 
added here in the future (I have Andrii's series in mind). So I would rather 
avoid a further renaming later on and some rework.
Regarding the name, this is a split of enter_hypervisor_from_guest(). Hence, why 
the first path is the same. The noirq merely help the user to know what to 
expect. This is better of yet an __ version. Feel free to suggest a better suffix.
> >      2) enter_hypervisor_from_guest() called with interrupts unmasked.
> >
> > Note that while enter_hypervisor_from_guest_noirq() does not use the
> > on-stack context registers, it is still passed as parameter to match the
> > rest of the C functions called from the entry path.
> As I pointed in the previous email, enter_hypervisor_from_guest() does
> not use on-stack registers as well.
I am well aware of this, hence my comment here in the commit message ;). The 
reason it is like that is because I wanted to keep the prototype the same for 
all functions called from the entry path (this includes do_trap_*).
[...]

> > ---
> >   xen/arch/arm/arm64/entry.S |  2 ++
> >   xen/arch/arm/traps.c       | 16 +++++++++++++---
> >   2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> > index 9eafae516b..458d12f188 100644
> > --- a/xen/arch/arm/arm64/entry.S
> > +++ b/xen/arch/arm/arm64/entry.S
> > @@ -173,6 +173,8 @@
> >           ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
> >                       "nop; nop",
> >                       SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> > +        mov     x0, sp
> > +        bl      enter_hypervisor_from_guest_noirq
> >           msr     daifclr, \iflags
> >           mov     x0, sp
> >           bl      enter_hypervisor_from_guest
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 20ba34ec91..5848dd8399 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -2007,16 +2007,26 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
> >   }
> >
> >   /*
> > - * Actions that needs to be done after exiting the guest and before any
> > - * request from it is handled.
> > + * Actions that needs to be done after exiting the guest and before the
> > + * interrupts are unmasked.
> >    */
> > -void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> > +void enter_hypervisor_from_guest_noirq(struct cpu_user_regs *regs)
> >   {
> >       struct vcpu *v = current;
> >
> >       /* If the guest has disabled the workaround, bring it back on. */
> >       if ( needs_ssbd_flip(v) )
> >           arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
> > +}
> > +
> > +/*
> > + * Actions that needs to be done after exiting the guest and before any
> > + * request from it is handled. Depending on the exception trap, this may
> > + * be called with interrupts unmasked.
> > + */
> > +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> > +{
> > +    struct vcpu *v = current;
> >
> >       /*
> >        * If we pended a virtual abort, preserve it until it gets cleared.
>
> --
> Volodymyr Babchuk at EPAM
Cheers,

--
Julien Grall

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