Re: [Xen-devel] [PATCH RFC for-4.13 03/10] xen/arm: traps: Rework entry/exit from the guest path

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 2 Oct 2019, Julien Grall wrote:
> Hi,
> 
> On 10/2/19 1:41 PM, Stefano Stabellini wrote:
> > On Tue, 1 Oct 2019, Stefano Stabellini wrote:
> > > On Tue, 1 Oct 2019, Julien Grall wrote:
> > > > Hi,
> > > > 
> > > > On 01/10/2019 21:12, Stefano Stabellini wrote:
> > > > > On Thu, 26 Sep 2019, Julien Grall wrote:
> > > > > > At the moment, enter_hypervisor_head() and leave_hypervisor_tail()
> > > > > > are
> > > > > > used to deal with actions to be done before/after any guest request
> > > > > > is
> > > > > > handled.
> > > > > > 
> > > > > > While they are meant to work in pair, the former is called for most
> > > > > > of
> > > > > > the traps, including traps from the same exception level (i.e.
> > > > > > hypervisor) whilst the latter will only be called when returning to
> > > > > > the
> > > > > > guest.
> > > > > > 
> > > > > > As pointed out, the enter_hypervisor_head() is not called from all
> > > > > > the
> > > > > > traps, so this makes potentially difficult to extend it for the
> > > > > > dealing
> > > > > > with same exception level.
> > > > > > 
> > > > > > Furthermore, some assembly only path will require to call
> > > > > > enter_hypervisor_tail(). So the function is now directly call by
> > > > > > assembly in for guest vector only. This means that the check whether
> > > > > > we
> > > > > > are called in a guest trap can now be removed.
> > > > > > 
> > > > > > Take the opportunity to rename enter_hypervisor_tail() and
> > > > > > leave_hypervisor_tail() to something more meaningful and document
> > > > > > them.
> > > > > > This should help everyone to understand the purpose of the two
> > > > > > functions.
> > > > > > 
> > > > > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > > > > 
> > > > > > ---
> > > > > > 
> > > > > > I haven't done the 32-bits part yet. I wanted to gather feedback
> > > > > > before
> > > > > > looking in details how to integrate that with Arm32.
> > > > > > ---
> > > > > >    xen/arch/arm/arm64/entry.S |  4 ++-
> > > > > >    xen/arch/arm/traps.c       | 71
> > > > > > ++++++++++++++++++++++------------------------
> > > > > >    2 files changed, 37 insertions(+), 38 deletions(-)
> > > > > > 
> > > > > > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> > > > > > index 40d9f3ec8c..9eafae516b 100644
> > > > > > --- a/xen/arch/arm/arm64/entry.S
> > > > > > +++ b/xen/arch/arm/arm64/entry.S
> > > > > > @@ -147,7 +147,7 @@
> > > > > >               .if \hyp == 0         /* Guest mode */
> > > > > >    -        bl      leave_hypervisor_tail /* Disables interrupts on
> > > > > > return */
> > > > > > +        bl      leave_hypervisor_to_guest /* Disables interrupts on
> > > > > > return */
> > > > > >               exit_guest \compat
> > > > > >    @@ -175,6 +175,8 @@
> > > > > >                        SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> > > > > >            msr     daifclr, \iflags
> > > > > >            mov     x0, sp
> > > > > > +        bl      enter_hypervisor_from_guest
> > > > > > +        mov     x0, sp
> > > > > >            bl      do_trap_\trap
> > > > > >    1:
> > > > > >            exit    hyp=0, compat=\compat
> > > > > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > > > > index a3b961bd06..20ba34ec91 100644
> > > > > > --- a/xen/arch/arm/traps.c
> > > > > > +++ b/xen/arch/arm/traps.c
> > > > > > @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct
> > > > > > vcpu *v)
> > > > > >                 cpu_require_ssbd_mitigation();
> > > > > >    }
> > > > > >    -static void enter_hypervisor_head(struct cpu_user_regs *regs)
> > > > > > +/*
> > > > > > + * Actions that needs to be done after exiting the guest and before
> > > > > > any
> > > > > > + * request from it is handled.
> > > > > > + */
> > > > > > +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > > -    if ( guest_mode(regs) )
> > > > > > -    {
> > > > > > -        struct vcpu *v = current;
> > > > > > +    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);
> > > > > > +    /* 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);
> > > > > >    -        /*
> > > > > > -         * If we pended a virtual abort, preserve it until it gets
> > > > > > cleared.
> > > > > > -         * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for
> > > > > > details,
> > > > > > -         * but the crucial bit is "On taking a vSError interrupt,
> > > > > > HCR_EL2.VSE
> > > > > > -         * (alias of HCR.VA) is cleared to 0."
> > > > > > -         */
> > > > > > -        if ( v->arch.hcr_el2 & HCR_VA )
> > > > > > -            v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> > > > > > +    /*
> > > > > > +     * If we pended a virtual abort, preserve it until it gets
> > > > > > cleared.
> > > > > > +     * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for
> > > > > > details,
> > > > > > +     * but the crucial bit is "On taking a vSError interrupt,
> > > > > > HCR_EL2.VSE
> > > > > > +     * (alias of HCR.VA) is cleared to 0."
> > > > > > +     */
> > > > > > +    if ( v->arch.hcr_el2 & HCR_VA )
> > > > > > +        v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> > > > > >       #ifdef CONFIG_NEW_VGIC
> > > > > > -        /*
> > > > > > -         * We need to update the state of our emulated devices
> > > > > > using level
> > > > > > -         * triggered interrupts before syncing back the VGIC state.
> > > > > > -         *
> > > > > > -         * TODO: Investigate whether this is necessary to do on
> > > > > > every
> > > > > > -         * trap and how it can be optimised.
> > > > > > -         */
> > > > > > -        vtimer_update_irqs(v);
> > > > > > -        vcpu_update_evtchn_irq(v);
> > > > > > +    /*
> > > > > > +     * We need to update the state of our emulated devices using
> > > > > > level
> > > > > > +     * triggered interrupts before syncing back the VGIC state.
> > > > > > +     *
> > > > > > +     * TODO: Investigate whether this is necessary to do on every
> > > > > > +     * trap and how it can be optimised.
> > > > > > +     */
> > > > > > +    vtimer_update_irqs(v);
> > > > > > +    vcpu_update_evtchn_irq(v);
> > > > > >    #endif
> > > > > >    -        vgic_sync_from_lrs(v);
> > > > > > -    }
> > > > > > +    vgic_sync_from_lrs(v);
> > > > > >    }
> > > > > >       void do_trap_guest_sync(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > >        const union hsr hsr = { .bits = regs->hsr };
> > > > > >    -    enter_hypervisor_head(regs);
> > > > > > -
> > > > > >        switch ( hsr.ec )
> > > > > >        {
> > > > > >        case HSR_EC_WFI_WFE:
> > > > > > @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs
> > > > > > *regs)
> > > > > >    {
> > > > > >        const union hsr hsr = { .bits = regs->hsr };
> > > > > >    -    enter_hypervisor_head(regs);
> > > > > > -
> > > > > >        switch ( hsr.ec )
> > > > > >        {
> > > > > >    #ifdef CONFIG_ARM_64
> > > > > > @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs
> > > > > > *regs)
> > > > > >       void do_trap_hyp_serror(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > > -    enter_hypervisor_head(regs);
> > > > > > -
> > > > > >        __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
> > > > > >    }
> > > > > >       void do_trap_guest_serror(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > > -    enter_hypervisor_head(regs);
> > > > > > -
> > > > > >        __do_trap_serror(regs, true);
> > > > > >    }
> > > > > >       void do_trap_irq(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > > -    enter_hypervisor_head(regs);
> > > > > >        gic_interrupt(regs, 0);
> > > > > >    }
> > > > > >       void do_trap_fiq(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > > -    enter_hypervisor_head(regs);
> > > > > >        gic_interrupt(regs, 1);
> > > > > >    }
> > > > > 
> > > > > I am OK with the general approach but one thing to note is that the
> > > > > fiq
> > > > > handler doesn't use the guest_vector macro at the moment.
> > > > 
> > > > do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode().
> > > > So I don't see an issue here.
> > > > 
> > > > As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler
> > > > does not use guest_vector.
> > > > 
> > > > That said, it is interesting to see that we don't deal the same way the
> > > > FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the
> > > > latter will crash the guest.
> > > > 
> > > > It would be good if we can have the same behavior accross the two arch
> > > > if possible. I vaguely recall someone (Andre?) mentioning some changes
> > > > around FIQ in KVM recently. Andre, are FIQ meant to work with Guest?
> > > > 
> > > > Also, a side effect of not calling enter_hypervisor_head() is workaround
> > > > are not re-enabled. We are going to panic soon after, so it may not be
> > > > that much an issue.
> > > 
> > > Right, that is what I was thinking too, but I wanted to highlight it. At
> > > least it would be worth adding a sentence to the commit message about
> > > it.
> > 
> > Actually on second thought, maybe we have to apply the workaround anyway
> > to identify/spot that the guest somehow managed to trigger a serror? I
> > mean: maybe it is important enough that we should let the user know.
> 
> I am sorry but I don't understand how this is related to this patch. There are
> strictly no change in the SError handling here.

That was my reflection on whether it would be a good idea or a bad idea
to have a SERROR check on the fiq hypervisor entries. Not necessarely in
this patch. Probably not in this patch.

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