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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.06.2020 17:44, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 09 June 2020 16:33
>>
>> On 09.06.2020 17:28, Paul Durrant wrote:
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: 09 June 2020 16:08
>>>>
>>>> On 08.06.2020 13:04, Paul Durrant wrote:
>>>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>> 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.
>>
>> But such wakeup is taken care of by "goto recheck", not by the main
>> loop in the function.
> 
> Oh yes, sorry, I misread. It would be nice to get rid of the goto.

I've done this in a pair of patches - first one doing as I suggested,
second one doing as you suggested: Getting rid of the label and
converting the fake loop put in place by the 1st one into a real loop
again. I think it's better in two steps. But this isn't for 4.14, so
I won't submit the pair right away.

Jan