From patchwork Fri Nov 15 18:59:30 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Igor Druzhinin X-Patchwork-Id: 11246973 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 04CDC13B2 for ; Fri, 15 Nov 2019 19:01:17 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C212C20732 for ; Fri, 15 Nov 2019 19:01:16 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=citrix.com header.i=@citrix.com header.b="VChKuprU" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C212C20732 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=citrix.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iVgok-0004Gn-VW; Fri, 15 Nov 2019 18:59:38 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iVgoi-0004Gh-PM for xen-devel@lists.xenproject.org; Fri, 15 Nov 2019 18:59:37 +0000 X-Inumbo-ID: 10800b6e-07da-11ea-b678-bc764e2007e4 Received: from esa6.hc3370-68.iphmx.com (unknown [216.71.155.175]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 10800b6e-07da-11ea-b678-bc764e2007e4; Fri, 15 Nov 2019 18:59:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=citrix.com; s=securemail; t=1573844375; h=from:to:cc:subject:date:message-id:mime-version; bh=hREYSKwA+ircjoKXkytb7GkRnOOLW2vS7p1+vChYNrk=; b=VChKuprU3ecjRmT3Xtekl4AuiK0+QO90J+cMW+4RQlO6YaQ642sxsxfL 1iNUudsAtRGqFSoJSECPPxIqwivRBebicdkXdefcFhRIjhr2x0B7y4jS/ DhGfPw+/bdBBkG8w0GFYBlGNFQHNfCxSkFtuWXLwGgLIvQUMFKliAF4qx Q=; Authentication-Results: esa6.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=igor.druzhinin@citrix.com; spf=Pass smtp.mailfrom=igor.druzhinin@citrix.com; spf=None smtp.helo=postmaster@mail.citrix.com Received-SPF: None (esa6.hc3370-68.iphmx.com: no sender authenticity information available from domain of igor.druzhinin@citrix.com) identity=pra; client-ip=162.221.158.21; receiver=esa6.hc3370-68.iphmx.com; envelope-from="igor.druzhinin@citrix.com"; x-sender="igor.druzhinin@citrix.com"; x-conformance=sidf_compatible Received-SPF: Pass (esa6.hc3370-68.iphmx.com: domain of igor.druzhinin@citrix.com designates 162.221.158.21 as permitted sender) identity=mailfrom; client-ip=162.221.158.21; receiver=esa6.hc3370-68.iphmx.com; envelope-from="igor.druzhinin@citrix.com"; x-sender="igor.druzhinin@citrix.com"; x-conformance=sidf_compatible; x-record-type="v=spf1"; x-record-text="v=spf1 ip4:209.167.231.154 ip4:178.63.86.133 ip4:195.66.111.40/30 ip4:85.115.9.32/28 ip4:199.102.83.4 ip4:192.28.146.160 ip4:192.28.146.107 ip4:216.52.6.88 ip4:216.52.6.188 ip4:162.221.158.21 ip4:162.221.156.83 ip4:168.245.78.127 ~all" Received-SPF: None (esa6.hc3370-68.iphmx.com: no sender authenticity information available from domain of postmaster@mail.citrix.com) identity=helo; client-ip=162.221.158.21; receiver=esa6.hc3370-68.iphmx.com; envelope-from="igor.druzhinin@citrix.com"; x-sender="postmaster@mail.citrix.com"; x-conformance=sidf_compatible IronPort-SDR: 0pGolBTciiw/lLIurEhPAOxUhVL62GIslzL3uS2J9tC/MK5F6Fmm8uLpooH8z61NzL5mhZfwcQ 6GgnFk4bbBCThW3sgEYhbSbtY9coaK3n1ttPc0myXSsmcYUwRBAgkKQhc1dw2AtBhAcm5fQt4n v06136JdPdeF0nD1BwIuFJ9Kr5nFWIeedYG3gJiZi7K0aYBCPSf+aV3pNc26S/meK0FWEoAJFA UlA8YjTraTX0hmxwZdpWEysNKWudMmdobPaYwUZMcDtIbgC6ntnZluI1JzjBecQ6V6oBLiSK/f Aog= X-SBRS: 2.7 X-MesageID: 8812890 X-Ironport-Server: esa6.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.68,309,1569297600"; d="scan'208";a="8812890" From: Igor Druzhinin To: Date: Fri, 15 Nov 2019 18:59:30 +0000 Message-ID: <1573844370-29159-1-git-send-email-igor.druzhinin@citrix.com> X-Mailer: git-send-email 2.7.4 MIME-Version: 1.0 Subject: [Xen-devel] [PATCH for-4.13 v3] passthrough: simplify locking and logging X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: jgross@suse.com, Igor Druzhinin , pdurrant@amazon.com, Paul Durrant , jbeulich@suse.com Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" From: Paul Durrant 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. Signed-off-by: Paul Durrant Signed-off-by: Igor Druzhinin Acked-by: Jan Beulich --- Since Paul doesn't mind and kindly agreed - I'm taking ownership of this patch review process from now on. Changes in v3: - Dropped controversial hunk with error code processing of device_assigned(). Readability is worse with it and I don't think we can safely stop converting the error code to avoid userspace breakage. - Addressed other minor comments. - Fixed Paul's email again to reflect that the code was made in Citrix. --- xen/drivers/passthrough/pci.c | 78 ++++++++++++------------------------------- 1 file changed, 22 insertions(+), 56 deletions(-) diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 18a7dc7..8a25d4f 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -932,30 +932,26 @@ 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 (%04x:%02x:%02x.%u) failed (%d)\n", + d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret); + return ret; } @@ -976,10 +972,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(); @@ -1475,8 +1468,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 ) @@ -1490,11 +1482,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); @@ -1513,23 +1504,11 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) p2m_get_hostp2m(d)->global_logdirty) ) 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 ) { @@ -1550,19 +1529,16 @@ 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_WARNING "%pd: assign (%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; } @@ -1718,6 +1694,7 @@ 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 ) { @@ -1730,17 +1707,12 @@ int iommu_do_pci_domctl( } 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: @@ -1772,12 +1744,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: