Re: [PATCH v9 05/10] uacce: Enable IOMMU_DEV_FEAT_IOPF

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

 




On 1/8/2021 7:52 AM, Jean-Philippe Brucker wrote:
The IOPF (I/O Page Fault) feature is now enabled independently from the
SVA feature, because some IOPF implementations are device-specific and
do not require IOMMU support for PCIe PRI or Arm SMMU stall.

Enable IOPF unconditionally when enabling SVA for now. In the future, if
a device driver implementing a uacce interface doesn't need IOPF
support, it will need to tell the uacce module, for example with a new
flag.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
---
Cc: Arnd Bergmann <arnd@xxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Cc: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx>
Cc: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
---
  drivers/misc/uacce/uacce.c | 32 +++++++++++++++++++++++++-------
  1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index d07af4edfcac..41ef1eb62a14 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -385,6 +385,24 @@ static void uacce_release(struct device *dev)
  	kfree(uacce);
  }
+static unsigned int uacce_enable_sva(struct device *parent, unsigned int flags)
+{
+	if (!(flags & UACCE_DEV_SVA))
+		return flags;
+
+	flags &= ~UACCE_DEV_SVA;
+
+	if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_IOPF))
+		return flags;
+
+	if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA)) {
+		iommu_dev_disable_feature(parent, IOMMU_DEV_FEAT_IOPF);
+		return flags;
+	}

Sorry to jump in a bit late on this and not specifically towards the intent of this patch. But I'd like to start a discussion on if we want to push the iommu dev feature enabling to the device driver itself rather than having UACCE control this? Maybe allow the device driver to manage the feature bits and UACCE only verify that they are enabled?

1. The device driver knows what platform it's on and what specific
   feature bits its devices supports. Maybe in the future if there are
   feature bits that's needed on one platform and not on another?
2. This allows the possibility of multiple uacce device registered to 1
   pci dev, which for a device with asymmetric queues (Intel DSA/idxd
   driver) that is desirable feature. The current setup forces a single
   uacce device per pdev. If additional uacce devs are registered, the
   first removal of uacce device will disable the feature bit for the
   rest of the registered devices. With uacce managing the feature bit,
   it would need to add device context to the parent pdev and ref
   counting. It may be cleaner to just allow device driver to manage
   the feature bits and the driver should have all the information on
   when the feature needs to be turned on and off.

- DaveJ


+
+	return flags | UACCE_DEV_SVA;
+}
+
  /**
   * uacce_alloc() - alloc an accelerator
   * @parent: pointer of uacce parent device
@@ -404,11 +422,7 @@ struct uacce_device *uacce_alloc(struct device *parent,
  	if (!uacce)
  		return ERR_PTR(-ENOMEM);
- if (flags & UACCE_DEV_SVA) {
-		ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);
-		if (ret)
-			flags &= ~UACCE_DEV_SVA;
-	}
+	flags = uacce_enable_sva(parent, flags);
uacce->parent = parent;
  	uacce->flags = flags;
@@ -432,8 +446,10 @@ struct uacce_device *uacce_alloc(struct device *parent,
  	return uacce;
err_with_uacce:
-	if (flags & UACCE_DEV_SVA)
+	if (flags & UACCE_DEV_SVA) {
  		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);
+		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
+	}
  	kfree(uacce);
  	return ERR_PTR(ret);
  }
@@ -487,8 +503,10 @@ void uacce_remove(struct uacce_device *uacce)
  	mutex_unlock(&uacce->queues_lock);
/* disable sva now since no opened queues */
-	if (uacce->flags & UACCE_DEV_SVA)
+	if (uacce->flags & UACCE_DEV_SVA) {
  		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);
+		iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
+	}
if (uacce->cdev)
  		cdev_device_del(uacce->cdev, &uacce->dev);



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux