Re: [XEN v10 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions

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

Hi Julien,

On 11/03/2022 18:34, Julien Grall wrote:
> Hi,
>
> On 10/03/2022 17:45, Ayan Kumar Halder wrote:
> > When the data abort is caused due to cache maintenance for an address,
> > there are three scenarios:-
> >
> > 1. Address belonging to a non emulated region - For this, Xen should
> > set the corresponding bit in the translation table entry to valid and
> > return to the guest to retry the instruction. This can happen sometimes
> > as Xen need to set the translation table entry to invalid. (for eg
> > 'Break-Before-Make' sequence). Xen returns to the guest to retry the
> > instruction.
> >
> > 2. Address belongs to an emulated region - Xen should ignore the
> > instruction (ie increment the PC) and return to the guest.
> >
> > 3. Address is invalid - Xen should forward the data abort to the guest.
> >
> > Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxxxxx>
> > ---
> >
> > Changelog:-
> >
> > v1...v8 - NA
> >
> > v9 - Extracted this change from "[XEN v7 2/2] xen/arm64: io: Support
> > instructions (for which ISS is not ..." into a separate patch of its
> > own. The reason being this addresses an existing bug in the codebase.
> >
> > v10 - 1. To check if the address belongs to an emulated region, one
> > needs to check if it has a mmio handler or an ioreq server. In this
> > case, Xen should increment the PC
> > 2. If the address is invalid (niether emulated MMIO nor the translation
> > could be resolved via p2m or mapping the MMIO region), then Xen should
> > forward the abort to the guest.
> >
> >   xen/arch/arm/include/asm/mmio.h |  1 +
> >   xen/arch/arm/io.c               | 20 ++++++++++++++++++++
> >   xen/arch/arm/ioreq.c            | 15 ++++++++++++++-
> >   3 files changed, 35 insertions(+), 1 deletion(-)
> >
> > 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 e6c77e16bf..c5b2980a5f 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,15 @@ 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 )
> > +    {
> > +        return IO_HANDLED;
> > +    }
> > +
> >       /*
> >        * At this point, we know that the instruction is either valid 
> > or has been
> >        * decoded successfully. Thus, Xen should be allowed to execute 
> > the
> > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
> > index cc9bf23213..0dd2d452f7 100644
> > --- a/xen/arch/arm/ioreq.c
> > +++ b/xen/arch/arm/ioreq.c
> > @@ -29,10 +29,14 @@ enum io_state handle_ioserv(struct cpu_user_regs 
> > *regs, struct vcpu *v)
> >       const struct hsr_dabt dabt = hsr.dabt;
> >       /* Code is similar to handle_read */
> >       register_t r = v->io.req.data;
> > +    const struct instr_details instr = v->io.info.dabt_instr;
> >         /* We are done with the IO */
> >       v->io.req.state = STATE_IOREQ_NONE;
> >   +    if ( instr.state == INSTR_CACHE )
> > +        return IO_HANDLED;
> The request will not be forwarded to the IOREQ, so why do we need 
> check instr.state here?
I think it is not needed for the following reason.

leave_hypervisor_to_guest() ---> check_for_vcpu_work() --> 
vcpu_ioreq_handle_completion() --> get_pending_vcpu(v, &s) will return 
NULL. Is my understanding correct ?
- Ayan

> > +
> >       if ( dabt.write )
> >           return IO_HANDLED;
> >   @@ -47,7 +51,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs 
> > *regs,
> >                                struct vcpu *v, mmio_info_t *info)
> >   {
> >       struct vcpu_io *vio = &v->io;
> > -    struct instr_details instr = info->dabt_instr;
> > +    const struct instr_details instr = info->dabt_instr; >       
> > struct hsr_dabt dabt = info->dabt;
> >       ioreq_t p = {
> >           .type = IOREQ_TYPE_COPY,
> > @@ -78,6 +82,15 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs 
> > *regs,
> >       if ( !s )
> >           return IO_UNHANDLED;
> >   +    /*
> > +     * When the data abort is caused due to cache maintenance and 
> > the address
> > +     * belongs to an emulated region, Xen should ignore this 
> > instruction.
> > +     */
> > +    if ( instr.state == INSTR_CACHE )
> > +    {
> > +        return IO_HANDLED;
> > +    }
> > +
> >       ASSERT(dabt.valid);
> >         vio->req = p;
> Cheers,