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

[PATCH v2 0/6] 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 some case that can be affected
by the erratum 834220 on Cortex-A57, we need to implement a workaround
for it (see patch #6).

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

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

Yours sincerely,

[1] https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg02816.html

Julien Grall (6):
  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                  | 81 ++++++++++++++++++++++-------------
 xen/include/asm-arm/arm32/processor.h |  4 --
 xen/include/asm-arm/arm64/processor.h |  2 -
 xen/include/asm-arm/cpuerrata.h       | 42 ++++++++++++++++++
 xen/include/asm-arm/cpufeature.h      |  4 +-
 xen/include/asm-arm/processor.h       |  2 +
 9 files changed, 136 insertions(+), 36 deletions(-)

-- 
1.9.1


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

[PATCH v2 1/6] xen/arm: traps: Simplify the switch in do_trap_*_abort_guest

[Julien Grall]

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

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

---
    The switch has not been replaced by a simple if because more case
    will be added in follow-up patches.

    Changes in v2:
        - Fix typoes in the commit message
---
 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 2d05936..b34c46f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2387,9 +2387,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 = {
@@ -2450,9 +2450,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
https://lists.xen.org/xen-devel

[PATCH v2 2/6] 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>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
    Changes in v2:
        - Add Konrad's reviewed-by
---
 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 c495ee5..2982a92 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

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

[Julien Grall]

Currently, Xen is accessing the stored 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>

---
    Changes in v2:
        - Update the commit message
---
 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 b34c46f..28982a4 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -46,6 +46,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
@@ -2481,7 +2482,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 2982a92..5880e77 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
https://lists.xen.org/xen-devel

[PATCH v2 4/6] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Add Stefano's reviewed-by
---
 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 28982a4..ea105f2 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2467,35 +2467,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
https://lists.xen.org/xen-devel

[PATCH v2 5/6] 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>

---
    Changes in v2:
        - Use fsc in the switch in do_trap_data_abort_guest
---
 xen/arch/arm/traps.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ea105f2..83a30fa 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2382,13 +2382,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:
     {
@@ -2399,7 +2414,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
         {
@@ -2434,6 +2449,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
@@ -2442,7 +2458,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
     {
@@ -2451,7 +2467,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
             return; /* Try again */
     }
 
-    switch ( dabt.dfsc & ~FSC_LL_MASK )
+    switch ( fsc )
     {
     case FSC_FLT_PERM:
     {
-- 
1.9.1


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

[PATCH v2 6/6] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Add Stefano's reviewed-by
---
 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 83a30fa..13935e8 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2388,12 +2388,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 5880e77..5e35b4f 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 v2 5/6] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Sergej Proskurin]

[-- Attachment #1.1: Type: text/plain, Size: 3250 bytes --]

Hi Julien,


On 07/27/2016 07:09 PM, 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>
>
> ---
>     Changes in v2:
>         - Use fsc in the switch in do_trap_data_abort_guest
> ---
>  xen/arch/arm/traps.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ea105f2..83a30fa 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2382,13 +2382,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:
>      {
> @@ -2399,7 +2414,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
>          {
> @@ -2434,6 +2449,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
> @@ -2442,7 +2458,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) )

I belive this should be:

hpfar_is_valid(hsr.*dabt*.s1ptw, fsc)

>          info.gpa = get_faulting_ipa(info.gva);
>      else
>      {
> @@ -2451,7 +2467,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>              return; /* Try again */
>      }
>  
> -    switch ( dabt.dfsc & ~FSC_LL_MASK )
> +    switch ( fsc )
>      {
>      case FSC_FLT_PERM:
>      {

Cheers,
~Sergej

[-- Attachment #1.2: Type: text/html, Size: 3874 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v2 5/6] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Julien Grall]

On 27/07/16 18:28, Sergej Proskurin wrote:
> Hi Julien,

Hello Sergej,

Please reply to all (or at least CC me) when answering to xen-devel. 
Otherwise your mail may be missed or the answer may be delayed because I 
filter any mail where I am not the recipients (a lot of people do this 
on the ML).

>
> On 07/27/2016 07:09 PM, Julien Grall wrote:
>> @@ -2442,7 +2458,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) )
>
> I belive this should be:
>
> hpfar_is_valid(hsr.*dabt*.s1ptw, fsc)

You are right, I did not spot it during my testing because the field is 
exactly the same bits in the HSR.

>
>>          info.gpa = get_faulting_ipa(info.gva);
>>      else
>>      {
>> @@ -2451,7 +2467,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>>              return; /* Try again */
>>      }
>>
>> -    switch ( dabt.dfsc & ~FSC_LL_MASK )
>> +    switch ( fsc )
>>      {
>>      case FSC_FLT_PERM:
>>      {

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 1/6] xen/arm: traps: Simplify the switch in do_trap_*_abort_guest

[Stefano Stabellini]

On Wed, 27 Jul 2016, Julien Grall wrote:
> The fault status we care are in the form BBBBxx where xx is the lookup
> level that gave the fault. We can simplify the code by masking the 2 least
> significant bits.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     The switch has not been replaced by a simple if because more case
>     will be added in follow-up patches.
> 
>     Changes in v2:
>         - Fix typoes in the commit message
> ---
>  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 2d05936..b34c46f 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2387,9 +2387,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 = {
> @@ -2450,9 +2450,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
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/6] xen/arm: Provide macros to help creating workaround helpers

[Stefano Stabellini]

On Wed, 27 Jul 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>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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


> ---
>     Changes in v2:
>         - Add Konrad's reviewed-by
> ---
>  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 c495ee5..2982a92 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 v2 3/6] xen/arm: Use check_workaround to handle the erratum 766422

[Stefano Stabellini]

On Wed, 27 Jul 2016, Julien Grall wrote:
> Currently, Xen is accessing the stored 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>

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


> ---
>     Changes in v2:
>         - Update the commit message
> ---
>  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 b34c46f..28982a4 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -46,6 +46,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
> @@ -2481,7 +2482,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 2982a92..5880e77 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
https://lists.xen.org/xen-devel

Re: [PATCH v2 5/6] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Shanker Donthineni]

Hi Julien,

On 07/27/2016 12:09 PM, 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>
>
> ---
>      Changes in v2:
>          - Use fsc in the switch in do_trap_data_abort_guest
> ---
>   xen/arch/arm/traps.c | 24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ea105f2..83a30fa 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2382,13 +2382,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);

Yes, XEN is not supporting the stage 2 access flag but we should handle 
a stage 2 address size fault.
I think we should do some thing like to below to match ARM ARM.

return s1ptw || (fsc != FSC_FLT_PERM);


> +}
> +
>   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:
>       {
> @@ -2399,7 +2414,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
>           {
> @@ -2434,6 +2449,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
> @@ -2442,7 +2458,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
>       {
> @@ -2451,7 +2467,7 @@ static void do_trap_data_abort_guest(struct
> cpu_user_regs *regs,
>               return; /* Try again */
>       }
>
> -    switch ( dabt.dfsc & ~FSC_LL_MASK )
> +    switch ( fsc )
>       {
>       case FSC_FLT_PERM:
>       {

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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

Re: [PATCH v2 5/6] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Julien Grall]

On 17/08/16 03:19, Shanker Donthineni wrote:
> Hi Julien,

Hello Shanker,

> On 07/27/2016 12:09 PM, 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>
>>
>> ---
>>      Changes in v2:
>>          - Use fsc in the switch in do_trap_data_abort_guest
>> ---
>>   xen/arch/arm/traps.c | 24 ++++++++++++++++++++----
>>   1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index ea105f2..83a30fa 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2382,13 +2382,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);
>
> Yes, XEN is not supporting the stage 2 access flag but we should handle
> a stage 2 address size fault.

The function hpfar_is_valid indicates whether the register HPFAR is 
valid. If the function returns false, Xen will use the hardware do the 
translation.

It will only lead to a waste of cycle but this is fine as the address 
size fault is not a hot path for now.

> I think we should do some thing like to below to match ARM ARM.
>
> return s1ptw || (fsc != FSC_FLT_PERM);

This does not match the ARM ARM, with this check you consider that HPFAR 
will be valid for all the fault but permission ones which is not true.

I purposefully choose a white list because it is safer to use the 
hardware to do the translation more often than the invert.

So I don't see why we should handle stage 2 access flag with the current 
Xen. If you still disagree, please explain why with a concrete example.

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 5/6] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Shanker Donthineni]

On 08/17/2016 06:11 AM, Julien Grall wrote:
> On 17/08/16 03:19, Shanker Donthineni wrote:
>> Hi Julien,
>
> Hello Shanker,
>
>> On 07/27/2016 12:09 PM, 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>
>>>
>>> ---
>>>      Changes in v2:
>>>          - Use fsc in the switch in do_trap_data_abort_guest
>>> ---
>>>   xen/arch/arm/traps.c | 24 ++++++++++++++++++++----
>>>   1 file changed, 20 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index ea105f2..83a30fa 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -2382,13 +2382,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);
>>
>> Yes, XEN is not supporting the stage 2 access flag but we should handle
>> a stage 2 address size fault.
>
> The function hpfar_is_valid indicates whether the register HPFAR is 
> valid. If the function returns false, Xen will use the hardware do the 
> translation.
>
> It will only lead to a waste of cycle but this is fine as the address 
> size fault is not a hot path for now.
>
>> I think we should do some thing like to below to match ARM ARM.
>>
>> return s1ptw || (fsc != FSC_FLT_PERM);
>
> This does not match the ARM ARM, with this check you consider that 
> HPFAR will be valid for all the fault but permission ones which is not 
> true.
>
> I purposefully choose a white list because it is safer to use the 
> hardware to do the translation more often than the invert.
>
> So I don't see why we should handle stage 2 access flag with the 
> current Xen. If you still disagree, please explain why with a concrete 
> example.
>

Agree with you, I have suggested the above change because I saw the same 
check in Linux KVM.
As per ARM ARM, it should be 'return s1ptw || (fsc == FSC_FLT_TRANS) || 
(fsc == FSC_FLT_ACCESS) || (fsc == 0x00)';

> Regards,
>

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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

Re: [PATCH v2 5/6] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Julien Grall]

On 17/08/16 21:08, Shanker Donthineni wrote:
>
>
> On 08/17/2016 06:11 AM, Julien Grall wrote:
>> On 17/08/16 03:19, Shanker Donthineni wrote:
>>> Hi Julien,
>>
>> Hello Shanker,
>>
>>> On 07/27/2016 12:09 PM, 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>
>>>>
>>>> ---
>>>>      Changes in v2:
>>>>          - Use fsc in the switch in do_trap_data_abort_guest
>>>> ---
>>>>   xen/arch/arm/traps.c | 24 ++++++++++++++++++++----
>>>>   1 file changed, 20 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index ea105f2..83a30fa 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -2382,13 +2382,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);
>>>
>>> Yes, XEN is not supporting the stage 2 access flag but we should handle
>>> a stage 2 address size fault.
>>
>> The function hpfar_is_valid indicates whether the register HPFAR is
>> valid. If the function returns false, Xen will use the hardware do the
>> translation.
>>
>> It will only lead to a waste of cycle but this is fine as the address
>> size fault is not a hot path for now.
>>
>>> I think we should do some thing like to below to match ARM ARM.
>>>
>>> return s1ptw || (fsc != FSC_FLT_PERM);
>>
>> This does not match the ARM ARM, with this check you consider that
>> HPFAR will be valid for all the fault but permission ones which is not
>> true.
>>
>> I purposefully choose a white list because it is safer to use the
>> hardware to do the translation more often than the invert.
>>
>> So I don't see why we should handle stage 2 access flag with the
>> current Xen. If you still disagree, please explain why with a concrete
>> example.
>>
>
> Agree with you, I have suggested the above change because I saw the same
> check in Linux KVM.
> As per ARM ARM, it should be 'return s1ptw || (fsc == FSC_FLT_TRANS) ||
> (fsc == FSC_FLT_ACCESS) || (fsc == 0x00)';

Feel free to send a patch for this explaining why we would need to have 
the full check. But I don't think it is worth to test every case and 
therefore increasing the number of cycles of this function for faults we 
don't care so far.

Regards,

-- 
Julien Grall

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