[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.13 v2] passthrough: simplify locking and logging
> -----Original Message----- > From: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx> > Sent: 01 November 2019 19:28 > To: xen-devel@xxxxxxxxxxxxxxxxxxxx > Cc: Durrant, Paul <pdurrant@xxxxxxxxxx>; jbeulich@xxxxxxxx; > jgross@xxxxxxxx > Subject: [PATCH for-4.13 v2] passthrough: simplify locking and logging > > From: Paul Durrant <pdurrant@xxxxxxxxxx> > > Dropping the pcidevs lock between calling device_assigned() and > assign_device() means that the latter has to do the same check as the > former for no obvious gain. Also, since long running operations under > pcidevs lock already drop the lock and return -ERESTART periodically there > is little point in immediately failing an assignment operation with > -ERESTART just because the pcidevs lock could not be acquired (for the > second time, having already blocked on acquiring the lock in > device_assigned()). > > This patch instead acquires the lock once for assignment (or test assign) > operations directly in iommu_do_pci_domctl() and thus can remove the > duplicate domain ownership check in assign_device(). Whilst in the > neighbourhood, the patch also removes some debug logging from > assign_device() and deassign_device() and replaces it with proper error > logging, which allows error logging in iommu_do_pci_domctl() to be > removed. Also, since device_assigned() can tell the difference between a > guest assigned device and a non-existent one, log the actual error > condition rather then being ambiguous for the sake a few extra lines of > code. > > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx> > --- > > This is XSA-302 followup and contains some changes important for > XenServer. > Juergen, could this be considered for 4.13 inclusion? > > v2: updated Paul's email address Reviewed-by: Paul Durrant <pdurrant@xxxxxxxxxx> > > --- > xen/drivers/passthrough/pci.c | 101 ++++++++++++++++++------------------- > ----- > 1 file changed, 42 insertions(+), 59 deletions(-) > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index e64666d..ea0770d 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -932,30 +932,27 @@ static int deassign_device(struct domain *d, > uint16_t seg, uint8_t bus, > break; > ret = hd->platform_ops->reassign_device(d, target, devfn, > pci_to_dev(pdev)); > - if ( !ret ) > - continue; > - > - printk(XENLOG_G_ERR "%pd: deassign %04x:%02x:%02x.%u failed > (%d)\n", > - d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret); > - return ret; > + if ( ret ) > + goto out; > } > > devfn = pdev->devfn; > ret = hd->platform_ops->reassign_device(d, target, devfn, > pci_to_dev(pdev)); > if ( ret ) > - { > - dprintk(XENLOG_G_ERR, > - "%pd: deassign device (%04x:%02x:%02x.%u) failed\n", > - d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); > - return ret; > - } > + goto out; > > if ( pdev->domain == hardware_domain ) > pdev->quarantine = false; > > pdev->fault.count = 0; > > +out: > + if ( ret ) > + printk(XENLOG_G_ERR > + "%pd: deassign device (%04x:%02x:%02x.%u) failed (%d)\n", > d, > + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret); > + > return ret; > } > > @@ -976,10 +973,7 @@ int pci_release_devices(struct domain *d) > { > bus = pdev->bus; > devfn = pdev->devfn; > - if ( deassign_device(d, pdev->seg, bus, devfn) ) > - printk("domain %d: deassign device (%04x:%02x:%02x.%u) > failed!\n", > - d->domain_id, pdev->seg, bus, > - PCI_SLOT(devfn), PCI_FUNC(devfn)); > + deassign_device(d, pdev->seg, bus, devfn); > } > pcidevs_unlock(); > > @@ -1534,8 +1528,7 @@ static int device_assigned(u16 seg, u8 bus, u8 > devfn) > struct pci_dev *pdev; > int rc = 0; > > - pcidevs_lock(); > - > + ASSERT(pcidevs_locked()); > pdev = pci_get_pdev(seg, bus, devfn); > > if ( !pdev ) > @@ -1549,11 +1542,10 @@ static int device_assigned(u16 seg, u8 bus, u8 > devfn) > pdev->domain != dom_io ) > rc = -EBUSY; > > - pcidevs_unlock(); > - > return rc; > } > > +/* caller should hold the pcidevs_lock */ > static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 > flag) > { > const struct domain_iommu *hd = dom_iommu(d); > @@ -1571,23 +1563,11 @@ static int assign_device(struct domain *d, u16 > seg, u8 bus, u8 devfn, u32 flag) > vm_event_check_ring(d->vm_event_paging)) ) > return -EXDEV; > > - if ( !pcidevs_trylock() ) > - return -ERESTART; > - > + /* device_assigned() should already have cleared the device for > assignment */ > + ASSERT(pcidevs_locked()); > pdev = pci_get_pdev(seg, bus, devfn); > - > - rc = -ENODEV; > - if ( !pdev ) > - goto done; > - > - rc = 0; > - if ( d == pdev->domain ) > - goto done; > - > - rc = -EBUSY; > - if ( pdev->domain != hardware_domain && > - pdev->domain != dom_io ) > - goto done; > + ASSERT(pdev && (pdev->domain == hardware_domain || > + pdev->domain == dom_io)); > > if ( pdev->msix ) > { > @@ -1608,19 +1588,17 @@ static int assign_device(struct domain *d, u16 > seg, u8 bus, u8 devfn, u32 flag) > if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) ) > break; > rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), > flag); > - if ( rc ) > - printk(XENLOG_G_WARNING "d%d: assign %04x:%02x:%02x.%u failed > (%d)\n", > - d->domain_id, seg, bus, PCI_SLOT(devfn), > PCI_FUNC(devfn), > - rc); > } > > done: > + if ( rc ) > + printk(XENLOG_G_ERR > + "%pd: assign device (%04x:%02x:%02x.%u) failed (%d)\n", d, > + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), rc); > /* The device is assigned to dom_io so mark it as quarantined */ > - if ( !rc && d == dom_io ) > + else if ( d == dom_io ) > pdev->quarantine = true; > > - pcidevs_unlock(); > - > return rc; > } > > @@ -1776,29 +1754,40 @@ int iommu_do_pci_domctl( > bus = PCI_BUS(machine_sbdf); > devfn = PCI_DEVFN2(machine_sbdf); > > + pcidevs_lock(); > ret = device_assigned(seg, bus, devfn); > if ( domctl->cmd == XEN_DOMCTL_test_assign_device ) > { > - if ( ret ) > + switch ( ret ) > { > + case 0: > + break; > + > + case -ENODEV: > printk(XENLOG_G_INFO > - "%04x:%02x:%02x.%u already assigned, or non- > existent\n", > + "%04x:%02x:%02x.%u non-existent\n", > seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); > ret = -EINVAL; > + break; > + > + case -EBUSY: > + printk(XENLOG_G_INFO > + "%04x:%02x:%02x.%u already assigned\n", > + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); > + ret = -EINVAL; > + break; > + > + default: > + ret = -EINVAL; > + break; > } > - break; > } > - if ( !ret ) > + else if ( !ret ) > ret = assign_device(d, seg, bus, devfn, flags); > + pcidevs_unlock(); > if ( ret == -ERESTART ) > ret = hypercall_create_continuation(__HYPERVISOR_domctl, > "h", u_domctl); > - else if ( ret ) > - printk(XENLOG_G_ERR > - "assign %04x:%02x:%02x.%u to dom%d failed (%d)\n", > - seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > - d->domain_id, ret); > - > break; > > case XEN_DOMCTL_deassign_device: > @@ -1830,12 +1819,6 @@ int iommu_do_pci_domctl( > pcidevs_lock(); > ret = deassign_device(d, seg, bus, devfn); > pcidevs_unlock(); > - if ( ret ) > - printk(XENLOG_G_ERR > - "deassign %04x:%02x:%02x.%u from dom%d failed (%d)\n", > - seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > - d->domain_id, ret); > - > break; > > default: > -- > 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |