From patchwork Tue Jun 13 10:32:26 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Volodymyr Babchuk X-Patchwork-Id: 13278337 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 0849EC88CB6 for ; Tue, 13 Jun 2023 10:33:15 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.548007.855788 (Exim 4.92) (envelope-from ) id 1q91KW-0003pi-Af; Tue, 13 Jun 2023 10:32:52 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 548007.855788; Tue, 13 Jun 2023 10:32:52 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1q91KV-0003mg-Sz; Tue, 13 Jun 2023 10:32:51 +0000 Received: by outflank-mailman (input) for mailman id 548007; Tue, 13 Jun 2023 10:32:50 +0000 Received: from se1-gles-sth1-in.inumbo.com ([159.253.27.254] helo=se1-gles-sth1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1q91KT-0001im-Vp for xen-devel@lists.xenproject.org; Tue, 13 Jun 2023 10:32:50 +0000 Received: from mx0b-0039f301.pphosted.com (mx0b-0039f301.pphosted.com [148.163.137.242]) by se1-gles-sth1.inumbo.com (Halon) with ESMTPS id a354493f-09d5-11ee-b232-6b7b168915f2; Tue, 13 Jun 2023 12:32:47 +0200 (CEST) Received: from pps.filterd (m0174683.ppops.net [127.0.0.1]) by mx0b-0039f301.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 35DAO5KZ019280; Tue, 13 Jun 2023 10:32:34 GMT Received: from eur05-am6-obe.outbound.protection.outlook.com (mail-am6eur05lp2106.outbound.protection.outlook.com [104.47.18.106]) by mx0b-0039f301.pphosted.com (PPS) with ESMTPS id 3r5yahmgn9-3 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 13 Jun 2023 10:32:34 +0000 Received: from VI1PR03MB3710.eurprd03.prod.outlook.com (2603:10a6:803:31::18) by DU0PR03MB9706.eurprd03.prod.outlook.com (2603:10a6:10:44e::5) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6455.33; Tue, 13 Jun 2023 10:32:27 +0000 Received: from VI1PR03MB3710.eurprd03.prod.outlook.com ([fe80::6fa2:d7b8:9bd4:1a51]) by VI1PR03MB3710.eurprd03.prod.outlook.com ([fe80::6fa2:d7b8:9bd4:1a51%5]) with mapi id 15.20.6455.045; Tue, 13 Jun 2023 10:32:27 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: a354493f-09d5-11ee-b232-6b7b168915f2 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=GxhWHWhs/pE8sBDlvFriQcSBFZDeeLDi3XpzE0Ka5KtWAA1t0Lqe6udxnPloKGWNej4zdiVYTSqEdvD4m5yHrSG0fSyCV3Dqveaoiwd8KcETBOcl0YFRn75YwxcYch0rcE4yaeDtRfMPfjfNbN75NVWzIZ6qsBG67o6pZ3jAw7cgkB27ZpU4h9IrRV4vsemkRKJyZdNW8A3Tcj7IRVRrlR3jFr6Bw0zuirgskSC+dAXe0v4EAoPVY5ku49C4GhxhB+IrDeE7Sr+a2F0bQpLLqK7zFKheebBJOQLReisSJFxeAln1ZnJhabEqro1WqxRXSxAeXHdKHiUkbqyoaqif+g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=l7KRV/9J4bj1bWWF2aqOnKB23fKF5Us5BL80aI/80+c=; b=nxoMiyKkvua2PlIqaIRNgFL7wTx1nA+gXXJaPKc4gK3lDA5zJIZZeH08Wh9EX8nWe6XSBR6AYXUP8x0BzmbDSCv1g8/YM7sVSHuTxVSgmIBRa320wA4i+02OerE9pRar5id4RJuhhJSZF/F2stOUSMoEdR6JNnlDJB8//RqEHKJY0cpOTX3Salxgou01NdUaW66Q9MI2wGG1rqzip8FKcsZgXZjL43InZs8uo+2I45IbLOK75freXkCdQDmgSN6i451m7bp/4R5q7QHJx4dh7mHCxY+pHtbClprbX4YaaZReO2075PuPLxQavxVLo5fPjTDLu2R1w1GZ55yflC5/7g== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=epam.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=l7KRV/9J4bj1bWWF2aqOnKB23fKF5Us5BL80aI/80+c=; b=mHWqvq5hdLoBH6UqG9Y491+C2RjEQC36lSIqrSbM8VnGXBQhdntZ0lGw4PxE/EEYAn4LRf9sM9Wik+jxSXk/ueBFdjdfHTNM19/4M43k26ofs0j+ujVaNO2QW2ketw+6DcXlwogkyO7a2nkSGHeo2HcAyECNkLl4My6ZlZCe4g7+4jgkacIFEpHFrN9NNDFPgJYnHWCl4n9pyVFcghypFBsm0MOPP4t0EAZlFZB8sxEr+dA1QUVIc/Uw7kG8zGtuoTwL8pvfuiw8bEDPYgNhtOO6+J1qkcHt8+e0z77PhUDREFsHiyfvSDadIxGtnRRZEPURgqgFhUZz9yk9m5bDAg== From: Volodymyr Babchuk To: "xen-devel@lists.xenproject.org" CC: Oleksandr Andrushchenko , Jan Beulich , Andrew Cooper , =?utf-8?q?Roger_Pau_Monn=C3=A9?= , Wei Liu , George Dunlap , Julien Grall , Stefano Stabellini , Paul Durrant Subject: [PATCH v7 01/12] vpci: introduce per-domain lock to protect vpci structure Thread-Topic: [PATCH v7 01/12] vpci: introduce per-domain lock to protect vpci structure Thread-Index: AQHZneJZdDE+HOoSO0y6VB9l0x1hDg== Date: Tue, 13 Jun 2023 10:32:26 +0000 Message-ID: <20230613103159.524763-2-volodymyr_babchuk@epam.com> References: <20230613103159.524763-1-volodymyr_babchuk@epam.com> In-Reply-To: <20230613103159.524763-1-volodymyr_babchuk@epam.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-mailer: git-send-email 2.40.1 x-ms-publictraffictype: Email x-ms-traffictypediagnostic: VI1PR03MB3710:EE_|DU0PR03MB9706:EE_ x-ms-office365-filtering-correlation-id: 418358fe-47b1-4849-fd6f-08db6bf97c08 x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: d8pqRNuGoCYKdA8wTkY0YHIOdODe8/RZb5/BPBIj9GOSwNbjsSpt+1/02me9GnfJSuAYBVejclOLZmaR5bzgjrt9IXi9r1BT4NCtwjjiKFIcfVD5iZgmsFjSohyPZdS5ANecslIUoUCOHUYeVgCWjmh3opju/p+qAgwptld9gGsfU93HWGJyJI6M+PjTgd3Ec4KVNj7ETBbWBLeRlnVTSD/YWixHzhgaxCwIKGAyD7YdiM0K4YRndURdD29/oZoNcFQh/E2d6s6hP36nGwk/Ph8hEIbJcLfxzN9uEvKQiPQGF4cyMmfUMXF8kQYUeKpDLxJOcdye5rcEStvonsl6JEi+dqFZXWnjm9UCaMs4KA53kJwEil1om3WIg3bt8qu2k+SA89JbzT95IY+6OxdRvmyNsRH2/mQoUugq8/AyQ3kr3PXY9hYQXKzjC6ouQ9kqhG83Vf1ydyJ+lIIVTqlE9rWIIXb21f917Fv2LWpvLXuWwNg/BRHfizeUVlTHLLieeB7a9m1Vqx60CEri2a+454/5HGeVF5dMGEEH45FfHn2PVgQaI+hsQ4vq80jUHaIVlJ9qTKHHV2aLQ/ocRFqoPo3+l41a7S/xoREWSn0AD+b0igNu5njYHSillVYs9oNG x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:VI1PR03MB3710.eurprd03.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230028)(4636009)(39860400002)(396003)(376002)(136003)(346002)(366004)(451199021)(966005)(91956017)(64756008)(76116006)(66556008)(66446008)(4326008)(6916009)(66476007)(71200400001)(5660300002)(66946007)(66899021)(41300700001)(8936002)(8676002)(316002)(36756003)(54906003)(30864003)(83380400001)(478600001)(2906002)(6486002)(38100700002)(186003)(6506007)(26005)(1076003)(6512007)(86362001)(55236004)(38070700005)(2616005)(122000001)(579004)(559001)(309714004);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?utf-8?q?Ph6wcIazWHiED5QOzLR6MGszPFLM?= =?utf-8?q?AtozqqZZf80Kx1uw4JqUtCBrQGXsxQ3zI8VnI6MSMYjSRuz9ZJngRK8o9nGV9kdU/?= =?utf-8?q?MYNo1lFQxV2LQKGd71KVH1swi7lynrui1hrJJzbv1BYz0qZcI2iKGrW5G4leNq2NF?= =?utf-8?q?AXRlIORF8RtWmYmhqNb9WZb7heW18uJxPMri0cmmn67XpGo/bDAho8atBLFLNg8PF?= =?utf-8?q?Z+RMQaXEtgIOfPcll/9UB4zSTLqE7gmn4HdrcNtNh2NyeZ/u8tA42I9dgwLUDFpsg?= =?utf-8?q?Z/6/phaWzpl0YHSvazfQGwxIpiNswph3tpmBUipBoPhOY2uR/5wPjN/+H/lTkxNvu?= =?utf-8?q?GlAnQza0S8Fz3PbXd7+EHl2ms+ChPtU//83wQVsM2QhBOD0V6Ft/EDg54wi+Lf3IL?= =?utf-8?q?kmcW9J/cGUN0gcM9wxN/wLHo8dWEfB6KXMY2ZDnog2owOu2g3d7I5SzFtUmbssfXs?= =?utf-8?q?glK8cqgfwpCFE5tjA4s1AuN+ZDRyzJdlufmmEdVgpIPrNOkLu54v/uXpjbrhHU7EF?= =?utf-8?q?Z+Gy2qQSfwkc11TlySYspsANudiKvsD1ZYCCTSmYw4o7rqT8kObbtWbKjel7ziLwN?= =?utf-8?q?UZqZ7aujetnNYezr+/DGohMdErTfrHpbBW5d9ASmjEoHejlr6TMZMB/8WKOyWJsD5?= =?utf-8?q?WMOPsyH0Zx8vO0Yai5knfFFx5SKhbRYnWKhe5mKl6OTxIG5Mxan5cl5RPRd8nZq51?= =?utf-8?q?huMOX5jjWK0qVofVxABxMPMkt5OtyHqatHTYeF/ubT0qgHpOUIEPoiRR3w89smAfp?= =?utf-8?q?18VLl8j3EXPpV46eOa1vVC/QmUWeYQQJHaMuLorblBX240zlgJ3BCKVlNgRqSNO7I?= =?utf-8?q?315ieulBZhxc/FQP5t2TFRLisxQZpynhaF64M2X/rheRe4N9AJTATpfIAFpjWCcX4?= =?utf-8?q?JhIl3nYueXfWtIAz++2uTey9m621RIfnUe6i/X68syxyfI8iGp4XJnV26d4CcSXUe?= =?utf-8?q?PaURRK+E1HIpfqV3IucaHsguKvfsXESnqGIwyQSPW4Wk3qz/e5ZHbdySJ4ejeT0aT?= =?utf-8?q?9vPL/Vw9NaEtmCU/fLryxNc4ym7cJp37aGczsBSoTQWs1ja7i0GPIfWnMyw4ZDqCW?= =?utf-8?q?iiuft8713CTCI15uLzRPn8Z6xAVSHgseHp4Eb5PK/+rsVcil/AKpUOcyedE6ZR72L?= =?utf-8?q?G3KVpMw61EkOh4WpRq8zZh2z/8vEt0DvmcYttBD37wik0QoFgBkzlERC1X6+qHHsy?= =?utf-8?q?4qApmmJCEHvJKGSwp1bMbDwbDtej9Gq3bzet/fZ4azX1dx90bgz58MV7BJLprtfCy?= =?utf-8?q?kXrEP5wV08TjG4AVTVVapcItlJH+vA5XxSDMDTUQB1d0xJeD/ewSr7Tj/UN9q6ruj?= =?utf-8?q?L55uljkX2yV3tK2ID+Q3d5Nv5nuHq8bok1fXy2YlhZ47v8ynmqobOtgWuWXAOKqUQ?= =?utf-8?q?Tn3qysoyhjWbGoxzFb2Debw/N/NRV1QPx8PKoIo8PbthyLtyDcOLF9XBb6A/QwEdJ?= =?utf-8?q?FwJDox/tnwT9fMXr8E0JSIfaKAO63MjeHt157Qsx+FDrD4HO6P652v8LwWPIMP7Jh?= =?utf-8?q?OcE/klb1b8320WSW6l7m+J6e+59yXBGpXg=3D=3D?= Content-ID: MIME-Version: 1.0 X-OriginatorOrg: epam.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: VI1PR03MB3710.eurprd03.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 418358fe-47b1-4849-fd6f-08db6bf97c08 X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Jun 2023 10:32:26.9819 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: b41b72d0-4e9f-4c26-8a69-f949f367c91d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: BGl6KtVUFFpq7bzfINa/ESgBU2h9U08hlRap6FMhsd7aas81bxpsopYvDEBUBcVsGqEK1ib6oxBruJcEa8KlYuWszDBL6gPvheT82eKV8Gk= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU0PR03MB9706 X-Proofpoint-GUID: gL1gMRsyVgjFA6e7UXKA3zTzOmHcmIpD X-Proofpoint-ORIG-GUID: gL1gMRsyVgjFA6e7UXKA3zTzOmHcmIpD X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26 definitions=2023-06-13_04,2023-06-12_02,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 suspectscore=0 mlxlogscore=999 spamscore=0 malwarescore=0 clxscore=1015 lowpriorityscore=0 bulkscore=0 priorityscore=1501 phishscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2305260000 definitions=main-2306130093 From: Oleksandr Andrushchenko Introduce a per-domain read/write lock to check whether vpci is present, so we are sure there are no accesses to the contents of the vpci struct if not. This lock can be used (and in a few cases is used right away) so that vpci removal can be performed while holding the lock in write mode. Previously such removal could race with vpci_read for example. 1. Per-domain's vpci_rwlock is used to protect pdev->vpci structure from being removed. 2. Writing the command register and ROM BAR register may trigger modify_bars to run, which in turn may access multiple pdevs while checking for the existing BAR's overlap. The overlapping check, if done under the read lock, requires vpci->lock to be acquired on both devices being compared, which may produce a deadlock. It is not possible to upgrade read lock to write lock in such a case. So, in order to prevent the deadlock, check which registers are going to be written and acquire the lock in the appropriate mode from the beginning. All other code, which doesn't lead to pdev->vpci destruction and does not access multiple pdevs at the same time, can still use a combination of the read lock and pdev->vpci->lock. 3. Optimize if ROM BAR write lock required detection by caching offset of the ROM BAR register in vpci->header->rom_reg which depends on header's type. 4. Reduce locked region in vpci_remove_device as it is now possible to set pdev->vpci to NULL early right after the write lock is acquired. 5. Reduce locked region in vpci_add_handlers as it is possible to initialize many more fields of the struct vpci before assigning it to pdev->vpci. 6. vpci_{add|remove}_register are required to be called with the write lock held, but it is not feasible to add an assert there as it requires struct domain to be passed for that. So, add a comment about this requirement to these and other functions with the equivalent constraints. 7. Drop const qualifier where the new rwlock is used and this is appropriate. 8. Do not call process_pending_softirqs with any locks held. For that unlock prior the call and re-acquire the locks after. After re-acquiring the lock there is no need to check if pdev->vpci exists: - in apply_map because of the context it is called (no race condition possible) - for MSI/MSI-X debug code because it is called at the end of pdev->vpci access and no further access to pdev->vpci is made 9. Check for !pdev->vpci in vpci_{read|write} after acquiring the lock and if so, allow reading or writing the hardware register directly. This is acceptable as we only deal with Dom0 as of now. Once DomU support is added the write will need to be ignored and read return all 0's for the guests, while Dom0 can still access the registers directly. 10. Introduce pcidevs_trylock, so there is a possibility to try locking the pcidev's lock. 11. Use pcidev's lock around for_each_pdev and pci_get_pdev_by_domain while accessing pdevs in vpci code. 12. This is based on the discussion at [1]. [1] https://lore.kernel.org/all/20220204063459.680961-4-andr2000@gmail.com/ Suggested-by: Roger Pau Monné Suggested-by: Jan Beulich Signed-off-by: Oleksandr Andrushchenko --- This was checked on x86: with and without PVH Dom0. --- xen/arch/x86/hvm/vmsi.c | 7 +++ xen/common/domain.c | 3 + xen/drivers/passthrough/pci.c | 5 ++ xen/drivers/vpci/header.c | 52 +++++++++++++++++ xen/drivers/vpci/msi.c | 25 +++++++- xen/drivers/vpci/msix.c | 51 +++++++++++++--- xen/drivers/vpci/vpci.c | 107 +++++++++++++++++++++++++--------- xen/include/xen/pci.h | 1 + xen/include/xen/sched.h | 3 + xen/include/xen/vpci.h | 6 ++ 10 files changed, 223 insertions(+), 37 deletions(-) diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c index 3cd4923060..f188450b1b 100644 --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -895,6 +895,9 @@ int vpci_msix_arch_print(const struct vpci_msix *msix) { unsigned int i; + ASSERT(rw_is_locked(&msix->pdev->domain->vpci_rwlock)); + ASSERT(pcidevs_locked()); + for ( i = 0; i < msix->max_entries; i++ ) { const struct vpci_msix_entry *entry = &msix->entries[i]; @@ -913,7 +916,11 @@ int vpci_msix_arch_print(const struct vpci_msix *msix) struct pci_dev *pdev = msix->pdev; spin_unlock(&msix->pdev->vpci->lock); + pcidevs_unlock(); + read_unlock(&pdev->domain->vpci_rwlock); process_pending_softirqs(); + read_lock(&pdev->domain->vpci_rwlock); + pcidevs_lock(); /* NB: we assume that pdev cannot go away for an alive domain. */ if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) ) return -EBUSY; diff --git a/xen/common/domain.c b/xen/common/domain.c index 6a440590fe..a4640acb62 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -622,6 +622,9 @@ struct domain *domain_create(domid_t domid, #ifdef CONFIG_HAS_PCI INIT_LIST_HEAD(&d->pdev_list); +#ifdef CONFIG_HAS_VPCI + rwlock_init(&d->vpci_rwlock); +#endif #endif /* All error paths can depend on the above setup. */ diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 07d1986d33..0afcb59af0 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -57,6 +57,11 @@ void pcidevs_lock(void) spin_lock_recursive(&_pcidevs_lock); } +int pcidevs_trylock(void) +{ + return spin_trylock_recursive(&_pcidevs_lock); +} + void pcidevs_unlock(void) { spin_unlock_recursive(&_pcidevs_lock); diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index ec2e978a4e..23b5223adf 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -152,12 +152,14 @@ bool vpci_process_pending(struct vcpu *v) if ( rc == -ERESTART ) return true; + read_lock(&v->domain->vpci_rwlock); spin_lock(&v->vpci.pdev->vpci->lock); /* Disable memory decoding unconditionally on failure. */ modify_decoding(v->vpci.pdev, rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd, !rc && v->vpci.rom_only); spin_unlock(&v->vpci.pdev->vpci->lock); + read_unlock(&v->domain->vpci_rwlock); rangeset_destroy(v->vpci.mem); v->vpci.mem = NULL; @@ -181,8 +183,20 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev, struct map_data data = { .d = d, .map = true }; int rc; + ASSERT(rw_is_write_locked(&d->vpci_rwlock)); + while ( (rc = rangeset_consume_ranges(mem, map_range, &data)) == -ERESTART ) + { + /* + * It's safe to drop and reacquire the lock in this context + * without risking pdev disappearing because devices cannot be + * removed until the initial domain has been started. + */ + write_unlock(&d->vpci_rwlock); process_pending_softirqs(); + write_lock(&d->vpci_rwlock); + } + rangeset_destroy(mem); if ( !rc ) modify_decoding(pdev, cmd, false); @@ -213,6 +227,7 @@ static void defer_map(struct domain *d, struct pci_dev *pdev, raise_softirq(SCHEDULE_SOFTIRQ); } +/* This must hold domain's vpci_rwlock in write mode. */ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) { struct vpci_header *header = &pdev->vpci->header; @@ -287,6 +302,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) * Check for overlaps with other BARs. Note that only BARs that are * currently mapped (enabled) are checked for overlaps. */ + pcidevs_lock(); for_each_pdev ( pdev->domain, tmp ) { if ( tmp == pdev ) @@ -326,10 +342,12 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n", start, end, rc); rangeset_destroy(mem); + pcidevs_unlock(); return rc; } } } + pcidevs_unlock(); ASSERT(dev); @@ -482,6 +500,8 @@ static int cf_check init_bars(struct pci_dev *pdev) struct vpci_bar *bars = header->bars; int rc; + ASSERT(rw_is_write_locked(&pdev->domain->vpci_rwlock)); + switch ( pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f ) { case PCI_HEADER_TYPE_NORMAL: @@ -570,6 +590,8 @@ static int cf_check init_bars(struct pci_dev *pdev) } } + ASSERT(!header->rom_reg); + /* Check expansion ROM. */ rc = pci_size_mem_bar(pdev->sbdf, rom_reg, &addr, &size, PCI_BAR_ROM); if ( rc > 0 && size ) @@ -586,12 +608,42 @@ static int cf_check init_bars(struct pci_dev *pdev) 4, rom); if ( rc ) rom->type = VPCI_BAR_EMPTY; + + header->rom_reg = rom_reg; } return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0; } REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE); +static bool overlap(unsigned int r1_offset, unsigned int r1_size, + unsigned int r2_offset, unsigned int r2_size) +{ + /* Return true if there is an overlap. */ + return r1_offset < r2_offset + r2_size && r2_offset < r1_offset + r1_size; +} + +bool vpci_header_need_write_lock(const struct pci_dev *pdev, + unsigned int start, unsigned int size) +{ + /* + * Writing the command register and ROM BAR register may trigger + * modify_bars to run, which in turn may access multiple pdevs while + * checking for the existing BAR's overlap. The overlapping check, if done + * under the read lock, requires vpci->lock to be acquired on both devices + * being compared, which may produce a deadlock. At the same time it is not + * possible to upgrade read lock to write lock in such a case. + * Check which registers are going to be written and return true if lock + * needs to be acquired in write mode. + */ + if ( overlap(start, size, PCI_COMMAND, 2) || + (pdev->vpci->header.rom_reg && + overlap(start, size, pdev->vpci->header.rom_reg, 4)) ) + return true; + + return false; +} + /* * Local variables: * mode: C diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c index 8f2b59e61a..e7d1f916a0 100644 --- a/xen/drivers/vpci/msi.c +++ b/xen/drivers/vpci/msi.c @@ -190,6 +190,8 @@ static int cf_check init_msi(struct pci_dev *pdev) uint16_t control; int ret; + ASSERT(rw_is_write_locked(&pdev->domain->vpci_rwlock)); + if ( !pos ) return 0; @@ -265,7 +267,7 @@ REGISTER_VPCI_INIT(init_msi, VPCI_PRIORITY_LOW); void vpci_dump_msi(void) { - const struct domain *d; + struct domain *d; rcu_read_lock(&domlist_read_lock); for_each_domain ( d ) @@ -277,6 +279,15 @@ void vpci_dump_msi(void) printk("vPCI MSI/MSI-X d%d\n", d->domain_id); + if ( !read_trylock(&d->vpci_rwlock) ) + continue; + + if ( !pcidevs_trylock() ) + { + read_unlock(&d->vpci_rwlock); + continue; + } + for_each_pdev ( d, pdev ) { const struct vpci_msi *msi; @@ -318,14 +329,22 @@ void vpci_dump_msi(void) * holding the lock. */ printk("unable to print all MSI-X entries: %d\n", rc); - process_pending_softirqs(); - continue; + goto pdev_done; } } spin_unlock(&pdev->vpci->lock); + pdev_done: + pcidevs_unlock(); + read_unlock(&d->vpci_rwlock); + process_pending_softirqs(); + + read_lock(&d->vpci_rwlock); + pcidevs_lock(); } + pcidevs_unlock(); + read_unlock(&d->vpci_rwlock); } rcu_read_unlock(&domlist_read_lock); } diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c index 25bde77586..b5a7dfdf9c 100644 --- a/xen/drivers/vpci/msix.c +++ b/xen/drivers/vpci/msix.c @@ -143,6 +143,7 @@ static void cf_check control_write( pci_conf_write16(pdev->sbdf, reg, val); } +/* This must hold domain's vpci_rwlock in write mode. */ static struct vpci_msix *msix_find(const struct domain *d, unsigned long addr) { struct vpci_msix *msix; @@ -163,7 +164,13 @@ static struct vpci_msix *msix_find(const struct domain *d, unsigned long addr) static int cf_check msix_accept(struct vcpu *v, unsigned long addr) { - return !!msix_find(v->domain, addr); + int rc; + + read_lock(&v->domain->vpci_rwlock); + rc = !!msix_find(v->domain, addr); + read_unlock(&v->domain->vpci_rwlock); + + return rc; } static bool access_allowed(const struct pci_dev *pdev, unsigned long addr, @@ -358,21 +365,34 @@ static int adjacent_read(const struct domain *d, const struct vpci_msix *msix, static int cf_check msix_read( struct vcpu *v, unsigned long addr, unsigned int len, unsigned long *data) { - const struct domain *d = v->domain; - struct vpci_msix *msix = msix_find(d, addr); + struct domain *d = v->domain; + struct vpci_msix *msix; const struct vpci_msix_entry *entry; unsigned int offset; *data = ~0ul; + read_lock(&d->vpci_rwlock); + + msix = msix_find(d, addr); if ( !msix ) + { + read_unlock(&d->vpci_rwlock); return X86EMUL_RETRY; + } if ( adjacent_handle(msix, addr) ) - return adjacent_read(d, msix, addr, len, data); + { + int rc = adjacent_read(d, msix, addr, len, data); + read_unlock(&d->vpci_rwlock); + return rc; + } if ( !access_allowed(msix->pdev, addr, len) ) + { + read_unlock(&d->vpci_rwlock); return X86EMUL_OKAY; + } spin_lock(&msix->pdev->vpci->lock); entry = get_entry(msix, addr); @@ -404,6 +424,7 @@ static int cf_check msix_read( break; } spin_unlock(&msix->pdev->vpci->lock); + read_unlock(&d->vpci_rwlock); return X86EMUL_OKAY; } @@ -491,19 +512,32 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix, static int cf_check msix_write( struct vcpu *v, unsigned long addr, unsigned int len, unsigned long data) { - const struct domain *d = v->domain; - struct vpci_msix *msix = msix_find(d, addr); + struct domain *d = v->domain; + struct vpci_msix *msix; struct vpci_msix_entry *entry; unsigned int offset; + read_lock(&d->vpci_rwlock); + + msix = msix_find(d, addr); if ( !msix ) + { + read_unlock(&d->vpci_rwlock); return X86EMUL_RETRY; + } if ( adjacent_handle(msix, addr) ) - return adjacent_write(d, msix, addr, len, data); + { + int rc = adjacent_write(d, msix, addr, len, data); + read_unlock(&d->vpci_rwlock); + return rc; + } if ( !access_allowed(msix->pdev, addr, len) ) + { + read_unlock(&d->vpci_rwlock); return X86EMUL_OKAY; + } spin_lock(&msix->pdev->vpci->lock); entry = get_entry(msix, addr); @@ -579,6 +613,7 @@ static int cf_check msix_write( break; } spin_unlock(&msix->pdev->vpci->lock); + read_unlock(&d->vpci_rwlock); return X86EMUL_OKAY; } @@ -665,6 +700,8 @@ static int cf_check init_msix(struct pci_dev *pdev) struct vpci_msix *msix; int rc; + ASSERT(rw_is_write_locked(&pdev->domain->vpci_rwlock)); + msix_offset = pci_find_cap_offset(pdev->seg, pdev->bus, slot, func, PCI_CAP_ID_MSIX); if ( !msix_offset ) diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index 652807a4a4..1270174e78 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -38,20 +38,32 @@ extern vpci_register_init_t *const __end_vpci_array[]; void vpci_remove_device(struct pci_dev *pdev) { - if ( !has_vpci(pdev->domain) || !pdev->vpci ) + struct vpci *vpci; + + if ( !has_vpci(pdev->domain) ) return; - spin_lock(&pdev->vpci->lock); + write_lock(&pdev->domain->vpci_rwlock); + if ( !pdev->vpci ) + { + write_unlock(&pdev->domain->vpci_rwlock); + return; + } + + vpci = pdev->vpci; + pdev->vpci = NULL; + write_unlock(&pdev->domain->vpci_rwlock); + while ( !list_empty(&pdev->vpci->handlers) ) { - struct vpci_register *r = list_first_entry(&pdev->vpci->handlers, + struct vpci_register *r = list_first_entry(&vpci->handlers, struct vpci_register, node); list_del(&r->node); xfree(r); } - spin_unlock(&pdev->vpci->lock); + if ( pdev->vpci->msix ) { unsigned int i; @@ -61,29 +73,33 @@ void vpci_remove_device(struct pci_dev *pdev) if ( pdev->vpci->msix->table[i] ) iounmap(pdev->vpci->msix->table[i]); } - xfree(pdev->vpci->msix); - xfree(pdev->vpci->msi); - xfree(pdev->vpci); - pdev->vpci = NULL; + xfree(vpci->msix); + xfree(vpci->msi); + xfree(vpci); } int vpci_add_handlers(struct pci_dev *pdev) { + struct vpci *vpci; unsigned int i; int rc = 0; if ( !has_vpci(pdev->domain) ) return 0; + vpci = xzalloc(struct vpci); + if ( !vpci ) + return -ENOMEM; + + INIT_LIST_HEAD(&vpci->handlers); + spin_lock_init(&vpci->lock); + + write_lock(&pdev->domain->vpci_rwlock); + /* We should not get here twice for the same device. */ ASSERT(!pdev->vpci); - pdev->vpci = xzalloc(struct vpci); - if ( !pdev->vpci ) - return -ENOMEM; - - INIT_LIST_HEAD(&pdev->vpci->handlers); - spin_lock_init(&pdev->vpci->lock); + pdev->vpci = vpci; for ( i = 0; i < NUM_VPCI_INIT; i++ ) { @@ -91,6 +107,7 @@ int vpci_add_handlers(struct pci_dev *pdev) if ( rc ) break; } + write_unlock(&pdev->domain->vpci_rwlock); if ( rc ) vpci_remove_device(pdev); @@ -139,6 +156,7 @@ uint32_t cf_check vpci_hw_read32( return pci_conf_read32(pdev->sbdf, reg); } +/* This must hold domain's vpci_rwlock in write mode. */ int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler, vpci_write_t *write_handler, unsigned int offset, unsigned int size, void *data) @@ -162,8 +180,6 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler, r->offset = offset; r->private = data; - spin_lock(&vpci->lock); - /* The list of handlers must be kept sorted at all times. */ list_for_each ( prev, &vpci->handlers ) { @@ -175,25 +191,23 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler, break; if ( cmp == 0 ) { - spin_unlock(&vpci->lock); xfree(r); return -EEXIST; } } list_add_tail(&r->node, prev); - spin_unlock(&vpci->lock); return 0; } +/* This must hold domain's vpci_rwlock in write mode. */ int vpci_remove_register(struct vpci *vpci, unsigned int offset, unsigned int size) { const struct vpci_register r = { .offset = offset, .size = size }; struct vpci_register *rm; - spin_lock(&vpci->lock); list_for_each_entry ( rm, &vpci->handlers, node ) { int cmp = vpci_register_cmp(&r, rm); @@ -205,14 +219,12 @@ int vpci_remove_register(struct vpci *vpci, unsigned int offset, if ( !cmp && rm->offset == offset && rm->size == size ) { list_del(&rm->node); - spin_unlock(&vpci->lock); xfree(rm); return 0; } if ( cmp <= 0 ) break; } - spin_unlock(&vpci->lock); return -ENOENT; } @@ -320,7 +332,7 @@ static uint32_t merge_result(uint32_t data, uint32_t new, unsigned int size, uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) { - const struct domain *d = current->domain; + struct domain *d = current->domain; const struct pci_dev *pdev; const struct vpci_register *r; unsigned int data_offset = 0; @@ -333,10 +345,18 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) } /* Find the PCI dev matching the address. */ + pcidevs_lock(); pdev = pci_get_pdev(d, sbdf); - if ( !pdev || !pdev->vpci ) + pcidevs_unlock(); + if ( !pdev ) return vpci_read_hw(sbdf, reg, size); + read_lock(&d->vpci_rwlock); + if ( !pdev->vpci ) + { + read_unlock(&d->vpci_rwlock); + return vpci_read_hw(sbdf, reg, size); + } spin_lock(&pdev->vpci->lock); /* Read from the hardware or the emulated register handlers. */ @@ -381,6 +401,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) ASSERT(data_offset < size); } spin_unlock(&pdev->vpci->lock); + read_unlock(&d->vpci_rwlock); if ( data_offset < size ) { @@ -423,11 +444,12 @@ static void vpci_write_helper(const struct pci_dev *pdev, void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, uint32_t data) { - const struct domain *d = current->domain; + struct domain *d = current->domain; const struct pci_dev *pdev; const struct vpci_register *r; unsigned int data_offset = 0; const unsigned long *ro_map = pci_get_ro_map(sbdf.seg); + bool write_locked = false; if ( !size ) { @@ -443,14 +465,38 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, * Find the PCI dev matching the address. * Passthrough everything that's not trapped. */ + pcidevs_lock(); pdev = pci_get_pdev(d, sbdf); - if ( !pdev || !pdev->vpci ) + pcidevs_unlock(); + if ( !pdev ) { vpci_write_hw(sbdf, reg, size, data); return; } - spin_lock(&pdev->vpci->lock); + if ( vpci_header_need_write_lock(pdev, reg, size) ) + { + /* Gain exclusive access to all of the domain pdevs vpci. */ + write_lock(&d->vpci_rwlock); + if ( !pdev->vpci ) + { + write_unlock(&d->vpci_rwlock); + vpci_write_hw(sbdf, reg, size, data); + return; + } + write_locked = true; + } + else + { + read_lock(&d->vpci_rwlock); + if ( !pdev->vpci ) + { + read_unlock(&d->vpci_rwlock); + vpci_write_hw(sbdf, reg, size, data); + return; + } + spin_lock(&pdev->vpci->lock); + } /* Write the value to the hardware or emulated registers. */ list_for_each_entry ( r, &pdev->vpci->handlers, node ) @@ -485,7 +531,14 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, break; ASSERT(data_offset < size); } - spin_unlock(&pdev->vpci->lock); + + if ( write_locked ) + write_unlock(&d->vpci_rwlock); + else + { + spin_unlock(&pdev->vpci->lock); + read_unlock(&d->vpci_rwlock); + } if ( data_offset < size ) /* Tailing gap, write the remaining. */ diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 5975ca2f30..4512910dca 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -157,6 +157,7 @@ struct pci_dev { */ void pcidevs_lock(void); +int pcidevs_trylock(void); void pcidevs_unlock(void); bool_t __must_check pcidevs_locked(void); diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 85242a73d3..78227b7a1d 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -460,6 +460,9 @@ struct domain #ifdef CONFIG_HAS_PCI struct list_head pdev_list; +#ifdef CONFIG_HAS_VPCI + rwlock_t vpci_rwlock; +#endif #endif #ifdef CONFIG_HAS_PASSTHROUGH diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index 0b8a2a3c74..3a7fcc4463 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -57,6 +57,9 @@ uint32_t cf_check vpci_hw_read32( */ bool __must_check vpci_process_pending(struct vcpu *v); +bool vpci_header_need_write_lock(const struct pci_dev *pdev, + unsigned int start, unsigned int size); + struct vpci { /* List of vPCI handlers for a device. */ struct list_head handlers; @@ -83,6 +86,9 @@ struct vpci { } bars[PCI_HEADER_NORMAL_NR_BARS + 1]; /* At most 6 BARS + 1 expansion ROM BAR. */ + /* Offset to the ROM BAR register if any. */ + unsigned int rom_reg; + /* * Store whether the ROM enable bit is set (doesn't imply ROM BAR * is mapped into guest p2m) if there's a ROM BAR on the device.