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

[Julien Grall <julien@xxxxxxx>]

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 :).

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 :).
Cheers,

--
Julien Grall