[Xen-devel,09/27] xen/arm: traps: Don't define FAR_EL2 for ARM32

Message ID 20170814142418.13267-10-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: Memory subsystem clean-up
Related show

Commit Message

Julien Grall Aug. 14, 2017, 2:24 p.m.
Aliasing FAR_EL2 to HIFAR makes the code confusing because on ARMv8
FAR_EL2[31:0] is architecturally mapped to HDFAR and FAR_EL2[63:32] to
FAR_EL2. See D7.2.30 in ARM DDI 0487B.a. Open-code the alias instead.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/traps.c         | 8 +++++++-
 xen/include/asm-arm/cpregs.h | 1 -
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

Andre Przywara Aug. 22, 2017, 2:12 p.m. | #1
Hi,

On 14/08/17 15:24, Julien Grall wrote:
> Aliasing FAR_EL2 to HIFAR makes the code confusing because on ARMv8
> FAR_EL2[31:0] is architecturally mapped to HDFAR and FAR_EL2[63:32] to
> FAR_EL2.
  ^^^^^^^
I guess you mean HIFAR here.
Otherwise the patch makes sense.

> See D7.2.30 in ARM DDI 0487B.a. Open-code the alias instead.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  xen/arch/arm/traps.c         | 8 +++++++-
>  xen/include/asm-arm/cpregs.h | 1 -
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index c07999b518..498d8c594a 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2560,11 +2560,17 @@ 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);
> +    register_t gva;
>      uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
>      paddr_t gpa;
>      mfn_t mfn;
>  
> +#ifdef CONFIG_ARM_32
> +    gva = READ_CP32(HIFAR);
> +#else
> +    gva = READ_SYSREG64(FAR_EL2);
> +#endif
> +
>      /*
>       * If this bit has been set, it means that this instruction abort is caused
>       * by a guest external abort. We can handle this instruction abort as guest
> diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
> index af45ec7a65..1889d7cbfb 100644
> --- a/xen/include/asm-arm/cpregs.h
> +++ b/xen/include/asm-arm/cpregs.h
> @@ -307,7 +307,6 @@
>  #define ESR_EL1                 DFSR
>  #define ESR_EL2                 HSR
>  #define FAR_EL1                 HIFAR
> -#define FAR_EL2                 HIFAR
>  #define HCR_EL2                 HCR
>  #define HPFAR_EL2               HPFAR
>  #define HSTR_EL2                HSTR
>
Julien Grall Aug. 23, 2017, 7:05 p.m. | #2
On 08/22/2017 03:12 PM, Andre Przywara wrote:
> Hi,

Hi Andre,

> On 14/08/17 15:24, Julien Grall wrote:
>> Aliasing FAR_EL2 to HIFAR makes the code confusing because on ARMv8
>> FAR_EL2[31:0] is architecturally mapped to HDFAR and FAR_EL2[63:32] to
>> FAR_EL2.
>    ^^^^^^^
> I guess you mean HIFAR here.
> Otherwise the patch makes sense.
Whoops yes. I will fix it in the next version.

Cheers,

Patch

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index c07999b518..498d8c594a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2560,11 +2560,17 @@  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);
+    register_t gva;
     uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
     paddr_t gpa;
     mfn_t mfn;
 
+#ifdef CONFIG_ARM_32
+    gva = READ_CP32(HIFAR);
+#else
+    gva = READ_SYSREG64(FAR_EL2);
+#endif
+
     /*
      * If this bit has been set, it means that this instruction abort is caused
      * by a guest external abort. We can handle this instruction abort as guest
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index af45ec7a65..1889d7cbfb 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -307,7 +307,6 @@ 
 #define ESR_EL1                 DFSR
 #define ESR_EL2                 HSR
 #define FAR_EL1                 HIFAR
-#define FAR_EL2                 HIFAR
 #define HCR_EL2                 HCR
 #define HPFAR_EL2               HPFAR
 #define HSTR_EL2                HSTR