[Xen-devel] [PATCH RFC for-4.13 00/10] xen/arm: XSA-201 and XSA-263 fixes

[Xen-devel] [PATCH RFC for-4.13 00/10] xen/arm: XSA-201 and XSA-263 fixes

[Julien Grall]

Hi all,

This patch series aims to fix two bugs in the entry path from the guest:
    1) Make sure that SSBD workaround is enabled before executing any hypervisor code
    2) Avoid guest state corruption when an virtual SError is received

The full series is candidate for Xen 4.13. Without it, the hypervisor would
not be properly protected against SSB vulnerability and the guest state may
get corrupted if an SError is received.

This is in RFC state because the entry code is now quite different and
arm32 changes are not yet implemented. I will modify arm32 once we agreed
on the approach.

Cheers,

Cc: jgross@suse.com

Julien Grall (9):
  xen/arm64: entry: Introduce a macro to generate guest vector and use
    it
  xen/arm64: head: Check if an SError is pending when receiving a
    vSError
  xen/arm: traps: Rework entry/exit from the guest path
  xen/arm: Ensure the SSBD workaround is re-enabled right after exiting
    a guest
  xen/arm: alternative: Remove unused parameter for
    alternative_if_not_cap
  xen/arm: Move ARCH_PATCH_INSN_SIZE out of the header livepatch.h
  xen/arm: Allow insn.h to be called from assembly
  xen/arm: asm: Replace use of ALTERNATIVE with alternative_if
  xen/arm64: entry: Ensure the guest state is synced when receiving a
    vSError

Mark Rutland (1):
  xen/arm: alternative: add auto-nop infrastructure

 xen/arch/arm/alternative.c        |   2 -
 xen/arch/arm/arm32/entry.S        |   9 ++-
 xen/arch/arm/arm64/entry.S        | 121 +++++++++++++++-----------------------
 xen/arch/arm/traps.c              |  81 +++++++++++++------------
 xen/include/asm-arm/alternative.h |  74 ++++++++++++++++-------
 xen/include/asm-arm/insn.h        |  11 ++++
 xen/include/asm-arm/livepatch.h   |   4 +-
 xen/include/asm-arm/macros.h      |   7 +++
 8 files changed, 172 insertions(+), 137 deletions(-)

-- 
2.11.0


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

[Xen-devel] [PATCH RFC for-4.13 01/10] xen/arm64: entry: Introduce a macro to generate guest vector and use it

[Julien Grall]

Most of the guest vectors are using the same pattern. This makes fairly
tedious to alter the pattern and risk introducing mistakes when updating
each path.

A new macro is introduced to generate the guest vectors and now use it
in the one that use the open-code version.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/entry.S | 84 ++++++++++++++++------------------------------
 1 file changed, 28 insertions(+), 56 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 2d9a2713a1..8665d2844a 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -157,6 +157,30 @@
 
         .endm
 
+        /*
+         * Generate a guest vector.
+         *
+         * iflags: Correspond to the list of interrupts to unmask
+         * save_x0_x1: See the description on top of the macro 'entry'
+         */
+        .macro  guest_vector compat, iflags, trap, save_x0_x1=1
+        entry   hyp=0, compat=\compat, save_x0_x1=\save_x0_x1
+        /*
+         * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
+         * is not set. If a vSError took place, the initial exception will be
+         * skipped. Exit ASAP
+         */
+        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
+                    "nop; nop",
+                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
+        msr     daifclr, \iflags
+        mov     x0, sp
+        bl      do_trap_\trap
+1:
+        exit    hyp=0, compat=\compat
+        .endm
+
+
 /*
  * Bad Abort numbers
  *-----------------
@@ -290,36 +314,10 @@ guest_sync_slowpath:
          * x0/x1 may have been scratch by the fast path above, so avoid
          * to save them.
          */
-        entry   hyp=0, compat=0, save_x0_x1=0
-        /*
-         * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
-         * is not set. If a vSError took place, the initial exception will be
-         * skipped. Exit ASAP
-         */
-        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
-                    "nop; nop",
-                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
-        msr     daifclr, #6
-        mov     x0, sp
-        bl      do_trap_guest_sync
-1:
-        exit    hyp=0, compat=0
+        guest_vector compat=0, iflags=6, trap=guest_sync, save_x0_x1=0
 
 guest_irq:
-        entry   hyp=0, compat=0
-        /*
-         * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
-         * is not set. If a vSError took place, the initial exception will be
-         * skipped. Exit ASAP
-         */
-        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
-                    "nop; nop",
-                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
-        msr     daifclr, #4
-        mov     x0, sp
-        bl      do_trap_irq
-1:
-        exit    hyp=0, compat=0
+        guest_vector compat=0, iflags=4, trap=irq
 
 guest_fiq_invalid:
         entry   hyp=0, compat=0
@@ -333,36 +331,10 @@ guest_error:
         exit    hyp=0, compat=0
 
 guest_sync_compat:
-        entry   hyp=0, compat=1
-        /*
-         * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
-         * is not set. If a vSError took place, the initial exception will be
-         * skipped. Exit ASAP
-         */
-        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
-                    "nop; nop",
-                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
-        msr     daifclr, #6
-        mov     x0, sp
-        bl      do_trap_guest_sync
-1:
-        exit    hyp=0, compat=1
+        guest_vector compat=1, iflags=6, trap=guest_sync
 
 guest_irq_compat:
-        entry   hyp=0, compat=1
-        /*
-         * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
-         * is not set. If a vSError took place, the initial exception will be
-         * skipped. Exit ASAP
-         */
-        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
-                    "nop; nop",
-                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
-        msr     daifclr, #4
-        mov     x0, sp
-        bl      do_trap_irq
-1:
-        exit    hyp=0, compat=1
+        guest_vector compat=1, iflags=4, trap=irq
 
 guest_fiq_invalid_compat:
         entry   hyp=0, compat=1
-- 
2.11.0


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

[Xen-devel] [PATCH RFC for-4.13 02/10] xen/arm64: head: Check if an SError is pending when receiving a vSError

[Julien Grall]

At the moment, when we receive an SError exception from the guest, we
don't check if there are any other pending. For hardening the code, we
should ensure any pending SError are accounted to the guest before
executing any code with SError unmasked.

The recently introduced macro 'guest_vector' could used to generate the
two vectors and therefore take advantage of any change required in the
future.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/entry.S | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 8665d2844a..40d9f3ec8c 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -324,11 +324,7 @@ guest_fiq_invalid:
         invalid BAD_FIQ
 
 guest_error:
-        entry   hyp=0, compat=0
-        msr     daifclr, #6
-        mov     x0, sp
-        bl      do_trap_guest_serror
-        exit    hyp=0, compat=0
+        guest_vector compat=0, iflags=6, trap=guest_serror
 
 guest_sync_compat:
         guest_vector compat=1, iflags=6, trap=guest_sync
@@ -341,11 +337,7 @@ guest_fiq_invalid_compat:
         invalid BAD_FIQ
 
 guest_error_compat:
-        entry   hyp=0, compat=1
-        msr     daifclr, #6
-        mov     x0, sp
-        bl      do_trap_guest_serror
-        exit    hyp=0, compat=1
+        guest_vector compat=1, iflags=6, trap=guest_serror
 
 ENTRY(return_to_new_vcpu32)
         exit    hyp=0, compat=1
-- 
2.11.0


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

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

[Julien Grall]

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@arm.com>

---

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);
 }
 
@@ -2281,7 +2272,13 @@ static void check_for_vcpu_work(void)
     local_irq_disable();
 }
 
-void leave_hypervisor_tail(void)
+/*
+ * Actions that needs to be done before entering the guest. This is the
+ * last thing executed before the guest context is fully restored.
+ *
+ * The function will return with interrupts disabled.
+ */
+void leave_hypervisor_to_guest(void)
 {
     local_irq_disable();
 
-- 
2.11.0


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

[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]

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

Fixes: a7898e4c59 ("xen/arm: Add ARCH_WORKAROUND_2 support for guests")
Reported-by: Andrii Anisov <andrii_anisov@epam.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>

---

Note the Arm32 code has not been changed yet. I am also open on turn
both enter_hypervisor_from_guest_noirq() and enter_hypervisor_from()
to functions not taking any parameters.
---
 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.
-- 
2.11.0


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

[Xen-devel] [PATCH RFC for-4.13 05/10] xen/arm: alternative: Remove unused parameter for alternative_if_not_cap

[Julien Grall]

The macro alternative_if_not_cap is taking two parameters. The second
parameter is never used and it is hard to see how this can be used
correctly as it is only protecting the alternative section magic.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/alternative.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index dedb6dd001..2830a6da2d 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -116,13 +116,11 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
  * The code that follows this macro will be assembled and linked as
  * normal. There are no restrictions on this code.
  */
-.macro alternative_if_not cap, enable = 1
-	.if \enable
+.macro alternative_if_not cap
 	.pushsection .altinstructions, "a"
 	altinstruction_entry 661f, 663f, \cap, 662f-661f, 664f-663f
 	.popsection
 661:
-	.endif
 .endm
 
 /*
-- 
2.11.0


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

[Xen-devel] [PATCH RFC for-4.13 06/10] xen/arm: Move ARCH_PATCH_INSN_SIZE out of the header livepatch.h

[Julien Grall]

At the moment, ARCH_PATCH_INSN_SIZE is defined in the header
livepatch.h. However, this is also used in the alternative code.

Rather than including livepatch.h just for using the define, move it in
the header insn.h which seems more suitable.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/alternative.c      | 2 --
 xen/include/asm-arm/insn.h      | 3 +++
 xen/include/asm-arm/livepatch.h | 4 +---
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 52ed7edf69..237c4e5642 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -30,8 +30,6 @@
 #include <asm/byteorder.h>
 #include <asm/cpufeature.h>
 #include <asm/insn.h>
-/* XXX: Move ARCH_PATCH_INSN_SIZE out of livepatch.h */
-#include <asm/livepatch.h>
 #include <asm/page.h>
 
 /* Override macros from asm/page.h to make them work with mfn_t */
diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
index 3489179826..19277212e1 100644
--- a/xen/include/asm-arm/insn.h
+++ b/xen/include/asm-arm/insn.h
@@ -11,6 +11,9 @@
 # error "unknown ARM variant"
 #endif
 
+/* On ARM32,64 instructions are always 4 bytes long. */
+#define ARCH_PATCH_INSN_SIZE 4
+
 #endif /* !__ARCH_ARM_INSN */
 /*
  * Local variables:
diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h
index 6bca79deb9..026af5e7dc 100644
--- a/xen/include/asm-arm/livepatch.h
+++ b/xen/include/asm-arm/livepatch.h
@@ -7,9 +7,7 @@
 #define __XEN_ARM_LIVEPATCH_H__
 
 #include <xen/sizes.h> /* For SZ_* macros. */
-
-/* On ARM32,64 instructions are always 4 bytes long. */
-#define ARCH_PATCH_INSN_SIZE 4
+#include <asm/insn.h>
 
 /*
  * The va of the hypervisor .text region. We need this as the
-- 
2.11.0


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

[Xen-devel] [PATCH RFC for-4.13 07/10] xen/arm: Allow insn.h to be called from assembly

[Julien Grall]

A follow-up patch will require to include insn.h from assembly code. So
wee need to protect any C-specific definition to avoid compilation
error when used in assembly code.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/insn.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
index 19277212e1..00391f83f9 100644
--- a/xen/include/asm-arm/insn.h
+++ b/xen/include/asm-arm/insn.h
@@ -1,8 +1,14 @@
 #ifndef __ARCH_ARM_INSN
 #define __ARCH_ARM_INSN
 
+#ifndef __ASSEMBLY__
+
 #include <xen/types.h>
 
+/*
+ * At the moment, arch-specific headers contain only definition for C
+ * code.
+ */
 #if defined(CONFIG_ARM_64)
 # include <asm/arm64/insn.h>
 #elif defined(CONFIG_ARM_32)
@@ -11,6 +17,8 @@
 # error "unknown ARM variant"
 #endif
 
+#endif /* __ASSEMBLY__ */
+
 /* On ARM32,64 instructions are always 4 bytes long. */
 #define ARCH_PATCH_INSN_SIZE 4
 
-- 
2.11.0


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

[Xen-devel] [PATCH RFC for-4.13 08/10] xen/arm: alternative: add auto-nop infrastructure

[Julien Grall]

From: Mark Rutland <mark.rutland@arm.com>

In some cases, one side of an alternative sequence is simply a number of
NOPs used to balance the other side. Keeping track of this manually is
tedious, and the presence of large chains of NOPs makes the code more
painful to read than necessary.

To ameliorate matters, this patch adds a new alternative_else_nop_endif,
which automatically balances an alternative sequence with a trivial NOP
sled.

In many cases, we would like a NOP-sled in the default case, and
instructions patched in in the presence of a feature. To enable the NOPs
to be generated automatically for this case, this patch also adds a new
alternative_if, and updates alternative_else and alternative_endif to
work with either alternative_if or alternative_endif.

The alternative infrastructure was originally ported from Linux. So this
is pretty much a straight backport from commit 792d47379f4d "arm64:
alternative: add auto-nop infrastructure". The only difference is the
nops macro added as not yet existing in Xen.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
[will: use new nops macro to generate nop sequences]
Signed-off-by: Will Deacon <will.deacon@arm.com>
[julien: Add nops and port to Xen]
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/alternative.h | 70 +++++++++++++++++++++++++++++----------
 xen/include/asm-arm/macros.h      |  7 ++++
 2 files changed, 60 insertions(+), 17 deletions(-)

diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index 2830a6da2d..e8271ac04e 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -2,6 +2,7 @@
 #define __ASM_ALTERNATIVE_H
 
 #include <asm/cpufeature.h>
+#include <asm/insn.h>
 
 #define ARM_CB_PATCH ARM_NCAPS
 
@@ -111,34 +112,55 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
 .endm
 
 /*
- * Begin an alternative code sequence.
+ * Alternative sequences
+ *
+ * The code for the case where the capability is not present will be
+ * assembled and linked as normal. There are no restrictions on this
+ * code.
+ *
+ * The code for the case where the capability is present will be
+ * assembled into a special section to be used for dynamic patching.
+ * Code for that case must:
+ *
+ * 1. Be exactly the same length (in bytes) as the default code
+ *    sequence.
  *
- * The code that follows this macro will be assembled and linked as
- * normal. There are no restrictions on this code.
+ * 2. Not contain a branch target that is used outside of the
+ *    alternative sequence it is defined in (branches into an
+ *    alternative sequence are not fixed up).
+ */
+
+/*
+ * Begin an alternative code sequence.
  */
 .macro alternative_if_not cap
+	.set .Lasm_alt_mode, 0
 	.pushsection .altinstructions, "a"
 	altinstruction_entry 661f, 663f, \cap, 662f-661f, 664f-663f
 	.popsection
 661:
 .endm
 
+.macro alternative_if cap
+	.set .Lasm_alt_mode, 1
+	.pushsection .altinstructions, "a"
+	altinstruction_entry 663f, 661f, \cap, 664f-663f, 662f-661f
+	.popsection
+	.pushsection .altinstr_replacement, "ax"
+	.align 2	/* So GAS knows label 661 is suitably aligned */
+661:
+.endm
+
 /*
- * Provide the alternative code sequence.
- *
- * The code that follows this macro is assembled into a special
- * section to be used for dynamic patching. Code that follows this
- * macro must:
- *
- * 1. Be exactly the same length (in bytes) as the default code
- *    sequence.
- *
- * 2. Not contain a branch target that is used outside of the
- *    alternative sequence it is defined in (branches into an
- *    alternative sequence are not fixed up).
+ * Provide the other half of the alternative code sequence.
  */
 .macro alternative_else
-662:	.pushsection .altinstr_replacement, "ax"
+662:
+	.if .Lasm_alt_mode==0
+	.pushsection .altinstr_replacement, "ax"
+	.else
+	.popsection
+	.endif
 663:
 .endm
 
@@ -154,12 +176,26 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
  * Complete an alternative code sequence.
  */
 .macro alternative_endif
-664:	.popsection
+664:
+	.if .Lasm_alt_mode==0
+	.popsection
+	.endif
 	.org	. - (664b-663b) + (662b-661b)
 	.org	. - (662b-661b) + (664b-663b)
 .endm
 
 /*
+ * Provides a trivial alternative or default sequence consisting solely
+ * of NOPs. The number of NOPs is chosen automatically to match the
+ * previous case.
+ */
+.macro alternative_else_nop_endif
+alternative_else
+	nops	(662b-661b) / ARCH_PATCH_INSN_SIZE
+alternative_endif
+.endm
+
+/*
  * Callback-based alternative epilogue
  */
 .macro alternative_cb_end
diff --git a/xen/include/asm-arm/macros.h b/xen/include/asm-arm/macros.h
index 1d4bb41d15..91ea3505e4 100644
--- a/xen/include/asm-arm/macros.h
+++ b/xen/include/asm-arm/macros.h
@@ -13,4 +13,11 @@
 # error "unknown ARM variant"
 #endif
 
+    /* NOP sequence  */
+    .macro nops, num
+    .rept   \num
+    nop
+    .endr
+    .endm
+
 #endif /* __ASM_ARM_MACROS_H */
-- 
2.11.0


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

[Xen-devel] [PATCH RFC for-4.13 09/10] xen/arm: asm: Replace use of ALTERNATIVE with alternative_if

[Julien Grall]

Using alternative_if makes the code a bit more streamlined.

Take the opportunity to use the new auto-nop infrastructure to avoid
counting the number of nop in the else part for arch/arm/arm64/entry.S

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    This is pretty much a matter of taste, but at least for arm64 this
    allows us to use the auto-nop infrastructure. So the arm32 is more
    to keep inline with arm64.
---
 xen/arch/arm/arm32/entry.S | 9 ++++++---
 xen/arch/arm/arm64/entry.S | 8 +++++---
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 0b4cd19abd..1428cd3583 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -65,9 +65,12 @@ save_guest_regs:
          * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
          * feature, the checking of pending SErrors will be skipped.
          */
-        ALTERNATIVE("nop",
-                    "b skip_check",
-                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
+        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
+        nop
+        alternative_else
+        b   skip_check
+        alternative_endif
+
         /*
          * Start to check pending virtual abort in the gap of Guest -> HYP
          * world switch.
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 458d12f188..91cf6ee6f4 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -170,9 +170,11 @@
          * is not set. If a vSError took place, the initial exception will be
          * skipped. Exit ASAP
          */
-        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
-                    "nop; nop",
-                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
+        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
+        bl      check_pending_vserror
+        cbnz    x0, 1f
+        alternative_else_nop_endif
+
         mov     x0, sp
         bl      enter_hypervisor_from_guest_noirq
         msr     daifclr, \iflags
-- 
2.11.0


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

[Xen-devel] [PATCH RFC for-4.13 10/10] xen/arm64: entry: Ensure the guest state is synced when receiving a vSError

[Julien Grall]

At the moment, when a SError is received while checking for a pending
one, we will skip the handling the initial exception.

This includes call to exit_from_guest{, _noirq} that is used to
synchronize part of the guest state with the internal representation.
However, we still call leave_hypervisor_tail() which is used for preempting
the guest and synchronizing back part of the guest state.

exit_from_guest{, _noirq} works in pair with leave_hypervisor_tail(), so
skipping if former may result to a loss of some part of  guest state.
An example is the new vGIC which will save the state of the LRS on exit
from the guest and rewrite all of them on entry to the guest.

For now, calling leave_hypervisor_tail() is not necessary when injecting
a vSError to the guest. But as the path is spread accross multiple file,
it is hard to enforce that for the future (someone we may want to crash the
domain). Therefore it is best to call exit_from_guest{, _noirq} in the
vSError path as well.

Note that the return value of check_pending_vserror is now set in x19
instead of x0. This is because we want to keep the value across call to
C-function and x0, unlike x19, will not be saved by the callee.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

I am not aware of any issues other than with the new vGIC. But I
haven't looked hard enough so I think it would be worth to try to fix it
for Xen 4.13.
---
 xen/arch/arm/arm64/entry.S | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 91cf6ee6f4..f5350247e1 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -168,11 +168,13 @@
         /*
          * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
          * is not set. If a vSError took place, the initial exception will be
-         * skipped. Exit ASAP
+         * skipped.
+         *
+         * However, we still need to call exit_from_guest{,_noirq} as the
+         * return path to the guest may rely on state saved by them.
          */
         alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
         bl      check_pending_vserror
-        cbnz    x0, 1f
         alternative_else_nop_endif
 
         mov     x0, sp
@@ -180,6 +182,11 @@
         msr     daifclr, \iflags
         mov     x0, sp
         bl      enter_hypervisor_from_guest
+
+        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
+        cbnz    x19, 1f
+        alternative_else_nop_endif
+
         mov     x0, sp
         bl      do_trap_\trap
 1:
@@ -383,9 +390,9 @@ return_from_trap:
 /*
  * This function is used to check pending virtual SError in the gap of
  * EL1 -> EL2 world switch.
- * The x0 register will be used to indicate the results of detection.
- * x0 -- Non-zero indicates a pending virtual SError took place.
- * x0 -- Zero indicates no pending virtual SError took place.
+ * The register x19 will be used to indicate the results of detection.
+ * x19 -- Non-zero indicates a pending virtual SError took place.
+ * x19 -- Zero indicates no pending virtual SError took place.
  */
 check_pending_vserror:
         /*
@@ -432,9 +439,9 @@ abort_guest_exit_end:
 
         /*
          * Not equal, the pending SError exception took place, set
-         * x0 to non-zero.
+         * x19 to non-zero.
          */
-        cset    x0, ne
+        cset    x19, ne
 
         ret
 
-- 
2.11.0


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

Re: [Xen-devel] [PATCH RFC for-4.13 00/10] xen/arm: XSA-201 and XSA-263 fixes

[Jürgen Groß]

On 26.09.19 20:37, Julien Grall wrote:
> Hi all,
> 
> This patch series aims to fix two bugs in the entry path from the guest:
>      1) Make sure that SSBD workaround is enabled before executing any hypervisor code
>      2) Avoid guest state corruption when an virtual SError is received
> 
> The full series is candidate for Xen 4.13. Without it, the hypervisor would
> not be properly protected against SSB vulnerability and the guest state may
> get corrupted if an SError is received.
> 
> This is in RFC state because the entry code is now quite different and
> arm32 changes are not yet implemented. I will modify arm32 once we agreed
> on the approach.
> 
> Cheers,
> 
> Cc: jgross@suse.com

I think the explanation of the motivation qualifies the series to be
marked as a blocker for 4.13.


Juergen

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

Re: [Xen-devel] [PATCH RFC for-4.13 01/10] xen/arm64: entry: Introduce a macro to generate guest vector and use it

[Volodymyr Babchuk]

Hello Julien,

Julien Grall writes:

> Most of the guest vectors are using the same pattern. This makes fairly
> tedious to alter the pattern and risk introducing mistakes when updating
> each path.
>
> A new macro is introduced to generate the guest vectors and now use it
> in the one that use the open-code version.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RFC for-4.13 02/10] xen/arm64: head: Check if an SError is pending when receiving a vSError

[Volodymyr Babchuk]

Julien Grall writes:

> At the moment, when we receive an SError exception from the guest, we
> don't check if there are any other pending. For hardening the code, we
> should ensure any pending SError are accounted to the guest before
> executing any code with SError unmasked.
>
> The recently introduced macro 'guest_vector' could used to generate the
> two vectors and therefore take advantage of any change required in the
> future.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Volodymyr Babchuk]

Julien,

Julien Grall writes:

> 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@arm.com>
>
> ---
>
> I haven't done the 32-bits part yet. I wanted to gather feedback before
> looking in details how to integrate that with Arm32.
I'm looking at patches one by one and it is looking okay so far.


> ---
>  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
Looks like this mov can be removed (see commend below).

> +        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.
Maybe it is me only, but the phrasing is confusing. I had to read it two
times before I get it. What about "Actions that needs to be done when
raising exception level"? Or maybe "Actions that needs to be done when
switching from guest to hypervisor mode" ?

> + */
> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
With the guest_mode(regs) check removal , this function does not use regs
anymore.

>  {
> -    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);
>  }
>
> @@ -2281,7 +2272,13 @@ static void check_for_vcpu_work(void)
>      local_irq_disable();
>  }
>
> -void leave_hypervisor_tail(void)
> +/*
> + * Actions that needs to be done before entering the guest. This is the
> + * last thing executed before the guest context is fully restored.
> + *
> + * The function will return with interrupts disabled.
> + */
> +void leave_hypervisor_to_guest(void)
>  {
>      local_irq_disable();


--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RFC for-4.13 05/10] xen/arm: alternative: Remove unused parameter for alternative_if_not_cap

[Volodymyr Babchuk]

Julien Grall writes:

> The macro alternative_if_not_cap is taking two parameters. The second
> parameter is never used and it is hard to see how this can be used
> correctly as it is only protecting the alternative section magic.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

> ---
>  xen/include/asm-arm/alternative.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
> index dedb6dd001..2830a6da2d 100644
> --- a/xen/include/asm-arm/alternative.h
> +++ b/xen/include/asm-arm/alternative.h
> @@ -116,13 +116,11 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
>   * The code that follows this macro will be assembled and linked as
>   * normal. There are no restrictions on this code.
>   */
> -.macro alternative_if_not cap, enable = 1
> -	.if \enable
> +.macro alternative_if_not cap
>  	.pushsection .altinstructions, "a"
>  	altinstruction_entry 661f, 663f, \cap, 662f-661f, 664f-663f
>  	.popsection
>  661:
> -	.endif
>  .endm
>  
>  /*


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RFC for-4.13 06/10] xen/arm: Move ARCH_PATCH_INSN_SIZE out of the header livepatch.h

[Volodymyr Babchuk]

Julien Grall writes:

> At the moment, ARCH_PATCH_INSN_SIZE is defined in the header
> livepatch.h. However, this is also used in the alternative code.
>
> Rather than including livepatch.h just for using the define, move it in
> the header insn.h which seems more suitable.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RFC for-4.13 07/10] xen/arm: Allow insn.h to be called from assembly

[Volodymyr Babchuk]

Julien Grall writes:

> A follow-up patch will require to include insn.h from assembly code. So
> wee need to protect any C-specific definition to avoid compilation
> error when used in assembly code.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
>  xen/include/asm-arm/insn.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
> index 19277212e1..00391f83f9 100644
> --- a/xen/include/asm-arm/insn.h
> +++ b/xen/include/asm-arm/insn.h
> @@ -1,8 +1,14 @@
>  #ifndef __ARCH_ARM_INSN
>  #define __ARCH_ARM_INSN
>  
> +#ifndef __ASSEMBLY__
> +
>  #include <xen/types.h>
>  
> +/*
> + * At the moment, arch-specific headers contain only definition for C
> + * code.
> + */
>  #if defined(CONFIG_ARM_64)
>  # include <asm/arm64/insn.h>
>  #elif defined(CONFIG_ARM_32)
> @@ -11,6 +17,8 @@
>  # error "unknown ARM variant"
>  #endif
>  
> +#endif /* __ASSEMBLY__ */
> +
>  /* On ARM32,64 instructions are always 4 bytes long. */
>  #define ARCH_PATCH_INSN_SIZE 4


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Volodymyr Babchuk]

Julien,

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()" ?

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

> Fixes: a7898e4c59 ("xen/arm: Add ARCH_WORKAROUND_2 support for guests")
> Reported-by: Andrii Anisov <andrii_anisov@epam.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
>
> Note the Arm32 code has not been changed yet. I am also open on turn
> both enter_hypervisor_from_guest_noirq() and enter_hypervisor_from()
> to functions not taking any parameters.
That would be appropriate in my opinion.

> ---
>  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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RFC for-4.13 06/10] xen/arm: Move ARCH_PATCH_INSN_SIZE out of the header livepatch.h

[Ross Lagerwall]

On 9/26/19 7:38 PM, Julien Grall wrote:
> At the moment, ARCH_PATCH_INSN_SIZE is defined in the header
> livepatch.h. However, this is also used in the alternative code.
> 
> Rather than including livepatch.h just for using the define, move it in
> the header insn.h which seems more suitable.
> 
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

Re: [Xen-devel] [PATCH RFC for-4.13 09/10] xen/arm: asm: Replace use of ALTERNATIVE with alternative_if

[Volodymyr Babchuk]

Julien Grall writes:

> Using alternative_if makes the code a bit more streamlined.
>
> Take the opportunity to use the new auto-nop infrastructure to avoid
> counting the number of nop in the else part for arch/arm/arm64/entry.S
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
>     This is pretty much a matter of taste, but at least for arm64 this
>     allows us to use the auto-nop infrastructure. So the arm32 is more
>     to keep inline with arm64.
> ---
>  xen/arch/arm/arm32/entry.S | 9 ++++++---
>  xen/arch/arm/arm64/entry.S | 8 +++++---
>  2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 0b4cd19abd..1428cd3583 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -65,9 +65,12 @@ save_guest_regs:
>           * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
>           * feature, the checking of pending SErrors will be skipped.
>           */
> -        ALTERNATIVE("nop",
> -                    "b skip_check",
> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> +        nop
> +        alternative_else
> +        b   skip_check
> +        alternative_endif
> +
for the arm32 code you can have my r-b:
Reviewed-By: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

>          /*
>           * Start to check pending virtual abort in the gap of Guest -> HYP
>           * world switch.
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 458d12f188..91cf6ee6f4 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -170,9 +170,11 @@
>           * is not set. If a vSError took place, the initial exception will be
>           * skipped. Exit ASAP
>           */
> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
> -                    "nop; nop",
> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> +        bl      check_pending_vserror
> +        cbnz    x0, 1f
> +        alternative_else_nop_endif
> +
You asked other people to do not introduce new code in one patch and
rewrite it in the following patch. But there you are doing exactly the
same. I believe, it is possible to move all "alternative" patches to the
very beginning of the patch series and only then introduce macro
guest_vector.

>          mov     x0, sp
>          bl      enter_hypervisor_from_guest_noirq
>          msr     daifclr, \iflags


--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Julien Grall]

On 27/09/2019 12:45, Volodymyr Babchuk wrote:
> 
> Julien,

Hi...

> Julien Grall writes:
> 
>> 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@arm.com>
>>
>> ---
>>
>> I haven't done the 32-bits part yet. I wanted to gather feedback before
>> looking in details how to integrate that with Arm32.
> I'm looking at patches one by one and it is looking okay so far.
> 
> 
>> ---
>>   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
> Looks like this mov can be removed (see commend below).
> 
>> +        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.
> Maybe it is me only, but the phrasing is confusing. I had to read it two
> times before I get it. What about "Actions that needs to be done when
> raising exception level"? Or maybe "Actions that needs to be done when
> switching from guest to hypervisor mode" ?

Is it a suggestion to replace the full sentence or just the first before (i.e. 
before 'and')?

> 
>> + */
>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> With the guest_mode(regs) check removal , this function does not use regs
> anymore.

I have nearly done it while working on the series, but then I thought that it 
would be better keep all the functions called from the entry path in assembly 
similar.

Cheers,

-- 
Julien Grall

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

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]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Volodymyr Babchuk]

Julien Grall writes:

> On 27/09/2019 12:45, Volodymyr Babchuk wrote:
>>
>> Julien,
>
> Hi...
>
>> Julien Grall writes:
>>
>>> 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@arm.com>
>>>
>>> ---
>>>
>>> I haven't done the 32-bits part yet. I wanted to gather feedback before
>>> looking in details how to integrate that with Arm32.
>> I'm looking at patches one by one and it is looking okay so far.
>>
>>
>>> ---
>>>   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
>> Looks like this mov can be removed (see commend below).
>>
>>> +        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.
>> Maybe it is me only, but the phrasing is confusing. I had to read it two
>> times before I get it. What about "Actions that needs to be done when
>> raising exception level"? Or maybe "Actions that needs to be done when
>> switching from guest to hypervisor mode" ?
>
> Is it a suggestion to replace the full sentence or just the first
> before (i.e. before 'and')?
This is a suggestion for the first part.

>>
>>> + */
>>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
>> With the guest_mode(regs) check removal , this function does not use regs
>> anymore.
>
> I have nearly done it while working on the series, but then I thought
> that it would be better keep all the functions called from the entry
> path in assembly similar.
This can save one assembly instruction in the entry path. But I'm not
sure if it is worth it. So it is up to you.

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RFC for-4.13 09/10] xen/arm: asm: Replace use of ALTERNATIVE with alternative_if

[Julien Grall]

Hi,

On 27/09/2019 13:11, Volodymyr Babchuk wrote:
> 
> 
> Julien Grall writes:
> 
>> Using alternative_if makes the code a bit more streamlined.
>>
>> Take the opportunity to use the new auto-nop infrastructure to avoid
>> counting the number of nop in the else part for arch/arm/arm64/entry.S
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      This is pretty much a matter of taste, but at least for arm64 this
>>      allows us to use the auto-nop infrastructure. So the arm32 is more
>>      to keep inline with arm64.
>> ---
>>   xen/arch/arm/arm32/entry.S | 9 ++++++---
>>   xen/arch/arm/arm64/entry.S | 8 +++++---
>>   2 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>> index 0b4cd19abd..1428cd3583 100644
>> --- a/xen/arch/arm/arm32/entry.S
>> +++ b/xen/arch/arm/arm32/entry.S
>> @@ -65,9 +65,12 @@ save_guest_regs:
>>            * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
>>            * feature, the checking of pending SErrors will be skipped.
>>            */
>> -        ALTERNATIVE("nop",
>> -                    "b skip_check",
>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>> +        nop
>> +        alternative_else
>> +        b   skip_check
>> +        alternative_endif
>> +
> for the arm32 code you can have my r-b:
> Reviewed-By: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
>>           /*
>>            * Start to check pending virtual abort in the gap of Guest -> HYP
>>            * world switch.
>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>> index 458d12f188..91cf6ee6f4 100644
>> --- a/xen/arch/arm/arm64/entry.S
>> +++ b/xen/arch/arm/arm64/entry.S
>> @@ -170,9 +170,11 @@
>>            * is not set. If a vSError took place, the initial exception will be
>>            * skipped. Exit ASAP
>>            */
>> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>> -                    "nop; nop",
>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>> +        bl      check_pending_vserror
>> +        cbnz    x0, 1f
>> +        alternative_else_nop_endif
>> +
> You asked other people to do not introduce new code in one patch and
> rewrite it in the following patch. But there you are doing exactly the
> same.

This is a fairly borderline comment knowing that I usually don't request 
clean-up and code consolidation in the same patch.

> I believe, it is possible to move all "alternative" patches to the
> very beginning of the patch series and only then introduce macro
> guest_vector.

For a first, the first patch is definitely not new code. This is code 
consolidation and therefore I don't tend to mix the two for clarity. So this 
should have been a patch before the first patch.

Secondly, the first 4 patches are candidate for backport. The rest of the series 
would be good to backport but I am not aware of a critical issue in previous Xen 
release to strongly push for it.

Cheers,

-- 
Julien Grall

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

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

[Volodymyr Babchuk]

Julien Grall writes:

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

>
> 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.
I'm bad at naming things :)

I understand that is two halves of one function. But func_name_noirq()
pattern is widely used for other case: when we have func_name_noirq()
function and some func_name() that disables interrupts like this:

void func_name()
{
        disable_irqs();
        func_name_noirq();
        enable_irqs();
}

I like principle of least surprise, so it is better to use some other
naming pattern there.

maybe something like enter_hypervisor_from_guest_pt1() and
enter_hypervisor_from_guest_pt2()?

Or maybe, we should not split the function at all? Instead, we enable
interrupts right in the middle of it.

>
>>
>>>      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_*).
Let's continue those discussion in the other thread.
[...]

--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Julien Grall]

Hi,

On 27/09/2019 13:27, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> On 27/09/2019 12:45, Volodymyr Babchuk wrote:
>>>
>>> Julien,
>>
>> Hi...
>>
>>> Julien Grall writes:
>>>
>>>> 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@arm.com>
>>>>
>>>> ---
>>>>
>>>> I haven't done the 32-bits part yet. I wanted to gather feedback before
>>>> looking in details how to integrate that with Arm32.
>>> I'm looking at patches one by one and it is looking okay so far.
>>>
>>>
>>>> ---
>>>>    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
>>> Looks like this mov can be removed (see commend below).
>>>
>>>> +        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.
>>> Maybe it is me only, but the phrasing is confusing. I had to read it two
>>> times before I get it. What about "Actions that needs to be done when
>>> raising exception level"? Or maybe "Actions that needs to be done when
>>> switching from guest to hypervisor mode" ?
>>
>> Is it a suggestion to replace the full sentence or just the first
>> before (i.e. before 'and')?
> This is a suggestion for the first part.

How about:

"Actions that needs to be done after entering the hypervisor from the guest and 
before we handle any request."

> 
>>>
>>>> + */
>>>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
>>> With the guest_mode(regs) check removal , this function does not use regs
>>> anymore.
>>
>> I have nearly done it while working on the series, but then I thought
>> that it would be better keep all the functions called from the entry
>> path in assembly similar.
> This can save one assembly instruction in the entry path. But I'm not
> sure if it is worth it. So it is up to you.

My concern is user may decide to use guest_cpu_user_regs() when the 'regs' 
parameter could have been used. But I guess, we can notice it during review.

So I will drop it.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH RFC for-4.13 09/10] xen/arm: asm: Replace use of ALTERNATIVE with alternative_if

[Volodymyr Babchuk]

Julien Grall writes:

> Hi,
>
> On 27/09/2019 13:11, Volodymyr Babchuk wrote:
>>
>>
>> Julien Grall writes:
>>
>>> Using alternative_if makes the code a bit more streamlined.
>>>
>>> Take the opportunity to use the new auto-nop infrastructure to avoid
>>> counting the number of nop in the else part for arch/arm/arm64/entry.S
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>
>>> ---
>>>      This is pretty much a matter of taste, but at least for arm64 this
>>>      allows us to use the auto-nop infrastructure. So the arm32 is more
>>>      to keep inline with arm64.
>>> ---
>>>   xen/arch/arm/arm32/entry.S | 9 ++++++---
>>>   xen/arch/arm/arm64/entry.S | 8 +++++---
>>>   2 files changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>>> index 0b4cd19abd..1428cd3583 100644
>>> --- a/xen/arch/arm/arm32/entry.S
>>> +++ b/xen/arch/arm/arm32/entry.S
>>> @@ -65,9 +65,12 @@ save_guest_regs:
>>>            * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
>>>            * feature, the checking of pending SErrors will be skipped.
>>>            */
>>> -        ALTERNATIVE("nop",
>>> -                    "b skip_check",
>>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>>> +        nop
>>> +        alternative_else
>>> +        b   skip_check
>>> +        alternative_endif
>>> +
>> for the arm32 code you can have my r-b:
>> Reviewed-By: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>
>>>           /*
>>>            * Start to check pending virtual abort in the gap of Guest -> HYP
>>>            * world switch.
>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>>> index 458d12f188..91cf6ee6f4 100644
>>> --- a/xen/arch/arm/arm64/entry.S
>>> +++ b/xen/arch/arm/arm64/entry.S
>>> @@ -170,9 +170,11 @@
>>>            * is not set. If a vSError took place, the initial exception will be
>>>            * skipped. Exit ASAP
>>>            */
>>> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>>> -                    "nop; nop",
>>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>>> +        bl      check_pending_vserror
>>> +        cbnz    x0, 1f
>>> +        alternative_else_nop_endif
>>> +
>> You asked other people to do not introduce new code in one patch and
>> rewrite it in the following patch. But there you are doing exactly the
>> same.
>
> This is a fairly borderline comment knowing that I usually don't
> request clean-up and code consolidation in the same patch.
I understand this. Also I understand why are you asking for clean-up.
No one likes to review the same code twice.

Anyways, I not wanted to be offensive. Sorry for that.


>> I believe, it is possible to move all "alternative" patches to the
>> very beginning of the patch series and only then introduce macro
>> guest_vector.
>
> For a first, the first patch is definitely not new code. This is code
> consolidation and therefore I don't tend to mix the two for
> clarity. So this should have been a patch before the first patch.
>
> Secondly, the first 4 patches are candidate for backport. The rest of
> the series would be good to backport but I am not aware of a critical
> issue in previous Xen release to strongly push for it.
I see. Yes, I'm always forgetting about backporting :(
So, for the rest of the patch:

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Volodymyr Babchuk]

Julien Grall writes:

> Hi,
>
> On 27/09/2019 13:27, Volodymyr Babchuk wrote:
>>
>> Julien Grall writes:
>>
>>> On 27/09/2019 12:45, Volodymyr Babchuk wrote:
>>>>
>>>> Julien,
>>>
>>> Hi...
>>>
>>>> Julien Grall writes:
>>>>
>>>>> 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@arm.com>
>>>>>
>>>>> ---
>>>>>
>>>>> I haven't done the 32-bits part yet. I wanted to gather feedback before
>>>>> looking in details how to integrate that with Arm32.
>>>> I'm looking at patches one by one and it is looking okay so far.
>>>>
>>>>
>>>>> ---
>>>>>    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
>>>> Looks like this mov can be removed (see commend below).
>>>>
>>>>> +        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.
>>>> Maybe it is me only, but the phrasing is confusing. I had to read it two
>>>> times before I get it. What about "Actions that needs to be done when
>>>> raising exception level"? Or maybe "Actions that needs to be done when
>>>> switching from guest to hypervisor mode" ?
>>>
>>> Is it a suggestion to replace the full sentence or just the first
>>> before (i.e. before 'and')?
>> This is a suggestion for the first part.
>
> How about:
>
> "Actions that needs to be done after entering the hypervisor from the
> guest and before we handle any request."
Sound perfect.

[...]

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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]

Hi,

On 27/09/2019 13:39, Volodymyr Babchuk wrote:
> Julien Grall writes:
>> On 27/09/2019 12:56, Volodymyr Babchuk wrote:
>>> 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.
> Fair enough
> 
>>
>> 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.
> I'm bad at naming things :)

Me too ;).

> 
> I understand that is two halves of one function. But func_name_noirq()
> pattern is widely used for other case: when we have func_name_noirq()
> function and some func_name() that disables interrupts like this:
> 
> void func_name()
> {
>          disable_irqs();
>          func_name_noirq();
>          enable_irqs();
> }
> 
> I like principle of least surprise, so it is better to use some other
> naming pattern there.

I can't find any function suffixed with _noirq in Xen. So I don't think this 
would be a major issue here.

> 
> maybe something like enter_hypervisor_from_guest_pt1() and
> enter_hypervisor_from_guest_pt2()?
Hmmm, it reminds me uni when we had to limit function size to 20 lines :).

I chose _noirq because the other name I had in mind was quite verbose. I was 
thinking: enter_hypervisor_from_guest_before_interrupts().

> 
> Or maybe, we should not split the function at all? Instead, we enable
> interrupts right in the middle of it.

I thought about this but I didn't much like the resulting code.

The instruction to unmask interrupts requires to take an immediate (indicates 
which interrupts to unmask). As not all the traps require to unmask the same 
interrupts, we would end up to have to a bunch of if in the code to select the 
right unmasking.

So the split solution was the best I had in mind. I am open to better suggestion 
here.

Cheers,

-- 
Julien Grall

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

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

[Volodymyr Babchuk]

Hi,

Julien Grall writes:

> Hi,
>
> On 27/09/2019 13:39, Volodymyr Babchuk wrote:
>> Julien Grall writes:
>>> On 27/09/2019 12:56, Volodymyr Babchuk wrote:
>>>> 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.
>> Fair enough
>>
>>>
>>> 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.
>> I'm bad at naming things :)
>
> Me too ;).
>
>>
>> I understand that is two halves of one function. But func_name_noirq()
>> pattern is widely used for other case: when we have func_name_noirq()
>> function and some func_name() that disables interrupts like this:
>>
>> void func_name()
>> {
>>          disable_irqs();
>>          func_name_noirq();
>>          enable_irqs();
>> }
>>
>> I like principle of least surprise, so it is better to use some other
>> naming pattern there.
>
> I can't find any function suffixed with _noirq in Xen. So I don't
> think this would be a major issue here.
Yes, there are no such functions in Xen. But it may confuse developers
who come from another projects.

>>
>> maybe something like enter_hypervisor_from_guest_pt1() and
>> enter_hypervisor_from_guest_pt2()?
> Hmmm, it reminds me uni when we had to limit function size to 20 lines :).
>
> I chose _noirq because the other name I had in mind was quite
> verbose. I was thinking:
> enter_hypervisor_from_guest_before_interrupts().
A was thinking about something like this too.
What about enter_hypervisor_from_guest_preirq()?

I think that "_pre" better shows the relation to
enter_hypervisor_from_guest()

>
>>
>> Or maybe, we should not split the function at all? Instead, we enable
>> interrupts right in the middle of it.
>
> I thought about this but I didn't much like the resulting code.
>
> The instruction to unmask interrupts requires to take an immediate
> (indicates which interrupts to unmask). As not all the traps require
> to unmask the same interrupts, we would end up to have to a bunch of
> if in the code to select the right unmasking.
Ah, yes, this is the problem. We can provide callback to
enter_hypervisor_from_guest().

Or switch() instead of multiple ifs. Maybe in some helper function.

--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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]

On 27/09/2019 14:33, Volodymyr Babchuk wrote:
> Julien Grall writes:
>> On 27/09/2019 13:39, Volodymyr Babchuk wrote:
>>> Julien Grall writes:
>>>> On 27/09/2019 12:56, Volodymyr Babchuk wrote:
>>>>> 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.
>>> Fair enough
>>>
>>>>
>>>> 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.
>>> I'm bad at naming things :)
>>
>> Me too ;).
>>
>>>
>>> I understand that is two halves of one function. But func_name_noirq()
>>> pattern is widely used for other case: when we have func_name_noirq()
>>> function and some func_name() that disables interrupts like this:
>>>
>>> void func_name()
>>> {
>>>           disable_irqs();
>>>           func_name_noirq();
>>>           enable_irqs();
>>> }
>>>
>>> I like principle of least surprise, so it is better to use some other
>>> naming pattern there.
>>
>> I can't find any function suffixed with _noirq in Xen. So I don't
>> think this would be a major issue here.
> Yes, there are no such functions in Xen. But it may confuse developers
> who come from another projects.

Well, each projects have their own style. So there are always some adaptations 
needed to move to a new project. What matters is the documentation clarifies 
what is the exact use. But...

> 
>>>
>>> maybe something like enter_hypervisor_from_guest_pt1() and
>>> enter_hypervisor_from_guest_pt2()?
>> Hmmm, it reminds me uni when we had to limit function size to 20 lines :).
>>
>> I chose _noirq because the other name I had in mind was quite
>> verbose. I was thinking:
>> enter_hypervisor_from_guest_before_interrupts().
> A was thinking about something like this too.
> What about enter_hypervisor_from_guest_preirq()?

... this would be indeed better.

> 
> I think that "_pre" better shows the relation to
> enter_hypervisor_from_guest()
> 
>>
>>>
>>> Or maybe, we should not split the function at all? Instead, we enable
>>> interrupts right in the middle of it.
>>
>> I thought about this but I didn't much like the resulting code.
>>
>> The instruction to unmask interrupts requires to take an immediate
>> (indicates which interrupts to unmask). As not all the traps require
>> to unmask the same interrupts, we would end up to have to a bunch of
>> if in the code to select the right unmasking.
> Ah, yes, this is the problem. We can provide callback to
> enter_hypervisor_from_guest().

I am not sure what you mean by this. Do you mean a callback that will unmask the 
interrupts?

> 
> Or switch() instead of multiple ifs. Maybe in some helper function.

Well, my point about "ifs" is that you add a few branch instruction for 
something that can mostly be static (we will always unmask the same interrupts 
for a given exception).

Anyway, such solutions is a no-go for me. This is only muddying the code and I 
care about long-term maintenance.

Cheers,

-- 
Julien Grall

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

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

[Volodymyr Babchuk]

Julien Grall writes:

> On 27/09/2019 14:33, Volodymyr Babchuk wrote:
>> Julien Grall writes:
>>> On 27/09/2019 13:39, Volodymyr Babchuk wrote:
>>>> Julien Grall writes:
>>>>> On 27/09/2019 12:56, Volodymyr Babchuk wrote:
>>>>>> 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.
>>>> Fair enough
>>>>
>>>>>
>>>>> 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.
>>>> I'm bad at naming things :)
>>>
>>> Me too ;).
>>>
>>>>
>>>> I understand that is two halves of one function. But func_name_noirq()
>>>> pattern is widely used for other case: when we have func_name_noirq()
>>>> function and some func_name() that disables interrupts like this:
>>>>
>>>> void func_name()
>>>> {
>>>>           disable_irqs();
>>>>           func_name_noirq();
>>>>           enable_irqs();
>>>> }
>>>>
>>>> I like principle of least surprise, so it is better to use some other
>>>> naming pattern there.
>>>
>>> I can't find any function suffixed with _noirq in Xen. So I don't
>>> think this would be a major issue here.
>> Yes, there are no such functions in Xen. But it may confuse developers
>> who come from another projects.
>
> Well, each projects have their own style. So there are always some
> adaptations needed to move to a new project. What matters is the
> documentation clarifies what is the exact use. But...
>
>>
>>>>
>>>> maybe something like enter_hypervisor_from_guest_pt1() and
>>>> enter_hypervisor_from_guest_pt2()?
>>> Hmmm, it reminds me uni when we had to limit function size to 20 lines :).
>>>
>>> I chose _noirq because the other name I had in mind was quite
>>> verbose. I was thinking:
>>> enter_hypervisor_from_guest_before_interrupts().
>> A was thinking about something like this too.
>> What about enter_hypervisor_from_guest_preirq()?
>
> ... this would be indeed better.

>>
>> I think that "_pre" better shows the relation to
>> enter_hypervisor_from_guest()
>>
>>>
>>>>
>>>> Or maybe, we should not split the function at all? Instead, we enable
>>>> interrupts right in the middle of it.
>>>
>>> I thought about this but I didn't much like the resulting code.
>>>
>>> The instruction to unmask interrupts requires to take an immediate
>>> (indicates which interrupts to unmask). As not all the traps require
>>> to unmask the same interrupts, we would end up to have to a bunch of
>>> if in the code to select the right unmasking.
>> Ah, yes, this is the problem. We can provide callback to
>> enter_hypervisor_from_guest().
>
> I am not sure what you mean by this. Do you mean a callback that will
> unmask the interrupts?
Yes. You can pass function pointer to enter_hypervisor_from_guest(). To
a function, that will unmask the interrupts. I'm sure that guest_vector
macro can generate it for you. Something like this:

        .macro  guest_vector compat, iflags, trap, save_x0_x1=1
        entry   hyp=0, compat=\compat, save_x0_x1=\save_x0_x1
        /*
         * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
         * is not set. If a vSError took place, the initial exception will be
         * skipped. Exit ASAP
         */
        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
                    "nop; nop",
                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
        ldr     x0, =1f
        bl      enter_hypervisor_from_guest
        mov     x0, sp
        bl      do_trap_\trap
        b       1f
2:
        msr     daifclr, \iflags
        ret
1:
        exit    hyp=0, compat=\compat
        .endm



-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RFC for-4.13 10/10] xen/arm64: entry: Ensure the guest state is synced when receiving a vSError

[Volodymyr Babchuk]

Hi Julien,

Julien Grall writes:

> At the moment, when a SError is received while checking for a pending
> one, we will skip the handling the initial exception.
>
> This includes call to exit_from_guest{, _noirq} that is used to
Did you mean enter_hypervisor_from_guest{, _noirq} there? Otherwise, I'm
confused a lot.

> synchronize part of the guest state with the internal representation.
> However, we still call leave_hypervisor_tail() which is used for preempting
> the guest and synchronizing back part of the guest state.
>
> exit_from_guest{, _noirq} works in pair with leave_hypervisor_tail(), so
> skipping if former may result to a loss of some part of  guest state.
> An example is the new vGIC which will save the state of the LRS on exit
> from the guest and rewrite all of them on entry to the guest.
>
> For now, calling leave_hypervisor_tail() is not necessary when injecting
> a vSError to the guest. But as the path is spread accross multiple file,
> it is hard to enforce that for the future (someone we may want to crash the
> domain). Therefore it is best to call exit_from_guest{, _noirq} in the
> vSError path as well.
>
> Note that the return value of check_pending_vserror is now set in x19
> instead of x0. This is because we want to keep the value across call to
> C-function and x0, unlike x19, will not be saved by the callee.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
>
> I am not aware of any issues other than with the new vGIC. But I
> haven't looked hard enough so I think it would be worth to try to fix it
> for Xen 4.13.
> ---
>  xen/arch/arm/arm64/entry.S | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 91cf6ee6f4..f5350247e1 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -168,11 +168,13 @@
>          /*
>           * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>           * is not set. If a vSError took place, the initial exception will be
> -         * skipped. Exit ASAP
> +         * skipped.
> +         *
> +         * However, we still need to call exit_from_guest{,_noirq} as the
> +         * return path to the guest may rely on state saved by them.
>           */
>          alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>          bl      check_pending_vserror
> -        cbnz    x0, 1f
>          alternative_else_nop_endif
>  
>          mov     x0, sp
> @@ -180,6 +182,11 @@
>          msr     daifclr, \iflags
>          mov     x0, sp
>          bl      enter_hypervisor_from_guest
> +
> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> +        cbnz    x19, 1f
> +        alternative_else_nop_endif
> +
>          mov     x0, sp
>          bl      do_trap_\trap
>  1:
> @@ -383,9 +390,9 @@ return_from_trap:
>  /*
>   * This function is used to check pending virtual SError in the gap of
>   * EL1 -> EL2 world switch.
> - * The x0 register will be used to indicate the results of detection.
> - * x0 -- Non-zero indicates a pending virtual SError took place.
> - * x0 -- Zero indicates no pending virtual SError took place.
> + * The register x19 will be used to indicate the results of detection.
> + * x19 -- Non-zero indicates a pending virtual SError took place.
> + * x19 -- Zero indicates no pending virtual SError took place.
>   */
>  check_pending_vserror:
>          /*
> @@ -432,9 +439,9 @@ abort_guest_exit_end:
>  
>          /*
>           * Not equal, the pending SError exception took place, set
> -         * x0 to non-zero.
> +         * x19 to non-zero.
>           */
> -        cset    x0, ne
> +        cset    x19, ne
>  
>          ret


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RFC for-4.13 08/10] xen/arm: alternative: add auto-nop infrastructure

[Volodymyr Babchuk]

Julien Grall writes:

> From: Mark Rutland <mark.rutland@arm.com>
>
> In some cases, one side of an alternative sequence is simply a number of
> NOPs used to balance the other side. Keeping track of this manually is
> tedious, and the presence of large chains of NOPs makes the code more
> painful to read than necessary.
>
> To ameliorate matters, this patch adds a new alternative_else_nop_endif,
> which automatically balances an alternative sequence with a trivial NOP
> sled.
>
> In many cases, we would like a NOP-sled in the default case, and
> instructions patched in in the presence of a feature. To enable the NOPs
> to be generated automatically for this case, this patch also adds a new
> alternative_if, and updates alternative_else and alternative_endif to
> work with either alternative_if or alternative_endif.
>
> The alternative infrastructure was originally ported from Linux. So this
> is pretty much a straight backport from commit 792d47379f4d "arm64:
> alternative: add auto-nop infrastructure". The only difference is the
> nops macro added as not yet existing in Xen.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> [will: use new nops macro to generate nop sequences]
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> [julien: Add nops and port to Xen]
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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]

Hi,

On 27/09/2019 15:21, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> On 27/09/2019 14:33, Volodymyr Babchuk wrote:
>>> Julien Grall writes:
>>>> On 27/09/2019 13:39, Volodymyr Babchuk wrote:
>>>>> Julien Grall writes:
>>>>>> On 27/09/2019 12:56, Volodymyr Babchuk wrote:
>>>>>>> Julien Grall writes:
>>>>> Or maybe, we should not split the function at all? Instead, we enable
>>>>> interrupts right in the middle of it.
>>>>
>>>> I thought about this but I didn't much like the resulting code.
>>>>
>>>> The instruction to unmask interrupts requires to take an immediate
>>>> (indicates which interrupts to unmask). As not all the traps require
>>>> to unmask the same interrupts, we would end up to have to a bunch of
>>>> if in the code to select the right unmasking.
>>> Ah, yes, this is the problem. We can provide callback to
>>> enter_hypervisor_from_guest().
>>
>> I am not sure what you mean by this. Do you mean a callback that will
>> unmask the interrupts?
> Yes. You can pass function pointer to enter_hypervisor_from_guest(). To
> a function, that will unmask the interrupts. I'm sure that guest_vector
> macro can generate it for you. Something like this:
> 
>          .macro  guest_vector compat, iflags, trap, save_x0_x1=1
>          entry   hyp=0, compat=\compat, save_x0_x1=\save_x0_x1
>          /*
>           * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>           * is not set. If a vSError took place, the initial exception will be
>           * skipped. Exit ASAP
>           */
>          ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>                      "nop; nop",
>                      SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>          ldr     x0, =1f
>          bl      enter_hypervisor_from_guest
>          mov     x0, sp
>          bl      do_trap_\trap
>          b       1f
> 2:
>          msr     daifclr, \iflags
>          ret
> 1:
>          exit    hyp=0, compat=\compat
>          .endm

TBH, I don't see what's the point you are trying to make here. Yes, there are 
many way to write a code and possibility to make one function. You could also 
create a skeleton macro for enter_hypervisor_from_guest and generate N of them 
(one per set of unmask interrupts) and call them.

But are they really worth it?

Cheers,

-- 
Julien Grall

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

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

[Volodymyr Babchuk]

Julien Grall writes:

> Hi,
>
> On 27/09/2019 15:21, Volodymyr Babchuk wrote:
>>
>> Julien Grall writes:
>>
>>> On 27/09/2019 14:33, Volodymyr Babchuk wrote:
>>>> Julien Grall writes:
>>>>> On 27/09/2019 13:39, Volodymyr Babchuk wrote:
>>>>>> Julien Grall writes:
>>>>>>> On 27/09/2019 12:56, Volodymyr Babchuk wrote:
>>>>>>>> Julien Grall writes:
>>>>>> Or maybe, we should not split the function at all? Instead, we enable
>>>>>> interrupts right in the middle of it.
>>>>>
>>>>> I thought about this but I didn't much like the resulting code.
>>>>>
>>>>> The instruction to unmask interrupts requires to take an immediate
>>>>> (indicates which interrupts to unmask). As not all the traps require
>>>>> to unmask the same interrupts, we would end up to have to a bunch of
>>>>> if in the code to select the right unmasking.
>>>> Ah, yes, this is the problem. We can provide callback to
>>>> enter_hypervisor_from_guest().
>>>
>>> I am not sure what you mean by this. Do you mean a callback that will
>>> unmask the interrupts?
>> Yes. You can pass function pointer to enter_hypervisor_from_guest(). To
>> a function, that will unmask the interrupts. I'm sure that guest_vector
>> macro can generate it for you. Something like this:
>>
>>          .macro  guest_vector compat, iflags, trap, save_x0_x1=1
>>          entry   hyp=0, compat=\compat, save_x0_x1=\save_x0_x1
>>          /*
>>           * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>>           * is not set. If a vSError took place, the initial exception will be
>>           * skipped. Exit ASAP
>>           */
>>          ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>>                      "nop; nop",
>>                      SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>          ldr     x0, =1f
>>          bl      enter_hypervisor_from_guest
>>          mov     x0, sp
>>          bl      do_trap_\trap
>>          b       1f
>> 2:
>>          msr     daifclr, \iflags
>>          ret
>> 1:
>>          exit    hyp=0, compat=\compat
>>          .endm
>
> TBH, I don't see what's the point you are trying to make here. Yes,
> there are many way to write a code and possibility to make one
> function. You could also create a skeleton macro for
> enter_hypervisor_from_guest and generate N of them (one per set of
> unmask interrupts) and call them.
>
> But are they really worth it?
The point is that you are trying to split one entity into two.
As I see it: semantically we have one function:
enter_hypervisor_from_guest(). The purpose of this function is clear:
finish transition from guest mode to hypervisor mode.

But because of some architectural limitation (daifclr requires only
immediate argument) you are forced to divide this function in two.
I don't like this, because entry path now is more complex. To follow
what is going you need to bounce between head.S and traps.c one extra time.

Anyways, this is only my opinion. I'm not forcing you to implement
enter_hypervisor_from_guest() in this way.

I'm okay with enter_hypervisor_from_guest_preirq() as well.

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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]

Hi,

On 27/09/2019 18:58, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> Hi,
>>
>> On 27/09/2019 15:21, Volodymyr Babchuk wrote:
>>>
>>> Julien Grall writes:
>>>
>>>> On 27/09/2019 14:33, Volodymyr Babchuk wrote:
>>>>> Julien Grall writes:
>>>>>> On 27/09/2019 13:39, Volodymyr Babchuk wrote:
>>>>>>> Julien Grall writes:
>>>>>>>> On 27/09/2019 12:56, Volodymyr Babchuk wrote:
>>>>>>>>> Julien Grall writes:
>>>>>>> Or maybe, we should not split the function at all? Instead, we enable
>>>>>>> interrupts right in the middle of it.
>>>>>>
>>>>>> I thought about this but I didn't much like the resulting code.
>>>>>>
>>>>>> The instruction to unmask interrupts requires to take an immediate
>>>>>> (indicates which interrupts to unmask). As not all the traps require
>>>>>> to unmask the same interrupts, we would end up to have to a bunch of
>>>>>> if in the code to select the right unmasking.
>>>>> Ah, yes, this is the problem. We can provide callback to
>>>>> enter_hypervisor_from_guest().
>>>>
>>>> I am not sure what you mean by this. Do you mean a callback that will
>>>> unmask the interrupts?
>>> Yes. You can pass function pointer to enter_hypervisor_from_guest(). To
>>> a function, that will unmask the interrupts. I'm sure that guest_vector
>>> macro can generate it for you. Something like this:
>>>
>>>           .macro  guest_vector compat, iflags, trap, save_x0_x1=1
>>>           entry   hyp=0, compat=\compat, save_x0_x1=\save_x0_x1
>>>           /*
>>>            * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>>>            * is not set. If a vSError took place, the initial exception will be
>>>            * skipped. Exit ASAP
>>>            */
>>>           ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>>>                       "nop; nop",
>>>                       SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>>           ldr     x0, =1f
>>>           bl      enter_hypervisor_from_guest
>>>           mov     x0, sp
>>>           bl      do_trap_\trap
>>>           b       1f
>>> 2:
>>>           msr     daifclr, \iflags
>>>           ret
>>> 1:
>>>           exit    hyp=0, compat=\compat
>>>           .endm
>>
>> TBH, I don't see what's the point you are trying to make here. Yes,
>> there are many way to write a code and possibility to make one
>> function. You could also create a skeleton macro for
>> enter_hypervisor_from_guest and generate N of them (one per set of
>> unmask interrupts) and call them.
>>
>> But are they really worth it?
> The point is that you are trying to split one entity into two.
> As I see it: semantically we have one function:
> enter_hypervisor_from_guest(). The purpose of this function is clear:
> finish transition from guest mode to hypervisor mode.
> 
> But because of some architectural limitation (daifclr requires only
> immediate argument) you are forced to divide this function in two.
> I don't like this, because entry path now is more complex. To follow
> what is going you need to bounce between head.S and traps.c one extra time.

Ok. If I understand correctly, this is mostly a matter of taste. Right?

I am going to ignore the "matter of taste" and just focus on the code 
itself. While I quite like the idea of a single function, I have two 
concerns with this:
    1) Because this is a callback, you will use an indirect branch. The 
address used is loaded from the literal pool (ldr x0, =...), therefore 
your branch will depend on a load. Such construction may stall the 
pipeline for a long time as most likely you will have to fetch the 
address from memory and not the cache (the cache is likely to be 
populated with mostly guest stuff). Depending on the core, this may have 
quite an impact. I am aware that we have been using in quite a few 
places such pattern within Xen but we are trying to get away. For 
instance, on x86 they recently introduced a way to converting indirect 
branch to direct branch if the address is fixed after boot (see the 
alternative_call macro).

    2) With the split functions, it is easier to spot in a diff if 
someone is trying to add code before the interrupts are unmasked. So I 
feel this is more maintainable as I have one less thing to worry when 
reviewing.

The second one is borderline matter of taste, so it is less important. 
But the first one is important to me.

So any solution should address this.

Cheers,

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

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

[Volodymyr Babchuk]

Hi Julien,

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.

To summarize our discussion in this mail thread: providing that you'll
rename enter_hypervisor_from_guest_noirq to
enter_hypervisor_from_guest_preirq():

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

>     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.
>
> Fixes: a7898e4c59 ("xen/arm: Add ARCH_WORKAROUND_2 support for guests")
> Reported-by: Andrii Anisov <andrii_anisov@epam.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
>
> Note the Arm32 code has not been changed yet. I am also open on turn
> both enter_hypervisor_from_guest_noirq() and enter_hypervisor_from()
> to functions not taking any parameters.
> ---
>  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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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]

On 30/09/2019 13:14, Volodymyr Babchuk wrote:
> 
> Hi Julien,
> 
> 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.
> 
> To summarize our discussion in this mail thread: providing that you'll
> rename enter_hypervisor_from_guest_noirq to
> enter_hypervisor_from_guest_preirq():
> 
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Thank you. I will try to summarize the discussion we had in the commit message. 
So we know the rationale of the split here.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH RFC for-4.13 01/10] xen/arm64: entry: Introduce a macro to generate guest vector and use it

[Stefano Stabellini]

On Fri, 27 Sep 2019, Volodymyr Babchuk wrote:
> 
> Hello Julien,
> 
> Julien Grall writes:
> 
> > Most of the guest vectors are using the same pattern. This makes fairly
> > tedious to alter the pattern and risk introducing mistakes when updating
> > each path.
> >
> > A new macro is introduced to generate the guest vectors and now use it
> > in the one that use the open-code version.
> >
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

Re: [Xen-devel] [PATCH RFC for-4.13 02/10] xen/arm64: head: Check if an SError is pending when receiving a vSError

[Stefano Stabellini]

On Fri, 27 Sep 2019, Volodymyr Babchuk wrote:
> Julien Grall writes:
> 
> > At the moment, when we receive an SError exception from the guest, we
> > don't check if there are any other pending. For hardening the code, we
> > should ensure any pending SError are accounted to the guest before
> > executing any code with SError unmasked.
> >
> > The recently introduced macro 'guest_vector' could used to generate the
> > two vectors and therefore take advantage of any change required in the
> > future.
> >
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

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

[Stefano Stabellini]

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@arm.com>
> 
> ---
> 
> 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.


> @@ -2281,7 +2272,13 @@ static void check_for_vcpu_work(void)
>      local_irq_disable();
>  }
>  
> -void leave_hypervisor_tail(void)
> +/*
> + * Actions that needs to be done before entering the guest. This is the
> + * last thing executed before the guest context is fully restored.
> + *
> + * The function will return with interrupts disabled.
> + */
> +void leave_hypervisor_to_guest(void)
>  {
>      local_irq_disable();
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH RFC for-4.13 05/10] xen/arm: alternative: Remove unused parameter for alternative_if_not_cap

[Stefano Stabellini]

On Fri, 27 Sep 2019, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
> > The macro alternative_if_not_cap is taking two parameters. The second
> > parameter is never used and it is hard to see how this can be used
> > correctly as it is only protecting the alternative section magic.
> >
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> > ---
> >  xen/include/asm-arm/alternative.h | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
> > index dedb6dd001..2830a6da2d 100644
> > --- a/xen/include/asm-arm/alternative.h
> > +++ b/xen/include/asm-arm/alternative.h
> > @@ -116,13 +116,11 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
> >   * The code that follows this macro will be assembled and linked as
> >   * normal. There are no restrictions on this code.
> >   */
> > -.macro alternative_if_not cap, enable = 1
> > -	.if \enable
> > +.macro alternative_if_not cap
> >  	.pushsection .altinstructions, "a"
> >  	altinstruction_entry 661f, 663f, \cap, 662f-661f, 664f-663f
> >  	.popsection
> >  661:
> > -	.endif
> >  .endm
> >  
> >  /*
> 
> 
> -- 
> Volodymyr Babchuk at EPAM

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

Re: [Xen-devel] [PATCH RFC for-4.13 06/10] xen/arm: Move ARCH_PATCH_INSN_SIZE out of the header livepatch.h

[Stefano Stabellini]

On Thu, 26 Sep 2019, Julien Grall wrote:
> At the moment, ARCH_PATCH_INSN_SIZE is defined in the header
> livepatch.h. However, this is also used in the alternative code.
> 
> Rather than including livepatch.h just for using the define, move it in
> the header insn.h which seems more suitable.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/alternative.c      | 2 --
>  xen/include/asm-arm/insn.h      | 3 +++
>  xen/include/asm-arm/livepatch.h | 4 +---
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 52ed7edf69..237c4e5642 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -30,8 +30,6 @@
>  #include <asm/byteorder.h>
>  #include <asm/cpufeature.h>
>  #include <asm/insn.h>
> -/* XXX: Move ARCH_PATCH_INSN_SIZE out of livepatch.h */
> -#include <asm/livepatch.h>
>  #include <asm/page.h>
>  
>  /* Override macros from asm/page.h to make them work with mfn_t */
> diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
> index 3489179826..19277212e1 100644
> --- a/xen/include/asm-arm/insn.h
> +++ b/xen/include/asm-arm/insn.h
> @@ -11,6 +11,9 @@
>  # error "unknown ARM variant"
>  #endif
>  
> +/* On ARM32,64 instructions are always 4 bytes long. */
> +#define ARCH_PATCH_INSN_SIZE 4
> +
>  #endif /* !__ARCH_ARM_INSN */
>  /*
>   * Local variables:
> diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h
> index 6bca79deb9..026af5e7dc 100644
> --- a/xen/include/asm-arm/livepatch.h
> +++ b/xen/include/asm-arm/livepatch.h
> @@ -7,9 +7,7 @@
>  #define __XEN_ARM_LIVEPATCH_H__
>  
>  #include <xen/sizes.h> /* For SZ_* macros. */
> -
> -/* On ARM32,64 instructions are always 4 bytes long. */
> -#define ARCH_PATCH_INSN_SIZE 4
> +#include <asm/insn.h>
>  
>  /*
>   * The va of the hypervisor .text region. We need this as the
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH RFC for-4.13 07/10] xen/arm: Allow insn.h to be called from assembly

[Stefano Stabellini]

On Thu, 26 Sep 2019, Julien Grall wrote:
> A follow-up patch will require to include insn.h from assembly code. So
> wee need to protect any C-specific definition to avoid compilation
  ^ we                               ^ definitions

> error when used in assembly code.
  ^ errors


> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/include/asm-arm/insn.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
> index 19277212e1..00391f83f9 100644
> --- a/xen/include/asm-arm/insn.h
> +++ b/xen/include/asm-arm/insn.h
> @@ -1,8 +1,14 @@
>  #ifndef __ARCH_ARM_INSN
>  #define __ARCH_ARM_INSN
>  
> +#ifndef __ASSEMBLY__
> +
>  #include <xen/types.h>
>  
> +/*
> + * At the moment, arch-specific headers contain only definition for C
> + * code.
> + */

Please remove "At the moment, " because in-code comment should always
reflect the latest state of the codebase.


>  #if defined(CONFIG_ARM_64)
>  # include <asm/arm64/insn.h>
>  #elif defined(CONFIG_ARM_32)
> @@ -11,6 +17,8 @@
>  # error "unknown ARM variant"
>  #endif
>  
> +#endif /* __ASSEMBLY__ */
> +
>  /* On ARM32,64 instructions are always 4 bytes long. */
>  #define ARCH_PATCH_INSN_SIZE 4
>  
> -- 
> 2.11.0
> 

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

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

[Julien Grall]

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@arm.com>
>>
>> ---
>>
>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RFC for-4.13 08/10] xen/arm: alternative: add auto-nop infrastructure

[Stefano Stabellini]

On Fri, 27 Sep 2019, Volodymyr Babchuk wrote:
> Julien Grall writes:
> 
> > From: Mark Rutland <mark.rutland@arm.com>
> >
> > In some cases, one side of an alternative sequence is simply a number of
> > NOPs used to balance the other side. Keeping track of this manually is
> > tedious, and the presence of large chains of NOPs makes the code more
> > painful to read than necessary.
> >
> > To ameliorate matters, this patch adds a new alternative_else_nop_endif,
> > which automatically balances an alternative sequence with a trivial NOP
> > sled.
> >
> > In many cases, we would like a NOP-sled in the default case, and
> > instructions patched in in the presence of a feature. To enable the NOPs
> > to be generated automatically for this case, this patch also adds a new
> > alternative_if, and updates alternative_else and alternative_endif to
> > work with either alternative_if or alternative_endif.
> >
> > The alternative infrastructure was originally ported from Linux. So this
> > is pretty much a straight backport from commit 792d47379f4d "arm64:
> > alternative: add auto-nop infrastructure". The only difference is the
> > nops macro added as not yet existing in Xen.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > [will: use new nops macro to generate nop sequences]
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > [julien: Add nops and port to Xen]
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

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

Re: [Xen-devel] [PATCH RFC for-4.13 09/10] xen/arm: asm: Replace use of ALTERNATIVE with alternative_if

[Stefano Stabellini]

On Thu, 26 Sep 2019, Julien Grall wrote:
> Using alternative_if makes the code a bit more streamlined.
> 
> Take the opportunity to use the new auto-nop infrastructure to avoid
> counting the number of nop in the else part for arch/arm/arm64/entry.S
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     This is pretty much a matter of taste, but at least for arm64 this
>     allows us to use the auto-nop infrastructure. So the arm32 is more
>     to keep inline with arm64.
> ---
>  xen/arch/arm/arm32/entry.S | 9 ++++++---
>  xen/arch/arm/arm64/entry.S | 8 +++++---
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 0b4cd19abd..1428cd3583 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -65,9 +65,12 @@ save_guest_regs:
>           * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
>           * feature, the checking of pending SErrors will be skipped.
>           */
> -        ALTERNATIVE("nop",
> -                    "b skip_check",
> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> +        nop
> +        alternative_else
> +        b   skip_check
> +        alternative_endif

This could be written as:

alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
b   skip_check
alternative_else_nop_endif


>          /*
>           * Start to check pending virtual abort in the gap of Guest -> HYP
>           * world switch.
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 458d12f188..91cf6ee6f4 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -170,9 +170,11 @@
>           * is not set. If a vSError took place, the initial exception will be
>           * skipped. Exit ASAP
>           */
> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
> -                    "nop; nop",
> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> +        bl      check_pending_vserror
> +        cbnz    x0, 1f
> +        alternative_else_nop_endif
> +
>          mov     x0, sp
>          bl      enter_hypervisor_from_guest_noirq
>          msr     daifclr, \iflags
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH RFC for-4.13 09/10] xen/arm: asm: Replace use of ALTERNATIVE with alternative_if

[Julien Grall]

Hi Stefano,

On 01/10/2019 23:19, Stefano Stabellini wrote:
> On Thu, 26 Sep 2019, Julien Grall wrote:
>> Using alternative_if makes the code a bit more streamlined.
>>
>> Take the opportunity to use the new auto-nop infrastructure to avoid
>> counting the number of nop in the else part for arch/arm/arm64/entry.S
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      This is pretty much a matter of taste, but at least for arm64 this
>>      allows us to use the auto-nop infrastructure. So the arm32 is more
>>      to keep inline with arm64.
>> ---
>>   xen/arch/arm/arm32/entry.S | 9 ++++++---
>>   xen/arch/arm/arm64/entry.S | 8 +++++---
>>   2 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>> index 0b4cd19abd..1428cd3583 100644
>> --- a/xen/arch/arm/arm32/entry.S
>> +++ b/xen/arch/arm/arm32/entry.S
>> @@ -65,9 +65,12 @@ save_guest_regs:
>>            * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
>>            * feature, the checking of pending SErrors will be skipped.
>>            */
>> -        ALTERNATIVE("nop",
>> -                    "b skip_check",
>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>> +        nop
>> +        alternative_else
>> +        b   skip_check
>> +        alternative_endif
> 
> This could be written as:
> 
> alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> b   skip_check
> alternative_else_nop_endif

Actually my implementation is wrong :/. We want to skip the check if the 
cap is set. So this should be:

alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
b    skip_check
alternative_else_nop_endif

> 
> 
>>           /*
>>            * Start to check pending virtual abort in the gap of Guest -> HYP
>>            * world switch.
>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>> index 458d12f188..91cf6ee6f4 100644
>> --- a/xen/arch/arm/arm64/entry.S
>> +++ b/xen/arch/arm/arm64/entry.S
>> @@ -170,9 +170,11 @@
>>            * is not set. If a vSError took place, the initial exception will be
>>            * skipped. Exit ASAP
>>            */
>> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>> -                    "nop; nop",
>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT

This would need to be alternative_if_not as want to call the function 
when the cap is not set.

>> +        bl      check_pending_vserror
>> +        cbnz    x0, 1f
>> +        alternative_else_nop_endif
>> +
>>           mov     x0, sp
>>           bl      enter_hypervisor_from_guest_noirq
>>           msr     daifclr, \iflags
>> -- 
>> 2.11.0
>>

Cheeers,

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

Re: [Xen-devel] [PATCH RFC for-4.13 09/10] xen/arm: asm: Replace use of ALTERNATIVE with alternative_if

[Stefano Stabellini]

On Tue, 1 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/10/2019 23:19, Stefano Stabellini wrote:
> > On Thu, 26 Sep 2019, Julien Grall wrote:
> >> Using alternative_if makes the code a bit more streamlined.
> >>
> >> Take the opportunity to use the new auto-nop infrastructure to avoid
> >> counting the number of nop in the else part for arch/arm/arm64/entry.S
> >>
> >> Signed-off-by: Julien Grall <julien.grall@arm.com>
> >>
> >> ---
> >>      This is pretty much a matter of taste, but at least for arm64 this
> >>      allows us to use the auto-nop infrastructure. So the arm32 is more
> >>      to keep inline with arm64.
> >> ---
> >>   xen/arch/arm/arm32/entry.S | 9 ++++++---
> >>   xen/arch/arm/arm64/entry.S | 8 +++++---
> >>   2 files changed, 11 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> >> index 0b4cd19abd..1428cd3583 100644
> >> --- a/xen/arch/arm/arm32/entry.S
> >> +++ b/xen/arch/arm/arm32/entry.S
> >> @@ -65,9 +65,12 @@ save_guest_regs:
> >>            * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
> >>            * feature, the checking of pending SErrors will be skipped.
> >>            */
> >> -        ALTERNATIVE("nop",
> >> -                    "b skip_check",
> >> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> >> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> >> +        nop
> >> +        alternative_else
> >> +        b   skip_check
> >> +        alternative_endif
> > 
> > This could be written as:
> > 
> > alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> > b   skip_check
> > alternative_else_nop_endif
> 
> Actually my implementation is wrong :/. We want to skip the check if the 
> cap is set. So this should be:
> 
> alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> b    skip_check
> alternative_else_nop_endif

Ah, yes of course, like the name suggests


> > 
> >>           /*
> >>            * Start to check pending virtual abort in the gap of Guest -> HYP
> >>            * world switch.
> >> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> >> index 458d12f188..91cf6ee6f4 100644
> >> --- a/xen/arch/arm/arm64/entry.S
> >> +++ b/xen/arch/arm/arm64/entry.S
> >> @@ -170,9 +170,11 @@
> >>            * is not set. If a vSError took place, the initial exception will be
> >>            * skipped. Exit ASAP
> >>            */
> >> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
> >> -                    "nop; nop",
> >> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> >> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> 
> This would need to be alternative_if_not as want to call the function 
> when the cap is not set.

Yep


> >> +        bl      check_pending_vserror
> >> +        cbnz    x0, 1f
> >> +        alternative_else_nop_endif
> >> +
> >>           mov     x0, sp
> >>           bl      enter_hypervisor_from_guest_noirq
> >>           msr     daifclr, \iflags
> >> -- 
> >> 2.11.0
> >>
> 
> Cheeers,
> 
> -- 
> Julien Grall
> 

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

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

[Stefano Stabellini]

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@arm.com>
> >>
> >> ---
> >>
> >> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RFC for-4.13 10/10] xen/arm64: entry: Ensure the guest state is synced when receiving a vSError

[Stefano Stabellini]

On Thu, 26 Sep 2019, Julien Grall wrote:
> At the moment, when a SError is received while checking for a pending
> one, we will skip the handling the initial exception.
> 
> This includes call to exit_from_guest{, _noirq} that is used to
> synchronize part of the guest state with the internal representation.
> However, we still call leave_hypervisor_tail() which is used for preempting
> the guest and synchronizing back part of the guest state.

I second Volodymyr's comment about exit_from_guest* being the wrong
name.


> exit_from_guest{, _noirq} works in pair with leave_hypervisor_tail(), so
> skipping if former may result to a loss of some part of  guest state.
> An example is the new vGIC which will save the state of the LRS on exit
> from the guest and rewrite all of them on entry to the guest.
> 
> For now, calling leave_hypervisor_tail() is not necessary when injecting
> a vSError to the guest. But as the path is spread accross multiple file,
> it is hard to enforce that for the future (someone we may want to crash the
> domain). Therefore it is best to call exit_from_guest{, _noirq} in the
> vSError path as well.
> 
> Note that the return value of check_pending_vserror is now set in x19
> instead of x0. This is because we want to keep the value across call to
> C-function and x0, unlike x19, will not be saved by the callee.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
> I am not aware of any issues other than with the new vGIC. But I
> haven't looked hard enough so I think it would be worth to try to fix it
> for Xen 4.13.
> ---
>  xen/arch/arm/arm64/entry.S | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 91cf6ee6f4..f5350247e1 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -168,11 +168,13 @@
>          /*
>           * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>           * is not set. If a vSError took place, the initial exception will be
> -         * skipped. Exit ASAP
> +         * skipped.
> +         *
> +         * However, we still need to call exit_from_guest{,_noirq} as the
> +         * return path to the guest may rely on state saved by them.
>           */
>          alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>          bl      check_pending_vserror
> -        cbnz    x0, 1f
>          alternative_else_nop_endif
>  
>          mov     x0, sp
> @@ -180,6 +182,11 @@
>          msr     daifclr, \iflags
>          mov     x0, sp
>          bl      enter_hypervisor_from_guest
> +
> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> +        cbnz    x19, 1f
> +        alternative_else_nop_endif

I like the idea of always calling enter_hypervisor_from_guest(_preirq)
even when we are about to inject a SERROR into the guest. It is worth
noting that with this patch the guest-related SERROR would still get
delivered (due to the call to check_pending_vserror) before the guest
state is saved by enter_hypervisor_from_guest(_preirq). So it is more
like the following right?

- enter hypervisor from guest
- guest_vector macro
- check_pending_vserror
- SERROR is delivered
- do_trap_hyp_serror
- inject_vabt_exception
- back to guest_vector macro
- call enter_hypervisor_from_guest_preirq & enter_hypervisor_from_guest
- leave_hypervisor_tail

Which is better than what we would have without the patch, but not what
one would expect. Would you be up for adding a in-code comment that
describes the sequence, to clarify things?



>          mov     x0, sp
>          bl      do_trap_\trap
>  1:
> @@ -383,9 +390,9 @@ return_from_trap:
>  /*
>   * This function is used to check pending virtual SError in the gap of
>   * EL1 -> EL2 world switch.
> - * The x0 register will be used to indicate the results of detection.
> - * x0 -- Non-zero indicates a pending virtual SError took place.
> - * x0 -- Zero indicates no pending virtual SError took place.
> + * The register x19 will be used to indicate the results of detection.
> + * x19 -- Non-zero indicates a pending virtual SError took place.
> + * x19 -- Zero indicates no pending virtual SError took place.
>   */
>  check_pending_vserror:
>          /*
> @@ -432,9 +439,9 @@ abort_guest_exit_end:
>  
>          /*
>           * Not equal, the pending SError exception took place, set
> -         * x0 to non-zero.
> +         * x19 to non-zero.
>           */
> -        cset    x0, ne
> +        cset    x19, ne
>  
>          ret
>  
> -- 
> 2.11.0
> 

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

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

[Julien Grall]

Hi Stefano,

On 10/2/19 1:16 AM, Stefano Stabellini wrote:
> On Tue, 1 Oct 2019, Julien Grall wrote:
>> On 01/10/2019 21:12, Stefano Stabellini wrote:
>>> On Thu, 26 Sep 2019, Julien Grall wrote:
>>> 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.
As I pointed out above, this patch does not change anything in this 
particular Arm64 vector so I don't see why I should mention it in the 
commit message.

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

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@arm.com>
> > >>
> > >> ---
> > >>
> > >> 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.

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

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

[Julien Grall]

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@arm.com>
>>>>>
>>>>> ---
>>>>>
>>>>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Stefano Stabellini]

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@arm.com>
> > > > > > 
> > > > > > ---
> > > > > > 
> > > > > > 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Julien Grall]

Hi Stefano,

On 02/10/2019 23:26, Stefano Stabellini wrote:
> On Wed, 2 Oct 2019, Julien Grall wrote:
> 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.

If you receive a FIQ exception on Arm64, then you are already doomed because the 
hypervisor will crash (see do_bad_mode()). So receiving the SError is going to 
be your last concern here.

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Thu, 3 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 02/10/2019 23:26, Stefano Stabellini wrote:
> > On Wed, 2 Oct 2019, Julien Grall wrote:
> > 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.
> 
> If you receive a FIQ exception on Arm64, then you are already doomed because
> the hypervisor will crash (see do_bad_mode()). So receiving the SError is
> going to be your last concern here.

I realize that Xen is doomed anyway, but if I was the user, I would
still want to know about the SError: it is not going to save the
platform in any way but it might make me realize there is something
wrong with the guest configuration (in addition to the FIQ problem.)

But because there is no way to get a FIQ in Xen, at least on paper, this
is kind of a theoretical exercise.

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

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

[Julien Grall]

Hi,

On 03/10/2019 18:48, Stefano Stabellini wrote:
> On Thu, 3 Oct 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 02/10/2019 23:26, Stefano Stabellini wrote:
>>> On Wed, 2 Oct 2019, Julien Grall wrote:
>>> 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.
>>
>> If you receive a FIQ exception on Arm64, then you are already doomed because
>> the hypervisor will crash (see do_bad_mode()). So receiving the SError is
>> going to be your last concern here.
> 
> I realize that Xen is doomed anyway, but if I was the user, I would
> still want to know about the SError: it is not going to save the
> platform in any way but it might make me realize there is something
> wrong with the guest configuration (in addition to the FIQ problem.)

This is not something I am going to look at in the near future. There are more 
concerning problem in arch/arm*/entry.S. Although, patches are welcomed.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH RFC for-4.13 07/10] xen/arm: Allow insn.h to be called from assembly

[Julien Grall]

Hi Stefano,

On 10/1/19 10:00 PM, Stefano Stabellini wrote:
> On Thu, 26 Sep 2019, Julien Grall wrote:
>> A follow-up patch will require to include insn.h from assembly code. So
>> wee need to protect any C-specific definition to avoid compilation
>    ^ we                               ^ definitions
> 
>> error when used in assembly code.
>    ^ errors
> 
> 
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/include/asm-arm/insn.h | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
>> index 19277212e1..00391f83f9 100644
>> --- a/xen/include/asm-arm/insn.h
>> +++ b/xen/include/asm-arm/insn.h
>> @@ -1,8 +1,14 @@
>>   #ifndef __ARCH_ARM_INSN
>>   #define __ARCH_ARM_INSN
>>   
>> +#ifndef __ASSEMBLY__
>> +
>>   #include <xen/types.h>
>>   
>> +/*
>> + * At the moment, arch-specific headers contain only definition for C
>> + * code.
>> + */
> 
> Please remove "At the moment, " because in-code comment should always
> reflect the latest state of the codebase.

Well, we do have a lot of "Currently"/"At the moment"/"For now" in the 
code. Some of my patches went in recently with such wording, so why this 
particular one is a problem?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH RFC for-4.13 07/10] xen/arm: Allow insn.h to be called from assembly

[Stefano Stabellini]

On Mon, 21 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/1/19 10:00 PM, Stefano Stabellini wrote:
> > On Thu, 26 Sep 2019, Julien Grall wrote:
> > > A follow-up patch will require to include insn.h from assembly code. So
> > > wee need to protect any C-specific definition to avoid compilation
> >    ^ we                               ^ definitions
> > 
> > > error when used in assembly code.
> >    ^ errors
> > 
> > 
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > ---
> > >   xen/include/asm-arm/insn.h | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
> > > index 19277212e1..00391f83f9 100644
> > > --- a/xen/include/asm-arm/insn.h
> > > +++ b/xen/include/asm-arm/insn.h
> > > @@ -1,8 +1,14 @@
> > >   #ifndef __ARCH_ARM_INSN
> > >   #define __ARCH_ARM_INSN
> > >   +#ifndef __ASSEMBLY__
> > > +
> > >   #include <xen/types.h>
> > >   +/*
> > > + * At the moment, arch-specific headers contain only definition for C
> > > + * code.
> > > + */
> > 
> > Please remove "At the moment, " because in-code comment should always
> > reflect the latest state of the codebase.
> 
> Well, we do have a lot of "Currently"/"At the moment"/"For now" in the code.
> Some of my patches went in recently with such wording, so why this particular
> one is a problem?

Yeah, I realize that a lot of our conventions are still tribal knowledge
(i.e. not written down anywhere.)

Let me explain my take on this. When one writes "At the
moment"/"Currently"/"For now" he/she implies that the code is just
temporary, or at least sub-optimal and something should be down about
it to improve. Maybe not immediately, but it would be nice to have.

I think it is OK to write something like "At the moment," to express
that meaning. That's OK.

In this specific case, the reason why I wrote that reply is that when I
read the in-code comment, it jarred for it ("jarred" as in something was
off, out of place.) And I think the reason is that the code looked OK,
and I didn't get what you think we should "improve" in the code after
this patch is applied.

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