RE: [PATCH-for-4.14] ioreq: handle pending emulation racing with ioreq server destruction

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 09 June 2020 16:08
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Marek Marczykowski-Górecki'
> <marmarek@invisiblethingslab.com>
> Subject: Re: [PATCH-for-4.14] ioreq: handle pending emulation racing with ioreq server destruction
> 
> On 08.06.2020 13:04, Paul Durrant wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 08 June 2020 11:58
> >>
> >> On 08.06.2020 11:46, Paul Durrant wrote:
> >>> --- a/xen/arch/x86/hvm/ioreq.c
> >>> +++ b/xen/arch/x86/hvm/ioreq.c
> >>> @@ -109,15 +109,7 @@ static void hvm_io_assist(struct hvm_ioreq_vcpu *sv, uint64_t data)
> >>>      ioreq_t *ioreq = &v->arch.hvm.hvm_io.io_req;
> >>>
> >>>      if ( hvm_ioreq_needs_completion(ioreq) )
> >>> -    {
> >>> -        ioreq->state = STATE_IORESP_READY;
> >>>          ioreq->data = data;
> >>> -    }
> >>> -    else
> >>> -        ioreq->state = STATE_IOREQ_NONE;
> >>> -
> >>> -    msix_write_completion(v);
> >>> -    vcpu_end_shutdown_deferral(v);
> >>>
> >>>      sv->pending = false;
> >>>  }
> >>
> >> With this, is the function worth keeping at all?
> >
> > I did think about that, but it is called in more than one place. So,
> > in the interest of trying to make back-porting straightforward, I
> > thought it best to keep it simple.
> 
> Fair enough, but going forward I still think it would be nice to get
> rid of the function. To do this sufficiently cleanly, the main
> question I have is: Why is hvm_wait_for_io() implemented as a loop?

I guess the idea is that it should tolerate the emulator kicking the event channel spuriously. I don't know whether this was the case at one time, but it seems reasonable to be robust against waking up from wait_on_xen_event_channel() before state has made it to IORESP_READY.

> Hasn't this become pointless with the XSA-262 fix? Switching this to
> a do {} while(false) construct (seeing that the only caller has
> already checked sv->pending) would look to allow moving what is now
> in hvm_io_assist() immediately past this construct, at the expense
> of a local variable holding ~0ul initially and then in the common
> case p->data.

That sounds ok. We can do that even with the current while loop by just setting sv->pending to false when we see state == IORESP_READY. After the loop terminates then we can grab the result and set state back to IOREQ_NONE.

> 
> Thoughts? (I'll be happy to make such a patch, I'm just checking
> whether I'm overlooking something crucial.)
> 

Only that I don't think we should use do {} while(false) just in case of early wakeup.

  Paul

> Jan