| Message ID | 20170814142418.13267-10-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Memory subsystem clean-up | expand |
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 >
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,
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
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(-)