[PATCH v2 00/19] Provide a command line option to choose how to handle SErrors

[PATCH v2 00/19] Provide a command line option to choose how to handle SErrors

[Wei Chen]

From XSA-201, we know that, a guest could trigger SErrors when accessing
memory mapped HW in a non-conventional way. In the patches for XSA-201,
we crash the guest when we captured such asynchronous aborts to avoid data
corruption.

In order to distinguish guest-generated SErrors from hypervisor-generated
SErrors. We have to place SError checking code in every EL1 -> EL2 paths.
That will be an overhead on entries caused by dsb/isb.

But not all platforms want to categorize the SErrors. For example, a host
that is running with trusted guests. The administrator can confirm that
all guests that are running on the host will not trigger such SErrors. In
this user scene, we should provide some options to administrator to avoid
categorizing the SErrors. And then reduce the overhead of dsb/isb.

We provided following 3 options to administrator to determine how to handle
the SErrors:

* `diverse`:
  The hypervisor will distinguish guest SErrors from hypervisor SErrors.
  The guest generated SErrors will be forwarded to guests, the hypervisor
  generated SErrors will cause the whole system crash.
  It requires:
  1. Place dsb/isb on all EL1 -> EL2 trap entries to categorize SErrors
     correctly.
  2. Place dsb/isb on EL2 -> EL1 return paths to prevent slipping hypervisor
     SErrors to guests.
  3. Place dsb/isb in context switch to isolate the SErrors between 2 vCPUs.

* `forward`:
  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
  All SErrors will be forwarded to guests, except the SErrors generated when
  idle vCPU is running. The idle domain doesn't have the ability to hanle the
  SErrors, so we have to crash the whole system when we get SErros with idle
  vCPU. This option will avoid most overhead of the dsb/isb, except the dsb/isb
  in context switch which is used to isolate the SErrors between 2 vCPUs.

* `panic`:
  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
  All SErrors will crash the whole system. This option will avoid all overhead
  of the dsb/isb.

---
v1->v2 changes has been placed in separated patchs.

Wei Chen (19):
  xen/arm: Save ESR_EL2 to avoid using mismatched value in syndrome
    check
  xen/arm: Remove vwfi while setting HCR_EL2 in init_traps
  xen/arm: Move parse_vwfi from trap.c to domain.c
  xen/arm: Restore HCR_EL2 register
  xen/arm: Avoid setting/clearing HCR_RW at every context switch
  xen/arm: Save HCR_EL2 when a guest took the SError
  xen/arm: Introduce a virtual abort injection helper
  xen/arm: Introduce a command line parameter for SErrors/Aborts
  xen/arm: Introduce a initcall to update cpu_hwcaps by serror_op
  xen/arm64: Use alternative to skip the check of pending serrors
  xen/arm32: Use alternative to skip the check of pending serrors
  xen/arm: Move macro VABORT_GEN_BY_GUEST to common header
  xen/arm: Introduce new helpers to handle guest/hyp SErrors
  xen/arm: Replace do_trap_guest_serror with new helpers
  xen/arm: Unmask the Abort/SError bit in the exception entries
  xen/arm: Introduce a helper to synchronize SError
  xen/arm: Isolate the SError between the context switch of 2 vCPUs
  xen/arm: Prevent slipping hypervisor SError to guest
  xen/arm: Handle guest external abort as guest SError

 docs/misc/xen-command-line.markdown   |  43 ++++++++
 xen/arch/arm/arm32/asm-offsets.c      |   1 +
 xen/arch/arm/arm32/entry.S            |  28 ++++-
 xen/arch/arm/arm32/traps.c            |   5 +-
 xen/arch/arm/arm64/asm-offsets.c      |   1 +
 xen/arch/arm/arm64/domctl.c           |   6 ++
 xen/arch/arm/arm64/entry.S            | 105 +++++++++----------
 xen/arch/arm/domain.c                 |  37 +++++++
 xen/arch/arm/domain_build.c           |  14 +++
 xen/arch/arm/p2m.c                    |  10 +-
 xen/arch/arm/traps.c                  | 188 ++++++++++++++++++++++++++++------
 xen/include/asm-arm/arm32/processor.h |  12 +--
 xen/include/asm-arm/arm64/processor.h |   3 +-
 xen/include/asm-arm/cpufeature.h      |   4 +-
 xen/include/asm-arm/domain.h          |   4 +
 xen/include/asm-arm/processor.h       |  17 ++-
 16 files changed, 361 insertions(+), 117 deletions(-)

-- 
2.7.4


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

[PATCH v2 01/19] xen/arm: Save ESR_EL2 to avoid using mismatched value in syndrome check

[Wei Chen]

Xen will do exception syndrome check while some types of exception
take place in EL2. The syndrome check code read the ESR_EL2 register
directly, but in some situation this register maybe overridden by
nested exception.

For example, if we re-enable IRQ before reading ESR_EL2 which means
Xen may enter in IRQ exception mode and return the processor with
clobbered ESR_EL2 (See ARM ARM DDI 0487A.j D7.2.25)

In this case the guest exception syndrome has been overridden, we will
check the syndrome for guest sync exception with an incorrect ESR_EL2
value. So we want to save ESR_EL2 to cpu_user_regs as soon as the
exception takes place in EL2 to avoid using an incorrect syndrome value.

In order to save ESR_EL2, we added a 32-bit member hsr to cpu_user_regs.
But while saving registers in trap entry, we use stp to save ELR and
CPSR at the same time through 64-bit general registers. If we keep this
code, the hsr will be overridden by upper 32-bit of CPSR. So adjust the
code to use str to save ELR in a separate instruction and use stp to
save CPSR and HSR at the same time through 32-bit general registers.
This change affects the registers restore in trap exit, we can't use the
ldp to restore ELR and CPSR from stack at the same time. We have to use
ldr to restore them separately.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>

---
v1->v2:
1. Use more accurate words in the commit message.
2. Remove pointless comment message in cpu_user_regs.
3. Explain the changes of the registers save/restore order in trap
   entry/exit.
---
 xen/arch/arm/arm32/asm-offsets.c      |  1 +
 xen/arch/arm/arm32/entry.S            |  3 +++
 xen/arch/arm/arm64/asm-offsets.c      |  1 +
 xen/arch/arm/arm64/entry.S            | 13 +++++++++----
 xen/arch/arm/traps.c                  |  2 +-
 xen/include/asm-arm/arm32/processor.h |  2 +-
 xen/include/asm-arm/arm64/processor.h |  3 +--
 7 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
index f8e6b53..5b543ab 100644
--- a/xen/arch/arm/arm32/asm-offsets.c
+++ b/xen/arch/arm/arm32/asm-offsets.c
@@ -26,6 +26,7 @@ void __dummy__(void)
    OFFSET(UREGS_lr, struct cpu_user_regs, lr);
    OFFSET(UREGS_pc, struct cpu_user_regs, pc);
    OFFSET(UREGS_cpsr, struct cpu_user_regs, cpsr);
+   OFFSET(UREGS_hsr, struct cpu_user_regs, hsr);
 
    OFFSET(UREGS_LR_usr, struct cpu_user_regs, lr_usr);
    OFFSET(UREGS_SP_usr, struct cpu_user_regs, sp_usr);
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 2a6f4f0..2187226 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -23,6 +23,9 @@
         add r11, sp, #UREGS_kernel_sizeof+4;                            \
         str r11, [sp, #UREGS_sp];                                       \
                                                                         \
+        mrc CP32(r11, HSR);             /* Save exception syndrome */   \
+        str r11, [sp, #UREGS_hsr];                                      \
+                                                                        \
         mrs r11, SPSR_hyp;                                              \
         str r11, [sp, #UREGS_cpsr];                                     \
         and r11, #PSR_MODE_MASK;                                        \
diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
index 69ea92a..ce24e44 100644
--- a/xen/arch/arm/arm64/asm-offsets.c
+++ b/xen/arch/arm/arm64/asm-offsets.c
@@ -27,6 +27,7 @@ void __dummy__(void)
    OFFSET(UREGS_SP, struct cpu_user_regs, sp);
    OFFSET(UREGS_PC, struct cpu_user_regs, pc);
    OFFSET(UREGS_CPSR, struct cpu_user_regs, cpsr);
+   OFFSET(UREGS_ESR_el2, struct cpu_user_regs, hsr);
 
    OFFSET(UREGS_SPSR_el1, struct cpu_user_regs, spsr_el1);
 
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index c181b5e..02802c0 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -121,9 +121,13 @@ lr      .req    x30             // link register
 
         stp     lr, x21, [sp, #UREGS_LR]
 
-        mrs     x22, elr_el2
-        mrs     x23, spsr_el2
-        stp     x22, x23, [sp, #UREGS_PC]
+        mrs     x21, elr_el2
+        str     x21, [sp, #UREGS_PC]
+
+        add     x21, sp, #UREGS_CPSR
+        mrs     x22, spsr_el2
+        mrs     x23, esr_el2
+        stp     w22, w23, [x21]
 
         .endm
 
@@ -307,7 +311,8 @@ ENTRY(return_to_new_vcpu64)
 return_from_trap:
         msr     daifset, #2 /* Mask interrupts */
 
-        ldp     x21, x22, [sp, #UREGS_PC]       // load ELR, SPSR
+        ldr     x21, [sp, #UREGS_PC]            // load ELR
+        ldr     w22, [sp, #UREGS_CPSR]          // load SPSR
 
         pop     x0, x1
         pop     x2, x3
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 614501f..1da6d24 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2641,7 +2641,7 @@ static void enter_hypervisor_head(struct cpu_user_regs *regs)
 
 asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
 {
-    const union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };
+    const union hsr hsr = { .bits = regs->hsr };
 
     enter_hypervisor_head(regs);
 
diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
index db3b17b..f6d5df3 100644
--- a/xen/include/asm-arm/arm32/processor.h
+++ b/xen/include/asm-arm/arm32/processor.h
@@ -37,7 +37,7 @@ struct cpu_user_regs
         uint32_t pc, pc32;
     };
     uint32_t cpsr; /* Return mode */
-    uint32_t pad0; /* Doubleword-align the kernel half of the frame */
+    uint32_t hsr;  /* Exception Syndrome */
 
     /* Outer guest frame only from here on... */
 
diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h
index b0726ff..24f836b 100644
--- a/xen/include/asm-arm/arm64/processor.h
+++ b/xen/include/asm-arm/arm64/processor.h
@@ -66,8 +66,7 @@ struct cpu_user_regs
     /* Return address and mode */
     __DECL_REG(pc,           pc32);             /* ELR_EL2 */
     uint32_t cpsr;                              /* SPSR_EL2 */
-
-    uint32_t pad0; /* Align end of kernel frame. */
+    uint32_t hsr;                               /* ESR_EL2 */
 
     /* Outer guest frame only from here on... */
 
-- 
2.7.4


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

[PATCH v2 02/19] xen/arm: Remove vwfi while setting HCR_EL2 in init_traps

[Wei Chen]

We will set HCR_EL2 for each domain individually at the place where each
domain is created. vwfi will affect the behavior of VM trap. Initialize
the HCR_EL2 in init_traps is a generic setting for AT translations while
creating dom0. After dom0 has been created, the HCR_EL2 will use the saved
value in dom0's context. So checking vwfi in init_trap is pointless.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
I have tried to remove HCR_EL2 setting from init_traps, but the Xen will
hang at the place of creating domain0. The console hot key can work, so
the Xen is alive, not panic.
---
 xen/arch/arm/traps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 1da6d24..2f03f29 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -140,8 +140,8 @@ void init_traps(void)
 
     /* Setup hypervisor traps */
     WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
-                 (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
-                 HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,HCR_EL2);
+                 HCR_TWI|HCR_TWE|HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,
+                 HCR_EL2);
     isb();
 }
 
-- 
2.7.4


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

[PATCH v2 03/19] xen/arm: Move parse_vwfi from trap.c to domain.c

[Wei Chen]

In the past, we'll set the HCR_EL2 while Xen is booting in init_traps.
And the command line parameter "vwfi" will affect the value of setting
HCR_EL2. So we placed the parse_vwfi in trap.c to avoid exporting vwfi
to other source files.

But in previous patch of this series, we had removed the HCR_EL2 setting
from init_traps. Keep parse_vwfi in trap.c is pointless. And we will set
the HCR_EL2 for each domain individually while it's creating in the later
patch of this series. So we move parse_vwfi to domain.c for preparation.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/arch/arm/domain.c | 14 ++++++++++++++
 xen/arch/arm/traps.c  | 14 --------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index bb327da..a327a3b 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -38,6 +38,20 @@
 
 DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
 
+static enum {
+	TRAP,
+	NATIVE,
+} vwfi;
+
+static void __init parse_vwfi(const char *s)
+{
+	if ( !strcmp(s, "native") )
+		vwfi = NATIVE;
+	else
+		vwfi = TRAP;
+}
+custom_param("vwfi", parse_vwfi);
+
 void idle_loop(void)
 {
     for ( ; ; )
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2f03f29..48b3e3c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -101,20 +101,6 @@ static int debug_stack_lines = 40;
 
 integer_param("debug_stack_lines", debug_stack_lines);
 
-static enum {
-	TRAP,
-	NATIVE,
-} vwfi;
-
-static void __init parse_vwfi(const char *s)
-{
-	if ( !strcmp(s, "native") )
-		vwfi = NATIVE;
-	else
-		vwfi = TRAP;
-}
-custom_param("vwfi", parse_vwfi);
-
 void init_traps(void)
 {
     /* Setup Hyp vector base */
-- 
2.7.4


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

[PATCH v2 04/19] xen/arm: Restore HCR_EL2 register

[Wei Chen]

Different domains may have different HCR_EL2 flags. For example, the
64-bit domain needs HCR_RW flag but the 32-bit does not need it. So
we give each domain a default HCR_EL2 value and save it in the vCPU's
context.

HCR_EL2 register has only one bit can be updated automatically without
explicit write (HCR_VSE). But we haven't used this bit currently, so
we can consider that the HCR_EL2 register will not be modified while
the guest is running. So save the HCR_EL2 while guest exiting to
hypervisor is not neccessary. We just have to restore this register for
each vCPU while context switching.

The p2m_restore_state which will be invoked in context switch progress
has included the writing of HCR_EL2 already. It updates the HCR_EL2.RW
bit to tell the hardware how to interpret the stage-1 page table as the
encodings are different between AArch64 and AArch32. We can reuse this
write to restore the HCR_EL2 for each vCPU. Of course, the value of each
vCPU's HCR_EL2 should be adjusted to have proper HCR_EL2.RW bit in this
function. In the later patch of this series, we will set the HCR_EL2.RW
for each vCPU while the domain is creating.

Signed-off-by: wei chen <Wei.Chen@arm.com>
---
v1->v2:
1. Use vwfi to set HCR_EL2 for each vCPU directly.
2. Back HCR_EL2 for idle vCPU to avoid using p2m_restore_state with
   an uninitialized HCR_EL2 value in context.
3. Remove writing to HCR_EL2 from leave_hypervisor_tail.
4. Update commit message to explain why restore HCR_EL2 in
   p2m_restore_state.
---
 xen/arch/arm/domain.c        | 4 ++++
 xen/arch/arm/domain_build.c  | 7 +++++++
 xen/arch/arm/p2m.c           | 9 +++------
 xen/include/asm-arm/domain.h | 3 +++
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index a327a3b..c69e204 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -527,6 +527,10 @@ int vcpu_initialise(struct vcpu *v)
 
     v->arch.actlr = READ_SYSREG32(ACTLR_EL1);
 
+    v->arch.hcr_el2 = HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
+                      (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
+                      HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB;
+
     processor_vcpu_initialise(v);
 
     if ( (rc = vcpu_vgic_init(v)) != 0 )
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index de59e5f..8af223e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2171,6 +2171,13 @@ int construct_dom0(struct domain *d)
         return rc;
 
     /*
+     * The HCR_EL2 will temporarily switch to dom0's HCR_EL2 value
+     * by p2m_restore_state. We have to save HCR_EL2 to idle vCPU's
+     * context for restoring it in later.
+     */
+    current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
+
+    /*
      * The following loads use the domain's p2m and require current to
      * be a vcpu of the domain, temporarily switch
      */
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 6263760..83c4b7d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -128,27 +128,24 @@ void p2m_save_state(struct vcpu *p)
 
 void p2m_restore_state(struct vcpu *n)
 {
-    register_t hcr;
     struct p2m_domain *p2m = &n->domain->arch.p2m;
     uint8_t *last_vcpu_ran;
 
     if ( is_idle_vcpu(n) )
         return;
 
-    hcr = READ_SYSREG(HCR_EL2);
-
     WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
     isb();
 
     if ( is_32bit_domain(n->domain) )
-        hcr &= ~HCR_RW;
+        n->arch.hcr_el2 &= ~HCR_RW;
     else
-        hcr |= HCR_RW;
+        n->arch.hcr_el2 |= HCR_RW;
 
     WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
     isb();
 
-    WRITE_SYSREG(hcr, HCR_EL2);
+    WRITE_SYSREG(n->arch.hcr_el2, HCR_EL2);
     isb();
 
     last_vcpu_ran = &p2m->last_vcpu_ran[smp_processor_id()];
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 09fe502..7b1dacc 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -204,6 +204,9 @@ struct arch_vcpu
     register_t tpidr_el1;
     register_t tpidrro_el0;
 
+    /* HYP configuration */
+    register_t hcr_el2;
+
     uint32_t teecr, teehbr; /* ThumbEE, 32-bit guests only */
 #ifdef CONFIG_ARM_32
     /*
-- 
2.7.4


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

[PATCH v2 05/19] xen/arm: Avoid setting/clearing HCR_RW at every context switch

[Wei Chen]

The HCR_EL2 flags for 64-bit and 32-bit domains are different. But
when we initialized the HCR_EL2 for vcpu0 of Dom0 and all vcpus of
DomU in vcpu_initialise, we didn't know the domain's address size
information. We had to use compatible flags to initialize HCR_EL2,
and set HCR_RW for 64-bit domain or clear HCR_RW for 32-bit domain
at every context switch.

But, after we added the HCR_EL2 to vcpu's context, this behaviour
seems a little fussy. We can update the HCR_RW bit in vcpu's context
as soon as we get the domain's address size to avoid setting/clearing
HCR_RW at every context switch.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/arch/arm/arm64/domctl.c  | 6 ++++++
 xen/arch/arm/domain.c        | 5 +++++
 xen/arch/arm/domain_build.c  | 7 +++++++
 xen/arch/arm/p2m.c           | 5 -----
 xen/include/asm-arm/domain.h | 1 +
 5 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
index 44e1e7b..ab8781f 100644
--- a/xen/arch/arm/arm64/domctl.c
+++ b/xen/arch/arm/arm64/domctl.c
@@ -14,6 +14,8 @@
 
 static long switch_mode(struct domain *d, enum domain_type type)
 {
+    struct vcpu *v;
+
     if ( d == NULL )
         return -EINVAL;
     if ( d->tot_pages != 0 )
@@ -23,6 +25,10 @@ static long switch_mode(struct domain *d, enum domain_type type)
 
     d->arch.type = type;
 
+    if ( is_64bit_domain(d) )
+        for_each_vcpu(d, v)
+            vcpu_switch_to_aarch64_mode(v);
+
     return 0;
 }
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index c69e204..18b34e7 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -553,6 +553,11 @@ void vcpu_destroy(struct vcpu *v)
     free_xenheap_pages(v->arch.stack, STACK_ORDER);
 }
 
+void vcpu_switch_to_aarch64_mode(struct vcpu *v)
+{
+    v->arch.hcr_el2 |= HCR_RW;
+}
+
 int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                        struct xen_arch_domainconfig *config)
 {
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8af223e..14475a5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2148,6 +2148,10 @@ int construct_dom0(struct domain *d)
         return -EINVAL;
     }
     d->arch.type = kinfo.type;
+
+    if ( is_64bit_domain(d) )
+        vcpu_switch_to_aarch64_mode(v);
+
 #endif
 
     allocate_memory(d, &kinfo);
@@ -2247,6 +2251,9 @@ int construct_dom0(struct domain *d)
             printk("Failed to allocate dom0 vcpu %d on pcpu %d\n", i, cpu);
             break;
         }
+
+        if ( is_64bit_domain(d) )
+            vcpu_switch_to_aarch64_mode(d->vcpu[i]);
     }
 
     return 0;
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 83c4b7d..34d5776 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -137,11 +137,6 @@ void p2m_restore_state(struct vcpu *n)
     WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
     isb();
 
-    if ( is_32bit_domain(n->domain) )
-        n->arch.hcr_el2 &= ~HCR_RW;
-    else
-        n->arch.hcr_el2 |= HCR_RW;
-
     WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
     isb();
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 7b1dacc..68185e2 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -268,6 +268,7 @@ struct arch_vcpu
 
 void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
+void vcpu_switch_to_aarch64_mode(struct vcpu *);
 
 unsigned int domain_max_vcpus(const struct domain *);
 
-- 
2.7.4


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

[PATCH v2 06/19] xen/arm: Save HCR_EL2 when a guest took the SError

[Wei Chen]

The HCR_EL2.VSE (HCR.VA for aarch32) bit can be used to generate a
virtual abort to guest. The HCR_EL2.VSE bit has a peculiar feature
of getting cleared when the guest has taken the abort (this is the
only bit that behaves as such in HCR_EL2 register).

This means that if we set the HCR_EL2.VSE bit to signal such an abort,
we must preserve it in the guest context until it disappears from
HCR_EL2, and at which point it must be cleared from the context. This
is achieved by reading back from HCR_EL2 until the guest takes the
fault.

If we preserved a pending VSE in guest context, we have to restore
it to HCR_EL2 when context switch to this guest. This is achieved
by writing saved HCR_EL2 value in guest context back to HCR_EL2
register before return to guest. This had been done by the patch
of "Restore HCR_EL2 register".

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/traps.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 48b3e3c..7762d18 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2622,7 +2622,18 @@ static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
 static void enter_hypervisor_head(struct cpu_user_regs *regs)
 {
     if ( guest_mode(regs) )
+    {
+        /*
+         * 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 ( current->arch.hcr_el2 & HCR_VA )
+            current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
+
         gic_clear_lrs(current);
+    }
 }
 
 asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
-- 
2.7.4


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

[PATCH v2 07/19] xen/arm: Introduce a virtual abort injection helper

[Wei Chen]

When guest triggers async aborts, in most platform, such aborts
will be routed to hypervisor. But we don't want the hypervisor
to handle such aborts, so we have to route such aborts back to
the guest.

This helper is using the HCR_EL2.VSE (HCR.VA for aarch32) bit to
route such aborts back to the guest. If the guest PC had been
advanced by SVC/HVC/SMC instructions before we caught the SError
in hypervisor, we have to adjust the guest PC to exact address
while the SError generated.

About HSR_EC_SVC32/64, even thought we don't trap SVC32/64 today,
we would like them to be handled here. This would be useful when
VM introspection will gain support of SVC32/64 trapping.

This helper will be used by the later patches in this series, we
use #if 0 to disable it in this patch temporarily to remove the
warning message of unused function from compiler.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
v1->v2:
1. After updating HCR_EL2.VSE bit of vCPU HCR_EL2, write the value
   to HCR_EL2 immediately. In this case we don't need to move the
   restoration of HCR_EL2 to leave_hypervisor_tail, and it worked
   regardless of whether we get preempted.
2. Add Stefano's Acked-by.
---
 xen/arch/arm/traps.c            | 33 +++++++++++++++++++++++++++++++++
 xen/include/asm-arm/processor.h |  1 +
 2 files changed, 34 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 7762d18..7850115 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -599,6 +599,39 @@ static void inject_dabt_exception(struct cpu_user_regs *regs,
 #endif
 }
 
+#if 0
+/* Inject a virtual Abort/SError into the guest. */
+static void inject_vabt_exception(struct cpu_user_regs *regs)
+{
+    const union hsr hsr = { .bits = regs->hsr };
+
+    /*
+     * SVC/HVC/SMC already have an adjusted PC (See ARM ARM DDI 0487A.j
+     * D1.10.1 for more details), which we need to correct in order to
+     * return to after having injected the SError.
+     */
+    switch ( hsr.ec )
+    {
+    case HSR_EC_SVC32:
+    case HSR_EC_HVC32:
+    case HSR_EC_SMC32:
+#ifdef CONFIG_ARM_64
+    case HSR_EC_SVC64:
+    case HSR_EC_HVC64:
+    case HSR_EC_SMC64:
+#endif
+        regs->pc -= hsr.len ? 4 : 2;
+        break;
+
+    default:
+        break;
+    }
+
+    current->arch.hcr_el2 |= HCR_VA;
+    WRITE_SYSREG(current->arch.hcr_el2, HCR_EL2);
+}
+#endif
+
 struct reg_ctxt {
     /* Guest-side state */
     uint32_t sctlr_el1;
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index afc0e9a..47e7026 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -252,6 +252,7 @@
 #define HSR_EC_HVC32                0x12
 #define HSR_EC_SMC32                0x13
 #ifdef CONFIG_ARM_64
+#define HSR_EC_SVC64                0x15
 #define HSR_EC_HVC64                0x16
 #define HSR_EC_SMC64                0x17
 #define HSR_EC_SYSREG               0x18
-- 
2.7.4


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

[PATCH v2 08/19] xen/arm: Introduce a command line parameter for SErrors/Aborts

[Wei Chen]

In order to distinguish guest-generated SErrors from hypervisor-generated
SErrors we have to place SError checking code in every EL1 -> EL2 paths.
That will cause overhead on entries due to dsb/isb.

However, not all platforms want to categorize SErrors. For example, a host
that is running with trusted guests. The administrator can confirm that
all guests that are running on the host will not trigger such SErrors. In
this user-case, we should provide some options to administrators to avoid
categorizing SErrors and then reduce the overhead of dsb/isb.

We provided the following 3 options to administrators to determine how
the hypervisors handle SErrors:

* `diverse`:
  The hypervisor will distinguish guest SErrors from hypervisor SErrors.
  The guest generated SErrors will be forwarded to guests, the hypervisor
  generated SErrors will cause the whole system to crash.
  It requires:
  1. dsb/isb on all EL1 -> EL2 trap entries to categorize SErrors
     correctly.
  2. dsb/isb on EL2 -> EL1 return paths to prevent slipping hypervisor
     SErrors to guests.
  3. dsb/isb in context switch to isolate SErrors between 2 vCPUs.

* `forward`:
  The hypervisor will not distinguish guest SErrors from hypervisor
  SErrors. All SErrors will be forwarded to guests, except the SErrors
  generated when  the idle vCPU is running. The idle domain doesn't have
  the ability to handle SErrors, so we have to crash the whole system when
  we get SErros with the idle vCPU. This option will avoid most overhead
  of the dsb/isb, except the dsb/isb in context switch which is used to
  isolate the SErrors between 2 vCPUs.

* `panic`:
  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
  All SErrors will crash the whole system. This option will avoid all
  overhead of the dsb/isb pairs.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v1->v2:
1. Correct words and syntax in the commit message and docs.
2. Add Stefano's Reviewed-by tag.
---
 docs/misc/xen-command-line.markdown | 43 +++++++++++++++++++++++++++++++++++++
 xen/arch/arm/traps.c                | 19 ++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index bdbdb8a..6d395c5 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1474,6 +1474,49 @@ enabling more sockets and cores to go into deeper sleep states.
 
 Set the serial transmit buffer size.
 
+### serrors (ARM)
+> `= diverse | forward | panic`
+
+> Default: `diverse`
+
+This parameter is provided to administrators to determine how the
+hypervisors handle SErrors.
+
+In order to distinguish guest-generated SErrors from hypervisor-generated
+SErrors we have to place SError checking code in every EL1 -> EL2 paths.
+That will cause overhead on entries due to dsb/isb. However, not all platforms
+need to categorize SErrors. For example, a host that is running with trusted
+guests. The administrator can confirm that all guests that are running on the
+host will not trigger such SErrors. In this case, the administrator can use
+this parameter to skip categorizing SErrors and reduce the overhead of dsb/isb.
+
+We provided the following 3 options to administrators to determine how the
+hypervisors handle SErrors:
+
+* `diverse`:
+  The hypervisor will distinguish guest SErrors from hypervisor SErrors.
+  The guest generated SErrors will be forwarded to guests, the hypervisor
+  generated SErrors will cause the whole system to crash.
+  It requires:
+  1. dsb/isb on all EL1 -> EL2 trap entries to categorize SErrors correctly.
+  2. dsb/isb on EL2 -> EL1 return paths to prevent slipping hypervisor
+     SErrors to guests.
+  3. dsb/isb in context switch to isolate SErrors between 2 vCPUs.
+
+* `forward`:
+  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
+  All SErrors will be forwarded to guests, except the SErrors generated when
+  the idle vCPU is running. The idle domain doesn't have the ability to handle
+  SErrors, so we have to crash the whole system when we get SErros with the
+  idle vCPU. This option will avoid most overhead of the dsb/isb, except the
+  dsb/isb in context switch which is used to isolate the SErrors between 2
+  vCPUs.
+
+* `panic`:
+  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
+  All SErrors will crash the whole system. This option will avoid all overhead
+  of the dsb/isb pairs.
+
 ### smap
 > `= <boolean> | hvm`
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 7850115..955d97c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -101,6 +101,25 @@ static int debug_stack_lines = 40;
 
 integer_param("debug_stack_lines", debug_stack_lines);
 
+static enum {
+    SERRORS_DIVERSE,
+    SERRORS_FORWARD,
+    SERRORS_PANIC,
+} serrors_op;
+
+static void __init parse_serrors_behavior(const char *str)
+{
+    if ( !strcmp(str, "forward") )
+        serrors_op = SERRORS_FORWARD;
+    else if ( !strcmp(str, "panic") )
+        serrors_op = SERRORS_PANIC;
+    else
+        serrors_op = SERRORS_DIVERSE;
+
+    return;
+}
+custom_param("serrors", parse_serrors_behavior);
+
 void init_traps(void)
 {
     /* Setup Hyp vector base */
-- 
2.7.4


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

[PATCH v2 09/19] xen/arm: Introduce a initcall to update cpu_hwcaps by serror_op

[Wei Chen]

In the later patches of this series, we want to use the alternative
patching framework to avoid checking serror_op in every entries.
So we define a new cpu feature "SKIP_CHECK_PENDING_VSERROR" for
serror_op. When serror_op is not equal to SERROR_DIVERSE, this
feature will be set to cpu_hwcaps.

Currently, the default serror_op is SERROR_DIVERSE, if we want to
change the serror_op value we have to place the serror parameter
in command line. It seems no problem to update cpu_hwcaps directly
in the serror parameter parsing function.

But one day, if we change the default serror_op to SERROR_PANIC or
SERROR_FORWARD by some security policy. We may not place the serror
parameter in command line. In this case, if we rely on the serror
parameter parsing function to update cpu_hwcaps, this function would
not be invoked and the "SKIP_CHECK_PENDING_VSERROR" could not be
set in cpu_hwcaps.

So, we introduce this initcall to guarantee the cpu_hwcaps can be
updated no matter the serror parameter is placed in the command line
or not.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
v1->v2:
1. Explain this initcall is to future-proof the code in commit
   message.
2. Fix a coding style of this initcall.
---
 xen/arch/arm/traps.c             | 9 +++++++++
 xen/include/asm-arm/cpufeature.h | 3 ++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 955d97c..dafb730 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -120,6 +120,15 @@ static void __init parse_serrors_behavior(const char *str)
 }
 custom_param("serrors", parse_serrors_behavior);
 
+static int __init update_serrors_cpu_caps(void)
+{
+    if ( serrors_op != SERRORS_DIVERSE )
+        cpus_set_cap(SKIP_CHECK_PENDING_VSERROR);
+
+    return 0;
+}
+__initcall(update_serrors_cpu_caps);
+
 void init_traps(void)
 {
     /* Setup Hyp vector base */
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index c0a25ae..ec3f9e5 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -40,8 +40,9 @@
 #define ARM32_WORKAROUND_766422 2
 #define ARM64_WORKAROUND_834220 3
 #define LIVEPATCH_FEATURE   4
+#define SKIP_CHECK_PENDING_VSERROR 5
 
-#define ARM_NCAPS           5
+#define ARM_NCAPS           6
 
 #ifndef __ASSEMBLY__
 
-- 
2.7.4


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

[PATCH v2 10/19] xen/arm64: Use alternative to skip the check of pending serrors

[Wei Chen]

We have provided an option to administrator to determine how to
handle the SErrors. In order to skip the check of pending SError,
in conventional way, we have to read the option every time before
we try to check the pending SError. This will add overhead to check
the option at every trap.

The ARM64 supports the alternative patching feature. We can use an
ALTERNATIVE to avoid checking option at every trap. We added a new
cpufeature named "SKIP_CHECK_PENDING_VSERROR". This feature will be
enabled when the option is not diverse.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/arm64/entry.S | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 02802c0..4baa3cb 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -1,5 +1,6 @@
 #include <asm/asm_defns.h>
 #include <asm/regs.h>
+#include <asm/alternative.h>
 #include <public/xen.h>
 
 /*
@@ -229,12 +230,14 @@ hyp_irq:
 
 guest_sync:
         entry   hyp=0, compat=0
-        bl      check_pending_vserror
         /*
-         * If x0 is Non-zero, a vSError took place, the initial exception
-         * doesn't have any significance to be handled. Exit ASAP
+         * The vSError will be checked while SKIP_CHECK_PENDING_VSERROR is
+         * not set. If a vSError took place, the initial exception will be
+         * skipped. Exit ASAP
          */
-        cbnz    x0, 1f
+        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
+                    "nop; nop",
+                    SKIP_CHECK_PENDING_VSERROR)
         msr     daifclr, #2
         mov     x0, sp
         bl      do_trap_hypervisor
@@ -243,12 +246,14 @@ guest_sync:
 
 guest_irq:
         entry   hyp=0, compat=0
-        bl      check_pending_vserror
         /*
-         * If x0 is Non-zero, a vSError took place, the initial exception
-         * doesn't have any significance to be handled. Exit ASAP
+         * The vSError will be checked while SKIP_CHECK_PENDING_VSERROR is
+         * not set. If a vSError took place, the initial exception will be
+         * skipped. Exit ASAP
          */
-        cbnz    x0, 1f
+        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
+                    "nop; nop",
+                    SKIP_CHECK_PENDING_VSERROR)
         mov     x0, sp
         bl      do_trap_irq
 1:
@@ -267,12 +272,14 @@ guest_error:
 
 guest_sync_compat:
         entry   hyp=0, compat=1
-        bl      check_pending_vserror
         /*
-         * If x0 is Non-zero, a vSError took place, the initial exception
-         * doesn't have any significance to be handled. Exit ASAP
+         * The vSError will be checked while SKIP_CHECK_PENDING_VSERROR is
+         * not set. If a vSError took place, the initial exception will be
+         * skipped. Exit ASAP
          */
-        cbnz    x0, 1f
+        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
+                    "nop; nop",
+                    SKIP_CHECK_PENDING_VSERROR)
         msr     daifclr, #2
         mov     x0, sp
         bl      do_trap_hypervisor
@@ -281,12 +288,14 @@ guest_sync_compat:
 
 guest_irq_compat:
         entry   hyp=0, compat=1
-        bl      check_pending_vserror
         /*
-         * If x0 is Non-zero, a vSError took place, the initial exception
-         * doesn't have any significance to be handled. Exit ASAP
+         * The vSError will be checked while SKIP_CHECK_PENDING_VSERROR is
+         * not set. If a vSError took place, the initial exception will be
+         * skipped. Exit ASAP
          */
-        cbnz    x0, 1f
+        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
+                    "nop; nop",
+                    SKIP_CHECK_PENDING_VSERROR)
         mov     x0, sp
         bl      do_trap_irq
 1:
-- 
2.7.4


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

[PATCH v2 11/19] xen/arm32: Use alternative to skip the check of pending serrors

[Wei Chen]

We have provided an option to administrator to determine how to
handle the SErrors. In order to skip the check of pending SError,
in conventional way, we have to read the option every time before
we try to check the pending SError. This will add overhead to check
the option at every trap.

The ARM32 supports the alternative patching feature. We can use an
ALTERNATIVE to avoid checking option at every trap. We added a new
cpufeature named "SKIP_CHECK_PENDING_VSERROR". This feature will be
enabled when the option is not diverse.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/arch/arm/arm32/entry.S | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 2187226..105cae8 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -1,5 +1,6 @@
 #include <asm/asm_defns.h>
 #include <asm/regs.h>
+#include <asm/alternative.h>
 #include <public/xen.h>
 
 #define SAVE_ONE_BANKED(reg)    mrs r11, reg; str r11, [sp, #UREGS_##reg]
@@ -44,6 +45,14 @@ save_guest_regs:
         SAVE_BANKED(fiq)
         SAVE_ONE_BANKED(R8_fiq); SAVE_ONE_BANKED(R9_fiq); SAVE_ONE_BANKED(R10_fiq)
         SAVE_ONE_BANKED(R11_fiq); SAVE_ONE_BANKED(R12_fiq);
+
+        /*
+         * If the SKIP_CHECK_PENDING_VSERROR has been set in the cpu feature,
+         * the checking of pending SErrors will be skipped.
+         */
+        ALTERNATIVE("nop",
+                    "b skip_check",
+                    SKIP_CHECK_PENDING_VSERROR)
         /*
          * Start to check pending virtual abort in the gap of Guest -> HYP
          * world switch.
@@ -99,6 +108,7 @@ abort_guest_exit_end:
          */
         bne return_from_trap
 
+skip_check:
         mov pc, lr
 
 #define DEFINE_TRAP_ENTRY(trap)                                         \
-- 
2.7.4


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

[PATCH v2 12/19] xen/arm: Move macro VABORT_GEN_BY_GUEST to common header

[Wei Chen]

We want to move part of SErrors checking code from hyp_error assembly code
to a function. This new function will use this macro to distinguish the
guest SErrors from hypervisor SErrors. So we have to move this macro to
common header.

The VABORT_GEN_BY_GUEST macro used two symbols abort_guest_exit_start and
abort_guest_exit_end. After we moved this macro to common header, we
should export these two symbols to other source files that will use
VABORT_GEN_BY_GUEST macro. So we change these two symbols to global.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
v1->v2:
1. Explain in commit message why we change abort_guest_exit_start and
   abort_guest_exit_end to global.
---
 xen/arch/arm/arm64/entry.S            |  2 ++
 xen/include/asm-arm/arm32/processor.h | 10 ----------
 xen/include/asm-arm/processor.h       | 10 ++++++++++
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 4baa3cb..113e1c3 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -380,10 +380,12 @@ check_pending_vserror:
          * exception handler, and the elr_el2 will be set to
          * abort_guest_exit_start or abort_guest_exit_end.
          */
+        .global abort_guest_exit_start
 abort_guest_exit_start:
 
         isb
 
+        .global abort_guest_exit_end
 abort_guest_exit_end:
         /* Mask PSTATE asynchronous abort bit, close the checking window. */
         msr     daifset, #4
diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
index f6d5df3..68cc821 100644
--- a/xen/include/asm-arm/arm32/processor.h
+++ b/xen/include/asm-arm/arm32/processor.h
@@ -56,16 +56,6 @@ struct cpu_user_regs
     uint32_t pad1; /* Doubleword-align the user half of the frame */
 };
 
-/* Functions for pending virtual abort checking window. */
-void abort_guest_exit_start(void);
-void abort_guest_exit_end(void);
-
-#define VABORT_GEN_BY_GUEST(r)  \
-( \
-    ( (unsigned long)abort_guest_exit_start == (r)->pc ) || \
-    ( (unsigned long)abort_guest_exit_end == (r)->pc ) \
-)
-
 #endif
 
 /* Layout as used in assembly, with src/dest registers mixed in */
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 47e7026..700bde9 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -709,6 +709,16 @@ int call_smc(register_t function_id, register_t arg0, register_t arg1,
 
 void do_trap_guest_error(struct cpu_user_regs *regs);
 
+/* Functions for pending virtual abort checking window. */
+void abort_guest_exit_start(void);
+void abort_guest_exit_end(void);
+
+#define VABORT_GEN_BY_GUEST(r)  \
+( \
+    ( (unsigned long)abort_guest_exit_start == (r)->pc ) || \
+    ( (unsigned long)abort_guest_exit_end == (r)->pc ) \
+)
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_ARM_PROCESSOR_H */
 /*
-- 
2.7.4


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

[PATCH v2 13/19] xen/arm: Introduce new helpers to handle guest/hyp SErrors

[Wei Chen]

Currently, ARM32 and ARM64 has different SError exception handlers.
These handlers include lots of code to check SError handle options
and code to distinguish guest-generated SErrors from hypervisor
SErrors.

The new helpers: do_trap_guest_serror and do_trap_hyp_serror are
wrappers of __do_trap_serror with constant guest/hyp parameters.
__do_trap_serror moves the option checking code and SError checking
code from assembly to C source. This will make the code become more
readable and avoid placing check code in too many places.

These two helpers only handle the following 3 types of SErrors:
1) Guest-generated SError and had been delivered in EL1 and then
   been forwarded to EL2.
2) Guest-generated SError but hadn't been delivered in EL1 before
   trapping to EL2. This SError would be caught in EL2 as soon as
   we just unmasked the PSTATE.A bit.
3) Hypervisor generated native SError, that would be a bug.

In the new helpers, we have used the function "inject_vabt_exception"
which was disabled by "#if 0" before. Now, we can remove the "#if 0"
to make this function to be available.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/traps.c            | 69 +++++++++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/processor.h |  4 +++
 2 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index dafb730..a6f6e24 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -627,7 +627,6 @@ static void inject_dabt_exception(struct cpu_user_regs *regs,
 #endif
 }
 
-#if 0
 /* Inject a virtual Abort/SError into the guest. */
 static void inject_vabt_exception(struct cpu_user_regs *regs)
 {
@@ -658,7 +657,59 @@ static void inject_vabt_exception(struct cpu_user_regs *regs)
     current->arch.hcr_el2 |= HCR_VA;
     WRITE_SYSREG(current->arch.hcr_el2, HCR_EL2);
 }
-#endif
+
+/*
+ * SError exception handler. We only handle the following 3 types of SErrors:
+ * 1) Guest-generated SError and had been delivered in EL1 and then
+ *    been forwarded to EL2.
+ * 2) Guest-generated SError but hadn't been delivered in EL1 before
+ *    trapping to EL2. This SError would be caught in EL2 as soon as
+ *    we just unmasked the PSTATE.A bit.
+ * 3) Hypervisor generated native SError, that would be a bug.
+ *
+ * A true parameter "guest" means that the SError is type#1 or type#2.
+ */
+static void __do_trap_serror(struct cpu_user_regs *regs, bool guest)
+{
+    /*
+     * Only "DIVERSE" option needs to distinguish the guest-generated SErrors
+     * from hypervisor SErrors.
+     */
+    if ( serrors_op == SERRORS_DIVERSE )
+    {
+        /* Forward the type#1 and type#2 SErrors to guests. */
+        if ( guest )
+            return inject_vabt_exception(regs);
+
+        /* Type#3 SErrors will panic the whole system */
+        goto crash_system;
+    }
+
+    /*
+     * The "FORWARD" option will forward all SErrors to the guests, except
+     * idle domain generated SErrors.
+     */
+    if ( serrors_op == SERRORS_FORWARD )
+    {
+        /*
+         * Because the idle domain doesn't have the ability to handle the
+         * SErrors, we have to crash the whole system while we get a SError
+         * generated by idle domain.
+         */
+        if ( is_idle_vcpu(current) )
+            goto crash_system;
+
+        return inject_vabt_exception(regs);
+    }
+
+crash_system:
+    /* Three possibilities to crash the whole system:
+     * 1) "DIVERSE" option with Hypervisor generated SErrors.
+     * 2) "FORWARD" option with Idle Domain generated SErrors.
+     * 3) "PANIC" option with all SErrors.
+     */
+    do_unexpected_trap("SError", regs);
+}
 
 struct reg_ctxt {
     /* Guest-side state */
@@ -2845,6 +2896,20 @@ asmlinkage void do_trap_guest_error(struct cpu_user_regs *regs)
     domain_crash_synchronous();
 }
 
+asmlinkage void do_trap_hyp_serror(struct cpu_user_regs *regs)
+{
+    enter_hypervisor_head(regs);
+
+    __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
+}
+
+asmlinkage void do_trap_guest_serror(struct cpu_user_regs *regs)
+{
+    enter_hypervisor_head(regs);
+
+    __do_trap_serror(regs, true);
+}
+
 asmlinkage void do_trap_irq(struct cpu_user_regs *regs)
 {
     enter_hypervisor_head(regs);
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 700bde9..2db2370 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -709,6 +709,10 @@ int call_smc(register_t function_id, register_t arg0, register_t arg1,
 
 void do_trap_guest_error(struct cpu_user_regs *regs);
 
+void do_trap_hyp_serror(struct cpu_user_regs *regs);
+
+void do_trap_guest_serror(struct cpu_user_regs *regs);
+
 /* Functions for pending virtual abort checking window. */
 void abort_guest_exit_start(void);
 void abort_guest_exit_end(void);
-- 
2.7.4


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

[PATCH v2 14/19] xen/arm: Replace do_trap_guest_serror with new helpers

[Wei Chen]

We have introduced two helpers to handle the guest/hyp SErrors:
do_trap_guest_serror and do_trap_guest_hyp_serror. These handlers
can take the role of do_trap_guest_serror and reduce the assembly
code in the same time. So we use these two helpers to replace it
and drop it now.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/arm32/traps.c      |  5 +----
 xen/arch/arm/arm64/entry.S      | 36 +++---------------------------------
 xen/arch/arm/traps.c            | 15 ---------------
 xen/include/asm-arm/processor.h |  2 --
 4 files changed, 4 insertions(+), 54 deletions(-)

diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
index 4176f0e..5bc5f64 100644
--- a/xen/arch/arm/arm32/traps.c
+++ b/xen/arch/arm/arm32/traps.c
@@ -62,10 +62,7 @@ asmlinkage void do_trap_prefetch_abort(struct cpu_user_regs *regs)
 
 asmlinkage void do_trap_data_abort(struct cpu_user_regs *regs)
 {
-    if ( VABORT_GEN_BY_GUEST(regs) )
-        do_trap_guest_error(regs);
-    else
-        do_unexpected_trap("Data Abort", regs);
+    do_trap_hyp_serror(regs);
 }
 
 /*
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 113e1c3..8d5a890 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -178,40 +178,10 @@ hyp_error_invalid:
         invalid BAD_ERROR
 
 hyp_error:
-        /*
-         * Only two possibilities:
-         * 1) Either we come from the exit path, having just unmasked
-         *    PSTATE.A: change the return code to an EL2 fault, and
-         *    carry on, as we're already in a sane state to handle it.
-         * 2) Or we come from anywhere else, and that's a bug: we panic.
-         */
         entry   hyp=1
         msr     daifclr, #2
-
-        /*
-         * The ELR_EL2 may be modified by an interrupt, so we have to use the
-         * saved value in cpu_user_regs to check whether we come from 1) or
-         * not.
-         */
-        ldr     x0, [sp, #UREGS_PC]
-        adr     x1, abort_guest_exit_start
-        cmp     x0, x1
-        adr     x1, abort_guest_exit_end
-        ccmp    x0, x1, #4, ne
         mov     x0, sp
-        mov     x1, #BAD_ERROR
-
-        /*
-         * Not equal, the exception come from 2). It's a bug, we have to
-         * panic the hypervisor.
-         */
-        b.ne    do_bad_mode
-
-        /*
-         * Otherwise, the exception come from 1). It happened because of
-         * the guest. Crash this guest.
-         */
-        bl      do_trap_guest_error
+        bl      do_trap_hyp_serror
         exit    hyp=1
 
 /* Traps taken in Current EL with SP_ELx */
@@ -267,7 +237,7 @@ guest_error:
         entry   hyp=0, compat=0
         msr     daifclr, #2
         mov     x0, sp
-        bl      do_trap_guest_error
+        bl      do_trap_guest_serror
         exit    hyp=0, compat=0
 
 guest_sync_compat:
@@ -309,7 +279,7 @@ guest_error_compat:
         entry   hyp=0, compat=1
         msr     daifclr, #2
         mov     x0, sp
-        bl      do_trap_guest_error
+        bl      do_trap_guest_serror
         exit    hyp=0, compat=1
 
 ENTRY(return_to_new_vcpu32)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index a6f6e24..f454fb5 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2881,21 +2881,6 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
     }
 }
 
-asmlinkage void do_trap_guest_error(struct cpu_user_regs *regs)
-{
-    enter_hypervisor_head(regs);
-
-    /*
-     * Currently, to ensure hypervisor safety, when we received a
-     * guest-generated vSerror/vAbort, we just crash the guest to protect
-     * the hypervisor. In future we can better handle this by injecting
-     * a vSerror/vAbort to the guest.
-     */
-    gdprintk(XENLOG_WARNING, "Guest(Dom-%u) will be crashed by vSError\n",
-             current->domain->domain_id);
-    domain_crash_synchronous();
-}
-
 asmlinkage void do_trap_hyp_serror(struct cpu_user_regs *regs)
 {
     enter_hypervisor_head(regs);
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 2db2370..ead6ad3 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -707,8 +707,6 @@ void vcpu_regs_user_to_hyp(struct vcpu *vcpu,
 int call_smc(register_t function_id, register_t arg0, register_t arg1,
              register_t arg2);
 
-void do_trap_guest_error(struct cpu_user_regs *regs);
-
 void do_trap_hyp_serror(struct cpu_user_regs *regs);
 
 void do_trap_guest_serror(struct cpu_user_regs *regs);
-- 
2.7.4


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

[PATCH v2 15/19] xen/arm: Unmask the Abort/SError bit in the exception entries

[Wei Chen]

Currently, we masked the Abort/SError bit in Xen exception entries.
So Xen could not capture any Abort/SError while it's running.
Now, Xen has the ability to handle the Abort/SError, we should unmask
the Abort/SError bit by default to let Xen capture Abort/SError while
it's running.

But in order to avoid receiving nested asynchronous abort, we don't
unmask Abort/SError bit in hyp_error and trap_data_abort.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/arm32/entry.S | 15 ++++++++++++++-
 xen/arch/arm/arm64/entry.S | 13 ++++++++-----
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 105cae8..592e4a8 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -116,6 +116,7 @@ skip_check:
 trap_##trap:                                                            \
         SAVE_ALL;                                                       \
         cpsie i;        /* local_irq_enable */                          \
+        cpsie a;        /* asynchronous abort enable */                 \
         adr lr, return_from_trap;                                       \
         mov r0, sp;                                                     \
         mov r11, sp;                                                    \
@@ -126,6 +127,18 @@ trap_##trap:                                                            \
         ALIGN;                                                          \
 trap_##trap:                                                            \
         SAVE_ALL;                                                       \
+        cpsie a;        /* asynchronous abort enable */                 \
+        adr lr, return_from_trap;                                       \
+        mov r0, sp;                                                     \
+        mov r11, sp;                                                    \
+        bic sp, #7; /* Align the stack pointer (noop on guest trap) */  \
+        b do_trap_##trap
+
+#define DEFINE_TRAP_ENTRY_NOABORT(trap)                                 \
+        ALIGN;                                                          \
+trap_##trap:                                                            \
+        SAVE_ALL;                                                       \
+        cpsie i;        /* local_irq_enable */                          \
         adr lr, return_from_trap;                                       \
         mov r0, sp;                                                     \
         mov r11, sp;                                                    \
@@ -146,10 +159,10 @@ GLOBAL(hyp_traps_vector)
 DEFINE_TRAP_ENTRY(undefined_instruction)
 DEFINE_TRAP_ENTRY(supervisor_call)
 DEFINE_TRAP_ENTRY(prefetch_abort)
-DEFINE_TRAP_ENTRY(data_abort)
 DEFINE_TRAP_ENTRY(hypervisor)
 DEFINE_TRAP_ENTRY_NOIRQ(irq)
 DEFINE_TRAP_ENTRY_NOIRQ(fiq)
+DEFINE_TRAP_ENTRY_NOABORT(data_abort)
 
 return_from_trap:
         mov sp, r11
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 8d5a890..0401a41 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -187,13 +187,14 @@ hyp_error:
 /* Traps taken in Current EL with SP_ELx */
 hyp_sync:
         entry   hyp=1
-        msr     daifclr, #2
+        msr     daifclr, #6
         mov     x0, sp
         bl      do_trap_hypervisor
         exit    hyp=1
 
 hyp_irq:
         entry   hyp=1
+        msr     daifclr, #4
         mov     x0, sp
         bl      do_trap_irq
         exit    hyp=1
@@ -208,7 +209,7 @@ guest_sync:
         ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
                     "nop; nop",
                     SKIP_CHECK_PENDING_VSERROR)
-        msr     daifclr, #2
+        msr     daifclr, #6
         mov     x0, sp
         bl      do_trap_hypervisor
 1:
@@ -224,6 +225,7 @@ guest_irq:
         ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
                     "nop; nop",
                     SKIP_CHECK_PENDING_VSERROR)
+        msr     daifclr, #4
         mov     x0, sp
         bl      do_trap_irq
 1:
@@ -235,7 +237,7 @@ guest_fiq_invalid:
 
 guest_error:
         entry   hyp=0, compat=0
-        msr     daifclr, #2
+        msr     daifclr, #6
         mov     x0, sp
         bl      do_trap_guest_serror
         exit    hyp=0, compat=0
@@ -250,7 +252,7 @@ guest_sync_compat:
         ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
                     "nop; nop",
                     SKIP_CHECK_PENDING_VSERROR)
-        msr     daifclr, #2
+        msr     daifclr, #6
         mov     x0, sp
         bl      do_trap_hypervisor
 1:
@@ -266,6 +268,7 @@ guest_irq_compat:
         ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
                     "nop; nop",
                     SKIP_CHECK_PENDING_VSERROR)
+        msr     daifclr, #4
         mov     x0, sp
         bl      do_trap_irq
 1:
@@ -277,7 +280,7 @@ guest_fiq_invalid_compat:
 
 guest_error_compat:
         entry   hyp=0, compat=1
-        msr     daifclr, #2
+        msr     daifclr, #6
         mov     x0, sp
         bl      do_trap_guest_serror
         exit    hyp=0, compat=1
-- 
2.7.4


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

[PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

[Wei Chen]

In previous patches, we have provided the ability to synchronize
SErrors in exception entries. But we haven't synchronized SErrors
while returning to guest and doing context switch.

So we still have two risks:
1. Slipping hypervisor SErrors to guest. For example, hypervisor
   triggers a SError while returning to guest, but this SError may be
   delivered after entering guest. In "DIVERSE" option, this SError
   would be routed back to guest and panic the guest. But actually,
   we should crash the whole system due to this hypervisor SError.
2. Slipping previous guest SErrors to the next guest. In "FORWARD"
   option, if hypervisor triggers a SError while context switching.
   This SError may be delivered after switching to next vCPU. In this
   case, this SError will be forwarded to next vCPU and may panic
   an incorrect guest.

So we have have to introduce this helper to synchronize SErrors while
returning to guest and doing context switch.

This function should be used out of trap.c in later patch of this
series. We have to export this helper in header file.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v1->v2:
1. Update commit message to explain why we introduce this helper.
2. Remove static for this helper.
3. Add Stefano's Reviewed-by tag.
---
 xen/arch/arm/traps.c            | 9 +++++++++
 xen/include/asm-arm/processor.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f454fb5..55d85ed 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2881,6 +2881,15 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
     }
 }
 
+void synchronize_serror(void)
+{
+    /* Synchronize against in-flight ld/st. */
+    dsb(sy);
+
+    /* A single instruction exception window */
+    isb();
+}
+
 asmlinkage void do_trap_hyp_serror(struct cpu_user_regs *regs)
 {
     enter_hypervisor_head(regs);
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index ead6ad3..3ebbe57 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -721,6 +721,8 @@ void abort_guest_exit_end(void);
     ( (unsigned long)abort_guest_exit_end == (r)->pc ) \
 )
 
+void synchronize_serror(void);
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_ARM_PROCESSOR_H */
 /*
-- 
2.7.4


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

[PATCH v2 17/19] xen/arm: Isolate the SError between the context switch of 2 vCPUs

[Wei Chen]

If there is a pending SError while we are doing context switch, if the
SError handle option is "FORWARD", We have to guranatee this serror to
be caught by current vCPU, otherwise it will be caught by next vCPU and
be forwarded to this wrong vCPU.

So we have to synchronize SError before switch to next vCPU. But this is
only required by "FORWARD" option. In this case we added a new flag
SKIP_CTXT_SWITCH_SERROR_SYNC in cpu_hwcaps to skip synchronizing SError
in context switch for other options. In the meantime, we don't need to
export serror_op accessing to other source files.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
v1->v2:
1. Added a new flag in cpu_hwcaps to avoid using serror_op to skip
   synchronizing SError in context switch.
2. Update commit message to explain why we added this cpu_hwcaps.
---
 xen/arch/arm/domain.c            | 14 ++++++++++++++
 xen/arch/arm/traps.c             |  3 +++
 xen/include/asm-arm/cpufeature.h |  3 ++-
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 18b34e7..e866ed3 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -29,6 +29,7 @@
 #include <asm/cpufeature.h>
 #include <asm/vfp.h>
 #include <asm/procinfo.h>
+#include <asm/alternative.h>
 
 #include <asm/gic.h>
 #include <asm/vgic.h>
@@ -326,6 +327,19 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
 
     local_irq_disable();
 
+    /*
+     * If the SErrors option is "FORWARD", we have to prevent forwarding
+     * serror to wrong vCPU. So before context switch, we have to use the
+     * synchronize_serror to guarantee that the pending serror would be
+     * caught by current vCPU.
+     *
+     * The SKIP_CTXT_SWITCH_SERROR_SYNC will be set to cpu_hwcaps when the
+     * SErrors option is NOT "FORWARD".
+     */
+    asm volatile(ALTERNATIVE("bl synchronize_serror",
+                             "nop",
+                             SKIP_CTXT_SWITCH_SERROR_SYNC));
+
     set_current(next);
 
     prev = __context_switch(prev, next);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 55d85ed..9b4546e 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -125,6 +125,9 @@ static int __init update_serrors_cpu_caps(void)
     if ( serrors_op != SERRORS_DIVERSE )
         cpus_set_cap(SKIP_CHECK_PENDING_VSERROR);
 
+    if ( serrors_op != SERRORS_FORWARD )
+        cpus_set_cap(SKIP_CTXT_SWITCH_SERROR_SYNC);
+
     return 0;
 }
 __initcall(update_serrors_cpu_caps);
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index ec3f9e5..99ecb2b 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -41,8 +41,9 @@
 #define ARM64_WORKAROUND_834220 3
 #define LIVEPATCH_FEATURE   4
 #define SKIP_CHECK_PENDING_VSERROR 5
+#define SKIP_CTXT_SWITCH_SERROR_SYNC 6
 
-#define ARM_NCAPS           6
+#define ARM_NCAPS           7
 
 #ifndef __ASSEMBLY__
 
-- 
2.7.4


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

[PATCH v2 18/19] xen/arm: Prevent slipping hypervisor SError to guest

[Wei Chen]

If there is a pending SError while we're returning from trap. If the
SError handle option is "DIVERSE", we have to prevent slipping this
hypervisor SError to guest. So we have to use the dsb/isb to guarantee
that the pending hypervisor SError would be caught in hypervisor before
return to guest.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/traps.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9b4546e..f3d794e 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2926,6 +2926,16 @@ asmlinkage void leave_hypervisor_tail(void)
         local_irq_disable();
         if (!softirq_pending(smp_processor_id())) {
             gic_inject();
+
+            /*
+             * If the SErrors handle option is "DIVERSE", we have to prevent
+             * slipping the hypervisor SError to guest. So before returning
+             * from trap, we use the synchronize_serror to guarantee that the
+             * pending SError would be caught in hypervisor.
+             */
+            if ( serrors_op == SERRORS_DIVERSE )
+                synchronize_serror();
+
             return;
         }
         local_irq_enable();
-- 
2.7.4


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

[PATCH v2 19/19] xen/arm: Handle guest external abort as guest SError

[Wei Chen]

The guest generated external data/instruction aborts can be treated
as guest SErrors. We already have a handler to handle the SErrors,
so we can reuse this handler to handle guest external aborts.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/traps.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f3d794e..ca18394 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2543,12 +2543,12 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
 
     /*
      * If this bit has been set, it means that this instruction abort is caused
-     * by a guest external abort. Currently we crash the guest to protect the
-     * hypervisor. In future one can better handle this by injecting a virtual
-     * abort to the guest.
+     * by a guest external abort. We can handle this instruction abort as guest
+     * SError.
      */
     if ( hsr.iabt.eat )
-        domain_crash_synchronous();
+        return __do_trap_serror(regs, true);
+
 
     if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
         gpa = get_faulting_ipa(gva);
@@ -2646,12 +2646,10 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
 
     /*
      * If this bit has been set, it means that this data abort is caused
-     * by a guest external abort. Currently we crash the guest to protect the
-     * hypervisor. In future one can better handle this by injecting a virtual
-     * abort to the guest.
+     * by a guest external abort. We treat this data abort as guest SError.
      */
     if ( dabt.eat )
-        domain_crash_synchronous();
+        return __do_trap_serror(regs, true);
 
     info.dabt = dabt;
 #ifdef CONFIG_ARM_32
-- 
2.7.4


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

Re: [PATCH v2 01/19] xen/arm: Save ESR_EL2 to avoid using mismatched value in syndrome check

[Julien Grall]

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:
> Xen will do exception syndrome check while some types of exception
> take place in EL2. The syndrome check code read the ESR_EL2 register
> directly, but in some situation this register maybe overridden by
> nested exception.
>
> For example, if we re-enable IRQ before reading ESR_EL2 which means
> Xen may enter in IRQ exception mode and return the processor with
> clobbered ESR_EL2 (See ARM ARM DDI 0487A.j D7.2.25)
>
> In this case the guest exception syndrome has been overridden, we will
> check the syndrome for guest sync exception with an incorrect ESR_EL2
> value. So we want to save ESR_EL2 to cpu_user_regs as soon as the
> exception takes place in EL2 to avoid using an incorrect syndrome value.
>
> In order to save ESR_EL2, we added a 32-bit member hsr to cpu_user_regs.
> But while saving registers in trap entry, we use stp to save ELR and
> CPSR at the same time through 64-bit general registers. If we keep this
> code, the hsr will be overridden by upper 32-bit of CPSR. So adjust the
> code to use str to save ELR in a separate instruction and use stp to
> save CPSR and HSR at the same time through 32-bit general registers.
> This change affects the registers restore in trap exit, we can't use the
> ldp to restore ELR and CPSR from stack at the same time. We have to use
> ldr to restore them separately.
>
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>
> ---

As mentioned in the internal review, it would have been helpful to 
mention this is a bug fix and should be backported on Xen 4.8 and Xen 4.7.


> v1->v2:
> 1. Use more accurate words in the commit message.
> 2. Remove pointless comment message in cpu_user_regs.
> 3. Explain the changes of the registers save/restore order in trap
>    entry/exit.
> ---
>  xen/arch/arm/arm32/asm-offsets.c      |  1 +
>  xen/arch/arm/arm32/entry.S            |  3 +++
>  xen/arch/arm/arm64/asm-offsets.c      |  1 +
>  xen/arch/arm/arm64/entry.S            | 13 +++++++++----
>  xen/arch/arm/traps.c                  |  2 +-
>  xen/include/asm-arm/arm32/processor.h |  2 +-
>  xen/include/asm-arm/arm64/processor.h |  3 +--

A quick grep gives of ESR_EL2 in the code gives me:

include/asm-arm/cpregs.h
308:#define ESR_EL2                 HSR

arch/arm/arm64/traps.c
35:    union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };

arch/arm/traps.c
846:    printk("   ESR_EL2: %08"PRIx32"\n", READ_SYSREG32(ESR_EL2));
2424:     *  (the bit ESR_EL2.S1PTW is set)
2644:    const union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };

The problem you describe also apply for the other READ_SYSREG32(ESR_EL2) 
and I was expecting to see them updated in this patch.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 02/19] xen/arm: Remove vwfi while setting HCR_EL2 in init_traps

[Julien Grall]

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:
> We will set HCR_EL2 for each domain individually at the place where each
> domain is created. vwfi will affect the behavior of VM trap. Initialize
> the HCR_EL2 in init_traps is a generic setting for AT translations while
> creating dom0.

This statement looks wrong. Any AT s1s2 translations (i.e on behalf of 
the guest) should be done after a call to p2m_restore_state that restore 
HCR_EL2, SCTLR_EL1, VTTBR_EL1. If it is not the case, then there is an 
error.

> After dom0 has been created, the HCR_EL2 will use the saved
> value in dom0's context. So checking vwfi in init_trap is pointless.
>
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>

This is a nack from me. We don't remove feature even temporarily without 
any strong reasons. This is making more difficult to track the history 
of a feature and a call to forget to restore it correctly later on or 
removing the feature if we ever decide to revert the patch which adds 
back the feature (see patch #4).

I would prefer to see patch #2, #3 squashed into #4, the patch will not 
be that big (50 lines) and avoid to drop a feature temporarily. But I am 
not convinced about your reasons.

> ---
> I have tried to remove HCR_EL2 setting from init_traps, but the Xen will
> hang at the place of creating domain0. The console hot key can work, so
> the Xen is alive, not panic.

With the limited description you provided it is hard to know what's 
going on. In the future please try to provide meaningful details 
(platform used, debugging you have done...). Anyway, I have done some 
debug and I don't end up to the same conclusion as you.

I tried on different boards, and it gets stuck when initializing stage-2 
page table configuration on each CPU (see setup_virt_paging). The 
secondary CPUs don't receive the SGI.

Reading a bit more the ARM ARM (D7.2.33 in ARM DDI 0487A.k_iss10775), 
HCR_EL2 is unknown at reset. Whilst I already knew that, I would have 
expected to have no impact on EL2 (at least in the non-VHE case). 
However, the value of the {A,I,F}MO bits will affect the routing of 
physical IRQs to Xen.

I have only gone quickly through the spec, so it might be possible to 
have other necessary bits. It might be good to keep initialization here 
until someone sit in front of the spec for few hours and check everything.

So in this case I would prefer to keep the helper avoiding to have 
declared twice the same flags. Stefano any opinions?

Also, whilst I was debugging the problem I noticed the vwfi option does 
not work properly on CPU0. Indeed, the command line has not been parsed 
when init_traps is called on CPU0. This is should be fixed by patch #4 
in this series, but I don't think we want to backport it. Stefano, would 
you be up to write a patch for this?

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 04/19] xen/arm: Restore HCR_EL2 register

[Julien Grall]

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index de59e5f..8af223e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2171,6 +2171,13 @@ int construct_dom0(struct domain *d)
>          return rc;
>
>      /*
> +     * The HCR_EL2 will temporarily switch to dom0's HCR_EL2 value
> +     * by p2m_restore_state. We have to save HCR_EL2 to idle vCPU's
> +     * context for restoring it in later.
> +     */
> +    current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);

I don't understand why we care here. idle vCPU will never restore 
HCR_EL2 nor return from the hypervisor.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 05/19] xen/arm: Avoid setting/clearing HCR_RW at every context switch

[Julien Grall]

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:
> The HCR_EL2 flags for 64-bit and 32-bit domains are different. But
> when we initialized the HCR_EL2 for vcpu0 of Dom0 and all vcpus of
> DomU in vcpu_initialise, we didn't know the domain's address size
> information. We had to use compatible flags to initialize HCR_EL2,
> and set HCR_RW for 64-bit domain or clear HCR_RW for 32-bit domain
> at every context switch.
>
> But, after we added the HCR_EL2 to vcpu's context, this behaviour
> seems a little fussy. We can update the HCR_RW bit in vcpu's context
> as soon as we get the domain's address size to avoid setting/clearing
> HCR_RW at every context switch.
>
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>

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

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 07/19] xen/arm: Introduce a virtual abort injection helper

[Julien Grall]

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:
> When guest triggers async aborts, in most platform, such aborts
> will be routed to hypervisor. But we don't want the hypervisor
> to handle such aborts, so we have to route such aborts back to
> the guest.
>
> This helper is using the HCR_EL2.VSE (HCR.VA for aarch32) bit to
> route such aborts back to the guest. If the guest PC had been
> advanced by SVC/HVC/SMC instructions before we caught the SError
> in hypervisor, we have to adjust the guest PC to exact address
> while the SError generated.
>
> About HSR_EC_SVC32/64, even thought we don't trap SVC32/64 today,
> we would like them to be handled here. This would be useful when
> VM introspection will gain support of SVC32/64 trapping.
>
> This helper will be used by the later patches in this series, we
> use #if 0 to disable it in this patch temporarily to remove the
> warning message of unused function from compiler.
>
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> v1->v2:
> 1. After updating HCR_EL2.VSE bit of vCPU HCR_EL2, write the value
>    to HCR_EL2 immediately. In this case we don't need to move the
>    restoration of HCR_EL2 to leave_hypervisor_tail, and it worked
>    regardless of whether we get preempted.

This should have been explained in the commit message and ...

> 2. Add Stefano's Acked-by.

I would not keep Acked-by/Reviewed-by with a change like above. Or at 
least, I would ask whether they are fine with this change.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 08/19] xen/arm: Introduce a command line parameter for SErrors/Aborts

[Julien Grall]

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:
> In order to distinguish guest-generated SErrors from hypervisor-generated
> SErrors we have to place SError checking code in every EL1 -> EL2 paths.
> That will cause overhead on entries due to dsb/isb.
>
> However, not all platforms want to categorize SErrors. For example, a host
> that is running with trusted guests. The administrator can confirm that
> all guests that are running on the host will not trigger such SErrors. In
> this user-case, we should provide some options to administrators to avoid

s/user-case/use-case/

> +### serrors (ARM)
> +> `= diverse | forward | panic`
> +
> +> Default: `diverse`
> +
> +This parameter is provided to administrators to determine how the
> +hypervisors handle SErrors.
> +
> +In order to distinguish guest-generated SErrors from hypervisor-generated
> +SErrors we have to place SError checking code in every EL1 -> EL2 paths.
> +That will cause overhead on entries due to dsb/isb. However, not all platforms

s/entries/entries and exits/ I think because none of the options you 
provide will effectively only do dsb/isb on entries.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 09/19] xen/arm: Introduce a initcall to update cpu_hwcaps by serror_op

[Julien Grall]

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:
> In the later patches of this series, we want to use the alternative
> patching framework to avoid checking serror_op in every entries.
> So we define a new cpu feature "SKIP_CHECK_PENDING_VSERROR" for
> serror_op. When serror_op is not equal to SERROR_DIVERSE, this
> feature will be set to cpu_hwcaps.
>
> Currently, the default serror_op is SERROR_DIVERSE, if we want to
> change the serror_op value we have to place the serror parameter
> in command line. It seems no problem to update cpu_hwcaps directly
> in the serror parameter parsing function.
>
> But one day, if we change the default serror_op to SERROR_PANIC or
> SERROR_FORWARD by some security policy. We may not place the serror
> parameter in command line. In this case, if we rely on the serror
> parameter parsing function to update cpu_hwcaps, this function would
> not be invoked and the "SKIP_CHECK_PENDING_VSERROR" could not be
> set in cpu_hwcaps.

This paragraph is a bit difficult to understand. I would reword to: 
"While the default option will be diverse today, this may change in the 
future. So we introduce ...." and then paste the paragraph below.

>
> So, we introduce this initcall to guarantee the cpu_hwcaps can be
> updated no matter the serror parameter is placed in the command line
> or not.
>
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>

With the change suggested above:

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

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 09/19] xen/arm: Introduce a initcall to update cpu_hwcaps by serror_op

[Julien Grall]

On 30/03/17 10:13, Wei Chen wrote:
> In the later patches of this series, we want to use the alternative
> patching framework to avoid checking serror_op in every entries.
> So we define a new cpu feature "SKIP_CHECK_PENDING_VSERROR" for
> serror_op. When serror_op is not equal to SERROR_DIVERSE, this
> feature will be set to cpu_hwcaps.
>
> Currently, the default serror_op is SERROR_DIVERSE, if we want to
> change the serror_op value we have to place the serror parameter
> in command line. It seems no problem to update cpu_hwcaps directly
> in the serror parameter parsing function.
>
> But one day, if we change the default serror_op to SERROR_PANIC or
> SERROR_FORWARD by some security policy. We may not place the serror
> parameter in command line. In this case, if we rely on the serror
> parameter parsing function to update cpu_hwcaps, this function would
> not be invoked and the "SKIP_CHECK_PENDING_VSERROR" could not be
> set in cpu_hwcaps.
>
> So, we introduce this initcall to guarantee the cpu_hwcaps can be
> updated no matter the serror parameter is placed in the command line
> or not.
>
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
> v1->v2:
> 1. Explain this initcall is to future-proof the code in commit
>    message.
> 2. Fix a coding style of this initcall.
> ---
>  xen/arch/arm/traps.c             | 9 +++++++++
>  xen/include/asm-arm/cpufeature.h | 3 ++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 955d97c..dafb730 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -120,6 +120,15 @@ static void __init parse_serrors_behavior(const char *str)
>  }
>  custom_param("serrors", parse_serrors_behavior);
>
> +static int __init update_serrors_cpu_caps(void)
> +{
> +    if ( serrors_op != SERRORS_DIVERSE )
> +        cpus_set_cap(SKIP_CHECK_PENDING_VSERROR);

Thinking a bit more of this. I am wondering if we should add a warning 
(see warning_add) if the user is selecting an option other than diverse. 
Two reasons for that:
	1) The user is fully aware that he is not classifying the SError at his 
own risks
	2) If someone send an e-mail saying: "My guest crashed the hypervisor 
with an SError". We can directly know from the log.

Any opinions?

-- 
Julien Grall

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

Re: [PATCH v2 11/19] xen/arm32: Use alternative to skip the check of pending serrors

[Julien Grall]

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:
> We have provided an option to administrator to determine how to
> handle the SErrors. In order to skip the check of pending SError,
> in conventional way, we have to read the option every time before
> we try to check the pending SError. This will add overhead to check
> the option at every trap.
>
> The ARM32 supports the alternative patching feature. We can use an
> ALTERNATIVE to avoid checking option at every trap. We added a new
> cpufeature named "SKIP_CHECK_PENDING_VSERROR". This feature will be
> enabled when the option is not diverse.
>
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>

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

> ---

Stefano, this is requiring the alternative patch (see [1]) which is 
still in review.

Wei, a general rule is to mention the dependencies between series. So we 
don't apply by mistake in the wrong order.

Cheers,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg04132.html

>  xen/arch/arm/arm32/entry.S | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 2187226..105cae8 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -1,5 +1,6 @@
>  #include <asm/asm_defns.h>
>  #include <asm/regs.h>
> +#include <asm/alternative.h>
>  #include <public/xen.h>
>
>  #define SAVE_ONE_BANKED(reg)    mrs r11, reg; str r11, [sp, #UREGS_##reg]
> @@ -44,6 +45,14 @@ save_guest_regs:
>          SAVE_BANKED(fiq)
>          SAVE_ONE_BANKED(R8_fiq); SAVE_ONE_BANKED(R9_fiq); SAVE_ONE_BANKED(R10_fiq)
>          SAVE_ONE_BANKED(R11_fiq); SAVE_ONE_BANKED(R12_fiq);
> +
> +        /*
> +         * If the SKIP_CHECK_PENDING_VSERROR has been set in the cpu feature,
> +         * the checking of pending SErrors will be skipped.
> +         */
> +        ALTERNATIVE("nop",
> +                    "b skip_check",
> +                    SKIP_CHECK_PENDING_VSERROR)
>          /*
>           * Start to check pending virtual abort in the gap of Guest -> HYP
>           * world switch.
> @@ -99,6 +108,7 @@ abort_guest_exit_end:
>           */
>          bne return_from_trap
>
> +skip_check:
>          mov pc, lr
>
>  #define DEFINE_TRAP_ENTRY(trap)                                         \
>

-- 
Julien Grall

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

Re: [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

[Julien Grall]

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:
> +void synchronize_serror(void)

Sorry for been late in the party. Looking at the way you use the 
function, you execute depending on the behavior chosen by the user when 
an SErrors happen. This behavior will not change at runtime, so always 
checking the option chosen in the hot path does not sound very efficient.

I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb). I.e

ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of 
the place.

> +{
> +    /* Synchronize against in-flight ld/st. */
> +    dsb(sy);
> +
> +    /* A single instruction exception window */
> +    isb();
> +}
> +

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

[Julien Grall]

On 30/03/17 19:28, Julien Grall wrote:
> Hi Wei,
>
> On 30/03/17 10:13, Wei Chen wrote:
>> +void synchronize_serror(void)
>
> Sorry for been late in the party. Looking at the way you use the
> function, you execute depending on the behavior chosen by the user when
> an SErrors happen. This behavior will not change at runtime, so always
> checking the option chosen in the hot path does not sound very efficient.
>
> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb). I.e
>
> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
> the place.

To complete what I was suggestion, you could define:

/* Synchronize SError unless the feature is selected */
#define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

[Julien Grall]

On 30/03/17 19:32, Julien Grall wrote:
> On 30/03/17 19:28, Julien Grall wrote:
>> Hi Wei,
>>
>> On 30/03/17 10:13, Wei Chen wrote:
>>> +void synchronize_serror(void)
>>
>> Sorry for been late in the party. Looking at the way you use the
>> function, you execute depending on the behavior chosen by the user when
>> an SErrors happen. This behavior will not change at runtime, so always
>> checking the option chosen in the hot path does not sound very efficient.
>>
>> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
>> I.e
>>
>> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
>> the place.
>
> To complete what I was suggestion, you could define:
>
> /* Synchronize SError unless the feature is selected */
> #define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");

Or even:

/*
  * Synchronize SError unless the feature is selected.
  * This is relying on the SErrors are currently unmasked.
  */
#define SYNCHRONIZE_SERROR(feat)				  \
	do {							  \
	  ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
	  ALTERNATIVE("dsb sy; isb", "nop; nop");		  \
	while (0)

The ASSERT is here to check that we have abort enabled. Otherwise, doing 
the synchronization would be pointless.

Note that the function local_abort_is_enabled is not implemented. But it 
is easy to write it. Looking how local_irq_is_enabled was implemented.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 05/19] xen/arm: Avoid setting/clearing HCR_RW at every context switch

[Stefano Stabellini]

On Thu, 30 Mar 2017, Wei Chen wrote:
> The HCR_EL2 flags for 64-bit and 32-bit domains are different. But
> when we initialized the HCR_EL2 for vcpu0 of Dom0 and all vcpus of
> DomU in vcpu_initialise, we didn't know the domain's address size
> information. We had to use compatible flags to initialize HCR_EL2,
> and set HCR_RW for 64-bit domain or clear HCR_RW for 32-bit domain
> at every context switch.
> 
> But, after we added the HCR_EL2 to vcpu's context, this behaviour
> seems a little fussy. We can update the HCR_RW bit in vcpu's context
> as soon as we get the domain's address size to avoid setting/clearing
> HCR_RW at every context switch.
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>

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

> ---
>  xen/arch/arm/arm64/domctl.c  | 6 ++++++
>  xen/arch/arm/domain.c        | 5 +++++
>  xen/arch/arm/domain_build.c  | 7 +++++++
>  xen/arch/arm/p2m.c           | 5 -----
>  xen/include/asm-arm/domain.h | 1 +
>  5 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> index 44e1e7b..ab8781f 100644
> --- a/xen/arch/arm/arm64/domctl.c
> +++ b/xen/arch/arm/arm64/domctl.c
> @@ -14,6 +14,8 @@
>  
>  static long switch_mode(struct domain *d, enum domain_type type)
>  {
> +    struct vcpu *v;
> +
>      if ( d == NULL )
>          return -EINVAL;
>      if ( d->tot_pages != 0 )
> @@ -23,6 +25,10 @@ static long switch_mode(struct domain *d, enum domain_type type)
>  
>      d->arch.type = type;
>  
> +    if ( is_64bit_domain(d) )
> +        for_each_vcpu(d, v)
> +            vcpu_switch_to_aarch64_mode(v);
> +
>      return 0;
>  }
>  
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index c69e204..18b34e7 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -553,6 +553,11 @@ void vcpu_destroy(struct vcpu *v)
>      free_xenheap_pages(v->arch.stack, STACK_ORDER);
>  }
>  
> +void vcpu_switch_to_aarch64_mode(struct vcpu *v)
> +{
> +    v->arch.hcr_el2 |= HCR_RW;
> +}
> +
>  int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>                         struct xen_arch_domainconfig *config)
>  {
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 8af223e..14475a5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2148,6 +2148,10 @@ int construct_dom0(struct domain *d)
>          return -EINVAL;
>      }
>      d->arch.type = kinfo.type;
> +
> +    if ( is_64bit_domain(d) )
> +        vcpu_switch_to_aarch64_mode(v);
> +
>  #endif
>  
>      allocate_memory(d, &kinfo);
> @@ -2247,6 +2251,9 @@ int construct_dom0(struct domain *d)
>              printk("Failed to allocate dom0 vcpu %d on pcpu %d\n", i, cpu);
>              break;
>          }
> +
> +        if ( is_64bit_domain(d) )
> +            vcpu_switch_to_aarch64_mode(d->vcpu[i]);
>      }
>  
>      return 0;
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 83c4b7d..34d5776 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -137,11 +137,6 @@ void p2m_restore_state(struct vcpu *n)
>      WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
>      isb();
>  
> -    if ( is_32bit_domain(n->domain) )
> -        n->arch.hcr_el2 &= ~HCR_RW;
> -    else
> -        n->arch.hcr_el2 |= HCR_RW;
> -
>      WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
>      isb();
>  
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 7b1dacc..68185e2 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -268,6 +268,7 @@ struct arch_vcpu
>  
>  void vcpu_show_execution_state(struct vcpu *);
>  void vcpu_show_registers(const struct vcpu *);
> +void vcpu_switch_to_aarch64_mode(struct vcpu *);
>  
>  unsigned int domain_max_vcpus(const struct domain *);
>  
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

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

Re: [PATCH v2 07/19] xen/arm: Introduce a virtual abort injection helper

[Stefano Stabellini]

On Thu, 30 Mar 2017, Julien Grall wrote:
> Hi Wei,
> 
> On 30/03/17 10:13, Wei Chen wrote:
> > When guest triggers async aborts, in most platform, such aborts
> > will be routed to hypervisor. But we don't want the hypervisor
> > to handle such aborts, so we have to route such aborts back to
> > the guest.
> > 
> > This helper is using the HCR_EL2.VSE (HCR.VA for aarch32) bit to
> > route such aborts back to the guest. If the guest PC had been
> > advanced by SVC/HVC/SMC instructions before we caught the SError
> > in hypervisor, we have to adjust the guest PC to exact address
> > while the SError generated.
> > 
> > About HSR_EC_SVC32/64, even thought we don't trap SVC32/64 today,
> > we would like them to be handled here. This would be useful when
> > VM introspection will gain support of SVC32/64 trapping.
> > 
> > This helper will be used by the later patches in this series, we
> > use #if 0 to disable it in this patch temporarily to remove the
> > warning message of unused function from compiler.
> > 
> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > v1->v2:
> > 1. After updating HCR_EL2.VSE bit of vCPU HCR_EL2, write the value
> >    to HCR_EL2 immediately. In this case we don't need to move the
> >    restoration of HCR_EL2 to leave_hypervisor_tail, and it worked
> >    regardless of whether we get preempted.
> 
> This should have been explained in the commit message and ...
> 
> > 2. Add Stefano's Acked-by.
> 
> I would not keep Acked-by/Reviewed-by with a change like above. Or at least, I
> would ask whether they are fine with this change.

That's true. Regardless, for this patch:

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

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

Re: [PATCH v2 09/19] xen/arm: Introduce a initcall to update cpu_hwcaps by serror_op

[Stefano Stabellini]

On Thu, 30 Mar 2017, Julien Grall wrote:
> On 30/03/17 10:13, Wei Chen wrote:
> > In the later patches of this series, we want to use the alternative
> > patching framework to avoid checking serror_op in every entries.
> > So we define a new cpu feature "SKIP_CHECK_PENDING_VSERROR" for
> > serror_op. When serror_op is not equal to SERROR_DIVERSE, this
> > feature will be set to cpu_hwcaps.
> > 
> > Currently, the default serror_op is SERROR_DIVERSE, if we want to
> > change the serror_op value we have to place the serror parameter
> > in command line. It seems no problem to update cpu_hwcaps directly
> > in the serror parameter parsing function.
> > 
> > But one day, if we change the default serror_op to SERROR_PANIC or
> > SERROR_FORWARD by some security policy. We may not place the serror
> > parameter in command line. In this case, if we rely on the serror
> > parameter parsing function to update cpu_hwcaps, this function would
> > not be invoked and the "SKIP_CHECK_PENDING_VSERROR" could not be
> > set in cpu_hwcaps.
> > 
> > So, we introduce this initcall to guarantee the cpu_hwcaps can be
> > updated no matter the serror parameter is placed in the command line
> > or not.
> > 
> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > ---
> > v1->v2:
> > 1. Explain this initcall is to future-proof the code in commit
> >    message.
> > 2. Fix a coding style of this initcall.
> > ---
> >  xen/arch/arm/traps.c             | 9 +++++++++
> >  xen/include/asm-arm/cpufeature.h | 3 ++-
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 955d97c..dafb730 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -120,6 +120,15 @@ static void __init parse_serrors_behavior(const char
> > *str)
> >  }
> >  custom_param("serrors", parse_serrors_behavior);
> > 
> > +static int __init update_serrors_cpu_caps(void)
> > +{
> > +    if ( serrors_op != SERRORS_DIVERSE )
> > +        cpus_set_cap(SKIP_CHECK_PENDING_VSERROR);
> 
> Thinking a bit more of this. I am wondering if we should add a warning (see
> warning_add) if the user is selecting an option other than diverse. Two
> reasons for that:
> 	1) The user is fully aware that he is not classifying the SError at
> his own risks
> 	2) If someone send an e-mail saying: "My guest crashed the hypervisor
> with an SError". We can directly know from the log.
> 
> Any opinions?

I would not do that: warnings are good to warn the user about something
unexpected or potentially unknown. In this case the user has to look up
the docs to find about the other options, where it's clearly written
what they are for. We have to expect taht they know what their are
doing.

For this patch:

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

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

Re: [PATCH v2 11/19] xen/arm32: Use alternative to skip the check of pending serrors

[Stefano Stabellini]

On Thu, 30 Mar 2017, Julien Grall wrote:
> Hi Wei,
> 
> On 30/03/17 10:13, Wei Chen wrote:
> > We have provided an option to administrator to determine how to
> > handle the SErrors. In order to skip the check of pending SError,
> > in conventional way, we have to read the option every time before
> > we try to check the pending SError. This will add overhead to check
> > the option at every trap.
> > 
> > The ARM32 supports the alternative patching feature. We can use an
> > ALTERNATIVE to avoid checking option at every trap. We added a new
> > cpufeature named "SKIP_CHECK_PENDING_VSERROR". This feature will be
> > enabled when the option is not diverse.
> > 
> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> 
> Reviewed-by: Julien Grall <julien.grall@arm.com>
> 
> > ---
> 
> Stefano, this is requiring the alternative patch (see [1]) which is still in
> review.
> 
> Wei, a general rule is to mention the dependencies between series. So we don't
> apply by mistake in the wrong order.

Right. Thanks Wei for sending both the other patch and this series
together, that helps me avoiding mistakes.

But please add a note about the dependencies, just in case I forget.


> Cheers,
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg04132.html
> 
> >  xen/arch/arm/arm32/entry.S | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> > index 2187226..105cae8 100644
> > --- a/xen/arch/arm/arm32/entry.S
> > +++ b/xen/arch/arm/arm32/entry.S
> > @@ -1,5 +1,6 @@
> >  #include <asm/asm_defns.h>
> >  #include <asm/regs.h>
> > +#include <asm/alternative.h>
> >  #include <public/xen.h>
> > 
> >  #define SAVE_ONE_BANKED(reg)    mrs r11, reg; str r11, [sp, #UREGS_##reg]
> > @@ -44,6 +45,14 @@ save_guest_regs:
> >          SAVE_BANKED(fiq)
> >          SAVE_ONE_BANKED(R8_fiq); SAVE_ONE_BANKED(R9_fiq);
> > SAVE_ONE_BANKED(R10_fiq)
> >          SAVE_ONE_BANKED(R11_fiq); SAVE_ONE_BANKED(R12_fiq);
> > +
> > +        /*
> > +         * If the SKIP_CHECK_PENDING_VSERROR has been set in the cpu
> > feature,
> > +         * the checking of pending SErrors will be skipped.
> > +         */
> > +        ALTERNATIVE("nop",
> > +                    "b skip_check",
> > +                    SKIP_CHECK_PENDING_VSERROR)
> >          /*
> >           * Start to check pending virtual abort in the gap of Guest -> HYP
> >           * world switch.
> > @@ -99,6 +108,7 @@ abort_guest_exit_end:
> >           */
> >          bne return_from_trap
> > 
> > +skip_check:
> >          mov pc, lr
> > 
> >  #define DEFINE_TRAP_ENTRY(trap)                                         \
> > 
> 
> -- 
> Julien Grall
> 

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

Re: [PATCH v2 12/19] xen/arm: Move macro VABORT_GEN_BY_GUEST to common header

[Stefano Stabellini]

On Thu, 30 Mar 2017, Wei Chen wrote:
> We want to move part of SErrors checking code from hyp_error assembly code
> to a function. This new function will use this macro to distinguish the
> guest SErrors from hypervisor SErrors. So we have to move this macro to
> common header.
> 
> The VABORT_GEN_BY_GUEST macro used two symbols abort_guest_exit_start and
> abort_guest_exit_end. After we moved this macro to common header, we
> should export these two symbols to other source files that will use
> VABORT_GEN_BY_GUEST macro. So we change these two symbols to global.

Please rewrite this to:

  The VABORT_GEN_BY_GUEST macro uses the symbols abort_guest_exit_start
  and abort_guest_exit_end. After we move this macro to a common header,
  we need to make sure that the two symbols are visible to other source
  files. Currently, they are declared .global in arm32/entry.S, but not
  arm64/entry.S. Fix that.

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


> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
> v1->v2:
> 1. Explain in commit message why we change abort_guest_exit_start and
>    abort_guest_exit_end to global.
> ---
>  xen/arch/arm/arm64/entry.S            |  2 ++
>  xen/include/asm-arm/arm32/processor.h | 10 ----------
>  xen/include/asm-arm/processor.h       | 10 ++++++++++
>  3 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 4baa3cb..113e1c3 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -380,10 +380,12 @@ check_pending_vserror:
>           * exception handler, and the elr_el2 will be set to
>           * abort_guest_exit_start or abort_guest_exit_end.
>           */
> +        .global abort_guest_exit_start
>  abort_guest_exit_start:
>  
>          isb
>  
> +        .global abort_guest_exit_end
>  abort_guest_exit_end:
>          /* Mask PSTATE asynchronous abort bit, close the checking window. */
>          msr     daifset, #4
> diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
> index f6d5df3..68cc821 100644
> --- a/xen/include/asm-arm/arm32/processor.h
> +++ b/xen/include/asm-arm/arm32/processor.h
> @@ -56,16 +56,6 @@ struct cpu_user_regs
>      uint32_t pad1; /* Doubleword-align the user half of the frame */
>  };
>  
> -/* Functions for pending virtual abort checking window. */
> -void abort_guest_exit_start(void);
> -void abort_guest_exit_end(void);
> -
> -#define VABORT_GEN_BY_GUEST(r)  \
> -( \
> -    ( (unsigned long)abort_guest_exit_start == (r)->pc ) || \
> -    ( (unsigned long)abort_guest_exit_end == (r)->pc ) \
> -)
> -
>  #endif
>  
>  /* Layout as used in assembly, with src/dest registers mixed in */
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 47e7026..700bde9 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -709,6 +709,16 @@ int call_smc(register_t function_id, register_t arg0, register_t arg1,
>  
>  void do_trap_guest_error(struct cpu_user_regs *regs);
>  
> +/* Functions for pending virtual abort checking window. */
> +void abort_guest_exit_start(void);
> +void abort_guest_exit_end(void);
> +
> +#define VABORT_GEN_BY_GUEST(r)  \
> +( \
> +    ( (unsigned long)abort_guest_exit_start == (r)->pc ) || \
> +    ( (unsigned long)abort_guest_exit_end == (r)->pc ) \
> +)
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_ARM_PROCESSOR_H */
>  /*
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

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

Re: [PATCH v2 17/19] xen/arm: Isolate the SError between the context switch of 2 vCPUs

[Stefano Stabellini]

On Thu, 30 Mar 2017, Wei Chen wrote:
> If there is a pending SError while we are doing context switch, if the
> SError handle option is "FORWARD", We have to guranatee this serror to
> be caught by current vCPU, otherwise it will be caught by next vCPU and
> be forwarded to this wrong vCPU.
> 
> So we have to synchronize SError before switch to next vCPU. But this is
> only required by "FORWARD" option. In this case we added a new flag
> SKIP_CTXT_SWITCH_SERROR_SYNC in cpu_hwcaps to skip synchronizing SError
> in context switch for other options. In the meantime, we don't need to
> export serror_op accessing to other source files.
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
> v1->v2:
> 1. Added a new flag in cpu_hwcaps to avoid using serror_op to skip
>    synchronizing SError in context switch.
> 2. Update commit message to explain why we added this cpu_hwcaps.
> ---
>  xen/arch/arm/domain.c            | 14 ++++++++++++++
>  xen/arch/arm/traps.c             |  3 +++
>  xen/include/asm-arm/cpufeature.h |  3 ++-
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 18b34e7..e866ed3 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -29,6 +29,7 @@
>  #include <asm/cpufeature.h>
>  #include <asm/vfp.h>
>  #include <asm/procinfo.h>
> +#include <asm/alternative.h>
>  
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
> @@ -326,6 +327,19 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>  
>      local_irq_disable();
>  
> +    /*
> +     * If the SErrors option is "FORWARD", we have to prevent forwarding
> +     * serror to wrong vCPU. So before context switch, we have to use the
> +     * synchronize_serror to guarantee that the pending serror would be
> +     * caught by current vCPU.
> +     *
> +     * The SKIP_CTXT_SWITCH_SERROR_SYNC will be set to cpu_hwcaps when the
> +     * SErrors option is NOT "FORWARD".
> +     */
> +    asm volatile(ALTERNATIVE("bl synchronize_serror",
> +                             "nop",
> +                             SKIP_CTXT_SWITCH_SERROR_SYNC));


This good, but here you need to add:

  barrier();

because the compiler is free to reorder even asm volatile instructions
(it could move the asm volatile after __context_switch theoretically).


>      set_current(next);
>  
>      prev = __context_switch(prev, next);
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 55d85ed..9b4546e 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -125,6 +125,9 @@ static int __init update_serrors_cpu_caps(void)
>      if ( serrors_op != SERRORS_DIVERSE )
>          cpus_set_cap(SKIP_CHECK_PENDING_VSERROR);
>  
> +    if ( serrors_op != SERRORS_FORWARD )
> +        cpus_set_cap(SKIP_CTXT_SWITCH_SERROR_SYNC);
> +
>      return 0;
>  }
>  __initcall(update_serrors_cpu_caps);
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index ec3f9e5..99ecb2b 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -41,8 +41,9 @@
>  #define ARM64_WORKAROUND_834220 3
>  #define LIVEPATCH_FEATURE   4
>  #define SKIP_CHECK_PENDING_VSERROR 5
> +#define SKIP_CTXT_SWITCH_SERROR_SYNC 6
>  
> -#define ARM_NCAPS           6
> +#define ARM_NCAPS           7
>  
>  #ifndef __ASSEMBLY__
>  
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

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

Re: [PATCH v2 17/19] xen/arm: Isolate the SError between the context switch of 2 vCPUs

[Julien Grall]

On 30/03/2017 22:49, Stefano Stabellini wrote:
> On Thu, 30 Mar 2017, Wei Chen wrote:
>> +    /*
>> +     * If the SErrors option is "FORWARD", we have to prevent forwarding
>> +     * serror to wrong vCPU. So before context switch, we have to use the
>> +     * synchronize_serror to guarantee that the pending serror would be
>> +     * caught by current vCPU.
>> +     *
>> +     * The SKIP_CTXT_SWITCH_SERROR_SYNC will be set to cpu_hwcaps when the
>> +     * SErrors option is NOT "FORWARD".
>> +     */
>> +    asm volatile(ALTERNATIVE("bl synchronize_serror",
>> +                             "nop",
>> +                             SKIP_CTXT_SWITCH_SERROR_SYNC));
>
>
> This good, but here you need to add:
>
>   barrier();
>
> because the compiler is free to reorder even asm volatile instructions
> (it could move the asm volatile after __context_switch theoretically).

... or it could moved before hand because there are no barrier... What 
you want to use is asm volatile(ALTERNATIVE(...) : : : "memory");

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 04/19] xen/arm: Restore HCR_EL2 register

[Stefano Stabellini]

On Thu, 30 Mar 2017, Julien Grall wrote:
> Hi Wei,
> 
> On 30/03/17 10:13, Wei Chen wrote:
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index de59e5f..8af223e 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2171,6 +2171,13 @@ int construct_dom0(struct domain *d)
> >          return rc;
> > 
> >      /*
> > +     * The HCR_EL2 will temporarily switch to dom0's HCR_EL2 value
> > +     * by p2m_restore_state. We have to save HCR_EL2 to idle vCPU's
> > +     * context for restoring it in later.
> > +     */
> > +    current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> 
> I don't understand why we care here. idle vCPU will never restore HCR_EL2 nor
> return from the hypervisor.

I don't understand this either

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

Re: [PATCH v2 02/19] xen/arm: Remove vwfi while setting HCR_EL2 in init_traps

[Stefano Stabellini]

On Thu, 30 Mar 2017, Julien Grall wrote:
> Hi Wei,
> 
> On 30/03/17 10:13, Wei Chen wrote:
> > We will set HCR_EL2 for each domain individually at the place where each
> > domain is created. vwfi will affect the behavior of VM trap. Initialize
> > the HCR_EL2 in init_traps is a generic setting for AT translations while
> > creating dom0.
> 
> This statement looks wrong. Any AT s1s2 translations (i.e on behalf of the
> guest) should be done after a call to p2m_restore_state that restore HCR_EL2,
> SCTLR_EL1, VTTBR_EL1. If it is not the case, then there is an error.
> 
> > After dom0 has been created, the HCR_EL2 will use the saved
> > value in dom0's context. So checking vwfi in init_trap is pointless.
> > 
> > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> 
> This is a nack from me. We don't remove feature even temporarily without any
> strong reasons. This is making more difficult to track the history of a
> feature and a call to forget to restore it correctly later on or removing the
> feature if we ever decide to revert the patch which adds back the feature (see
> patch #4).
> 
> I would prefer to see patch #2, #3 squashed into #4, the patch will not be
> that big (50 lines) and avoid to drop a feature temporarily.

Yes, please.


> But I am not convinced about your reasons.
> > ---
> > I have tried to remove HCR_EL2 setting from init_traps, but the Xen will
> > hang at the place of creating domain0. The console hot key can work, so
> > the Xen is alive, not panic.
> 
> With the limited description you provided it is hard to know what's going on.
> In the future please try to provide meaningful details (platform used,
> debugging you have done...). Anyway, I have done some debug and I don't end up
> to the same conclusion as you.
> 
> I tried on different boards, and it gets stuck when initializing stage-2 page
> table configuration on each CPU (see setup_virt_paging). The secondary CPUs
> don't receive the SGI.
> 
> Reading a bit more the ARM ARM (D7.2.33 in ARM DDI 0487A.k_iss10775), HCR_EL2
> is unknown at reset. Whilst I already knew that, I would have expected to have
> no impact on EL2 (at least in the non-VHE case). However, the value of the
> {A,I,F}MO bits will affect the routing of physical IRQs to Xen.
> 
> I have only gone quickly through the spec, so it might be possible to have
> other necessary bits. It might be good to keep initialization here until
> someone sit in front of the spec for few hours and check everything.
> 
> So in this case I would prefer to keep the helper avoiding to have declared
> twice the same flags. Stefano any opinions?

I don't particularly care whether we keep the helper or not. From what
you say we need to set HCR_EL2 in init_traps, but given that's only
temporary, we don't necessarily need to write the same set of flags: for
example HCR_TWI|HCR_TWE are useless at this stage. A minimal set would
suffice. Either way, it's fine by me.


> Also, whilst I was debugging the problem I noticed the vwfi option does not
> work properly on CPU0. Indeed, the command line has not been parsed when
> init_traps is called on CPU0. This is should be fixed by patch #4 in this
> series, but I don't think we want to backport it.

No, we don't want to backport patch #4.


> Stefano, would you be up to write a patch for this?

I am thinking that the patch should only to fix the backports, given
that unstable and 4.9 will be fixed by this series (and given that it's
pretty ugly). I'll send it separately shortly.

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

Re: [PATCH v2 04/19] xen/arm: Restore HCR_EL2 register

[Wei Chen]

Hi Julien and Stefano,

On 2017/3/31 6:03, Stefano Stabellini wrote:
> On Thu, 30 Mar 2017, Julien Grall wrote:
>> Hi Wei,
>>
>> On 30/03/17 10:13, Wei Chen wrote:
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index de59e5f..8af223e 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -2171,6 +2171,13 @@ int construct_dom0(struct domain *d)
>>>          return rc;
>>>
>>>      /*
>>> +     * The HCR_EL2 will temporarily switch to dom0's HCR_EL2 value
>>> +     * by p2m_restore_state. We have to save HCR_EL2 to idle vCPU's
>>> +     * context for restoring it in later.
>>> +     */
>>> +    current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
>>
>> I don't understand why we care here. idle vCPU will never restore HCR_EL2 nor
>> return from the hypervisor.
>
> I don't understand this either
>

Yes, idle vCPU will never return from hypervisor. But in construct_dom0,
it has one chance to restore HCR_EL2.
In in construct_dom0 we will call p2m_restore_state twice.
     saved_current = current;
     p2m_restore_state(v); --->> This is dom0's vCPU0
     [...]
     set_current(saved_current);
     p2m_restore_state(saved_current); --->> this is idle vCPU0

In p2m_restore_state, we will write vcpu->arch.hcr_el2 to HCR_EL2. in
this case, we will write an unknown value to HCR_EL2. Even though this
would not cause any problem from my current testing. But from code
scope, I think it would be a drawback.



-- 
Regards,
Wei Chen

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

Re: [PATCH v2 01/19] xen/arm: Save ESR_EL2 to avoid using mismatched value in syndrome check

[Wei Chen]

Hi Julien
On 2017/3/30 21:31, Julien Grall wrote:
> Hi Wei,
>
> On 30/03/17 10:13, Wei Chen wrote:
>> Xen will do exception syndrome check while some types of exception
>> take place in EL2. The syndrome check code read the ESR_EL2 register
>> directly, but in some situation this register maybe overridden by
>> nested exception.
>>
>> For example, if we re-enable IRQ before reading ESR_EL2 which means
>> Xen may enter in IRQ exception mode and return the processor with
>> clobbered ESR_EL2 (See ARM ARM DDI 0487A.j D7.2.25)
>>
>> In this case the guest exception syndrome has been overridden, we will
>> check the syndrome for guest sync exception with an incorrect ESR_EL2
>> value. So we want to save ESR_EL2 to cpu_user_regs as soon as the
>> exception takes place in EL2 to avoid using an incorrect syndrome value.
>>
>> In order to save ESR_EL2, we added a 32-bit member hsr to cpu_user_regs.
>> But while saving registers in trap entry, we use stp to save ELR and
>> CPSR at the same time through 64-bit general registers. If we keep this
>> code, the hsr will be overridden by upper 32-bit of CPSR. So adjust the
>> code to use str to save ELR in a separate instruction and use stp to
>> save CPSR and HSR at the same time through 32-bit general registers.
>> This change affects the registers restore in trap exit, we can't use the
>> ldp to restore ELR and CPSR from stack at the same time. We have to use
>> ldr to restore them separately.
>>
>> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>>
>> ---
>
> As mentioned in the internal review, it would have been helpful to
> mention this is a bug fix and should be backported on Xen 4.8 and Xen 4.7.
>

Sorry about that, I will add it to the note in next version.

>
>> v1->v2:
>> 1. Use more accurate words in the commit message.
>> 2. Remove pointless comment message in cpu_user_regs.
>> 3. Explain the changes of the registers save/restore order in trap
>>    entry/exit.
>> ---
>>  xen/arch/arm/arm32/asm-offsets.c      |  1 +
>>  xen/arch/arm/arm32/entry.S            |  3 +++
>>  xen/arch/arm/arm64/asm-offsets.c      |  1 +
>>  xen/arch/arm/arm64/entry.S            | 13 +++++++++----
>>  xen/arch/arm/traps.c                  |  2 +-
>>  xen/include/asm-arm/arm32/processor.h |  2 +-
>>  xen/include/asm-arm/arm64/processor.h |  3 +--
>
> A quick grep gives of ESR_EL2 in the code gives me:
>
> include/asm-arm/cpregs.h
> 308:#define ESR_EL2                 HSR
>
> arch/arm/arm64/traps.c
> 35:    union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };
>
> arch/arm/traps.c
> 846:    printk("   ESR_EL2: %08"PRIx32"\n", READ_SYSREG32(ESR_EL2));
> 2424:     *  (the bit ESR_EL2.S1PTW is set)
> 2644:    const union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };
>
> The problem you describe also apply for the other READ_SYSREG32(ESR_EL2)
> and I was expecting to see them updated in this patch.
>

Yes, that's what I hadn't considered. I would cover them in next version.


> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [PATCH v2 07/19] xen/arm: Introduce a virtual abort injection helper

[Wei Chen]

On 2017/3/31 1:20, Julien Grall wrote:
> Hi Wei,
>
> On 30/03/17 10:13, Wei Chen wrote:
>> When guest triggers async aborts, in most platform, such aborts
>> will be routed to hypervisor. But we don't want the hypervisor
>> to handle such aborts, so we have to route such aborts back to
>> the guest.
>>
>> This helper is using the HCR_EL2.VSE (HCR.VA for aarch32) bit to
>> route such aborts back to the guest. If the guest PC had been
>> advanced by SVC/HVC/SMC instructions before we caught the SError
>> in hypervisor, we have to adjust the guest PC to exact address
>> while the SError generated.
>>
>> About HSR_EC_SVC32/64, even thought we don't trap SVC32/64 today,
>> we would like them to be handled here. This would be useful when
>> VM introspection will gain support of SVC32/64 trapping.
>>
>> This helper will be used by the later patches in this series, we
>> use #if 0 to disable it in this patch temporarily to remove the
>> warning message of unused function from compiler.
>>
>> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>> ---
>> v1->v2:
>> 1. After updating HCR_EL2.VSE bit of vCPU HCR_EL2, write the value
>>    to HCR_EL2 immediately. In this case we don't need to move the
>>    restoration of HCR_EL2 to leave_hypervisor_tail, and it worked
>>    regardless of whether we get preempted.
>
> This should have been explained in the commit message and ...

Ok.

>
>> 2. Add Stefano's Acked-by.
>
> I would not keep Acked-by/Reviewed-by with a change like above. Or at
> least, I would ask whether they are fine with this change.

Thanks, I understand now.

>
> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [PATCH v2 08/19] xen/arm: Introduce a command line parameter for SErrors/Aborts

[Wei Chen]

Hi Julien,

On 2017/3/31 1:39, Julien Grall wrote:
> Hi Wei,
>
> On 30/03/17 10:13, Wei Chen wrote:
>> In order to distinguish guest-generated SErrors from hypervisor-generated
>> SErrors we have to place SError checking code in every EL1 -> EL2 paths.
>> That will cause overhead on entries due to dsb/isb.
>>
>> However, not all platforms want to categorize SErrors. For example, a host
>> that is running with trusted guests. The administrator can confirm that
>> all guests that are running on the host will not trigger such SErrors. In
>> this user-case, we should provide some options to administrators to avoid
>
> s/user-case/use-case/
>
>> +### serrors (ARM)
>> +> `= diverse | forward | panic`
>> +
>> +> Default: `diverse`
>> +
>> +This parameter is provided to administrators to determine how the
>> +hypervisors handle SErrors.
>> +
>> +In order to distinguish guest-generated SErrors from hypervisor-generated
>> +SErrors we have to place SError checking code in every EL1 -> EL2 paths.
>> +That will cause overhead on entries due to dsb/isb. However, not all platforms
>
> s/entries/entries and exits/ I think because none of the options you
> provide will effectively only do dsb/isb on entries.
>

Yes, the options will affect both entries and exits. I would update the
commit message and doc.

> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [PATCH v2 11/19] xen/arm32: Use alternative to skip the check of pending serrors

[Wei Chen]

On 2017/3/31 5:29, Stefano Stabellini wrote:
> On Thu, 30 Mar 2017, Julien Grall wrote:
>> Hi Wei,
>>
>> On 30/03/17 10:13, Wei Chen wrote:
>>> We have provided an option to administrator to determine how to
>>> handle the SErrors. In order to skip the check of pending SError,
>>> in conventional way, we have to read the option every time before
>>> we try to check the pending SError. This will add overhead to check
>>> the option at every trap.
>>>
>>> The ARM32 supports the alternative patching feature. We can use an
>>> ALTERNATIVE to avoid checking option at every trap. We added a new
>>> cpufeature named "SKIP_CHECK_PENDING_VSERROR". This feature will be
>>> enabled when the option is not diverse.
>>>
>>> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>>
>>> ---
>>
>> Stefano, this is requiring the alternative patch (see [1]) which is still in
>> review.
>>
>> Wei, a general rule is to mention the dependencies between series. So we don't
>> apply by mistake in the wrong order.
>
> Right. Thanks Wei for sending both the other patch and this series
> together, that helps me avoiding mistakes.
>
> But please add a note about the dependencies, just in case I forget.
>

Stefano and Julien, thank you. I would place a note about the
dependencies in next version.

>
>> Cheers,
>>
>> [1] https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg04132.html
>>
>>>  xen/arch/arm/arm32/entry.S | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>>> index 2187226..105cae8 100644
>>> --- a/xen/arch/arm/arm32/entry.S
>>> +++ b/xen/arch/arm/arm32/entry.S
>>> @@ -1,5 +1,6 @@
>>>  #include <asm/asm_defns.h>
>>>  #include <asm/regs.h>
>>> +#include <asm/alternative.h>
>>>  #include <public/xen.h>
>>>
>>>  #define SAVE_ONE_BANKED(reg)    mrs r11, reg; str r11, [sp, #UREGS_##reg]
>>> @@ -44,6 +45,14 @@ save_guest_regs:
>>>          SAVE_BANKED(fiq)
>>>          SAVE_ONE_BANKED(R8_fiq); SAVE_ONE_BANKED(R9_fiq);
>>> SAVE_ONE_BANKED(R10_fiq)
>>>          SAVE_ONE_BANKED(R11_fiq); SAVE_ONE_BANKED(R12_fiq);
>>> +
>>> +        /*
>>> +         * If the SKIP_CHECK_PENDING_VSERROR has been set in the cpu
>>> feature,
>>> +         * the checking of pending SErrors will be skipped.
>>> +         */
>>> +        ALTERNATIVE("nop",
>>> +                    "b skip_check",
>>> +                    SKIP_CHECK_PENDING_VSERROR)
>>>          /*
>>>           * Start to check pending virtual abort in the gap of Guest -> HYP
>>>           * world switch.
>>> @@ -99,6 +108,7 @@ abort_guest_exit_end:
>>>           */
>>>          bne return_from_trap
>>>
>>> +skip_check:
>>>          mov pc, lr
>>>
>>>  #define DEFINE_TRAP_ENTRY(trap)                                         \
>>>
>>
>> --
>> Julien Grall
>>
>


-- 
Regards,
Wei Chen

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

Re: [PATCH v2 12/19] xen/arm: Move macro VABORT_GEN_BY_GUEST to common header

[Wei Chen]

On 2017/3/31 5:36, Stefano Stabellini wrote:
> On Thu, 30 Mar 2017, Wei Chen wrote:
>> We want to move part of SErrors checking code from hyp_error assembly code
>> to a function. This new function will use this macro to distinguish the
>> guest SErrors from hypervisor SErrors. So we have to move this macro to
>> common header.
>>
>> The VABORT_GEN_BY_GUEST macro used two symbols abort_guest_exit_start and
>> abort_guest_exit_end. After we moved this macro to common header, we
>> should export these two symbols to other source files that will use
>> VABORT_GEN_BY_GUEST macro. So we change these two symbols to global.
>
> Please rewrite this to:
>
>   The VABORT_GEN_BY_GUEST macro uses the symbols abort_guest_exit_start
>   and abort_guest_exit_end. After we move this macro to a common header,
>   we need to make sure that the two symbols are visible to other source
>   files. Currently, they are declared .global in arm32/entry.S, but not
>   arm64/entry.S. Fix that.
>

Thanks for reorganization, I would rewrite in next version.

> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
>
>> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>> ---
>> v1->v2:
>> 1. Explain in commit message why we change abort_guest_exit_start and
>>    abort_guest_exit_end to global.
>> ---
>>  xen/arch/arm/arm64/entry.S            |  2 ++
>>  xen/include/asm-arm/arm32/processor.h | 10 ----------
>>  xen/include/asm-arm/processor.h       | 10 ++++++++++
>>  3 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>> index 4baa3cb..113e1c3 100644
>> --- a/xen/arch/arm/arm64/entry.S
>> +++ b/xen/arch/arm/arm64/entry.S
>> @@ -380,10 +380,12 @@ check_pending_vserror:
>>           * exception handler, and the elr_el2 will be set to
>>           * abort_guest_exit_start or abort_guest_exit_end.
>>           */
>> +        .global abort_guest_exit_start
>>  abort_guest_exit_start:
>>
>>          isb
>>
>> +        .global abort_guest_exit_end
>>  abort_guest_exit_end:
>>          /* Mask PSTATE asynchronous abort bit, close the checking window. */
>>          msr     daifset, #4
>> diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
>> index f6d5df3..68cc821 100644
>> --- a/xen/include/asm-arm/arm32/processor.h
>> +++ b/xen/include/asm-arm/arm32/processor.h
>> @@ -56,16 +56,6 @@ struct cpu_user_regs
>>      uint32_t pad1; /* Doubleword-align the user half of the frame */
>>  };
>>
>> -/* Functions for pending virtual abort checking window. */
>> -void abort_guest_exit_start(void);
>> -void abort_guest_exit_end(void);
>> -
>> -#define VABORT_GEN_BY_GUEST(r)  \
>> -( \
>> -    ( (unsigned long)abort_guest_exit_start == (r)->pc ) || \
>> -    ( (unsigned long)abort_guest_exit_end == (r)->pc ) \
>> -)
>> -
>>  #endif
>>
>>  /* Layout as used in assembly, with src/dest registers mixed in */
>> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
>> index 47e7026..700bde9 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -709,6 +709,16 @@ int call_smc(register_t function_id, register_t arg0, register_t arg1,
>>
>>  void do_trap_guest_error(struct cpu_user_regs *regs);
>>
>> +/* Functions for pending virtual abort checking window. */
>> +void abort_guest_exit_start(void);
>> +void abort_guest_exit_end(void);
>> +
>> +#define VABORT_GEN_BY_GUEST(r)  \
>> +( \
>> +    ( (unsigned long)abort_guest_exit_start == (r)->pc ) || \
>> +    ( (unsigned long)abort_guest_exit_end == (r)->pc ) \
>> +)
>> +
>>  #endif /* __ASSEMBLY__ */
>>  #endif /* __ASM_ARM_PROCESSOR_H */
>>  /*
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel
>>
>


-- 
Regards,
Wei Chen

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

Re: [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

[Wei Chen]

On 2017/3/31 2:38, Julien Grall wrote:
>
>
> On 30/03/17 19:32, Julien Grall wrote:
>> On 30/03/17 19:28, Julien Grall wrote:
>>> Hi Wei,
>>>
>>> On 30/03/17 10:13, Wei Chen wrote:
>>>> +void synchronize_serror(void)
>>>
>>> Sorry for been late in the party. Looking at the way you use the
>>> function, you execute depending on the behavior chosen by the user when
>>> an SErrors happen. This behavior will not change at runtime, so always
>>> checking the option chosen in the hot path does not sound very efficient.
>>>
>>> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
>>> I.e
>>>
>>> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
>>> the place.
>>
>> To complete what I was suggestion, you could define:
>>
>> /* Synchronize SError unless the feature is selected */
>> #define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");
>
> Or even:
>
> /*
>   * Synchronize SError unless the feature is selected.
>   * This is relying on the SErrors are currently unmasked.
>   */
> #define SYNCHRONIZE_SERROR(feat)				  \
> 	do {							  \
> 	  ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
> 	  ALTERNATIVE("dsb sy; isb", "nop; nop");		  \
> 	while (0)
>
> The ASSERT is here to check that we have abort enabled. Otherwise, doing
> the synchronization would be pointless.
>
> Note that the function local_abort_is_enabled is not implemented. But it
> is easy to write it. Looking how local_irq_is_enabled was implemented.
>

This is a better solution than function, I would do it.

> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [PATCH v2 17/19] xen/arm: Isolate the SError between the context switch of 2 vCPUs

[Wei Chen]

On 2017/3/31 6:00, Julien Grall wrote:
>
>
> On 30/03/2017 22:49, Stefano Stabellini wrote:
>> On Thu, 30 Mar 2017, Wei Chen wrote:
>>> +    /*
>>> +     * If the SErrors option is "FORWARD", we have to prevent forwarding
>>> +     * serror to wrong vCPU. So before context switch, we have to use the
>>> +     * synchronize_serror to guarantee that the pending serror would be
>>> +     * caught by current vCPU.
>>> +     *
>>> +     * The SKIP_CTXT_SWITCH_SERROR_SYNC will be set to cpu_hwcaps when the
>>> +     * SErrors option is NOT "FORWARD".
>>> +     */
>>> +    asm volatile(ALTERNATIVE("bl synchronize_serror",
>>> +                             "nop",
>>> +                             SKIP_CTXT_SWITCH_SERROR_SYNC));
>>
>>
>> This good, but here you need to add:
>>
>>   barrier();
>>
>> because the compiler is free to reorder even asm volatile instructions
>> (it could move the asm volatile after __context_switch theoretically).
>
> ... or it could moved before hand because there are no barrier... What
> you want to use is asm volatile(ALTERNATIVE(...) : : : "memory");
>

I would cover this consideration in the change of #16 in next version.

> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [PATCH v2 02/19] xen/arm: Remove vwfi while setting HCR_EL2 in init_traps

[Wei Chen]

On 2017/3/31 6:29, Stefano Stabellini wrote:
> On Thu, 30 Mar 2017, Julien Grall wrote:
>> Hi Wei,
>>
>> On 30/03/17 10:13, Wei Chen wrote:
>>> We will set HCR_EL2 for each domain individually at the place where each
>>> domain is created. vwfi will affect the behavior of VM trap. Initialize
>>> the HCR_EL2 in init_traps is a generic setting for AT translations while
>>> creating dom0.
>>
>> This statement looks wrong. Any AT s1s2 translations (i.e on behalf of the
>> guest) should be done after a call to p2m_restore_state that restore HCR_EL2,
>> SCTLR_EL1, VTTBR_EL1. If it is not the case, then there is an error.
>>
>>> After dom0 has been created, the HCR_EL2 will use the saved
>>> value in dom0's context. So checking vwfi in init_trap is pointless.
>>>
>>> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>>
>> This is a nack from me. We don't remove feature even temporarily without any
>> strong reasons. This is making more difficult to track the history of a
>> feature and a call to forget to restore it correctly later on or removing the
>> feature if we ever decide to revert the patch which adds back the feature (see
>> patch #4).
>>
>> I would prefer to see patch #2, #3 squashed into #4, the patch will not be
>> that big (50 lines) and avoid to drop a feature temporarily.
>
> Yes, please.

Ok, I understand. I will squashe #2 #3 into #4 and use the helper.

>
>
>> But I am not convinced about your reasons.
>>> ---
>>> I have tried to remove HCR_EL2 setting from init_traps, but the Xen will
>>> hang at the place of creating domain0. The console hot key can work, so
>>> the Xen is alive, not panic.
>>
>> With the limited description you provided it is hard to know what's going on.
>> In the future please try to provide meaningful details (platform used,
>> debugging you have done...). Anyway, I have done some debug and I don't end up
>> to the same conclusion as you.
>>
>> I tried on different boards, and it gets stuck when initializing stage-2 page
>> table configuration on each CPU (see setup_virt_paging). The secondary CPUs
>> don't receive the SGI.
>>
>> Reading a bit more the ARM ARM (D7.2.33 in ARM DDI 0487A.k_iss10775), HCR_EL2
>> is unknown at reset. Whilst I already knew that, I would have expected to have
>> no impact on EL2 (at least in the non-VHE case). However, the value of the
>> {A,I,F}MO bits will affect the routing of physical IRQs to Xen.
>>
>> I have only gone quickly through the spec, so it might be possible to have
>> other necessary bits. It might be good to keep initialization here until
>> someone sit in front of the spec for few hours and check everything.
>>
>> So in this case I would prefer to keep the helper avoiding to have declared
>> twice the same flags. Stefano any opinions?
>
> I don't particularly care whether we keep the helper or not. From what
> you say we need to set HCR_EL2 in init_traps, but given that's only
> temporary, we don't necessarily need to write the same set of flags: for
> example HCR_TWI|HCR_TWE are useless at this stage. A minimal set would
> suffice. Either way, it's fine by me.
>
>
>> Also, whilst I was debugging the problem I noticed the vwfi option does not
>> work properly on CPU0. Indeed, the command line has not been parsed when
>> init_traps is called on CPU0. This is should be fixed by patch #4 in this
>> series, but I don't think we want to backport it.
>
> No, we don't want to backport patch #4.
>
>
>> Stefano, would you be up to write a patch for this?
>
> I am thinking that the patch should only to fix the backports, given
> that unstable and 4.9 will be fixed by this series (and given that it's
> pretty ugly). I'll send it separately shortly.
>


-- 
Regards,
Wei Chen

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

Re: [PATCH v2 02/19] xen/arm: Remove vwfi while setting HCR_EL2 in init_traps

[Julien Grall]

Hi Stefano,

On 03/30/2017 11:29 PM, Stefano Stabellini wrote:
>> I tried on different boards, and it gets stuck when initializing stage-2 page
>> table configuration on each CPU (see setup_virt_paging). The secondary CPUs
>> don't receive the SGI.
>>
>> Reading a bit more the ARM ARM (D7.2.33 in ARM DDI 0487A.k_iss10775), HCR_EL2
>> is unknown at reset. Whilst I already knew that, I would have expected to have
>> no impact on EL2 (at least in the non-VHE case). However, the value of the
>> {A,I,F}MO bits will affect the routing of physical IRQs to Xen.
>>
>> I have only gone quickly through the spec, so it might be possible to have
>> other necessary bits. It might be good to keep initialization here until
>> someone sit in front of the spec for few hours and check everything.
>>
>> So in this case I would prefer to keep the helper avoiding to have declared
>> twice the same flags. Stefano any opinions?
>
> I don't particularly care whether we keep the helper or not. From what
> you say we need to set HCR_EL2 in init_traps, but given that's only
> temporary, we don't necessarily need to write the same set of flags: for
> example HCR_TWI|HCR_TWE are useless at this stage. A minimal set would
> suffice. Either way, it's fine by me.

If we don't write the same set of flags, we need some documentation in 
the code to explain why those flags are set for initialization (I 
suspect a lot of them are not necessary) and that HCR flags whilst a 
domain is running are set somewhere else.

Otherwise this is a call to miss setting some HCR flags in the right place.

That's why I suggested a helper (or a global variable), a single place 
to get the default HCR flags rather than spreading.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 04/19] xen/arm: Restore HCR_EL2 register

[Julien Grall]

Hi Wei,

On 03/31/2017 03:10 AM, Wei Chen wrote:
> Hi Julien and Stefano,
>
> On 2017/3/31 6:03, Stefano Stabellini wrote:
>> On Thu, 30 Mar 2017, Julien Grall wrote:
>>> Hi Wei,
>>>
>>> On 30/03/17 10:13, Wei Chen wrote:
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index de59e5f..8af223e 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -2171,6 +2171,13 @@ int construct_dom0(struct domain *d)
>>>>          return rc;
>>>>
>>>>      /*
>>>> +     * The HCR_EL2 will temporarily switch to dom0's HCR_EL2 value
>>>> +     * by p2m_restore_state. We have to save HCR_EL2 to idle vCPU's
>>>> +     * context for restoring it in later.
>>>> +     */
>>>> +    current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
>>>
>>> I don't understand why we care here. idle vCPU will never restore HCR_EL2 nor
>>> return from the hypervisor.
>>
>> I don't understand this either
>>
>
> Yes, idle vCPU will never return from hypervisor. But in construct_dom0,
> it has one chance to restore HCR_EL2.
> In in construct_dom0 we will call p2m_restore_state twice.
>      saved_current = current;
>      p2m_restore_state(v); --->> This is dom0's vCPU0
>      [...]
>      set_current(saved_current);
>      p2m_restore_state(saved_current); --->> this is idle vCPU0
>
> In p2m_restore_state, we will write vcpu->arch.hcr_el2 to HCR_EL2. in
> this case, we will write an unknown value to HCR_EL2. Even though this
> would not cause any problem from my current testing. But from code
> scope, I think it would be a drawback.

The p2m_restore_state will do nothing for idle vCPU (see check at the 
beginning of the function). So there are no issue.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 09/19] xen/arm: Introduce a initcall to update cpu_hwcaps by serror_op

[Julien Grall]

Hi Stefano,

On 03/30/2017 10:28 PM, Stefano Stabellini wrote:
> On Thu, 30 Mar 2017, Julien Grall wrote:
>> On 30/03/17 10:13, Wei Chen wrote:
>>> In the later patches of this series, we want to use the alternative
>>> patching framework to avoid checking serror_op in every entries.
>>> So we define a new cpu feature "SKIP_CHECK_PENDING_VSERROR" for
>>> serror_op. When serror_op is not equal to SERROR_DIVERSE, this
>>> feature will be set to cpu_hwcaps.
>>>
>>> Currently, the default serror_op is SERROR_DIVERSE, if we want to
>>> change the serror_op value we have to place the serror parameter
>>> in command line. It seems no problem to update cpu_hwcaps directly
>>> in the serror parameter parsing function.
>>>
>>> But one day, if we change the default serror_op to SERROR_PANIC or
>>> SERROR_FORWARD by some security policy. We may not place the serror
>>> parameter in command line. In this case, if we rely on the serror
>>> parameter parsing function to update cpu_hwcaps, this function would
>>> not be invoked and the "SKIP_CHECK_PENDING_VSERROR" could not be
>>> set in cpu_hwcaps.
>>>
>>> So, we introduce this initcall to guarantee the cpu_hwcaps can be
>>> updated no matter the serror parameter is placed in the command line
>>> or not.
>>>
>>> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>>> ---
>>> v1->v2:
>>> 1. Explain this initcall is to future-proof the code in commit
>>>    message.
>>> 2. Fix a coding style of this initcall.
>>> ---
>>>  xen/arch/arm/traps.c             | 9 +++++++++
>>>  xen/include/asm-arm/cpufeature.h | 3 ++-
>>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index 955d97c..dafb730 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -120,6 +120,15 @@ static void __init parse_serrors_behavior(const char
>>> *str)
>>>  }
>>>  custom_param("serrors", parse_serrors_behavior);
>>>
>>> +static int __init update_serrors_cpu_caps(void)
>>> +{
>>> +    if ( serrors_op != SERRORS_DIVERSE )
>>> +        cpus_set_cap(SKIP_CHECK_PENDING_VSERROR);
>>
>> Thinking a bit more of this. I am wondering if we should add a warning (see
>> warning_add) if the user is selecting an option other than diverse. Two
>> reasons for that:
>> 	1) The user is fully aware that he is not classifying the SError at
>> his own risks
>> 	2) If someone send an e-mail saying: "My guest crashed the hypervisor
>> with an SError". We can directly know from the log.
>>
>> Any opinions?
>
> I would not do that: warnings are good to warn the user about something
> unexpected or potentially unknown. In this case the user has to look up
> the docs to find about the other options, where it's clearly written
> what they are for. We have to expect taht they know what their are
> doing.

Fair enough :). Wei, please ignore my request.

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

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 04/19] xen/arm: Restore HCR_EL2 register

[Wei Chen]

On 2017/3/31 16:39, Julien Grall wrote:
> Hi Wei,
>
> On 03/31/2017 03:10 AM, Wei Chen wrote:
>> Hi Julien and Stefano,
>>
>> On 2017/3/31 6:03, Stefano Stabellini wrote:
>>> On Thu, 30 Mar 2017, Julien Grall wrote:
>>>> Hi Wei,
>>>>
>>>> On 30/03/17 10:13, Wei Chen wrote:
>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>>> index de59e5f..8af223e 100644
>>>>> --- a/xen/arch/arm/domain_build.c
>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>> @@ -2171,6 +2171,13 @@ int construct_dom0(struct domain *d)
>>>>>          return rc;
>>>>>
>>>>>      /*
>>>>> +     * The HCR_EL2 will temporarily switch to dom0's HCR_EL2 value
>>>>> +     * by p2m_restore_state. We have to save HCR_EL2 to idle vCPU's
>>>>> +     * context for restoring it in later.
>>>>> +     */
>>>>> +    current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
>>>>
>>>> I don't understand why we care here. idle vCPU will never restore HCR_EL2 nor
>>>> return from the hypervisor.
>>>
>>> I don't understand this either
>>>
>>
>> Yes, idle vCPU will never return from hypervisor. But in construct_dom0,
>> it has one chance to restore HCR_EL2.
>> In in construct_dom0 we will call p2m_restore_state twice.
>>      saved_current = current;
>>      p2m_restore_state(v); --->> This is dom0's vCPU0
>>      [...]
>>      set_current(saved_current);
>>      p2m_restore_state(saved_current); --->> this is idle vCPU0
>>
>> In p2m_restore_state, we will write vcpu->arch.hcr_el2 to HCR_EL2. in
>> this case, we will write an unknown value to HCR_EL2. Even though this
>> would not cause any problem from my current testing. But from code
>> scope, I think it would be a drawback.
>
> The p2m_restore_state will do nothing for idle vCPU (see check at the
> beginning of the function). So there are no issue.
>

Oh, yes, you are right. I missed the idle vCPU check at the beginning.
I would remove this code from next version.

> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

[Wei Chen]

Hi Julien,

On 2017/3/31 2:38, Julien Grall wrote:
>
>
> On 30/03/17 19:32, Julien Grall wrote:
>> On 30/03/17 19:28, Julien Grall wrote:
>>> Hi Wei,
>>>
>>> On 30/03/17 10:13, Wei Chen wrote:
>>>> +void synchronize_serror(void)
>>>
>>> Sorry for been late in the party. Looking at the way you use the
>>> function, you execute depending on the behavior chosen by the user when
>>> an SErrors happen. This behavior will not change at runtime, so always
>>> checking the option chosen in the hot path does not sound very efficient.
>>>
>>> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
>>> I.e
>>>
>>> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
>>> the place.
>>
>> To complete what I was suggestion, you could define:
>>
>> /* Synchronize SError unless the feature is selected */
>> #define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");
>
> Or even:
>
> /*
>   * Synchronize SError unless the feature is selected.
>   * This is relying on the SErrors are currently unmasked.
>   */
> #define SYNCHRONIZE_SERROR(feat)				  \
> 	do {							  \
> 	  ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
> 	  ALTERNATIVE("dsb sy; isb", "nop; nop");		  \
> 	while (0)
>
> The ASSERT is here to check that we have abort enabled. Otherwise, doing
> the synchronization would be pointless.
>

Think a bit more about it. This macro will import more check than read
serror_op. This macro seems as a generic macro, it can be used in any
place. So you enable the feature check and abort check. But in this
patch, we know where the macro will be used, we know the abort is
enabled. And want to reduce the overhead as much as possible.

I prefer to define a similar macro as the following:
#define SYNCHRONIZE_SERROR(feat)                                    \
     do {                                                            \
         ALTERNATIVE("dsb sy; isb" : : : "memory", "nop; nop", feat);\
     } while (0)

> Note that the function local_abort_is_enabled is not implemented. But it
> is easy to write it. Looking how local_irq_is_enabled was implemented.
>
> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

[Julien Grall]

On 31/03/17 11:55, Wei Chen wrote:
> Hi Julien,

Hi Wei,

> On 2017/3/31 2:38, Julien Grall wrote:
>>
>>
>> On 30/03/17 19:32, Julien Grall wrote:
>>> On 30/03/17 19:28, Julien Grall wrote:
>>>> Hi Wei,
>>>>
>>>> On 30/03/17 10:13, Wei Chen wrote:
>>>>> +void synchronize_serror(void)
>>>>
>>>> Sorry for been late in the party. Looking at the way you use the
>>>> function, you execute depending on the behavior chosen by the user when
>>>> an SErrors happen. This behavior will not change at runtime, so always
>>>> checking the option chosen in the hot path does not sound very efficient.
>>>>
>>>> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
>>>> I.e
>>>>
>>>> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
>>>> the place.
>>>
>>> To complete what I was suggestion, you could define:
>>>
>>> /* Synchronize SError unless the feature is selected */
>>> #define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");
>>
>> Or even:
>>
>> /*
>>   * Synchronize SError unless the feature is selected.
>>   * This is relying on the SErrors are currently unmasked.
>>   */
>> #define SYNCHRONIZE_SERROR(feat)                                \
>>        do {                                                      \
>>          ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
>>          ALTERNATIVE("dsb sy; isb", "nop; nop");                  \
>>        while (0)
>>
>> The ASSERT is here to check that we have abort enabled. Otherwise, doing
>> the synchronization would be pointless.
>>
>
> Think a bit more about it. This macro will import more check than read
> serror_op. This macro seems as a generic macro, it can be used in any
> place. So you enable the feature check and abort check. But in this
> patch, we know where the macro will be used, we know the abort is
> enabled. And want to reduce the overhead as much as possible.

ASSERTs are used to verify your assumption is correct, for non-debug 
built they will be turned into a NOP.

I care about overhead in non-debug build, it is not much a concern for 
debug build. In the latter it is more important to verify the correctness.

The abort bit is enabled at the entry in the hypervisor, and neither you 
nor I can tell if we need to disable abort in the future in some path. 
This ASSERT will catch those changes.

So please give me a reason to remove it, because I see only benefits so far.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

[Wei Chen]

On 2017/3/31 19:06, Julien Grall wrote:
>
>
> On 31/03/17 11:55, Wei Chen wrote:
>> Hi Julien,
>
> Hi Wei,
>
>> On 2017/3/31 2:38, Julien Grall wrote:
>>>
>>>
>>> On 30/03/17 19:32, Julien Grall wrote:
>>>> On 30/03/17 19:28, Julien Grall wrote:
>>>>> Hi Wei,
>>>>>
>>>>> On 30/03/17 10:13, Wei Chen wrote:
>>>>>> +void synchronize_serror(void)
>>>>>
>>>>> Sorry for been late in the party. Looking at the way you use the
>>>>> function, you execute depending on the behavior chosen by the user when
>>>>> an SErrors happen. This behavior will not change at runtime, so always
>>>>> checking the option chosen in the hot path does not sound very efficient.
>>>>>
>>>>> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
>>>>> I.e
>>>>>
>>>>> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
>>>>> the place.
>>>>
>>>> To complete what I was suggestion, you could define:
>>>>
>>>> /* Synchronize SError unless the feature is selected */
>>>> #define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");
>>>
>>> Or even:
>>>
>>> /*
>>>   * Synchronize SError unless the feature is selected.
>>>   * This is relying on the SErrors are currently unmasked.
>>>   */
>>> #define SYNCHRONIZE_SERROR(feat)                                \
>>>        do {                                                      \
>>>          ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
>>>          ALTERNATIVE("dsb sy; isb", "nop; nop");                  \
>>>        while (0)
>>>
>>> The ASSERT is here to check that we have abort enabled. Otherwise, doing
>>> the synchronization would be pointless.
>>>
>>
>> Think a bit more about it. This macro will import more check than read
>> serror_op. This macro seems as a generic macro, it can be used in any
>> place. So you enable the feature check and abort check. But in this
>> patch, we know where the macro will be used, we know the abort is
>> enabled. And want to reduce the overhead as much as possible.
>
> ASSERTs are used to verify your assumption is correct, for non-debug
> built they will be turned into a NOP.
>
> I care about overhead in non-debug build, it is not much a concern for
> debug build. In the latter it is more important to verify the correctness.
>
> The abort bit is enabled at the entry in the hypervisor, and neither you
> nor I can tell if we need to disable abort in the future in some path.
> This ASSERT will catch those changes.
>
> So please give me a reason to remove it, because I see only benefits so far.

Ok, thanks, I understand know. As it just affects the debug built. I
will keep them in the macro.

>
> Cheers,
>


-- 
Regards,
Wei Chen

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