|
[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 |