Re: [PATCH v2 5/6] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Shanker Donthineni <shankerd@codeaurora.org>]

Hi Julien,

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.
I think we should do some thing like to below to match ARM ARM.

return s1ptw || (fsc != FSC_FLT_PERM);


> +}
> +
>   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:
>       {

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel