> -----Original Message----- > From: Julien Grall <[email protected]> > Sent: 24 September 2020 19:01 > To: Oleksandr Tyshchenko <[email protected]>; [email protected] > Cc: Oleksandr Tyshchenko <[email protected]>; Andrew Cooper > <[email protected]>; > George Dunlap <[email protected]>; Ian Jackson > <[email protected]>; Jan Beulich > <[email protected]>; Stefano Stabellini <[email protected]>; Wei Liu > <[email protected]>; Roger Pau > Monné <[email protected]>; Paul Durrant <[email protected]>; Jun Nakajima > <[email protected]>; > Kevin Tian <[email protected]>; Tim Deegan <[email protected]>; Julien Grall > <[email protected]> > 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/[email protected]/ > > > -- > Julien Grall
