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

[Julien Grall <Julien.Grall@xxxxxxx>]

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.

I will have a think about it.

Cheers,

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