Re: [PATCH v2 11/11] xen/arm: Process pending vPCI map/unmap operations

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 27.09.21 11:06, Jan Beulich wrote:
> + Paul (retaining full context for this reason)
>
> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> vPCI may map and unmap PCI device memory (BARs) being passed through which
>> may take a lot of time. For this those operations may be deferred to be
>> performed later, so that they can be safely preempted.
>> Run the corresponding vPCI code while switching a vCPU.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> ---
>> Since v1:
>>   - Moved the check for pending vpci work from the common IOREQ code
>>     to hvm_do_resume on x86
> While perhaps obvious for Arm folks, I'd like to see the reason for this
> spelled out in the description.

I will add:

IOREQ is not enabled for Arm by default, so pending vPCI work has

no chance to be executed if the processing is left as is in the common IOREQ

code only. For that reason Arm processes that in arch specific code which 
results

in that the processing happens twice for Arm when IOREQ is enabled.

For x86, processing of vPCI in IOREQ code also doesn't seem to be the right

place, so move that to hvm_do_resume.

>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -549,6 +549,12 @@ void hvm_do_resume(struct vcpu *v)
>>       if ( !vcpu_ioreq_handle_completion(v) )
>>           return;
>>   
>> +    if ( has_vpci(v->domain) && vpci_process_pending(v) )
>> +    {
>> +        raise_softirq(SCHEDULE_SOFTIRQ);
>> +        return;
>> +    }
> Note that you're altering behavior here: Originally this was done ...
>
>> @@ -212,19 +210,12 @@ static bool wait_for_io(struct ioreq_vcpu *sv, ioreq_t 
>> *p)
>>   
>>   bool vcpu_ioreq_handle_completion(struct vcpu *v)
>>   {
>> -    struct domain *d = v->domain;
>>       struct vcpu_io *vio = &v->io;
>>       struct ioreq_server *s;
>>       struct ioreq_vcpu *sv;
>>       enum vio_completion completion;
>>       bool res = true;
>>   
>> -    if ( has_vpci(d) && vpci_process_pending(v) )
>> -    {
>> -        raise_softirq(SCHEDULE_SOFTIRQ);
>> -        return false;
>> -    }
> ... first thing. And I think it wants (perhaps even needs) to remain
> that way;
Will make it the first operation
>   otherwise you'll need to explain why not, and why the change
> is correct / safe.
>
> Jan
>
Thank you,

Oleksandr