Re: [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 4 Mar 2022, Julien Grall wrote:
> Hi Stefano,
> 
> On 04/03/2022 00:42, Stefano Stabellini wrote:
> > >   void register_mmio_handler(struct domain *d,
> > > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
> > > index 308650b400..58cd320b5a 100644
> > > --- a/xen/arch/arm/ioreq.c
> > > +++ b/xen/arch/arm/ioreq.c
> > > @@ -47,6 +47,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 dabt_instr instr = info->dabt_instr;
> > >       ioreq_t p = {
> > >           .type = IOREQ_TYPE_COPY,
> > >           .addr = info->gpa,
> > > @@ -76,10 +77,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs
> > > *regs,
> > >       if ( !s )
> > >           return IO_UNHANDLED;
> > >   -    if ( !info->dabt.valid )
> > > -        return IO_ABORT;
> > > +    ASSERT(dabt.valid);
> > 
> > I cannot see where we set dabt.valid on successfully decoding the
> > instruction. It looks like we don't? If we don't, then here the ASSERT
> > would fail in case of postindexing instructions, right?
> 
> We don't currently set dabt.valid. There are other reasons to set it (see my
> reply to Ayan). So...
> 
> > 
> > If we don't, then we should probably just get rid of this ASSERT: it is
> > not worth setting dabt.valid just so that this ASSERT would succeed.
> 
> ... I would keep the ASSERT.

OK