| Message ID | 1466601669-25398-9-git-send-email-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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,
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
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 {
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(-)