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

[Oleksandr <olekstysh@xxxxxxxxx>]

Hi Julien

On 25.09.20 11:19, Paul Durrant wrote:
> > -----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
May I please clarify whether a concern still stands (with what was said 
above) and we need an additional synchronization on Arm?

--
Regards,

Oleksandr Tyshchenko