> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: 24 September 2020 19:01
> To: Oleksandr Tyshchenko <olekst...@gmail.com>; xen-devel@lists.xenproject.org
> Cc: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>; Andrew Cooper
> <andrew.coop...@citrix.com>;
> George Dunlap <george.dun...@citrix.com>; Ian Jackson
> <ian.jack...@eu.citrix.com>; Jan Beulich
> <jbeul...@suse.com>; Stefano Stabellini <sstabell...@kernel.org>; Wei Liu
> <w...@xen.org>; Roger Pau
> Monné <roger....@citrix.com>; Paul Durrant <p...@xen.org>; Jun Nakajima
> <jun.nakaj...@intel.com>;
> Kevin Tian <kevin.t...@intel.com>; Tim Deegan <t...@xen.org>; Julien Grall
> <julien.gr...@arm.com>
> 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-2b99dabe1...@xen.org/
>
>
> --
> Julien Grall
