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

From: Paul Durrant <xadimgnik@gmail.com>
To: "'Julien Grall'" <julien@xen.org>,
	"'Oleksandr Tyshchenko'" <olekstysh@gmail.com>,
	<xen-devel@lists.xenproject.org>
Cc: "'Oleksandr Tyshchenko'" <oleksandr_tyshchenko@epam.com>,
	"'Andrew Cooper'" <andrew.cooper3@citrix.com>,
	"'George Dunlap'" <george.dunlap@citrix.com>,
	"'Ian Jackson'" <ian.jackson@eu.citrix.com>,
	"'Jan Beulich'" <jbeulich@suse.com>,
	"'Stefano Stabellini'" <sstabellini@kernel.org>,
	"'Wei Liu'" <wl@xen.org>,
	"'Roger Pau Monné'" <roger.pau@citrix.com>,
	"'Jun Nakajima'" <jun.nakajima@intel.com>,
	"'Kevin Tian'" <kevin.tian@intel.com>,
	"'Tim Deegan'" <tim@xen.org>,
	"'Julien Grall'" <julien.grall@arm.com>
Subject: RE: [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common
Date: Fri, 25 Sep 2020 09:19:29 +0100	[thread overview]
Message-ID: <000201d69314$97bd8fa0$c738aee0$@xen.org> (raw)
In-Reply-To: <3997a705-ccb1-4b8f-41ca-c5507360c759@xen.org>

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 24 September 2020 19:01
> To: Oleksandr Tyshchenko <olekstysh@gmail.com>; xen-devel@lists.xenproject.org
> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> George Dunlap <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Roger Pau
> Monné <roger.pau@citrix.com>; Paul Durrant <paul@xen.org>; Jun Nakajima <jun.nakajima@intel.com>;
> Kevin Tian <kevin.tian@intel.com>; Tim Deegan <tim@xen.org>; Julien Grall <julien.grall@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-2b99dabe166f@xen.org/
> 
> 
> --
> Julien Grall