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

[Julien Grall <julien@xxxxxxx>]

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.

Cheers,

--
Julien Grall