Re: [PATCH V5 14/22] arm/ioreq: Introduce arch specific bits for IOREQ/DM features

[Oleksandr <olekstysh@xxxxxxxxx>]

On 27.01.21 20:33, Julien Grall wrote:

Hi Julien

> Hi,
>
> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
> > +enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
> > +                             struct vcpu *v, mmio_info_t *info)
> > +{
> > +    struct vcpu_io *vio = &v->io;
> > +    ioreq_t p = {
> > +        .type = IOREQ_TYPE_COPY,
> > +        .addr = info->gpa,
> > +        .size = 1 << info->dabt.size,
> > +        .count = 1,
> > +        .dir = !info->dabt.write,
> > +        /*
> > +         * On x86, df is used by 'rep' instruction to tell the 
> > direction
> > +         * to iterate (forward or backward).
> > +         * On Arm, all the accesses to MMIO region will do a single
> > +         * memory access. So for now, we can safely always set to 0.
> > +         */
> > +        .df = 0,
> > +        .data = get_user_reg(regs, info->dabt.reg),
> > +        .state = STATE_IOREQ_READY,
> > +    };
> > +    struct ioreq_server *s = NULL;
> > +    enum io_state rc;
> > +
> > +    switch ( vio->req.state )
> > +    {
> > +    case STATE_IOREQ_NONE:
> > +        break;
> > +
> > +    default:
> > +        gdprintk(XENLOG_ERR, "wrong state %u\n", vio->req.state);
> > +        return IO_ABORT;
> > +    }
> NIT: Given there is only one case, I think this can become a:
>
> if ( vio->req.state != STATE_IOREQ_NONE )
> {
>   gdprintk(...);
>   return IO_ABORT;
> }
>
> It is up to you whether you want to fix it now, later, or never :).
V6 is planned, so will fix)


> Aside the discussion about dm.h, the rest of the code looks good to 
> me. It is nice to see the arch part of IOREQ is small :).
I do agree, it is much smaller than it was for RFC series)

Yes, I will add required changes to dm.h in the patch which introduces 
that header, but ...

>>> So I think we may be able to drop the include from asm/hvm/domain.h 
(this would avoid to include it everywhere...).
I have tried that, but other CUs use definitions from 
public/hvm/dm_op.h, for example:
p2m-pt.c: In function 'p2m_type_to_flags':
p2m-pt.c:87:33: error: 'XEN_DMOP_IOREQ_MEM_ACCESS_WRITE' undeclared 
(first use in this function)
         if ( p2m->ioreq.flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
                                 ^
So, I would prefer to leave it as is, please let me know if you think 
otherwise.

> Cheers,
--
Regards,

Oleksandr Tyshchenko