Re: [Xen-devel] [PATCH 8/9] xen/arm: traps: Avoid unnecessary VA -> IPA translation in abort handlers

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxx>
>
>  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
> 

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