| Message ID | 1572632881-9050-1-git-send-email-igor.druzhinin@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-4.13,v2] passthrough: simplify locking and logging | expand |
> -----Original Message----- > From: Igor Druzhinin <igor.druzhinin@citrix.com> > Sent: 01 November 2019 19:28 > To: xen-devel@lists.xenproject.org > Cc: Durrant, Paul <pdurrant@amazon.com>; jbeulich@suse.com; > jgross@suse.com > Subject: [PATCH for-4.13 v2] passthrough: simplify locking and logging > > 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 Reviewed-by: Paul Durrant <pdurrant@amazon.com> > > --- > 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
On 04/11/2019 08:31, Durrant, Paul wrote: >> -----Original Message----- >> From: Igor Druzhinin <igor.druzhinin@citrix.com> >> Sent: 01 November 2019 19:28 >> To: xen-devel@lists.xenproject.org >> Cc: Durrant, Paul <pdurrant@amazon.com>; jbeulich@suse.com; >> jgross@suse.com >> Subject: [PATCH for-4.13 v2] passthrough: simplify locking and logging >> >> 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 This was work you did at Citrix, yes? > Reviewed-by: Paul Durrant <pdurrant@amazon.com> SoB and R-by? ~Andrew
> -----Original Message----- > From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: 04 November 2019 11:06 > To: Durrant, Paul <pdurrant@amazon.com>; xen-devel@lists.xenproject.org > Cc: Igor Druzhinin <igor.druzhinin@citrix.com>; jgross@suse.com; > jbeulich@suse.com > Subject: Re: [Xen-devel] [PATCH for-4.13 v2] passthrough: simplify locking > and logging > > On 04/11/2019 08:31, Durrant, Paul wrote: > >> -----Original Message----- > >> From: Igor Druzhinin <igor.druzhinin@citrix.com> > >> Sent: 01 November 2019 19:28 > >> To: xen-devel@lists.xenproject.org > >> Cc: Durrant, Paul <pdurrant@amazon.com>; jbeulich@suse.com; > >> jgross@suse.com > >> Subject: [PATCH for-4.13 v2] passthrough: simplify locking and logging > >> > >> 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 > > This was work you did at Citrix, yes? > > > Reviewed-by: Paul Durrant <pdurrant@amazon.com> > > SoB and R-by? I did do the work while I was at Citrix, but surely the SoB must be updated since the patch is only now being posted? As for the R-b, why should that be historic? Paul > > ~Andrew
On Mon, Nov 04, 2019 at 11:13:48AM +0000, Durrant, Paul wrote: > > -----Original Message----- > > From: Andrew Cooper <andrew.cooper3@citrix.com> > > Sent: 04 November 2019 11:06 > > To: Durrant, Paul <pdurrant@amazon.com>; xen-devel@lists.xenproject.org > > Cc: Igor Druzhinin <igor.druzhinin@citrix.com>; jgross@suse.com; > > jbeulich@suse.com > > Subject: Re: [Xen-devel] [PATCH for-4.13 v2] passthrough: simplify locking > > and logging > > > > On 04/11/2019 08:31, Durrant, Paul wrote: > > >> -----Original Message----- > > >> From: Igor Druzhinin <igor.druzhinin@citrix.com> > > >> Sent: 01 November 2019 19:28 > > >> To: xen-devel@lists.xenproject.org > > >> Cc: Durrant, Paul <pdurrant@amazon.com>; jbeulich@suse.com; > > >> jgross@suse.com > > >> Subject: [PATCH for-4.13 v2] passthrough: simplify locking and logging > > >> > > >> From: Paul Durrant <pdurrant@amazon.com> > > >> > > >> > > >> Signed-off-by: Paul Durrant <pdurrant@amazon.com> > > >> --- > > >> > > >> > > >> v2: updated Paul's email address > > > > This was work you did at Citrix, yes? > > > > > Reviewed-by: Paul Durrant <pdurrant@amazon.com> > > > > SoB and R-by? > > I did do the work while I was at Citrix, but surely the SoB must be updated since the patch is only now being posted? I don't think it matters when a patch is publicly posted, the SoB shouldn't change. Also, Igor, I think you need to add your own SoB to the patch. This would be because of (b) or (c) of the "Developer's Certificate of Origin 1.1" [1]. > As for the R-b, why should that be historic? I think he meant that reviewing your own work is a bit weird. On the other hand, it is possible to have both a SoB and a R-b from the same persone, if the original patch has been modified. [1]: Developer's Certificate of Origin 1.1 By making a contribution to this project, I certify that: (a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it. (d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. Cheers,
> -----Original Message----- > From: Anthony PERARD <anthony.perard@citrix.com> > Sent: 04 November 2019 12:14 > To: Durrant, Paul <pdurrant@amazon.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; xen- > devel@lists.xenproject.org; jgross@suse.com; Igor Druzhinin > <igor.druzhinin@citrix.com>; jbeulich@suse.com > Subject: Re: [Xen-devel] [PATCH for-4.13 v2] passthrough: simplify locking > and logging > > On Mon, Nov 04, 2019 at 11:13:48AM +0000, Durrant, Paul wrote: > > > -----Original Message----- > > > From: Andrew Cooper <andrew.cooper3@citrix.com> > > > Sent: 04 November 2019 11:06 > > > To: Durrant, Paul <pdurrant@amazon.com>; xen- > devel@lists.xenproject.org > > > Cc: Igor Druzhinin <igor.druzhinin@citrix.com>; jgross@suse.com; > > > jbeulich@suse.com > > > Subject: Re: [Xen-devel] [PATCH for-4.13 v2] passthrough: simplify > locking > > > and logging > > > > > > On 04/11/2019 08:31, Durrant, Paul wrote: > > > >> -----Original Message----- > > > >> From: Igor Druzhinin <igor.druzhinin@citrix.com> > > > >> Sent: 01 November 2019 19:28 > > > >> To: xen-devel@lists.xenproject.org > > > >> Cc: Durrant, Paul <pdurrant@amazon.com>; jbeulich@suse.com; > > > >> jgross@suse.com > > > >> Subject: [PATCH for-4.13 v2] passthrough: simplify locking and > logging > > > >> > > > >> From: Paul Durrant <pdurrant@amazon.com> > > > >> > > > >> > > > >> Signed-off-by: Paul Durrant <pdurrant@amazon.com> > > > >> --- > > > >> > > > >> > > > >> v2: updated Paul's email address > > > > > > This was work you did at Citrix, yes? > > > > > > > Reviewed-by: Paul Durrant <pdurrant@amazon.com> > > > > > > SoB and R-by? > > > > I did do the work while I was at Citrix, but surely the SoB must be > updated since the patch is only now being posted? > > I don't think it matters when a patch is publicly posted, the SoB > shouldn't change. > Also, Igor, I think you need to add your own SoB to the patch. This would > be because of (b) or (c) of the "Developer's Certificate of Origin 1.1" > [1]. > > > As for the R-b, why should that be historic? > > I think he meant that reviewing your own work is a bit weird. On the > other hand, it is possible to have both a SoB and a R-b from the same > persone, if the original patch has been modified. I was merely reviewing the change of email address and verifying that it was the patch I wrote :-) Paul > > > [1]: > Developer's Certificate of Origin 1.1 > > By making a contribution to this project, I certify that: > > (a) The contribution was created in whole or in part by me and I > have the right to submit it under the open source license > indicated in the file; or > > (b) The contribution is based upon previous work that, to the best > of my knowledge, is covered under an appropriate open source > license and I have the right under that license to submit that > work with modifications, whether created in whole or in part > by me, under the same open source license (unless I am > permitted to submit under a different license), as indicated > in the file; or > > (c) The contribution was provided directly to me by some other > person who certified (a), (b) or (c) and I have not modified > it. > > (d) I understand and agree that this project and the contribution > are public and that a record of the contribution (including all > personal information I submit with it, including my sign-off) is > maintained indefinitely and may be redistributed consistent with > this project or the open source license(s) involved. > > > Cheers, > > -- > Anthony PERARD
On 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
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: