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

[Julien Grall <julien@xxxxxxx>]

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.

Cheers,

--
Julien Grall