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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 21 Jan 2021, Oleksandr wrote:
> On 20.01.21 21:47, Stefano Stabellini wrote:
> > On Wed, 20 Jan 2021, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 20/01/2021 00:50, Stefano Stabellini wrote:
> > > > On Tue, 19 Jan 2021, Oleksandr wrote:
> > > > > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
> > > > > index 40b9e59..0508bd8 100644
> > > > > --- a/xen/arch/arm/ioreq.c
> > > > > +++ b/xen/arch/arm/ioreq.c
> > > > > @@ -101,12 +101,10 @@ enum io_state try_fwd_ioserv(struct
> > > > > cpu_user_regs
> > > > > *regs,
> > > > > 
> > > > >    bool arch_ioreq_complete_mmio(void)
> > > > >    {
> > > > > -    struct vcpu *v = current;
> > > > >        struct cpu_user_regs *regs = guest_cpu_user_regs();
> > > > >        const union hsr hsr = { .bits = regs->hsr };
> > > > > -    paddr_t addr = v->io.req.addr;
> > > > > 
> > > > > -    if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED )
> > > > > +    if ( handle_ioserv(regs, current) == IO_HANDLED )
> > > > >        {
> > > > >            advance_pc(regs, hsr);
> > > > >            return true;
> > > > Yes, but I think we want to keep the check
> > > > 
> > > >       vio->req.state == STATE_IORESP_READY
> > > > 
> > > > So maybe (uncompiled, untested):
> > > > 
> > > >       if ( v->io.req.state != STATE_IORESP_READY )
> > > >           return false;
> > > Is it possible to reach this function with v->io.req.state !=
> > > STATE_IORESP_READY? If not, then I would suggest to add an
> > > ASSERT_UNREACHABLE() before the return.
> > If I am reading the state machine right it should *not* be possible to
> > get here with v->io.req.state != STATE_IORESP_READY, so yes,
> > ASSERT_UNREACHABLE() would work.
> Agree here. If the assumption is not correct (unlikely), I think I will catch
> this during testing.
> In addition, we can probably drop case STATE_IORESP_READY in try_fwd_ioserv().
> 
> 
> [not tested]

Yes, looks OK

 
> diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
> index 40b9e59..c7ee1a7 100644
> --- a/xen/arch/arm/ioreq.c
> +++ b/xen/arch/arm/ioreq.c
> @@ -71,9 +71,6 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
>      case STATE_IOREQ_NONE:
>          break;
> 
> -    case STATE_IORESP_READY:
> -        return IO_HANDLED;
> -
>      default:
>          gdprintk(XENLOG_ERR, "wrong state %u\n", vio->req.state);
>          return IO_ABORT;
> @@ -104,9 +101,14 @@ bool arch_ioreq_complete_mmio(void)
>      struct vcpu *v = current;
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
>      const union hsr hsr = { .bits = regs->hsr };
> -    paddr_t addr = v->io.req.addr;
> 
> -    if ( try_handle_mmio(regs, hsr, addr) == IO_HANDLED )
> +    if ( v->io.req.state != STATE_IORESP_READY )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return false;
> +    }
> +
> +    if ( handle_ioserv(regs, v) == IO_HANDLED )
>      {
>          advance_pc(regs, hsr);
>          return true;