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

> I will have a think about it.

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