[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH RFC] pci/ats: do not allow broken devices to be assigned


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Thu, 24 Feb 2022 15:53:37 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • 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=u6OFIDOaIaj/zUecjHYavG6hXt6oRTmZOnzyaM9GqfM=; b=YPBRReCQiW3d70hUcnN0+YiRg4/v/KfrS4V816dwDlk7s1c8JLjPQzE2ygv1dZBS2IEu+gRX0zrd95Z04nmxZdjGEEEyvuDyxPtiU0o4VHkvBd3Ra711M7wzv2DeRHFyVax3z/vqruzQAppwt/kmnGc406BJgAZfehmslSdaDGt5qEDM03WMqfHQG7iJC0CxOLrub2yelbdzJylC34zWIt82qo8g1IJ8hH7SK/NOisw7NmTfCMIZ2E94euVBG3m3Hff7D3AwbXnaZJxfX9fvFjxyhtMf/de0fo981cq01Bp3cRf6mvo0ndjmIglgQ46JvOsoHOz1uSFZGS+D69UqBw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jUdxex0QcaVl3WAnHng/161OnUUIZ8puZJaBN6rzr/ybZ2WswtJB7BpsW+kX50xNCxcCje91MCiIxMxwHHd4cwNEEQr5BG4cY6iHx/IOBAsdGDW5Cbg94UCeeLaicFEzw9Evlrbg3+vUTw9BeECSRVqnTkohMAPQ6vVmlTq2wNw25l19n+i9bqzTJQZ5cudQl2Rnfs2P68hQXgFpnYiaJn4ONCj4z/llYGqZQinNFHU8gD6YfB+hxKregdaruNCBnI0qXco/6aJc1hHrmFu6pHbVv4Dtesz4yjbtAJWKugnX1ii1OlaMkEIyJzCtG+pJyAvaPaMuvpXgz4OJ3M0JaA==
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Paul Durrant <paul@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>, Oleksandr Andrushchenko <andr2000@xxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 24 Feb 2022 14:54:04 +0000
  • Ironport-data: A9a23:Fc2FVKhkJ3XOC6ZOLAOV8dgcX161lhAKZh0ujC45NGQN5FlHY01je htvCzjSbveMZGL3f9B2PtnnoBkDuZ6GzYcwHgpt/C0zRHgb9cadCdqndUqhZCn6wu8v7a5EA 2fyTvGacajYm1eF/k/F3oDJ9CU6jefSLlbFILas1hpZHGeIcw98z0M78wIFqtQw24LhWVnR4 YiaT/D3YzdJ5RYlagr41IrbwP9flKyaVOQw5wFWiVhj5TcyplFNZH4tDfjZw0jQG+G4KtWSV efbpIxVy0uCl/sb5nFJpZ6gGqECaua60QFjERO6UYD66vRJjnRaPqrWqJPwwKqY4tmEt4kZ9 TlDiXC/YQMqO72SquBeagBzSAJbA5Nl45P2cGfq5KR/z2WeG5ft6/BnDUVwNowE4OdnR2pJ8 JT0KhhUMErF3bjvhuvmFK883azPL+GyVG8bkmtnwjzDS+4vXLjIQrnQ5M8e1zA17ixLNaiCN pNBMGo1BPjGSy9uYlgLDLwVoNqXlz68XxZK8niqmpNitgA/yyQuieOwYbI5YOeiRshLn0Deu mPP+Uz4BA0XMJqUzj/t2m2orv/Cm2X8Qo16PK218LtmjUOewkQXCQYKTh2rrP+hkEm8VtlDb UsO9UIGpK4+7hbzFoHVUBixoXrCtRkZM/JIGvA+wBGAzOzT+QnxLmoOQyNFadcmnNQrXjFs3 ViM9/vyHiBmurCRTXOb95+XoCm0NCxTKnUNDQcGUA8E7t/LsIw1yBXVQb5e/LWd14OvX2uqm nbT8XZ41+57YdM3O7uT+VPCk2yeotvwVyVuxSDpYEG+wjIoe9vwD2C30mTz4fFFJYefa1COu nkYhsSThNwz4YGxeD+lG7tUQuzwjxqRGHiF2AM0QcF9n9i40yP7JehtDCdCyFCF2yruURvge wfttAxY//e/11P6PPYsM+pd5ynHpJUM9OgJtNiJNrKigbArLWdrGR2Cg2bKgQgBd2B2zMkC1 W+zK5rEMJrjIf0PIMCKb+kcy6Q34Ss12HneQ5v2pzz+j+bDOiLPFe1ebwPVBgzc0E9iiF+Lm zq4H5HXoyizrcWkOnWHmWLtBQpiwYcH6WDe9JUMK7/rzvtOE2A9Ef7BqY7NiKQ+95m5Ytzgp ynnMmcBkQKXrSSedW2iNyAyAJuyDM0XhS9qYkQR0aOAhiFLjXCHt/xEKfPavNAPqYRe8BKDZ 6NbI57ZWqwXEFwqOV01NPHAkWCrTzzy7SqmNCu5ejkvOZlmQg3C4Nj/eQXzsiIJC0KKWQEW+ eHIOt/zKXbbezlfMQ==
  • Ironport-hdrordr: A9a23:GI4coKD3WRSd4i/lHeg1sceALOsnbusQ8zAXPh9KJiC9I/b1qy nxppkmPH/P6Qr4WBkb6Le90Y27MAnhHPlOkPQs1NaZLXLbUQ6TQr2KgrGSoQEIdxeOk9K1kJ 0QD5SWa+eAfGSS7/yKmTVQeuxIqLLskNHKuQ6d9QYUcegDUdAf0+4TMHf8LqQZfngjOXJvf6 Dsmfav6gDQMkj+Ka+Adww4dtmGg+eOuIPtYBYACRJiwA6SjQmw4Lq/NxSDxB8RXx5G3L9nqA H+4kHEz5Tml8v+5g7X1mfV4ZgTsNz9yuFbDMjJrsQOMD3jhiuheYwkcbyfuzIepv2p9T8R4Z PxiiZlG/42x2Laf2mzrxeo8w780Aw243un8lOciWuLm72OeBsKT+56wa5JeBrQ7EQt+Ptm1r hQ4m6fv51LSTvdgSXU/bHzJl9Xv3vxhUBnvf8YjnRZX4dbQqRWt5Yj8ERcF4pFND7m6bogDP JlAKjnlblrmGuhHjDkV1RUsZ+RtixZJGbFfqFCgL3Y79FupgE586NCr/Zv20vp9/oGOu55Dq r/Q+BVfYp1P7wrhJJGdZc8qPSMex7wqDL3QSuvyAfcZek600ykke+C3Fxy3pDsRKA1
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Thu, Feb 24, 2022 at 01:58:31PM +0100, Jan Beulich wrote:
> On 24.02.2022 13:43, Roger Pau Monne wrote:
> > Introduce a new field to mark devices as broken: having it set
> > prevents the device from being assigned to domains. Use the field in
> > order to mark ATS devices that have failed a flush as broken, thus
> > preventing them to be assigned to any guest.
> > 
> > This allows the device IOMMU context entry to be cleaned up properly,
> > as calling _pci_hide_device will just change the ownership of the
> > device, but the IOMMU context entry of the device would be left as-is.
> > It would also leak a Domain ID, as removing the device from it's
> > previous owner will allow releasing the DID used by the device without
> > having cleaned up the context entry.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > RFC: I haven't tested the code path, as I have no ATS devices on the
> > box I'm currently testing on. In any case, ATS is not supported, and
> > removing the call to _pci_hide_device in iommu_dev_iotlb_flush_timeout
> > should allow to remove the dependency on recursive pcidevs lock.
> 
> No objection in principle. Whether this is the only dependency on
> recursive pcidevs lock isn't really know though, is it?

Indeed. I didn't word this clearly. The recursive lock was introduced
for this specific use case. Whether we have gained more recursive
paths in the meantime I haven't assessed.

> > TBD: it's unclear whether we still need the pcidevs_lock in
> > iommu_dev_iotlb_flush_timeout. The caller of
> > iommu_dev_iotlb_flush_timeout is already bogus as it iterates over a
> > list of pdevs without holding the pcidevs_lock.
> 
> Analysis of whether / where recursive uses are needed should imo
> include cases where the lock ought to be held, but currently isn't
> (like apparently this case).

Well, I'm not opposed to that. I think just aiming to get the current
usages analyzed will already be hard.

> > @@ -1487,6 +1487,11 @@ static int assign_device(struct domain *d, u16 seg, 
> > u8 bus, u8 devfn, u32 flag)
> >      ASSERT(pdev && (pdev->domain == hardware_domain ||
> >                      pdev->domain == dom_io));
> >  
> > +    /* Do not allow broken devices to be assigned. */
> > +    rc = -EBADF;
> > +    if ( pdev->broken )
> > +        goto done;
> 
> I think this wants exceptions for Dom0 and DomIO. An admin may be
> able to fix things in Dom0, e.g. by updating device firmware.

Doesn't a device get assigned to DomIO (or Dom0 if not using quarantine
mode), and then when deassigned from DomIO gets assigned to Dom0?

So there's no usage of assign_device in the path that (re)assigns a
device used by a guest into Dom0?

Or would you rather imply that pdev->broken should get cleared at some
point (ie: when the device is assigned back to Dom0)?

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.