xen/drivers/passthrough/pci.c | 101 ++++++++++++++++++------------------------ 1 file changed, 42 insertions(+), 59 deletions(-)
From: Paul Durrant <pdurrant@amazon.com>
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@amazon.com>
---
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
---
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 01.11.2019 19:28, Igor Druzhinin wrote:
> 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.
In this last sentence it looks like you mean the caller of
device_assigned(), not the function itself.
> --- 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;
> }
There would have been quite a bit less code churn if you simply
replaced the dprintk(). If you really want to stick to the goto
approach you're introducing (which I dislike, but I know others
prefer it in cases like this one), then please indent the label and
shorten the message to the one that was used in the original printk().
> @@ -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)
Just like the comment ahead of deassign_device(), this one should
start with a capital letter.
> @@ -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);
I'd prefer if we could stick to this being XENLOG_G_WARNING: Other
than device de-assignment failure, assignment failure is unlikely
to be an issue to the host as a whole. Also please again shorten
the message to what it was before.
> @@ -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;
> }
Three separate but identical assignments to ret look a little odd at
least. Is there a reason why we need to (continue to) convert the
original error code? I can't seem to be able to find any after some
looking around, but I surely may have missed something.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.