Re: [PATCH V14 5/7] xen/arm: Instruction prefetch abort (X) mem_access event handling

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-03-26 at 23:05 +0100, Tamas K Lengyel wrote:
> Add missing structure definition for iabt and update the trap handling
> mechanism to only inject the exception if the mem_access checker
> decides to do so.
> 
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by:  Julien Grall <julien.grall@linaro.org>
> ---
> v14: - Query HPFAR_EL2 when valid in iabt handler
>      - Flush TLB before doing GVA->IPA translation in iabt handler
> v10: - Minor comment fix for describing s1ptw.
> v8: - Revert to arch specific p2m_mem_access_check.
>     - Retire iabt_fsc enum and use FSC_FLT instead.
>     - Complete the struct definition of hsr_iabt.
> v7: - Use the new common mem_access_check.
> v6: - Make npfec a const.
> v4: - Don't mark instruction fetch violation as read violation.
>     - Use new struct npfec to pass violation info.
> v2: - Add definition for instruction abort instruction fetch status codes
>        (enum iabt_ifsc) and only call p2m_mem_access_check for traps triggered
>        for permission violations.
> ---
>  xen/arch/arm/traps.c            | 46 +++++++++++++++++++++++++++++++++++++++--
>  xen/include/asm-arm/processor.h | 11 ++++++++++
>  2 files changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index e02c848..be90c9e 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -40,6 +40,7 @@
>  #include <asm/psci.h>
>  #include <asm/mmio.h>
>  #include <asm/cpufeature.h>
> +#include <asm/flushtlb.h>
>  
>  #include "decode.h"
>  #include "vtimer.h"
> @@ -1961,8 +1962,49 @@ done:
>  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>                                        union hsr hsr)
>  {
> -    register_t addr = READ_SYSREG(FAR_EL2);
> -    inject_iabt_exception(regs, addr, hsr.len);
> +    struct hsr_iabt iabt = hsr.iabt;
> +    int rc;
> +    paddr_t gpa;
> +    register_t gva = READ_SYSREG(FAR_EL2);
> +
> +    if ( iabt.s1ptw )
> +        gpa = READ_SYSREG(HPFAR_EL2);
> +    else

Can you not avoid the else case entirely by extending the if to cover
the other cases where HPFAR is explicitly valid? I can't be bothered to
go look right now but IIRC it included at least stage 2 access
permissions related failures, which would cover more xenaccess
scenarios, no?

> +    {
> +        /*
> +         * Flush the TLB to make sure the DTLB is clear before
> +         * doing GVA->IPA translation. If we got here because of
> +         * an entry only present in the ITLB, this translation may
> +         * still be inaccurate.
> +         */
> +        flush_tlb_domain(current->domain);
> +
> +        rc = gva_to_ipa(gva, &gpa, GV2M_READ);
> +        if ( rc == -EFAULT )
> +            goto bad_insn_abort;
> +    }
> +
> +    switch ( iabt.ifsc & 0x3f )
> +    {
> +    case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
> +    {
> +        const struct npfec npfec = {
> +            .insn_fetch = 1,
> +            .gla_valid = 1,
> +            .kind = iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
> +        };
> +
> +        rc = p2m_mem_access_check(gpa, gva, npfec);
> +
> +        /* Trap was triggered by mem_access, work here is done */
> +        if ( !rc )
> +            return;
> +    }
> +    break;
> +    }
> +
> +bad_insn_abort:
> +    inject_iabt_exception(regs, gva, hsr.len);
>  }
>  
>  static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index cf7ab7c..db07fdd 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -438,6 +438,17 @@ union hsr {
>      } sysreg; /* HSR_EC_SYSREG */
>  #endif
>  
> +    struct hsr_iabt {
> +        unsigned long ifsc:6;  /* Instruction fault status code */
> +        unsigned long res0:1;
> +        unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
> +        unsigned long res1:1;
> +        unsigned long eat:1;   /* External abort type */
> +        unsigned long res2:15;
> +        unsigned long len:1;   /* Instruction length */
> +        unsigned long ec:6;    /* Exception Class */
> +    } iabt; /* HSR_EC_INSTR_ABORT_* */
> +
>      struct hsr_dabt {
>          unsigned long dfsc:6;  /* Data Fault Status Code */
>          unsigned long write:1; /* Write / not Read */