Re: [PATCH v11 3/3] xen/arm64: io: Handle data abort due to cache maintenance instructions

[Ayan Kumar Halder <ayan.kumar.halder@xxxxxxxxxx>]

On 22/03/2022 12:06, Ayan Kumar Halder wrote:
> On 18/03/2022 18:26, Julien Grall wrote:
> > Hi Ayan,
> Hi Julien,
> > On 17/03/2022 14:00, Ayan Kumar Halder wrote:
> > > diff --git a/xen/arch/arm/include/asm/mmio.h 
> > > b/xen/arch/arm/include/asm/mmio.h
> > > index ca259a79c2..79e64d9af8 100644
> > > --- a/xen/arch/arm/include/asm/mmio.h
> > > +++ b/xen/arch/arm/include/asm/mmio.h
> > > @@ -35,6 +35,7 @@ enum instr_decode_state
> > >        * instruction.
> > >        */
> > >       INSTR_LDR_STR_POSTINDEXING,
> > > +    INSTR_CACHE,                    /* Cache Maintenance instr */
> > >   };
> > >     typedef struct
> > > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> > > index 6f458ee7fd..26c716b4a5 100644
> > > --- a/xen/arch/arm/io.c
> > > +++ b/xen/arch/arm/io.c
> > > @@ -139,6 +139,17 @@ void try_decode_instruction(const struct 
> > > cpu_user_regs *regs,
> > >           return;
> > >       }
> > >   +    /*
> > > +     * When the data abort is caused due to cache maintenance, Xen 
> > > should check
> > > +     * if the address belongs to an emulated MMIO region or not. 
> > > The behavior
> > > +     * will differ accordingly.
> > > +     */
> > > +    if ( info->dabt.cache )
> > > +    {
> > > +        info->dabt_instr.state = INSTR_CACHE;
> > > +        return;
> > > +    }
> > > +
> > >       /*
> > >        * Armv8 processor does not provide a valid syndrome for 
> > > decoding some
> > >        * instructions. So in order to process these instructions, 
> > > Xen must
> > > @@ -177,6 +188,13 @@ enum io_state try_handle_mmio(struct 
> > > cpu_user_regs *regs,
> > >           return rc;
> > >       }
> > >   +    /*
> > > +     * When the data abort is caused due to cache maintenance and 
> > > the address
> > > +     * belongs to an emulated region, Xen should ignore this 
> > > instruction.
> > > +     */
> > > +    if ( info->dabt_instr.state == INSTR_CACHE )
> > Reading the Arm Arm, the ISS should be invalid for cache 
> > instructions. So, I think the check at the beginning of 
> > try_handle_mmio() would prevent us to reach this check.
> > Can you check that cache instructions on emulated region will 
> > effectively be ignored?
> Yes, you are correct.
>
> I tested with the following (dis)assembly snippet :-
>
> 0x3001000 is the base address of GIC Distributor base.
>
>     __asm__ __volatile__("ldr x1, =0x3001000");
>     40000ca8:   58000301    ldr x1, 40000d08 <main+0x70>
>     __asm __volatile__("DC CVAU, x1");
>     40000cac:   d50b7b21    dc  cvau, x1
>
> This resulting in hitting the assertion :-
>
> (XEN) Assertion 'unreachable' failed at arch/arm/io.c:178
>
> I dumped the registers as follows, to determine that the fault is 
> caused by the instruction at 40000cac.
> HSR=0x00000092000147  regs->pc = 0x40000cac info.gpa = 0x3001000
>
>
> So, my patch needs to be modified as follows:-
>
> @@ -172,7 +173,7 @@ enum io_state try_handle_mmio(struct cpu_user_regs 
> *regs,
>      ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>
> -    if ( !info->dabt.valid )
> +    if ( !(info->dabt.valid || (info->dabt_instr.state == 
> INSTR_CACHE)) )
Actually this is not needed.

The following change is sufficient :-

@@ -146,7 +146,9 @@ void try_decode_instruction(const struct 
cpu_user_regs *regs,
      */
     if ( info->dabt.cache )
     {
         info->dabt_instr.state = INSTR_CACHE;
+        info->dabt.valid = 1;
         return;
     }

"info->dabt.valid == 1" means the instruction is valid or decoded 
successfully (this holds true for INSTR_CACHE as well).