Re: [PATCH for-4.5 v6 14/17] xen/arm: Instruction prefetch abort (X) mem_event handling

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

On Mon, 2014-09-15 at 16:02 +0200, 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>
> ---
> 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            | 39 ++++++++++++++++++++++++++++++++++++++-
>  xen/include/asm-arm/processor.h | 40 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 5bfcdf3..71c087f 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1828,7 +1828,44 @@ done:
>  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>                                        union hsr hsr)
>  {
> -    register_t addr = READ_SYSREG(FAR_EL2);
> +    struct hsr_iabt iabt = hsr.iabt;
> +    int rc;
> +    register_t addr;
> +    vaddr_t gva;
> +    paddr_t gpa;
> +
> +#ifdef CONFIG_ARM_32
> +    gva = READ_CP32(HIFAR);
> +#else
> +    gva = READ_SYSREG64(FAR_EL2);
> +#endif

You can just use READ_SYSREG(FAR_EL2) here and it will do the right
thing without the ifdef.
[...]
> +    addr = READ_SYSREG(FAR_EL2);

Like you do here ;-)
> index b844f1d..044de12 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -292,6 +292,36 @@ enum dabt_dfsc {
>      DABT_DFSC_TLB_CONFLICT      = 0b110000,
>  };
>  
> +/* Instruction abort instruction fault status codes */
> +enum iabt_ifsc {
> +    IABT_IFSC_ADDR_SIZE_0       = 0b000000,

Apart from the related comments on the last patch which mostly apply
here too, aren't these mostly common with the DABT codes? 

> @@ -371,10 +401,18 @@ 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;  /* Fault during a stage 1 translation table walk */
> +        unsigned long res1:1;
> +        unsigned long ea:1;     /* External abort type */

Please use eat for consistency here.

You should also include the common len/cc/etc bits and sufficient
padding that the whole thing adds up to 32-bits.

Ian.