diff mbox series

[RFC,3/7] iommufd: Add iommufd_device_bind_pasid()

Message ID 20231009085123.463179-4-yi.l.liu@intel.com (mailing list archive)
State New
Headers show
Series Add SIOV virtual device support | expand

Commit Message

Yi Liu Oct. 9, 2023, 8:51 a.m. UTC 
From: Kevin Tian <kevin.tian@intel.com>

Intel SIOV allows creating virtual devices of which the vRID is
represented by a pasid of a physical device. So such device can be
bound to an iommufd as physical device does and then later be attached
to an IOAS/hwpt using that pasid.

Binding a virtual device has different security contract comparing
to binding a physical device. There is no DMA ownership claim per
pasid since the parent device is already claimed by the parent driver
including the entire pasid space. With that we simply store the pasid
in the object once it passes other checks.

Signed-off-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/device.c          | 72 ++++++++++++++++++++++++-
 drivers/iommu/iommufd/iommufd_private.h |  7 +++
 include/linux/iommufd.h                 |  3 ++
 3 files changed, 80 insertions(+), 2 deletions(-)

Comments

Tian, Kevin Oct. 10, 2023, 8:19 a.m. UTC | #1 
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, October 9, 2023 4:51 PM
>
> +struct iommufd_device *iommufd_device_bind_pasid(struct iommufd_ctx
> *ictx,
> +						 struct device *dev,
> +						 u32 pasid, u32 *id)
> +{
> +	struct iommufd_device *idev;
> +	int rc;
> +
> +	/*
> +	 * iommufd always sets IOMMU_CACHE because we offer no way for
> userspace
> +	 * to restore cache coherency.
> +	 */
> +	if (!device_iommu_capable(dev, IOMMU_CAP_CACHE_COHERENCY))
> +		return ERR_PTR(-EINVAL);
> +
> +	/*
> +	 * No iommu supports pasid-granular msi message today. Here we
> +	 * just check whether the parent device can do safe interrupts.
> +	 * Isolation between virtual devices within the parent device
> +	 * relies on the parent driver to enforce.
> +	 */
> +	if (!iommufd_selftest_is_mock_dev(dev) &&
> +	    !msi_device_has_isolated_msi(dev)) {
> +		rc = iommufd_allow_unsafe_interrupts(dev);
> +		if (rc)
> +			return ERR_PTR(rc);
> +	}
> +

Only MemWr w/o pasid can be interpreted as an interrupt message
then we need msi isolation to protect.

But for SIOV all MemWr's are tagged with a pasid hence can never
trigger an interrupt. From this angle looks this check is unnecessary.
Yi Liu Nov. 8, 2023, 7:45 a.m. UTC | #2 
On 2023/10/10 16:19, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Monday, October 9, 2023 4:51 PM
>>
>> +struct iommufd_device *iommufd_device_bind_pasid(struct iommufd_ctx
>> *ictx,
>> +						 struct device *dev,
>> +						 u32 pasid, u32 *id)
>> +{
>> +	struct iommufd_device *idev;
>> +	int rc;
>> +
>> +	/*
>> +	 * iommufd always sets IOMMU_CACHE because we offer no way for
>> userspace
>> +	 * to restore cache coherency.
>> +	 */
>> +	if (!device_iommu_capable(dev, IOMMU_CAP_CACHE_COHERENCY))
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	/*
>> +	 * No iommu supports pasid-granular msi message today. Here we
>> +	 * just check whether the parent device can do safe interrupts.
>> +	 * Isolation between virtual devices within the parent device
>> +	 * relies on the parent driver to enforce.
>> +	 */
>> +	if (!iommufd_selftest_is_mock_dev(dev) &&
>> +	    !msi_device_has_isolated_msi(dev)) {
>> +		rc = iommufd_allow_unsafe_interrupts(dev);
>> +		if (rc)
>> +			return ERR_PTR(rc);
>> +	}
>> +
> 
> Only MemWr w/o pasid can be interpreted as an interrupt message
> then we need msi isolation to protect.

yes.

> 
> But for SIOV all MemWr's are tagged with a pasid hence can never
> trigger an interrupt. From this angle looks this check is unnecessary.

But the interrupts out from a SIOV virtual device do not have pasid (at
least today). Seems still need a check here if we consider this bind for
a SIOV virtual device just like binding a physical device.
Tian, Kevin Nov. 8, 2023, 8:46 a.m. UTC | #3 
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, November 8, 2023 3:45 PM
> 
> On 2023/10/10 16:19, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Monday, October 9, 2023 4:51 PM
> >>
> >> +struct iommufd_device *iommufd_device_bind_pasid(struct
> iommufd_ctx
> >> *ictx,
> >> +						 struct device *dev,
> >> +						 u32 pasid, u32 *id)
> >> +{
> >> +	struct iommufd_device *idev;
> >> +	int rc;
> >> +
> >> +	/*
> >> +	 * iommufd always sets IOMMU_CACHE because we offer no way for
> >> userspace
> >> +	 * to restore cache coherency.
> >> +	 */
> >> +	if (!device_iommu_capable(dev, IOMMU_CAP_CACHE_COHERENCY))
> >> +		return ERR_PTR(-EINVAL);
> >> +
> >> +	/*
> >> +	 * No iommu supports pasid-granular msi message today. Here we
> >> +	 * just check whether the parent device can do safe interrupts.
> >> +	 * Isolation between virtual devices within the parent device
> >> +	 * relies on the parent driver to enforce.
> >> +	 */
> >> +	if (!iommufd_selftest_is_mock_dev(dev) &&
> >> +	    !msi_device_has_isolated_msi(dev)) {
> >> +		rc = iommufd_allow_unsafe_interrupts(dev);
> >> +		if (rc)
> >> +			return ERR_PTR(rc);
> >> +	}
> >> +
> >
> > Only MemWr w/o pasid can be interpreted as an interrupt message
> > then we need msi isolation to protect.
> 
> yes.
> 
> >
> > But for SIOV all MemWr's are tagged with a pasid hence can never
> > trigger an interrupt. From this angle looks this check is unnecessary.
> 
> But the interrupts out from a SIOV virtual device do not have pasid (at
> least today). Seems still need a check here if we consider this bind for
> a SIOV virtual device just like binding a physical device.
> 

this check assumes the device is trusted. as long as there is no way
for malicious guest to generate arbitrary interrupt messages then
it's fine.

for physical device a MemWr can be interpreted as interrupt so
we need msi isolation.

for SIOV all MemWr has pasid then we don't have such worry.
IMS is under host's control so interrupt messages are already
sanitized.
Yi Liu Nov. 8, 2023, 9:03 a.m. UTC | #4 
On 2023/11/8 16:46, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Wednesday, November 8, 2023 3:45 PM
>>
>> On 2023/10/10 16:19, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Monday, October 9, 2023 4:51 PM
>>>>
>>>> +struct iommufd_device *iommufd_device_bind_pasid(struct
>> iommufd_ctx
>>>> *ictx,
>>>> +						 struct device *dev,
>>>> +						 u32 pasid, u32 *id)
>>>> +{
>>>> +	struct iommufd_device *idev;
>>>> +	int rc;
>>>> +
>>>> +	/*
>>>> +	 * iommufd always sets IOMMU_CACHE because we offer no way for
>>>> userspace
>>>> +	 * to restore cache coherency.
>>>> +	 */
>>>> +	if (!device_iommu_capable(dev, IOMMU_CAP_CACHE_COHERENCY))
>>>> +		return ERR_PTR(-EINVAL);
>>>> +
>>>> +	/*
>>>> +	 * No iommu supports pasid-granular msi message today. Here we
>>>> +	 * just check whether the parent device can do safe interrupts.
>>>> +	 * Isolation between virtual devices within the parent device
>>>> +	 * relies on the parent driver to enforce.
>>>> +	 */
>>>> +	if (!iommufd_selftest_is_mock_dev(dev) &&
>>>> +	    !msi_device_has_isolated_msi(dev)) {
>>>> +		rc = iommufd_allow_unsafe_interrupts(dev);
>>>> +		if (rc)
>>>> +			return ERR_PTR(rc);
>>>> +	}
>>>> +
>>>
>>> Only MemWr w/o pasid can be interpreted as an interrupt message
>>> then we need msi isolation to protect.
>>
>> yes.
>>
>>>
>>> But for SIOV all MemWr's are tagged with a pasid hence can never
>>> trigger an interrupt. From this angle looks this check is unnecessary.
>>
>> But the interrupts out from a SIOV virtual device do not have pasid (at
>> least today). Seems still need a check here if we consider this bind for
>> a SIOV virtual device just like binding a physical device.
>>
> 
> this check assumes the device is trusted. as long as there is no way
> for malicious guest to generate arbitrary interrupt messages then
> it's fine.
> 
> for physical device a MemWr can be interpreted as interrupt so
> we need msi isolation.
> 
> for SIOV all MemWr has pasid then we don't have such worry.
> IMS is under host's control so interrupt messages are already
> sanitized.

sure. this makes sense to me now.:)
diff mbox series

Patch

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 9dd76d92b7f6..35c1419ee96b 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -5,6 +5,7 @@ 
 #include <linux/slab.h>
 #include <linux/iommu.h>
 #include <uapi/linux/iommufd.h>
+#include <linux/msi.h>
 #include "../iommu-priv.h"
 
 #include "io_pagetable.h"
@@ -139,8 +140,10 @@  void iommufd_device_destroy(struct iommufd_object *obj)
 	WARN_ON(!xa_empty(&idev->pasid_hwpts));
 	if (idev->has_user_data)
 		dev_iommu_ops(idev->dev)->unset_dev_user_data(idev->dev);
-	iommu_device_release_dma_owner(idev->dev);
-	iommufd_put_group(idev->igroup);
+	if (idev->igroup) {
+		iommu_device_release_dma_owner(idev->dev);
+		iommufd_put_group(idev->igroup);
+	}
 	if (!iommufd_selftest_is_mock_dev(idev->dev))
 		iommufd_ctx_put(idev->ictx);
 }
@@ -257,6 +260,71 @@  struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_device_bind, IOMMUFD);
 
+/**
+ * iommufd_device_bind_pasid - Bind a virtual device to an iommu fd
+ * @ictx: iommufd file descriptor
+ * @dev: Pointer to the parent physical device struct
+ * @pasid: the pasid value representing vRID of this virtual device
+ * @id: Output ID number to return to userspace for this device
+ *
+ * The virtual device always tags its DMA with the provided pasid.
+ * A successful bind allows the pasid to be used in other iommufd
+ * operations e.g. attach/detach and returns struct iommufd_device
+ * pointer, otherwise returns error pointer.
+ *
+ * There is no ownership check per pasid. A driver using this API
+ * must already claim the DMA ownership over the parent device and
+ * the pasid is allocated by the driver itself.
+ *
+ * PASID is a device capability so unlike iommufd_device_bind() it
+ * has no iommu group associated.
+ *
+ * The caller must undo this with iommufd_device_unbind()
+ */
+struct iommufd_device *iommufd_device_bind_pasid(struct iommufd_ctx *ictx,
+						 struct device *dev,
+						 u32 pasid, u32 *id)
+{
+	struct iommufd_device *idev;
+	int rc;
+
+	/*
+	 * iommufd always sets IOMMU_CACHE because we offer no way for userspace
+	 * to restore cache coherency.
+	 */
+	if (!device_iommu_capable(dev, IOMMU_CAP_CACHE_COHERENCY))
+		return ERR_PTR(-EINVAL);
+
+	/*
+	 * No iommu supports pasid-granular msi message today. Here we
+	 * just check whether the parent device can do safe interrupts.
+	 * Isolation between virtual devices within the parent device
+	 * relies on the parent driver to enforce.
+	 */
+	if (!iommufd_selftest_is_mock_dev(dev) &&
+	    !msi_device_has_isolated_msi(dev)) {
+		rc = iommufd_allow_unsafe_interrupts(dev);
+		if (rc)
+			return ERR_PTR(rc);
+	}
+
+	idev = iommufd_alloc_device(ictx, dev);
+	if (IS_ERR(idev))
+		return idev;
+	idev->default_pasid = pasid;
+
+	/*
+	 * If the caller fails after this success it must call
+	 * iommufd_unbind_device() which is safe since we hold this refcount.
+	 * This also means the device is a leaf in the graph and no other
+	 * object can take a reference on it.
+	 */
+	iommufd_object_finalize(ictx, &idev->obj);
+	*id = idev->obj.id;
+	return idev;
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_device_bind_pasid, IOMMUFD);
+
 /**
  * iommufd_ctx_has_group - True if any device within the group is bound
  *                         to the ictx
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 06ebee4c87c5..7b3405fd6fd3 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -332,10 +332,17 @@  struct iommufd_group {
 struct iommufd_device {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;
+	/* valid if this is a physical device */
 	struct iommufd_group *igroup;
 	struct list_head group_item;
 	/* always the physical device */
 	struct device *dev;
+	/*
+	 * valid if this is a virtual device which gains pasid-granular
+	 * DMA isolation in IOMMU. The default pasid is used when attaching
+	 * this device to a IOAS/hwpt.
+	 */
+	u32 default_pasid;
 	struct xarray pasid_hwpts;
 	bool enforce_cache_coherency;
 	bool has_user_data;
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index 0b007c376306..402320d6eba1 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -20,6 +20,9 @@  struct iommu_group;
 
 struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
 					   struct device *dev, u32 *id);
+struct iommufd_device *iommufd_device_bind_pasid(struct iommufd_ctx *ictx,
+						 struct device *dev,
+						 u32 pasid, u32 *id);
 void iommufd_device_unbind(struct iommufd_device *idev);
 
 int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id);