[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.