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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>]

Hi, Julien!

On 03.09.21 12:04, Julien Grall wrote:
> Hi Oleksandr,
>
> On 03/09/2021 09:33, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> 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.
>
> IIUC, you are talking about the function map_range() in xen/drivers/vpci/header. The function has the following todo for Arm:
>
>         /*
>          * ARM TODOs:
>          * - On ARM whether the memory is prefetchable or not should be passed
>          *   to map_mmio_regions in order to decide which memory attributes
>          *   should be used.
>          *
>          * - {un}map_mmio_regions doesn't support preemption.
>          */
>
> This doesn't seem to be addressed in the two series for PCI passthrough sent so far. Do you have any plan to handle it?

No plan yet.

All the mappings are happening with p2m_mmio_direct_dev as of now.

>
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> ---
>>   xen/arch/arm/traps.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 219ab3c3fbde..1571fb8afd03 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -34,6 +34,7 @@
>>   #include <xen/symbols.h>
>>   #include <xen/version.h>
>>   #include <xen/virtual_region.h>
>> +#include <xen/vpci.h>
>>     #include <public/sched.h>
>>   #include <public/xen.h>
>> @@ -2304,6 +2305,11 @@ static bool check_for_vcpu_work(void)
>>       }
>>   #endif
>>   +    local_irq_enable();
>> +    if ( has_vpci(v->domain) && vpci_process_pending(v) )
>
> Looking at the code of vpci_process_pending(), it looks like there are some rework to do for guest. Do you plan to handle it as part of the vPCI series?
Yes, vPCI code is heavily touched to support guest non-identity mappings
>
>> +        raise_softirq(SCHEDULE_SOFTIRQ);
>> +    local_irq_disable();
>> +
>
> From my understanding of vcpi_process_pending(). The function will return true if there are more work to schedule.
Yes
> However, if check_for_vcpu_for_work() return false, then we will return to the guest before any work for vCPI has finished. This is because check_for_vcpu_work() will not be called again.
Correct
>
> In this case, I think you want to return as soon as you know we need to reschedule.
Not sure I understand this
>
> However, looking at the rest of the code, we already have a check for vpci in the common IOREQ code.

Which may not be enabled as it depends on CONFIG_IOREQ_SERVER.

My understanding is that for x86 it is always enabled, but this might not be the case for Arm

> So we would end up to call twice vpci_process_pending().
So, if CONFIG_IOREQ_SERVER is not enabled then we end up with only calling it from traps.c on Arm
> Maybe we should move the call from the IOREQ to arch-code.

Hm. I would better think of moving it from IOREQ to some other common code: for x86 (if

my understanding correct about CONFIG_IOREQ_SERVER) it is by coincidence that we call vPCI

code from there and IOREQ is always enabled.

>
> Cheers,
>
Thank you,

Oleksandr