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

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

Hi Julien,

On 22/03/2022 13:22, Julien Grall wrote:
> Hi Ayan,
>
> On 22/03/2022 12:38, Ayan Kumar Halder wrote:
> > 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;
> To me, 'info->dabt.valid' indicates whether the syndrome is valid. We 
> set to 1 for emulated instruction because the syndrome will be updated.
> But this is not the case for the cache instructions. So I would prefer 
> if it is kept as 0 and use your previous suggestion.
> Furthermore, I think try_fwd_ioserv() need to be adapted because the 
> function will use the fields SAS and SRT. From the Arm Arm they are 
> RES0, so while they are 0 today, we should not rely on this.
> Therefore, to be fully compliant with the Arm, we want to reorder a 
> bit the code:
>  * The field data could be set past ioreq_select_server().
>  * The field size should be set to the cache line size.
I am assuming that we need to invoke  dcache_line_size() (from 
xen/arch/arm/arm64/cache.S ) to get the cache line size.
I think the cache line may be 32 or 64 bytes. In which case, this cannot 
be represented by SAS (as it can represent 1, 2, 4 and 8 bytes).
Also, we are invoking ioreq_select_server() to determine if the address 
is emulated or not. So, can we use an assumed size (= 1 byte) ?
If it is emulated, Xen will ignore the instruction. If it is not 
emulated, Xen will forward the abort to the guest.
Thus, Xen will never execute the instruction. So the correctness of the 
size should not matter here.
- Ayan

> Cheers,