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

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

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)) )
     {
         ASSERT_UNREACHABLE();
         return IO_ABORT;

I will send a v12 patch with this change.

- Ayan

> Cheers,