RE: [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 24 September 2020 19:01
> To: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>; Andrew Cooper 
> <andrew.cooper3@xxxxxxxxxx>;
> George Dunlap <george.dunlap@xxxxxxxxxx>; Ian Jackson 
> <ian.jackson@xxxxxxxxxxxxx>; Jan Beulich
> <jbeulich@xxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu 
> <wl@xxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Paul Durrant <paul@xxxxxxx>; Jun Nakajima 
> <jun.nakajima@xxxxxxxxx>;
> Kevin Tian <kevin.tian@xxxxxxxxx>; Tim Deegan <tim@xxxxxxx>; Julien Grall 
> <julien.grall@xxxxxxx>
> Subject: Re: [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common
> 
> 
> 
> On 10/09/2020 21:21, Oleksandr Tyshchenko wrote:
> > +static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p)
> > +{
> > +    unsigned int prev_state = STATE_IOREQ_NONE;
> > +    unsigned int state = p->state;
> > +    uint64_t data = ~0;
> > +
> > +    smp_rmb();
> > +
> > +    /*
> > +     * The only reason we should see this condition be false is when an
> > +     * emulator dying races with I/O being requested.
> > +     */
> > +    while ( likely(state != STATE_IOREQ_NONE) )
> > +    {
> > +        if ( unlikely(state < prev_state) )
> > +        {
> > +            gdprintk(XENLOG_ERR, "Weird HVM ioreq state transition %u -> 
> > %u\n",
> > +                     prev_state, state);
> > +            sv->pending = false;
> > +            domain_crash(sv->vcpu->domain);
> > +            return false; /* bail */
> > +        }
> > +
> > +        switch ( prev_state = state )
> > +        {
> > +        case STATE_IORESP_READY: /* IORESP_READY -> NONE */
> > +            p->state = STATE_IOREQ_NONE;
> > +            data = p->data;
> > +            break;
> > +
> > +        case STATE_IOREQ_READY:  /* IOREQ_{READY,INPROCESS} -> 
> > IORESP_READY */
> > +        case STATE_IOREQ_INPROCESS:
> > +            wait_on_xen_event_channel(sv->ioreq_evtchn,
> > +                                      ({ state = p->state;
> > +                                         smp_rmb();
> > +                                         state != prev_state; }));
> > +            continue;
> 
> As I pointed out previously [1], this helper was implemented with the
> expectation that wait_on_xen_event_channel() will not return if the vCPU
> got rescheduled.
> 
> However, this assumption doesn't hold on Arm.
> 
> I can see two solution:
>     1) Re-execute the caller
>     2) Prevent an IOREQ to disappear until the loop finish.
> 
> @Paul any opinions?

The ioreq control plane is largely predicated on there being no pending I/O 
when the state of a server is modified, and it is assumed that domain_pause() 
is sufficient to achieve this. If that assumption doesn't hold then we need 
additional synchronization.

  Paul

> 
> Cheers,
> 
> [1]
> https://lore.kernel.org/xen-devel/6bfc3920-8f29-188c-cff4-2b99dabe166f@xxxxxxx/
> 
> 
> --
> Julien Grall