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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.06.2020 17:28, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 09 June 2020 16:08
>> To: paul@xxxxxxx
>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; 'Paul Durrant' <pdurrant@xxxxxxxxxx>; 
>> 'Marek Marczykowski-Górecki'
>> <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
>> 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@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.

Jan

>> 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
>