[PATCH 0/9] xen/arm: Simplify do_trap_*_abort_guest

[PATCH 0/9] xen/arm: Simplify do_trap_*_abort_guest

[Julien Grall]

Hello all,

The current data/instruction abort paths contain unnecessary code and
translate to often a VA to a IPA. This series aim to simplify this path.

Now that the register HPFAR_EL2 is read in case which can be affected
by the erratum 834220 on Cortex-A57, we need to implement a workaround
for it (see patch #9). This patch replaces the documentation one [1] in
the patch series that is part of "xen/arm: Introduce alternative runtime
paching for ARM64" [2].

This series is based on version 4 of "xen/arm: Introduce alternative runtime
patching for ARM64" [3]. A branch with the two series can be found on xenbits:

git://xenbits.xen.org/people/julieng/xen-unstable.git branch abort-handlers-v1

Yours sincerely,

[1] http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg00944.html
[2] http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg00932.html
[3] http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg02788.html

Julien Grall (9):
  xen/arm: Simply the definition of PAGE_SIZE by using the macro _AC
  xen/arm: traps: Second attempt to correctly use the content of
    HPFAR_EL2
  xen/arm: traps: Data Abort are always unconditional
  xen/arm: traps: Simplify the switch in do_trap_*_abort_guest
  xen/arm: Provide macros to help creating workaround helpers
  xen/arm: Use check_workaround to handle the erratum 766422
  xen/arm: traps: MMIO should only be emulated for fault translation
  xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort
    handlers
  xen/arm: arm64: Add Cortex-A57 erratum 834220 workaround

 docs/misc/arm/silicon-errata.txt      |  1 +
 xen/arch/arm/Kconfig                  | 21 ++++++++
 xen/arch/arm/cpuerrata.c              | 15 ++++++
 xen/arch/arm/traps.c                  | 99 +++++++++++++++++++++--------------
 xen/include/asm-arm/arm32/processor.h |  4 --
 xen/include/asm-arm/arm64/processor.h |  2 -
 xen/include/asm-arm/config.h          |  7 +--
 xen/include/asm-arm/cpuerrata.h       | 42 +++++++++++++++
 xen/include/asm-arm/cpufeature.h      |  4 +-
 xen/include/asm-arm/processor.h       |  2 +
 10 files changed, 145 insertions(+), 52 deletions(-)

-- 
1.9.1


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

[PATCH 1/9] xen/arm: Simply the definition of PAGE_SIZE by using the macro _AC

[Julien Grall]

The macro _AC is used to define constant for both assembly and C.

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

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 9417be6..a96f845 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -176,12 +176,7 @@
 #define FIXMAP_ACPI_END    (FIXMAP_ACPI_BEGIN + NUM_FIXMAP_ACPI_PAGES - 1)  /* End mappings of ACPI tables */
 
 #define PAGE_SHIFT              12
-
-#ifndef __ASSEMBLY__
-#define PAGE_SIZE           (1L << PAGE_SHIFT)
-#else
-#define PAGE_SIZE           (1 << PAGE_SHIFT)
-#endif
+#define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
 #define PAGE_MASK           (~(PAGE_SIZE-1))
 #define PAGE_FLAG_MASK      (~0)
 
-- 
1.9.1


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

[PATCH 2/9] xen/arm: traps: Second attempt to correctly use the content of HPFAR_EL2

[Julien Grall]

Commit c051618 "xen/arm: traps: Correctly interpret the content of the
register HPFAR_EL2" attempted to fix the interpretation of HPFAR_EL2.

However, the register contains a 4KB-aligned address. This means that
the reported address is not directly usable to know the faulting IPA.
The offset in the 4KB page can be found by looking at the associated virtual
address (FAR_EL2/HDFAR).

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

---

Cc: Tamas K Lengyel <tamas@tklengyel.com>

I overlooked the usage of HPFAR_EL2. This is currently only affecting
memaccess. Rather that return that exact address, the address will
always be the base addres of the 4KB page.

This would need to be backported up to Xen 4.6.
---
 xen/arch/arm/traps.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 0709deb..742f7d3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2371,11 +2371,15 @@ done:
     if (first) unmap_domain_page(first);
 }
 
-static inline paddr_t get_faulting_ipa(void)
+static inline paddr_t get_faulting_ipa(vaddr_t gva)
 {
     register_t hpfar = READ_SYSREG(HPFAR_EL2);
+    paddr_t ipa;
 
-    return ((paddr_t)(hpfar & HPFAR_MASK) << (12 - 4));
+    ipa = (paddr_t)(hpfar & HPFAR_MASK) << (12 - 4);
+    ipa |= gva & ~PAGE_MASK;
+
+    return ipa;
 }
 
 static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
@@ -2396,7 +2400,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
         };
 
         if ( hsr.iabt.s1ptw )
-            gpa = get_faulting_ipa();
+            gpa = get_faulting_ipa(gva);
         else
         {
             /*
@@ -2445,7 +2449,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
 #endif
 
     if ( dabt.s1ptw )
-        info.gpa = get_faulting_ipa();
+        info.gpa = get_faulting_ipa(info.gva);
     else
     {
         rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);
-- 
1.9.1


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

[PATCH 3/9] xen/arm: traps: Data Abort are always unconditional

[Julien Grall]

The HSR encoding for an exception from a data abort does not contain a
conditional code (see G6-4264 in ARM DDI 0487A.i) because they are
always conditional.

So drop the pointless condition check.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/traps.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 742f7d3..2e84b5a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2435,12 +2435,6 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
     int rc;
     mmio_info_t info;
 
-    if ( !check_conditional_instr(regs, hsr) )
-    {
-        advance_pc(regs, hsr);
-        return;
-    }
-
     info.dabt = dabt;
 #ifdef CONFIG_ARM_32
     info.gva = READ_CP32(HDFAR);
-- 
1.9.1


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

[PATCH 4/9] xen/arm: traps: Simplify the switch in do_trap_*_abort_guest

[Julien Grall]

The fault status we care are all the form BBBBxx where xx is the lookup
level that gave the fault. We can simply the code by masking the 2 least
significant bits.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/traps.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2e84b5a..8a3fac0 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2388,9 +2388,9 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
     int rc;
     register_t gva = READ_SYSREG(FAR_EL2);
 
-    switch ( hsr.iabt.ifsc & 0x3f )
+    switch ( hsr.iabt.ifsc & ~FSC_LL_MASK )
     {
-    case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
+    case FSC_FLT_PERM:
     {
         paddr_t gpa;
         const struct npfec npfec = {
@@ -2451,9 +2451,9 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
             return; /* Try again */
     }
 
-    switch ( dabt.dfsc & 0x3f )
+    switch ( dabt.dfsc & ~FSC_LL_MASK )
     {
-    case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
+    case FSC_FLT_PERM:
     {
         const struct npfec npfec = {
             .read_access = !dabt.write,
-- 
1.9.1


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

[PATCH 5/9] xen/arm: Provide macros to help creating workaround helpers

[Julien Grall]

Workarounds may require to execute a different path when the platform
is affected by the associated erratum. Furthermore, this may need to
be called in the common code.

To avoid too much intrusion/overhead, the workaround helpers need to
be a nop on architecture which will never have the workaround and have
to be quick to check whether the platform requires it.

The alternative framework is used to transform the check in a single
instruction. When the framework is not available, the helper will have
~6 instructions including 1 instruction load.

The macro will create a handler called check_workaround_xxxxx with
xxxx the erratum number.

For instance, the line bellow will create a workaround helper for
erratum #424242 which is enabled when the capability
ARM64_WORKAROUND_424242 is set and only available for ARM64:

CHECK_WORKAROUND_HELPER(424242, ARM64_WORKAROUND_42424242, CONFIG_ARM64)

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

diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
index fe93beb..b9d8dfc 100644
--- a/xen/include/asm-arm/cpuerrata.h
+++ b/xen/include/asm-arm/cpuerrata.h
@@ -1,8 +1,47 @@
 #ifndef __ARM_CPUERRATA_H
 #define __ARM_CPUERRATA_H
 
+#include <xen/config.h>
+#include <asm/cpufeature.h>
+#include <asm/alternative.h>
+
 void check_local_cpu_errata(void);
 
+#ifdef CONFIG_ALTERNATIVE
+
+#define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
+static inline bool_t check_workaround_##erratum(void)           \
+{                                                               \
+    if ( !IS_ENABLED(arch) )                                    \
+        return 0;                                               \
+    else                                                        \
+    {                                                           \
+        bool_t ret;                                             \
+                                                                \
+        asm volatile (ALTERNATIVE("mov %0, #0",                 \
+                                  "mov %0, #1",                 \
+                                  feature)                      \
+                      : "=r" (ret));                            \
+                                                                \
+        return unlikely(ret);                                   \
+    }                                                           \
+}
+
+#else /* CONFIG_ALTERNATIVE */
+
+#define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
+static inline bool_t check_workaround_##erratum(void)           \
+{                                                               \
+    if ( !IS_ENABLED(arch) )                                    \
+        return 0;                                               \
+    else                                                        \
+        return unlikely(cpus_have_cap(feature));                \
+}
+
+#endif
+
+#undef CHECK_WORKAROUND_HELPER
+
 #endif /* __ARM_CPUERRATA_H */
 /*
  * Local variables:
-- 
1.9.1


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

[PATCH 6/9] xen/arm: Use check_workaround to handle the erratum 766422

[Julien Grall]

Currently, Xen is reading the MIDR everytime it has to check whether
the processor is affected by the erratum 766422.

This could take advantage of the new capability bitfields to detect
whether the processor is affected at boot time.

With this patch, the number of instructions to check the erratum is
going down from ~13 (including 2 loads and a co-processor access) to
~6 instructions (include 1 load).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/cpuerrata.c              | 6 ++++++
 xen/arch/arm/traps.c                  | 3 ++-
 xen/include/asm-arm/arm32/processor.h | 4 ----
 xen/include/asm-arm/arm64/processor.h | 2 --
 xen/include/asm-arm/cpuerrata.h       | 2 ++
 xen/include/asm-arm/cpufeature.h      | 3 ++-
 xen/include/asm-arm/processor.h       | 2 ++
 7 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 3ac97b3..748e02e 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -17,6 +17,12 @@ is_affected_midr_range(const struct arm_cpu_capabilities *entry)
 }
 
 static const struct arm_cpu_capabilities arm_errata[] = {
+    {
+        /* Cortex-A15 r0p4 */
+        .desc = "ARM erratum 766422",
+        .capability = ARM32_WORKAROUND_766422,
+        MIDR_RANGE(MIDR_CORTEX_A15, 0x04, 0x04),
+    },
 #if defined(CONFIG_ARM64_ERRATUM_827319) || \
     defined(CONFIG_ARM64_ERRATUM_824069)
     {
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8a3fac0..785e3e9 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -47,6 +47,7 @@
 #include "vtimer.h"
 #include <asm/gic.h>
 #include <asm/vgic.h>
+#include <asm/cpuerrata.h>
 
 /* The base of the stack must always be double-word aligned, which means
  * that both the kernel half of struct cpu_user_regs (which is pushed in
@@ -2482,7 +2483,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
      * Erratum 766422: Thumb store translation fault to Hypervisor may
      * not have correct HSR Rt value.
      */
-    if ( cpu_has_erratum_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
+    if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
     {
         rc = decode_instruction(regs, &info.dabt);
         if ( rc )
diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
index f41644d..11366bb 100644
--- a/xen/include/asm-arm/arm32/processor.h
+++ b/xen/include/asm-arm/arm32/processor.h
@@ -115,10 +115,6 @@ struct cpu_user_regs
 #define READ_SYSREG(R...)       READ_SYSREG32(R)
 #define WRITE_SYSREG(V, R...)   WRITE_SYSREG32(V, R)
 
-/* Erratum 766422: only Cortex A15 r0p4 is affected */
-#define cpu_has_erratum_766422()                             \
-    (unlikely(current_cpu_data.midr.bits == 0x410fc0f4))
-
 #endif /* __ASSEMBLY__ */
 
 #endif /* __ASM_ARM_ARM32_PROCESSOR_H */
diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h
index fef35a5..b0726ff 100644
--- a/xen/include/asm-arm/arm64/processor.h
+++ b/xen/include/asm-arm/arm64/processor.h
@@ -111,8 +111,6 @@ struct cpu_user_regs
 #define READ_SYSREG(name)     READ_SYSREG64(name)
 #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
 
-#define cpu_has_erratum_766422() 0
-
 #endif /* __ASSEMBLY__ */
 
 #endif /* __ASM_ARM_ARM64_PROCESSOR_H */
diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
index b9d8dfc..aaf3edd 100644
--- a/xen/include/asm-arm/cpuerrata.h
+++ b/xen/include/asm-arm/cpuerrata.h
@@ -40,6 +40,8 @@ static inline bool_t check_workaround_##erratum(void)           \
 
 #endif
 
+CHECK_WORKAROUND_HELPER(766422, ARM32_WORKAROUND_766422, CONFIG_ARM_32)
+
 #undef CHECK_WORKAROUND_HELPER
 
 #endif /* __ARM_CPUERRATA_H */
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 78e2263..ac6eaf0 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -37,8 +37,9 @@
 
 #define ARM64_WORKAROUND_CLEAN_CACHE    0
 #define ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE    1
+#define ARM32_WORKAROUND_766422 2
 
-#define ARM_NCAPS           2
+#define ARM_NCAPS           3
 
 #ifndef __ASSEMBLY__
 
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 1708253..15bf890 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -46,9 +46,11 @@
 
 #define ARM_CPU_IMP_ARM             0x41
 
+#define ARM_CPU_PART_CORTEX_A15     0xC0F
 #define ARM_CPU_PART_CORTEX_A53     0xD03
 #define ARM_CPU_PART_CORTEX_A57     0xD07
 
+#define MIDR_CORTEX_A15 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A15)
 #define MIDR_CORTEX_A53 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A53)
 #define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57)
 
-- 
1.9.1


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

[PATCH 7/9] xen/arm: traps: MMIO should only be emulated for fault translation

[Julien Grall]

The function do_trap_data_abort_guest assumes that a stage-2 data abort
can only be taken for a translation fault or permission fault today.

Whilst this is true today, it might not be in the future. Rather than
emulating the MMIO for any fault other than the permission one, print
a warning message when the fault is not handled by Xen.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/traps.c | 51 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 785e3e9..591de3c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2468,35 +2468,40 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
         /* Trap was triggered by mem_access, work here is done */
         if ( !rc )
             return;
+        break;
     }
-    break;
-    }
-
-    if ( dabt.s1ptw )
-        goto bad_data_abort;
+    case FSC_FLT_TRANS:
+        if ( dabt.s1ptw )
+            goto bad_data_abort;
 
-    /* XXX: Decode the instruction if ISS is not valid */
-    if ( !dabt.valid )
-        goto bad_data_abort;
+        /* XXX: Decode the instruction if ISS is not valid */
+        if ( !dabt.valid )
+            goto bad_data_abort;
 
-    /*
-     * Erratum 766422: Thumb store translation fault to Hypervisor may
-     * not have correct HSR Rt value.
-     */
-    if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
-    {
-        rc = decode_instruction(regs, &info.dabt);
-        if ( rc )
+        /*
+         * Erratum 766422: Thumb store translation fault to Hypervisor may
+         * not have correct HSR Rt value.
+         */
+        if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
+             dabt.write )
         {
-            gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
-            goto bad_data_abort;
+            rc = decode_instruction(regs, &info.dabt);
+            if ( rc )
+            {
+                gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
+                goto bad_data_abort;
+            }
         }
-    }
 
-    if (handle_mmio(&info))
-    {
-        advance_pc(regs, hsr);
-        return;
+        if ( handle_mmio(&info) )
+        {
+            advance_pc(regs, hsr);
+            return;
+        }
+        break;
+    default:
+        gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
+                hsr.bits, dabt.dfsc);
     }
 
 bad_data_abort:
-- 
1.9.1


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

[PATCH 8/9] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Julien Grall]

Translating a VA to a IPA is expensive. Currently, Xen is assuming that
HPFAR_EL2 is only valid when the stage-2 data/instruction abort happened
during a translation table walk of a first stage translation (i.e S1PTW
is set).

However, based on the ARM ARM (D7.2.34 in DDI 0487A.j), the register is
also valid when the data/instruction abort occured for a translation
fault.

With this change, the VA -> IPA translation will only happen for
permission faults that are not related to a translation table of a
first stage translation.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/traps.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 591de3c..0edc2cc 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2383,13 +2383,28 @@ static inline paddr_t get_faulting_ipa(vaddr_t gva)
     return ipa;
 }
 
+static inline bool hpfar_is_valid(bool s1ptw, uint8_t fsc)
+{
+    /*
+     * HPFAR is valid if one of the following cases are true:
+     *  1. the stage 2 fault happen during a stage 1 page table walk
+     *  (the bit ESR_EL2.S1PTW is set)
+     *  2. the fault was due to a translation fault
+     *
+     * Note that technically HPFAR is valid for other cases, but they
+     * are currently not supported by Xen.
+     */
+    return s1ptw || (fsc == FSC_FLT_TRANS);
+}
+
 static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
                                       const union hsr hsr)
 {
     int rc;
     register_t gva = READ_SYSREG(FAR_EL2);
+    uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
 
-    switch ( hsr.iabt.ifsc & ~FSC_LL_MASK )
+    switch ( fsc )
     {
     case FSC_FLT_PERM:
     {
@@ -2400,7 +2415,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
             .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
         };
 
-        if ( hsr.iabt.s1ptw )
+        if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
             gpa = get_faulting_ipa(gva);
         else
         {
@@ -2435,6 +2450,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
     const struct hsr_dabt dabt = hsr.dabt;
     int rc;
     mmio_info_t info;
+    uint8_t fsc = hsr.dabt.dfsc & ~FSC_LL_MASK;
 
     info.dabt = dabt;
 #ifdef CONFIG_ARM_32
@@ -2443,7 +2459,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
     info.gva = READ_SYSREG64(FAR_EL2);
 #endif
 
-    if ( dabt.s1ptw )
+    if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
         info.gpa = get_faulting_ipa(info.gva);
     else
     {
-- 
1.9.1


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

[PATCH 9/9] xen/arm: arm64: Add Cortex-A57 erratum 834220 workaround

[Julien Grall]

The ARM erratum applies to certain revisions of Cortex-A57. The
processor may report a Stage 2 translation fault as the result of
Stage 1 fault for load crossing a page boundary when there is a
permission fault or device memory fault at stage 1 and a translation
fault at Stage 2.

So Xen needs to check that Stage 1 translation does not generate a fault
before handling the Stage 2 fault. If it is a Stage 1 translation fault,
return to the guest to let the processor injecting the correct fault.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 docs/misc/arm/silicon-errata.txt |  1 +
 xen/arch/arm/Kconfig             | 21 +++++++++++++++++++++
 xen/arch/arm/cpuerrata.c         |  9 +++++++++
 xen/arch/arm/traps.c             |  5 +++--
 xen/include/asm-arm/cpuerrata.h  |  1 +
 xen/include/asm-arm/cpufeature.h |  3 ++-
 6 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/docs/misc/arm/silicon-errata.txt b/docs/misc/arm/silicon-errata.txt
index 220f724..c9854c3 100644
--- a/docs/misc/arm/silicon-errata.txt
+++ b/docs/misc/arm/silicon-errata.txt
@@ -47,3 +47,4 @@ stable hypervisors.
 | ARM            | Cortex-A53      | #819472         | ARM64_ERRATUM_819472    |
 | ARM            | Cortex-A57      | #852523         | N/A                     |
 | ARM            | Cortex-A57      | #832075         | ARM64_ERRATUM_832075    |
+| ARM            | Cortex-A57      | #834220         | ARM64_ERRATUM_834220    |
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index e26c4c8..871c243 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -142,6 +142,27 @@ config ARM64_ERRATUM_832075
 
 	  If unsure, say Y.
 
+config ARM64_ERRATUM_834220
+	bool "Cortex-A57: 834220: Stage 2 translation fault might be incorrectly reported in presence of a Stage 1 fault"
+	default y
+	depends on ARM_64
+	help
+	  This option adds an alternative code sequence to work around ARM
+	  erratum 834220 on Cortex-A57 parts up to r1p2.
+
+	  Affected Cortex-A57 parts might report a Stage 2 translation
+	  fault as the result of a Stage 1 fault for load crossing a
+	  page boundary when there is a permission or device memory
+	  alignment fault at Stage 1 and a translation fault at Stage 2.
+
+	  The workaround is to verify that the Stage 1 translation
+	  doesn't generate a fault before handling the Stage 2 fault.
+	  Please note that this does not necessarily enable the workaround,
+	  as it depends on the alternative framework, which will only patch
+	  the kernel if an affected CPU is detected.
+
+	  If unsure, say Y.
+
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 748e02e..a3e8dda 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -49,6 +49,15 @@ static const struct arm_cpu_capabilities arm_errata[] = {
                    (1 << MIDR_VARIANT_SHIFT) | 2),
     },
 #endif
+#ifdef CONFIG_ARM64_ERRATUM_834220
+    {
+        /* Cortex-A57 r0p0 - r1p2 */
+        .desc = "ARM erratum 834220",
+        .capability = ARM64_WORKAROUND_834220,
+        MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
+                   (1 << MIDR_VARIANT_SHIFT) | 2),
+    },
+#endif
     {},
 };
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 0edc2cc..57e02e3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2389,12 +2389,13 @@ static inline bool hpfar_is_valid(bool s1ptw, uint8_t fsc)
      * HPFAR is valid if one of the following cases are true:
      *  1. the stage 2 fault happen during a stage 1 page table walk
      *  (the bit ESR_EL2.S1PTW is set)
-     *  2. the fault was due to a translation fault
+     *  2. the fault was due to a translation fault and the processor
+     *  does not carry erratum #8342220
      *
      * Note that technically HPFAR is valid for other cases, but they
      * are currently not supported by Xen.
      */
-    return s1ptw || (fsc == FSC_FLT_TRANS);
+    return s1ptw || (fsc == FSC_FLT_TRANS && !check_workaround_834220());
 }
 
 static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
index aaf3edd..d3d3080 100644
--- a/xen/include/asm-arm/cpuerrata.h
+++ b/xen/include/asm-arm/cpuerrata.h
@@ -41,6 +41,7 @@ static inline bool_t check_workaround_##erratum(void)           \
 #endif
 
 CHECK_WORKAROUND_HELPER(766422, ARM32_WORKAROUND_766422, CONFIG_ARM_32)
+CHECK_WORKAROUND_HELPER(834220, ARM64_WORKAROUND_834220, CONFIG_ARM_64)
 
 #undef CHECK_WORKAROUND_HELPER
 
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index ac6eaf0..66e930f 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -38,8 +38,9 @@
 #define ARM64_WORKAROUND_CLEAN_CACHE    0
 #define ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE    1
 #define ARM32_WORKAROUND_766422 2
+#define ARM64_WORKAROUND_834220 3
 
-#define ARM_NCAPS           3
+#define ARM_NCAPS           4
 
 #ifndef __ASSEMBLY__
 
-- 
1.9.1


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

Re: [PATCH 1/9] xen/arm: Simply the definition of PAGE_SIZE by using the macro _AC

[Stefano Stabellini]

On Wed, 22 Jun 2016, Julien Grall wrote:
> The macro _AC is used to define constant for both assembly and C.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


>  xen/include/asm-arm/config.h | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index 9417be6..a96f845 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -176,12 +176,7 @@
>  #define FIXMAP_ACPI_END    (FIXMAP_ACPI_BEGIN + NUM_FIXMAP_ACPI_PAGES - 1)  /* End mappings of ACPI tables */
>  
>  #define PAGE_SHIFT              12
> -
> -#ifndef __ASSEMBLY__
> -#define PAGE_SIZE           (1L << PAGE_SHIFT)
> -#else
> -#define PAGE_SIZE           (1 << PAGE_SHIFT)
> -#endif
> +#define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
>  #define PAGE_MASK           (~(PAGE_SIZE-1))
>  #define PAGE_FLAG_MASK      (~0)
>  
> -- 
> 1.9.1
> 

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

Re: [PATCH 2/9] xen/arm: traps: Second attempt to correctly use the content of HPFAR_EL2

[Stefano Stabellini]

On Wed, 22 Jun 2016, Julien Grall wrote:
> Commit c051618 "xen/arm: traps: Correctly interpret the content of the
> register HPFAR_EL2" attempted to fix the interpretation of HPFAR_EL2.
> 
> However, the register contains a 4KB-aligned address. This means that
> the reported address is not directly usable to know the faulting IPA.
> The offset in the 4KB page can be found by looking at the associated virtual
> address (FAR_EL2/HDFAR).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
> 
> Cc: Tamas K Lengyel <tamas@tklengyel.com>
> 
> I overlooked the usage of HPFAR_EL2. This is currently only affecting
> memaccess. Rather that return that exact address, the address will
> always be the base addres of the 4KB page.
> 
> This would need to be backported up to Xen 4.6.
> ---
>  xen/arch/arm/traps.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 0709deb..742f7d3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2371,11 +2371,15 @@ done:
>      if (first) unmap_domain_page(first);
>  }
>  
> -static inline paddr_t get_faulting_ipa(void)
> +static inline paddr_t get_faulting_ipa(vaddr_t gva)
>  {
>      register_t hpfar = READ_SYSREG(HPFAR_EL2);
> +    paddr_t ipa;
>  
> -    return ((paddr_t)(hpfar & HPFAR_MASK) << (12 - 4));
> +    ipa = (paddr_t)(hpfar & HPFAR_MASK) << (12 - 4);
> +    ipa |= gva & ~PAGE_MASK;
> +
> +    return ipa;
>  }
>  
>  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
> @@ -2396,7 +2400,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>          };
>  
>          if ( hsr.iabt.s1ptw )
> -            gpa = get_faulting_ipa();
> +            gpa = get_faulting_ipa(gva);
>          else
>          {
>              /*
> @@ -2445,7 +2449,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>  #endif
>  
>      if ( dabt.s1ptw )
> -        info.gpa = get_faulting_ipa();
> +        info.gpa = get_faulting_ipa(info.gva);
>      else
>      {
>          rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);
> -- 
> 1.9.1
> 

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

Re: [PATCH 3/9] xen/arm: traps: Data Abort are always unconditional

[Stefano Stabellini]

On Wed, 22 Jun 2016, Julien Grall wrote:
> The HSR encoding for an exception from a data abort does not contain a
> conditional code (see G6-4264 in ARM DDI 0487A.i) because they are
> always conditional.
> 
> So drop the pointless condition check.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


>  xen/arch/arm/traps.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 742f7d3..2e84b5a 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2435,12 +2435,6 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>      int rc;
>      mmio_info_t info;
>  
> -    if ( !check_conditional_instr(regs, hsr) )
> -    {
> -        advance_pc(regs, hsr);
> -        return;
> -    }
> -
>      info.dabt = dabt;
>  #ifdef CONFIG_ARM_32
>      info.gva = READ_CP32(HDFAR);
> -- 
> 1.9.1
> 

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

Re: [PATCH 4/9] xen/arm: traps: Simplify the switch in do_trap_*_abort_guest

[Stefano Stabellini]

On Wed, 22 Jun 2016, Julien Grall wrote:
> The fault status we care are all the form BBBBxx where xx is the lookup

                                 ^ in the form of

> level that gave the fault. We can simply the code by masking the 2 least

                                       ^ simplify

> significant bits.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/traps.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 2e84b5a..8a3fac0 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2388,9 +2388,9 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>      int rc;
>      register_t gva = READ_SYSREG(FAR_EL2);
>  
> -    switch ( hsr.iabt.ifsc & 0x3f )
> +    switch ( hsr.iabt.ifsc & ~FSC_LL_MASK )
>      {
> -    case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
> +    case FSC_FLT_PERM:
>      {

This is a good improvement in code readability. I would go one step
further and replace the switch with a simple if.


>          paddr_t gpa;
>          const struct npfec npfec = {
> @@ -2451,9 +2451,9 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>              return; /* Try again */
>      }
>  
> -    switch ( dabt.dfsc & 0x3f )
> +    switch ( dabt.dfsc & ~FSC_LL_MASK )
>      {
> -    case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
> +    case FSC_FLT_PERM:
>      {
>          const struct npfec npfec = {
>              .read_access = !dabt.write,

same here


> -- 
> 1.9.1
> 

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

Re: [PATCH 4/9] xen/arm: traps: Simplify the switch in do_trap_*_abort_guest

[Julien Grall]

Hi Stefano,

On 14/07/16 12:12, Stefano Stabellini wrote:
> On Wed, 22 Jun 2016, Julien Grall wrote:
>> The fault status we care are all the form BBBBxx where xx is the lookup
>
>                                   ^ in the form of
>
>> level that gave the fault. We can simply the code by masking the 2 least
>
>                                         ^ simplify
>
>> significant bits.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/traps.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 2e84b5a..8a3fac0 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2388,9 +2388,9 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>>       int rc;
>>       register_t gva = READ_SYSREG(FAR_EL2);
>>
>> -    switch ( hsr.iabt.ifsc & 0x3f )
>> +    switch ( hsr.iabt.ifsc & ~FSC_LL_MASK )
>>       {
>> -    case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
>> +    case FSC_FLT_PERM:
>>       {
>
> This is a good improvement in code readability. I would go one step
> further and replace the switch with a simple if.

I would prefer to keep the switch case here. The patch #7 adds another 
case for do_trap_data_abort_guest because we should not emulate MMIO for 
any kind of fault as it is done today.

Also, I have got more fixes coming up which require the switch here.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/9] xen/arm: traps: Simplify the switch in do_trap_*_abort_guest

[Stefano Stabellini]

On Thu, 14 Jul 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 14/07/16 12:12, Stefano Stabellini wrote:
> > On Wed, 22 Jun 2016, Julien Grall wrote:
> > > The fault status we care are all the form BBBBxx where xx is the lookup
> > 
> >                                   ^ in the form of
> > 
> > > level that gave the fault. We can simply the code by masking the 2 least
> > 
> >                                         ^ simplify
> > 
> > > significant bits.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > ---
> > >   xen/arch/arm/traps.c | 8 ++++----
> > >   1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > index 2e84b5a..8a3fac0 100644
> > > --- a/xen/arch/arm/traps.c
> > > +++ b/xen/arch/arm/traps.c
> > > @@ -2388,9 +2388,9 @@ static void do_trap_instr_abort_guest(struct
> > > cpu_user_regs *regs,
> > >       int rc;
> > >       register_t gva = READ_SYSREG(FAR_EL2);
> > > 
> > > -    switch ( hsr.iabt.ifsc & 0x3f )
> > > +    switch ( hsr.iabt.ifsc & ~FSC_LL_MASK )
> > >       {
> > > -    case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
> > > +    case FSC_FLT_PERM:
> > >       {
> > 
> > This is a good improvement in code readability. I would go one step
> > further and replace the switch with a simple if.
> 
> I would prefer to keep the switch case here. The patch #7 adds another case
> for do_trap_data_abort_guest because we should not emulate MMIO for any kind
> of fault as it is done today.
> 
> Also, I have got more fixes coming up which require the switch here.

all right

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

Re: [PATCH 5/9] xen/arm: Provide macros to help creating workaround helpers

[Stefano Stabellini]

On Wed, 22 Jun 2016, Julien Grall wrote:
> Workarounds may require to execute a different path when the platform
> is affected by the associated erratum. Furthermore, this may need to
> be called in the common code.
> 
> To avoid too much intrusion/overhead, the workaround helpers need to
> be a nop on architecture which will never have the workaround and have
> to be quick to check whether the platform requires it.
> 
> The alternative framework is used to transform the check in a single
> instruction. When the framework is not available, the helper will have
> ~6 instructions including 1 instruction load.
> 
> The macro will create a handler called check_workaround_xxxxx with
> xxxx the erratum number.
> 
> For instance, the line bellow will create a workaround helper for
> erratum #424242 which is enabled when the capability
> ARM64_WORKAROUND_424242 is set and only available for ARM64:
> 
> CHECK_WORKAROUND_HELPER(424242, ARM64_WORKAROUND_42424242, CONFIG_ARM64)
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

It looks good to me. CC'ing Konrad which is more knowledgeable than me
about ALTERNATIVE.


>  xen/include/asm-arm/cpuerrata.h | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
> index fe93beb..b9d8dfc 100644
> --- a/xen/include/asm-arm/cpuerrata.h
> +++ b/xen/include/asm-arm/cpuerrata.h
> @@ -1,8 +1,47 @@
>  #ifndef __ARM_CPUERRATA_H
>  #define __ARM_CPUERRATA_H
>  
> +#include <xen/config.h>
> +#include <asm/cpufeature.h>
> +#include <asm/alternative.h>
> +
>  void check_local_cpu_errata(void);
>  
> +#ifdef CONFIG_ALTERNATIVE
> +
> +#define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
> +static inline bool_t check_workaround_##erratum(void)           \
> +{                                                               \
> +    if ( !IS_ENABLED(arch) )                                    \
> +        return 0;                                               \
> +    else                                                        \
> +    {                                                           \
> +        bool_t ret;                                             \
> +                                                                \
> +        asm volatile (ALTERNATIVE("mov %0, #0",                 \
> +                                  "mov %0, #1",                 \
> +                                  feature)                      \
> +                      : "=r" (ret));                            \
> +                                                                \
> +        return unlikely(ret);                                   \
> +    }                                                           \
> +}
> +
> +#else /* CONFIG_ALTERNATIVE */
> +
> +#define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
> +static inline bool_t check_workaround_##erratum(void)           \
> +{                                                               \
> +    if ( !IS_ENABLED(arch) )                                    \
> +        return 0;                                               \
> +    else                                                        \
> +        return unlikely(cpus_have_cap(feature));                \
> +}
> +
> +#endif
> +
> +#undef CHECK_WORKAROUND_HELPER
> +
>  #endif /* __ARM_CPUERRATA_H */
>  /*
>   * Local variables:
> -- 
> 1.9.1
> 

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

Re: [PATCH 6/9] xen/arm: Use check_workaround to handle the erratum 766422

[Stefano Stabellini]

On Wed, 22 Jun 2016, Julien Grall wrote:
> Currently, Xen is reading the MIDR everytime it has to check whether
> the processor is affected by the erratum 766422.
> 
> This could take advantage of the new capability bitfields to detect
> whether the processor is affected at boot time.
> 
> With this patch, the number of instructions to check the erratum is
> going down from ~13 (including 2 loads and a co-processor access) to
> ~6 instructions (include 1 load).

The patch looks good but the midr bits were actually already stored in
cpu_data.  See:


> -/* Erratum 766422: only Cortex A15 r0p4 is affected */
> -#define cpu_has_erratum_766422()                             \
> -    (unlikely(current_cpu_data.midr.bits == 0x410fc0f4))

We weren't really accessing MIDR every time. Am I missing something?

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

Re: [PATCH 6/9] xen/arm: Use check_workaround to handle the erratum 766422

[Julien Grall]

Hi Stefano,

On 14/07/16 15:34, Stefano Stabellini wrote:
> On Wed, 22 Jun 2016, Julien Grall wrote:
>> Currently, Xen is reading the MIDR everytime it has to check whether
>> the processor is affected by the erratum 766422.
>>
>> This could take advantage of the new capability bitfields to detect
>> whether the processor is affected at boot time.
>>
>> With this patch, the number of instructions to check the erratum is
>> going down from ~13 (including 2 loads and a co-processor access) to
>> ~6 instructions (include 1 load).
>
> The patch looks good but the midr bits were actually already stored in
> cpu_data.  See:
>
>
>> -/* Erratum 766422: only Cortex A15 r0p4 is affected */
>> -#define cpu_has_erratum_766422()                             \
>> -    (unlikely(current_cpu_data.midr.bits == 0x410fc0f4))
>
> We weren't really accessing MIDR every time. Am I missing something?

The wording in the commit message is not clear. By "accessing the MDIR 
every time" I meant that the stored MIDR is checked every single time.

current_cpu_data turns into a complex series of assembly instructions to 
get the data of the current CPU which involves 2 loads.

I will rewrite the commit message in the next version.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 7/9] xen/arm: traps: MMIO should only be emulated for fault translation

[Stefano Stabellini]

On Wed, 22 Jun 2016, Julien Grall wrote:
> The function do_trap_data_abort_guest assumes that a stage-2 data abort
> can only be taken for a translation fault or permission fault today.
> 
> Whilst this is true today, it might not be in the future. Rather than
> emulating the MMIO for any fault other than the permission one, print
> a warning message when the fault is not handled by Xen.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/traps.c | 51 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 785e3e9..591de3c 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2468,35 +2468,40 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>          /* Trap was triggered by mem_access, work here is done */
>          if ( !rc )
>              return;
> +        break;
>      }
> -    break;
> -    }
> -
> -    if ( dabt.s1ptw )
> -        goto bad_data_abort;
> +    case FSC_FLT_TRANS:
> +        if ( dabt.s1ptw )
> +            goto bad_data_abort;
>  
> -    /* XXX: Decode the instruction if ISS is not valid */
> -    if ( !dabt.valid )
> -        goto bad_data_abort;
> +        /* XXX: Decode the instruction if ISS is not valid */
> +        if ( !dabt.valid )
> +            goto bad_data_abort;
>  
> -    /*
> -     * Erratum 766422: Thumb store translation fault to Hypervisor may
> -     * not have correct HSR Rt value.
> -     */
> -    if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
> -    {
> -        rc = decode_instruction(regs, &info.dabt);
> -        if ( rc )
> +        /*
> +         * Erratum 766422: Thumb store translation fault to Hypervisor may
> +         * not have correct HSR Rt value.
> +         */
> +        if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> +             dabt.write )
>          {
> -            gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> -            goto bad_data_abort;
> +            rc = decode_instruction(regs, &info.dabt);
> +            if ( rc )
> +            {
> +                gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> +                goto bad_data_abort;
> +            }
>          }
> -    }
>  
> -    if (handle_mmio(&info))
> -    {
> -        advance_pc(regs, hsr);
> -        return;
> +        if ( handle_mmio(&info) )
> +        {
> +            advance_pc(regs, hsr);
> +            return;
> +        }
> +        break;
> +    default:
> +        gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
> +                hsr.bits, dabt.dfsc);

Given that bad_data_abort, which is right after, will print HSR again, I
would remove it from this message as it's redundant.


>      }
>  
>  bad_data_abort:
> -- 
> 1.9.1
> 

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

Re: [PATCH 7/9] xen/arm: traps: MMIO should only be emulated for fault translation

[Julien Grall]

Hi Stefano,

On 14/07/16 16:06, Stefano Stabellini wrote:
> On Wed, 22 Jun 2016, Julien Grall wrote:
>> -    if (handle_mmio(&info))
>> -    {
>> -        advance_pc(regs, hsr);
>> -        return;
>> +        if ( handle_mmio(&info) )
>> +        {
>> +            advance_pc(regs, hsr);
>> +            return;
>> +        }
>> +        break;
>> +    default:
>> +        gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
>> +                hsr.bits, dabt.dfsc);
>
> Given that bad_data_abort, which is right after, will print HSR again, I
> would remove it from this message as it's redundant.

I agree this is redundant, however the message below will only be 
printed in debug build because a guest could trigger it easily. The 
message above is here to catch any possible misconfiguration of stage-2 
page table or hardware issue (ECC error, address size, TLB conflict...) 
and could be seen in non-debug build.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 8/9] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Stefano Stabellini]

On Wed, 22 Jun 2016, Julien Grall wrote:
> Translating a VA to a IPA is expensive. Currently, Xen is assuming that
> HPFAR_EL2 is only valid when the stage-2 data/instruction abort happened
> during a translation table walk of a first stage translation (i.e S1PTW
> is set).
> 
> However, based on the ARM ARM (D7.2.34 in DDI 0487A.j), the register is
> also valid when the data/instruction abort occured for a translation
> fault.
> 
> With this change, the VA -> IPA translation will only happen for
> permission faults that are not related to a translation table of a
> first stage translation.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
>  xen/arch/arm/traps.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 591de3c..0edc2cc 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2383,13 +2383,28 @@ static inline paddr_t get_faulting_ipa(vaddr_t gva)
>      return ipa;
>  }
>  
> +static inline bool hpfar_is_valid(bool s1ptw, uint8_t fsc)
> +{
> +    /*
> +     * HPFAR is valid if one of the following cases are true:
> +     *  1. the stage 2 fault happen during a stage 1 page table walk
> +     *  (the bit ESR_EL2.S1PTW is set)
> +     *  2. the fault was due to a translation fault
> +     *
> +     * Note that technically HPFAR is valid for other cases, but they
> +     * are currently not supported by Xen.
> +     */
> +    return s1ptw || (fsc == FSC_FLT_TRANS);
> +}
> +
>  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>                                        const union hsr hsr)
>  {
>      int rc;
>      register_t gva = READ_SYSREG(FAR_EL2);
> +    uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
>  
> -    switch ( hsr.iabt.ifsc & ~FSC_LL_MASK )
> +    switch ( fsc )
>      {
>      case FSC_FLT_PERM:
>      {
> @@ -2400,7 +2415,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>              .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
>          };
>  
> -        if ( hsr.iabt.s1ptw )
> +        if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
>              gpa = get_faulting_ipa(gva);
>          else
>          {
> @@ -2435,6 +2450,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>      const struct hsr_dabt dabt = hsr.dabt;
>      int rc;
>      mmio_info_t info;
> +    uint8_t fsc = hsr.dabt.dfsc & ~FSC_LL_MASK;

You should be able to modify the switch in this case too, right?


>      info.dabt = dabt;
>  #ifdef CONFIG_ARM_32
> @@ -2443,7 +2459,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>      info.gva = READ_SYSREG64(FAR_EL2);
>  #endif
>  
> -    if ( dabt.s1ptw )
> +    if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
>          info.gpa = get_faulting_ipa(info.gva);
>      else
>      {
> -- 
> 1.9.1
> 

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

Re: [PATCH 7/9] xen/arm: traps: MMIO should only be emulated for fault translation

[Stefano Stabellini]

On Thu, 14 Jul 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 14/07/16 16:06, Stefano Stabellini wrote:
> > On Wed, 22 Jun 2016, Julien Grall wrote:
> > > -    if (handle_mmio(&info))
> > > -    {
> > > -        advance_pc(regs, hsr);
> > > -        return;
> > > +        if ( handle_mmio(&info) )
> > > +        {
> > > +            advance_pc(regs, hsr);
> > > +            return;
> > > +        }
> > > +        break;
> > > +    default:
> > > +        gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
> > > +                hsr.bits, dabt.dfsc);
> > 
> > Given that bad_data_abort, which is right after, will print HSR again, I
> > would remove it from this message as it's redundant.
> 
> I agree this is redundant, however the message below will only be printed in
> debug build because a guest could trigger it easily. The message above is here
> to catch any possible misconfiguration of stage-2 page table or hardware issue
> (ECC error, address size, TLB conflict...) and could be seen in non-debug
> build.

Fair enough, it will just look a bit weird in the source code.

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

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

Re: [PATCH 7/9] xen/arm: traps: MMIO should only be emulated for fault translation

[Julien Grall]

On 14/07/16 16:28, Stefano Stabellini wrote:
> On Thu, 14 Jul 2016, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 14/07/16 16:06, Stefano Stabellini wrote:
>>> On Wed, 22 Jun 2016, Julien Grall wrote:
>>>> -    if (handle_mmio(&info))
>>>> -    {
>>>> -        advance_pc(regs, hsr);
>>>> -        return;
>>>> +        if ( handle_mmio(&info) )
>>>> +        {
>>>> +            advance_pc(regs, hsr);
>>>> +            return;
>>>> +        }
>>>> +        break;
>>>> +    default:
>>>> +        gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
>>>> +                hsr.bits, dabt.dfsc);
>>>
>>> Given that bad_data_abort, which is right after, will print HSR again, I
>>> would remove it from this message as it's redundant.
>>
>> I agree this is redundant, however the message below will only be printed in
>> debug build because a guest could trigger it easily. The message above is here
>> to catch any possible misconfiguration of stage-2 page table or hardware issue
>> (ECC error, address size, TLB conflict...) and could be seen in non-debug
>> build.
>
> Fair enough, it will just look a bit weird in the source code.

Would a comment in the source code make it less weird?

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

-- 
Julien Grall

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

Re: [PATCH 8/9] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Julien Grall]

On 14/07/16 16:27, Stefano Stabellini wrote:
> On Wed, 22 Jun 2016, Julien Grall wrote:
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 591de3c..0edc2cc 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2383,13 +2383,28 @@ static inline paddr_t get_faulting_ipa(vaddr_t gva)

[..]

>>   static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>>                                         const union hsr hsr)
>>   {
>>       int rc;
>>       register_t gva = READ_SYSREG(FAR_EL2);
>> +    uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
>>
>> -    switch ( hsr.iabt.ifsc & ~FSC_LL_MASK )
>> +    switch ( fsc )
>>       {
>>       case FSC_FLT_PERM:
>>       {
>> @@ -2400,7 +2415,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>>               .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
>>           };
>>
>> -        if ( hsr.iabt.s1ptw )
>> +        if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
>>               gpa = get_faulting_ipa(gva);
>>           else
>>           {
>> @@ -2435,6 +2450,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>>       const struct hsr_dabt dabt = hsr.dabt;
>>       int rc;
>>       mmio_info_t info;
>> +    uint8_t fsc = hsr.dabt.dfsc & ~FSC_LL_MASK;
>
> You should be able to modify the switch in this case too, right?

Correct. I am thinking to pull the changes in patch #4 to avoid 
extra-changes in this patch.

Regards,

-- 
Julien Grall

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

Re: [PATCH 9/9] xen/arm: arm64: Add Cortex-A57 erratum 834220 workaround

[Stefano Stabellini]

On Wed, 22 Jun 2016, Julien Grall wrote:
> The ARM erratum applies to certain revisions of Cortex-A57. The
> processor may report a Stage 2 translation fault as the result of
> Stage 1 fault for load crossing a page boundary when there is a
> permission fault or device memory fault at stage 1 and a translation
> fault at Stage 2.
> 
> So Xen needs to check that Stage 1 translation does not generate a fault
> before handling the Stage 2 fault. If it is a Stage 1 translation fault,
> return to the guest to let the processor injecting the correct fault.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


>  docs/misc/arm/silicon-errata.txt |  1 +
>  xen/arch/arm/Kconfig             | 21 +++++++++++++++++++++
>  xen/arch/arm/cpuerrata.c         |  9 +++++++++
>  xen/arch/arm/traps.c             |  5 +++--
>  xen/include/asm-arm/cpuerrata.h  |  1 +
>  xen/include/asm-arm/cpufeature.h |  3 ++-
>  6 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/misc/arm/silicon-errata.txt b/docs/misc/arm/silicon-errata.txt
> index 220f724..c9854c3 100644
> --- a/docs/misc/arm/silicon-errata.txt
> +++ b/docs/misc/arm/silicon-errata.txt
> @@ -47,3 +47,4 @@ stable hypervisors.
>  | ARM            | Cortex-A53      | #819472         | ARM64_ERRATUM_819472    |
>  | ARM            | Cortex-A57      | #852523         | N/A                     |
>  | ARM            | Cortex-A57      | #832075         | ARM64_ERRATUM_832075    |
> +| ARM            | Cortex-A57      | #834220         | ARM64_ERRATUM_834220    |
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index e26c4c8..871c243 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -142,6 +142,27 @@ config ARM64_ERRATUM_832075
>  
>  	  If unsure, say Y.
>  
> +config ARM64_ERRATUM_834220
> +	bool "Cortex-A57: 834220: Stage 2 translation fault might be incorrectly reported in presence of a Stage 1 fault"
> +	default y
> +	depends on ARM_64
> +	help
> +	  This option adds an alternative code sequence to work around ARM
> +	  erratum 834220 on Cortex-A57 parts up to r1p2.
> +
> +	  Affected Cortex-A57 parts might report a Stage 2 translation
> +	  fault as the result of a Stage 1 fault for load crossing a
> +	  page boundary when there is a permission or device memory
> +	  alignment fault at Stage 1 and a translation fault at Stage 2.
> +
> +	  The workaround is to verify that the Stage 1 translation
> +	  doesn't generate a fault before handling the Stage 2 fault.
> +	  Please note that this does not necessarily enable the workaround,
> +	  as it depends on the alternative framework, which will only patch
> +	  the kernel if an affected CPU is detected.
> +
> +	  If unsure, say Y.
> +
>  endmenu
>  
>  source "common/Kconfig"
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index 748e02e..a3e8dda 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -49,6 +49,15 @@ static const struct arm_cpu_capabilities arm_errata[] = {
>                     (1 << MIDR_VARIANT_SHIFT) | 2),
>      },
>  #endif
> +#ifdef CONFIG_ARM64_ERRATUM_834220
> +    {
> +        /* Cortex-A57 r0p0 - r1p2 */
> +        .desc = "ARM erratum 834220",
> +        .capability = ARM64_WORKAROUND_834220,
> +        MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
> +                   (1 << MIDR_VARIANT_SHIFT) | 2),
> +    },
> +#endif
>      {},
>  };
>  
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 0edc2cc..57e02e3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2389,12 +2389,13 @@ static inline bool hpfar_is_valid(bool s1ptw, uint8_t fsc)
>       * HPFAR is valid if one of the following cases are true:
>       *  1. the stage 2 fault happen during a stage 1 page table walk
>       *  (the bit ESR_EL2.S1PTW is set)
> -     *  2. the fault was due to a translation fault
> +     *  2. the fault was due to a translation fault and the processor
> +     *  does not carry erratum #8342220
>       *
>       * Note that technically HPFAR is valid for other cases, but they
>       * are currently not supported by Xen.
>       */
> -    return s1ptw || (fsc == FSC_FLT_TRANS);
> +    return s1ptw || (fsc == FSC_FLT_TRANS && !check_workaround_834220());
>  }
>  
>  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
> diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
> index aaf3edd..d3d3080 100644
> --- a/xen/include/asm-arm/cpuerrata.h
> +++ b/xen/include/asm-arm/cpuerrata.h
> @@ -41,6 +41,7 @@ static inline bool_t check_workaround_##erratum(void)           \
>  #endif
>  
>  CHECK_WORKAROUND_HELPER(766422, ARM32_WORKAROUND_766422, CONFIG_ARM_32)
> +CHECK_WORKAROUND_HELPER(834220, ARM64_WORKAROUND_834220, CONFIG_ARM_64)
>  
>  #undef CHECK_WORKAROUND_HELPER
>  
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index ac6eaf0..66e930f 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -38,8 +38,9 @@
>  #define ARM64_WORKAROUND_CLEAN_CACHE    0
>  #define ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE    1
>  #define ARM32_WORKAROUND_766422 2
> +#define ARM64_WORKAROUND_834220 3
>  
> -#define ARM_NCAPS           3
> +#define ARM_NCAPS           4
>  
>  #ifndef __ASSEMBLY__
>  
> -- 
> 1.9.1
> 

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

Re: [PATCH 8/9] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Stefano Stabellini]

On Thu, 14 Jul 2016, Julien Grall wrote:
> On 14/07/16 16:27, Stefano Stabellini wrote:
> > On Wed, 22 Jun 2016, Julien Grall wrote:
> > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > index 591de3c..0edc2cc 100644
> > > --- a/xen/arch/arm/traps.c
> > > +++ b/xen/arch/arm/traps.c
> > > @@ -2383,13 +2383,28 @@ static inline paddr_t get_faulting_ipa(vaddr_t
> > > gva)
> 
> [..]
> 
> > >   static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
> > >                                         const union hsr hsr)
> > >   {
> > >       int rc;
> > >       register_t gva = READ_SYSREG(FAR_EL2);
> > > +    uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
> > > 
> > > -    switch ( hsr.iabt.ifsc & ~FSC_LL_MASK )
> > > +    switch ( fsc )
> > >       {
> > >       case FSC_FLT_PERM:
> > >       {
> > > @@ -2400,7 +2415,7 @@ static void do_trap_instr_abort_guest(struct
> > > cpu_user_regs *regs,
> > >               .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt :
> > > npfec_kind_with_gla
> > >           };
> > > 
> > > -        if ( hsr.iabt.s1ptw )
> > > +        if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
> > >               gpa = get_faulting_ipa(gva);
> > >           else
> > >           {
> > > @@ -2435,6 +2450,7 @@ static void do_trap_data_abort_guest(struct
> > > cpu_user_regs *regs,
> > >       const struct hsr_dabt dabt = hsr.dabt;
> > >       int rc;
> > >       mmio_info_t info;
> > > +    uint8_t fsc = hsr.dabt.dfsc & ~FSC_LL_MASK;
> > 
> > You should be able to modify the switch in this case too, right?
> 
> Correct. I am thinking to pull the changes in patch #4 to avoid extra-changes
> in this patch.

Sure

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

Re: [PATCH 5/9] xen/arm: Provide macros to help creating workaround helpers

[Julien Grall]

Hi Konrad,

On 14/07/16 14:57, Stefano Stabellini wrote:
> On Wed, 22 Jun 2016, Julien Grall wrote:
>> Workarounds may require to execute a different path when the platform
>> is affected by the associated erratum. Furthermore, this may need to
>> be called in the common code.
>>
>> To avoid too much intrusion/overhead, the workaround helpers need to
>> be a nop on architecture which will never have the workaround and have
>> to be quick to check whether the platform requires it.
>>
>> The alternative framework is used to transform the check in a single
>> instruction. When the framework is not available, the helper will have
>> ~6 instructions including 1 instruction load.
>>
>> The macro will create a handler called check_workaround_xxxxx with
>> xxxx the erratum number.
>>
>> For instance, the line bellow will create a workaround helper for
>> erratum #424242 which is enabled when the capability
>> ARM64_WORKAROUND_424242 is set and only available for ARM64:
>>
>> CHECK_WORKAROUND_HELPER(424242, ARM64_WORKAROUND_42424242, CONFIG_ARM64)
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> It looks good to me. CC'ing Konrad which is more knowledgeable than me
> about ALTERNATIVE.

Do you have any opinions on this patch?

Cheers,

>
>
>>  xen/include/asm-arm/cpuerrata.h | 39 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
>> index fe93beb..b9d8dfc 100644
>> --- a/xen/include/asm-arm/cpuerrata.h
>> +++ b/xen/include/asm-arm/cpuerrata.h
>> @@ -1,8 +1,47 @@
>>  #ifndef __ARM_CPUERRATA_H
>>  #define __ARM_CPUERRATA_H
>>
>> +#include <xen/config.h>
>> +#include <asm/cpufeature.h>
>> +#include <asm/alternative.h>
>> +
>>  void check_local_cpu_errata(void);
>>
>> +#ifdef CONFIG_ALTERNATIVE
>> +
>> +#define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
>> +static inline bool_t check_workaround_##erratum(void)           \
>> +{                                                               \
>> +    if ( !IS_ENABLED(arch) )                                    \
>> +        return 0;                                               \
>> +    else                                                        \
>> +    {                                                           \
>> +        bool_t ret;                                             \
>> +                                                                \
>> +        asm volatile (ALTERNATIVE("mov %0, #0",                 \
>> +                                  "mov %0, #1",                 \
>> +                                  feature)                      \
>> +                      : "=r" (ret));                            \
>> +                                                                \
>> +        return unlikely(ret);                                   \
>> +    }                                                           \
>> +}
>> +
>> +#else /* CONFIG_ALTERNATIVE */
>> +
>> +#define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
>> +static inline bool_t check_workaround_##erratum(void)           \
>> +{                                                               \
>> +    if ( !IS_ENABLED(arch) )                                    \
>> +        return 0;                                               \
>> +    else                                                        \
>> +        return unlikely(cpus_have_cap(feature));                \
>> +}
>> +
>> +#endif
>> +
>> +#undef CHECK_WORKAROUND_HELPER
>> +
>>  #endif /* __ARM_CPUERRATA_H */
>>  /*
>>   * Local variables:
>> --
>> 1.9.1
>>
>

-- 
Julien Grall

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

Re: [PATCH 5/9] xen/arm: Provide macros to help creating workaround helpers

[Konrad Rzeszutek Wilk]

On Wed, Jul 20, 2016 at 8:43 AM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Konrad,
>
>>> For instance, the line bellow will create a workaround helper for
>>> erratum #424242 which is enabled when the capability
>>> ARM64_WORKAROUND_424242 is set and only available for ARM64:

42, eh?

>>>
>>> CHECK_WORKAROUND_HELPER(424242, ARM64_WORKAROUND_42424242, CONFIG_ARM64)
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>>
>> It looks good to me. CC'ing Konrad which is more knowledgeable than me
>> about ALTERNATIVE.
>
>
> Do you have any opinions on this patch?

Yes I do:

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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