[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 11/11] xen/arm: Process pending vPCI map/unmap operations
On 06.09.21 12:53, Julien Grall wrote: > Hi Oleksandr, > > On 06/09/2021 10:14, Oleksandr Andrushchenko wrote: >> >> On 06.09.21 11:48, Julien Grall wrote: >>> On 06/09/2021 08:02, Oleksandr Andrushchenko wrote: >>>> Hi, Julien! >>> >>> Hi Oleksandr, >>> >>>> 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@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. >>>>> >>>>> 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. >>> >>> So this address the first TODO but how about the second TODO? It refers to >>> the lack of preemption on Arm but in this patch you suggest there are some >>> and hence we need to call vpci_process_pending(). >>> >>> For a tech preview, the lack of preemption would be OK. However, the commit >>> message should be updated to make clear there are no such preemption yet or >>> avoid to mention it. >> >> Well, the comment was not added by me (by Roger I guess), I just keep it. > > I don't think it matters to know who added it. What matters is when those > comments are going to be handled. If they are already handled, then they > should be dropped. > > If they are not, the two TODOs listed above are probably OK to defer as you > only plan a tech preview. But they would need to be handled before vCPI is > selected by default and used in production. > > Note that I specifically wrote "the two TODOs listed above" because I haven't > looked at the other TODOs/FIXMEs and figued out they are fine to defer. Ok, then I leave the TODOs as they are > >> >> As to the preemption both map and unmap are happening via >> vpci_process_pending, so > > Right... this doesn't mean preemption is actually supported on Arm. > vpci_process_pending() doesn't do the preemption itself. It relies on > map_range() to do it. > > But even map_range() relies on the arch specific helper > {,un}map_mmio_regions() to do it. If you look at the x86 implementation they > are adding at max MAX_MMIO_MAX_ITER entries per call. On Arm, there are not > such limit. Therefore the function will always do the full {,un}mapping > before returning. IOW there are no preemption supported. Ok > >> >> what is true for map is also true for unmap with this respect >> >>> >>>> >>>>> >>>>>> >>>>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx> >>>>>> --- >>>>>> 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 >>> >>> I wasn't referring to the non-identity mappings here. I was referring to >>> TODOs such as: >>> >>> /* >>> * FIXME: in case of failure remove the device from the domain. >>> * Note that there might still be leftover mappings. While >>> this is >>> * safe for Dom0, for DomUs the domain will likely need to be >>> * killed in order to avoid leaking stale p2m mappings on >>> * failure. >>> */ >>> >>> You still have them after the series reworking the vPCI. As for the >>> preemption this is OK to ignore it for a tech preview. Although, we want to >>> at least track them. >> Please see above: both map and unmap are happening via vpci_process_pending > > I am not sure how this is relevant to what I just mentionned. > >>> >>>>> >>>>>> + 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 >>> >> I was more referring to "I think you want to return as soon as you know we >> need to reschedule." >>> The return value of check_for_vcpu_for_work() indicates whether we have >>> more work to do before returning to return to the guest. >>> >>> When vpci_process_pending() returns true, it tells us we need to call the >>> function at least one more time before returning to the guest. >>> >>> In your current implementation, you leave that decision to whoeever is next >>> in the function. >>> >>> It is not safe to return to the guest as long as vpci_process_pending() >>> returns true. So you want to write something like: >>> >>> if ( vpci_process_pending() ) >>> return true; >> --- a/xen/arch/arm/traps.c >> >> +++ b/xen/arch/arm/traps.c >> @@ -2291,6 +2291,9 @@ static bool check_for_vcpu_work(void) >> { >> struct vcpu *v = current; >> >> + if ( vpci_process_pending() ) >> + return true; >> + >> #ifdef CONFIG_IOREQ_SERVER >> if ( domain_has_ioreq_server(v->domain) ) >> { >> Do you mean something like this? > > Yes. Ok, I'll add this check > >>>>> 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. >>> >>> Right. My point is when CONFIG_IOREQ_SERVER is set then you would end up to >>> call twice vpci_process_pending(). This will have an impact how on long >>> your vCPU is going to running because you are doubling the work. >> >> So, you suggest that we have in the common IOREQ code something call like >> >> arch_vpci_process_pending? In case of x86 it will have the code currently >> found in the >> >> common IOREQ sources and for Arm it will be nop? > > No I am suggesting to move the call of the IOREQ code to hvm_do_resume() (on > x86) and check_for_vcpu_work() (on Arm). Ok, I can move vPCI code to hvm_do_resume, but vPCI is only used for x86 PVH Dom0. Do you still think hvm_do_resume is the right place? > > Cheers, > Thanks, Oleksandr
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |