[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 15.11.21 18:56, 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 > Is continuing safe in this case? I.e. isn't there the risk of a NULL > deref? I think it is safe to continue > >> 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. > You saying "we need to destroy them" made me look for a new domain_crash() > that you add, but there is none. What is this about? Yes, I guess we need to implicitly destroy the domain, @Roger are you ok with that? > >> @@ -165,6 +164,18 @@ bool vpci_process_pending(struct vcpu *v) >> return false; >> } >> >> +void vpci_cancel_pending(const struct pci_dev *pdev) >> +{ >> + struct vcpu *v = current; >> + >> + /* Cancel any pending work now. */ > Doesn't "any" include pending work on all vCPU-s of the guest, not > just current? Is current even relevant (as in: part of the correct > domain), considering ... > >> --- a/xen/drivers/vpci/vpci.c >> +++ b/xen/drivers/vpci/vpci.c >> @@ -51,6 +51,8 @@ void vpci_remove_device(struct pci_dev *pdev) >> xfree(r); >> } >> spin_unlock(&pdev->vpci->lock); >> + >> + vpci_cancel_pending(pdev); > ... this code path, when coming here from pci_{add,remove}_device()? > > I can agree that there's a problem here, but I think you need to > properly (i.e. in a race free manner) drain pending work. Yes, the code is inconsistent with this respect. I am thinking about: void vpci_cancel_pending(const struct pci_dev *pdev) { struct domain *d = pdev->domain; struct vcpu *v; /* Cancel any pending work now. */ domain_lock(d); for_each_vcpu ( d, v ) { vcpu_pause(v); if ( v->vpci.mem && v->vpci.pdev == pdev) { rangeset_destroy(v->vpci.mem); v->vpci.mem = NULL; } vcpu_unpause(v); } domain_unlock(d); } which seems to solve all the concerns. Is this what you mean? > > Jan > Thank you, Oleksandr
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |