[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 02/11] vpci: cancel pending map/unmap on vpci removal
On 18.11.2021 10:32, Oleksandr Andrushchenko wrote: > > > On 18.11.21 11:15, Jan Beulich wrote: >> On 18.11.2021 09:54, Oleksandr Andrushchenko wrote: >>> On 18.11.21 10:36, Jan Beulich wrote: >>>> On 18.11.2021 08:49, Oleksandr Andrushchenko wrote: >>>>> On 17.11.21 10:28, Jan Beulich wrote: >>>>>> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote: >>>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx> >>>>>>> >>>>>>> When a vPCI is removed for a PCI device it is possible that we have >>>>>>> scheduled a delayed work for map/unmap operations for that device. >>>>>>> For example, the following scenario can illustrate the problem: >>>>>>> >>>>>>> pci_physdev_op >>>>>>> pci_add_device >>>>>>> init_bars -> modify_bars -> defer_map -> >>>>>>> raise_softirq(SCHEDULE_SOFTIRQ) >>>>>>> iommu_add_device <- FAILS >>>>>>> vpci_remove_device -> xfree(pdev->vpci) >>>>>>> >>>>>>> leave_hypervisor_to_guest >>>>>>> vpci_process_pending: v->vpci.mem != NULL; v->vpci.pdev->vpci == >>>>>>> NULL >>>>>>> >>>>>>> For the hardware domain we continue execution as the worse that >>>>>>> could happen is that MMIO mappings are left in place when the >>>>>>> device has been deassigned >>>>>>> >>>>>>> For unprivileged domains that get a failure in the middle of a vPCI >>>>>>> {un}map operation we need to destroy them, as we don't know in which >>>>>>> state the p2m is. This can only happen in vpci_process_pending for >>>>>>> DomUs as they won't be allowed to call pci_add_device. >>>>>>> >>>>>>> Signed-off-by: Oleksandr Andrushchenko >>>>>>> <oleksandr_andrushchenko@xxxxxxxx> >>>>>> Thinking about it some more, I'm not convinced any of this is really >>>>>> needed in the presented form. >>>>> The intention of this patch was to handle error conditions which are >>>>> abnormal, e.g. when iommu_add_device fails and we are in the middle >>>>> of initialization. So, I am trying to cancel all pending work which might >>>>> already be there and not to crash. >>>> Only Dom0 may be able to prematurely access the device during "add". >>>> Yet unlike for DomU-s we generally expect Dom0 to be well-behaved. >>>> Hence I'm not sure I see the need for dealing with these. >>> Probably I don't follow you here. The issue I am facing is Dom0 >>> related, e.g. Xen was not able to initialize during "add" and thus >>> wanted to clean up the leftovers. As the result the already >>> scheduled work crashes as it was not neither canceled nor interrupted >>> in some safe manner. So, this sounds like something we need to take >>> care of, thus this patch. >> But my point was the question of why there would be any pending work >> in the first place in this case, when we expect Dom0 to be well-behaved. > I am not saying Dom0 misbehaves here. This is my real use-case > (as in the commit message): > > pci_physdev_op > pci_add_device > init_bars -> modify_bars -> defer_map -> > raise_softirq(SCHEDULE_SOFTIRQ) > iommu_add_device <- FAILS > vpci_remove_device -> xfree(pdev->vpci) > > leave_hypervisor_to_guest > vpci_process_pending: v->vpci.mem != NULL; v->vpci.pdev->vpci == NULL First of all I'm sorry for having lost track of that particular case in the course of the discussion. I wonder though whether that's something we really need to take care of. At boot (on x86) modify_bars() wouldn't call defer_map() anyway, but use apply_map() instead. I wonder whether this wouldn't be appropriate generally in the context of init_bars() when used for Dom0 (not sure whether init_bars() would find some form of use for DomU-s as well). This is even more so as it would better be the exception that devices discovered post-boot start out with memory decoding enabled (which is a prereq for modify_bars() to be called in the first place). Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |