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.gr...@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
