| Message ID | 1469639378-9244-6-git-send-email-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
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,
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,
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,
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: {
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(-)