Shared Virtual Addressing for the IOMMU

Shared Virtual Addressing (SVA) provides a way for device drivers to bind process address spaces to devices. This requires the IOMMU to support page table format and features compatible with the CPUs, and usually requires the system to support I/O Page Faults (IOPF) and Process Address Space ID (PASID). When all of these are available, DMA can access virtual addresses of a process. A PASID is allocated for each process, and the device driver programs it into the device in an implementation-specific way.

Add a new API for sharing process page tables with devices. Introduce two IOMMU operations, sva_device_init() and sva_device_shutdown(), that prepare the IOMMU driver for SVA. For example allocate PASID tables and fault queues. Subsequent patches will implement the bind() and unbind() operations.

Support for I/O Page Faults will be added in a later patch using a new feature bit (IOMMU_SVA_FEAT_IOPF). With the current API users must pin down all shared mappings. Other feature bits that may be added in the future are IOMMU_SVA_FEAT_PRIVATE, to support private PASID address spaces, and IOMMU_SVA_FEAT_NO_PASID, to bind the whole device address space to a process.

Signed-off-by Jean-Philippe Brucker <[email protected]>

---
v1->v2:
* Add sva_param structure to iommu_param
* CONFIG option is only selectable by IOMMU drivers

---
 drivers/iommu/Kconfig     |   4 ++
 drivers/iommu/Makefile    |   1 +
 drivers/iommu/iommu-sva.c | 110
++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h     |
32 +++++++++++ 4 files changed, 147 insertions(+)
 create mode 100644 drivers/iommu/iommu-sva.c

drivers/iommu/Kconfig +4 -0

index 7564237f788d..cca8e06903c7 100644
@@ -74,6 +74,10 @@ config IOMMU_DMA
 	select IOMMU_IOVA
 	select NEED_SG_DMA_LENGTH
 
+config IOMMU_SVA
+	bool
+	select IOMMU_API
+
 config FSL_PAMU
 	bool "Freescale IOMMU support"
 	depends on PCI

drivers/iommu/Makefile +1 -0

index 1fb695854809..1dbcc89ebe4c 100644
@@ -3,6 +3,7 @@ obj-$(CONFIG_IOMMU_API) += iommu.o
 obj-$(CONFIG_IOMMU_API) += iommu-traces.o
 obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
 obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
+obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o

drivers/iommu/iommu-sva.c new file +12 -0

new file mode 100644
index 000000000000..8b4afb7c63ae
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Manage PASIDs and bind process address spaces to devices.
+ *
+ * Copyright (C) 2018 ARM Ltd.
+ */
+
+#include <linux/iommu.h>
+#include <linux/slab.h>
+
+/**
+ * iommu_sva_device_init() - Initialize Shared Virtual Addressing

for a device
+ * @dev: the device
+ * @features: bitmask of features that need to be initialized
+ * @max_pasid: max PASID value supported by the device
+ *
+ * Users of the bind()/unbind() API must call this function to initialize all
+ * features required for SVA.
+ *
+ * The device must support multiple address spaces (e.g. PCI PASID). By default

+ * the PASID allocated during bind() is limited by the IOMMU capacity, and by
+ * the device PASID width defined in the PCI capability or in the firmware
+ * description. Setting @max_pasid to a non-zero value smaller than this limit
+ * overrides it.
+ *

+ * The device should not be performing any DMA while this function is running,
+ * otherwise the behavior is undefined.
+ *
+ * Return 0 if initialization succeeded, or an error.
+ */

drivers/iommu/iommu-sva.c new file +2 -0

+int iommu_sva_device_init(struct device *dev, unsigned long features,
+			  unsigned int max_pasid)

drivers/iommu/iommu-sva.c new file +10 -0

+
+	param = kzalloc(sizeof(*param), GFP_KERNEL);
+	if (!param)
+		return -ENOMEM;
+
+	param->features		= features;
+	param->max_pasid	= max_pasid;
+
+	/*
+	 * IOMMU driver updates the limits depending on the IOMMU

and device

drivers/iommu/iommu-sva.c new file +30 -0

+	 * capabilities.
+	 */
+	ret = domain->ops->sva_device_init(dev, param);
+	if (ret)
+		goto err_free_param;
+
+	mutex_lock(&dev->iommu_param->lock);
+	if (dev->iommu_param->sva_param)
+		ret = -EEXIST;
+	else
+		dev->iommu_param->sva_param = param;
+	mutex_unlock(&dev->iommu_param->lock);
+	if (ret)
+		goto err_device_shutdown;
+
+	return 0;
+
+err_device_shutdown:
+	if (domain->ops->sva_device_shutdown)
+		domain->ops->sva_device_shutdown(dev, param);
+
+err_free_param:
+	kfree(param);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_device_init);
+
+/**
+ * iommu_sva_device_shutdown() - Shutdown Shared Virtual Addressing

for a device
+ * @dev: the device
+ *
+ * Disable SVA. Device driver should ensure that the device isn't performing any
+ * DMA while this function is running.
+ */

drivers/iommu/iommu-sva.c new file +7 -0

+int iommu_sva_device_shutdown(struct device *dev)
+{
+	struct iommu_sva_param *param;
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (!domain)
+		return -ENODEV;

drivers/iommu/iommu-sva.c new file +6 -0

+
+	kfree(param);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_device_shutdown);

include/linux/iommu.h +8 -0

index 0933f726d2e6..2efe7738bedb 100644
@@ -212,6 +212,12 @@ struct page_response_msg {
 	u64 private_data;
 };
 
+struct iommu_sva_param {
+	unsigned long features;
+	unsigned int min_pasid;
+	unsigned int max_pasid;
+};
+
 /**
  * struct iommu_ops - iommu ops and capabilities
  * @capable: check capability

@@ -219,6 +225,8 @@ struct page_response_msg {
  * @domain_free: free iommu domain
  * @attach_dev: attach device to an iommu domain
  * @detach_dev: detach device from an iommu domain
+ * @sva_device_init: initialize Shared Virtual Adressing for a device
+ * @sva_device_shutdown: shutdown Shared Virtual Adressing for a

device
* @map: map a physically contiguous memory region to an iommu domain
* @unmap: unmap a physically contiguous memory region from an iommu
domain
* @map_sg: map a scatter-gather list of physically contiguous
memory chunks @@ -256,6 +264,10 @@ struct iommu_ops {

int (*attach_dev)(struct iommu_domain *domain, struct device
*dev); void (*detach_dev)(struct iommu_domain *domain, struct device *dev);

include/linux/iommu.h +4 -0

+	int (*sva_device_init)(struct device *dev,
+			       struct iommu_sva_param *param);
+	void (*sva_device_shutdown)(struct device *dev,
+				    struct iommu_sva_param *param);
 	int (*map)(struct iommu_domain *domain, unsigned long iova,
 		   phys_addr_t paddr, size_t size, int prot);
 	size_t (*unmap)(struct iommu_domain *domain, unsigned long

iova, @@ -413,6 +425,7 @@ struct iommu_fault_param {
* struct iommu_param - collection of per-device IOMMU data *
* @fault_param: IOMMU detected device fault reporting data
+ * @sva_param: SVA parameters
*
* TODO: migrate other per device data pointers under
iommu_dev_data, e.g.
* struct iommu_group *iommu_group;

include/linux/iommu.h +19 -0

@@ -421,6 +434,7 @@ struct iommu_fault_param {
 struct iommu_param {
 	struct mutex lock;
 	struct iommu_fault_param *fault_param;
+	struct iommu_sva_param *sva_param;
 };
 
 int  iommu_device_register(struct iommu_device *iommu);

@@ -920,4 +934,22 @@ static inline int iommu_sva_invalidate(struct
 iommu_domain *domain, 
 #endif /* CONFIG_IOMMU_API */
 
+#ifdef CONFIG_IOMMU_SVA
+extern int iommu_sva_device_init(struct device *dev, unsigned long
 features,
+				 unsigned int max_pasid);
+extern int iommu_sva_device_shutdown(struct device *dev);
+#else /* CONFIG_IOMMU_SVA */
+static inline int iommu_sva_device_init(struct device *dev,
+					unsigned long features,
+					unsigned int max_pasid)
+{
+	return -ENODEV;
+}
+
+static inline int iommu_sva_device_shutdown(struct device *dev)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_IOMMU_SVA */
+

#endif /* __LINUX_IOMMU_H */

Quoted 10 lines

+ * The device must support multiple address spaces (e.g. PCI PASID). By default
+ * the PASID allocated during bind() is limited by the IOMMU capacity, and by
+ * the device PASID width defined in the PCI capability or in the firmware
+ * description. Setting @max_pasid to a non-zero value smaller than this limit
+ * overrides it.
+ *

seems the min_pasid never gets used. do you really need it?

Quoted 11 lines

+ * The device should not be performing any DMA while this function is running,
+ * otherwise the behavior is undefined.
+ *
+ * Return 0 if initialization succeeded, or an error.
+ */

drivers/iommu/iommu-sva.c new file +5 -0

+int iommu_sva_device_init(struct device *dev, unsigned long features,
+			  unsigned int max_pasid)
+{
+	int ret;
+	struct iommu_sva_param *param;

drivers/iommu/iommu-sva.c line 42

Quoted 7 lines

drivers/iommu/iommu-sva.c new file +7 -0

+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (!domain || !domain->ops->sva_device_init)
+		return -ENODEV;
+
+	if (features)
+		return -EINVAL;

should it be !features?

Quoted 4 lines

drivers/iommu/iommu-sva.c new file +4 -0

+	mutex_lock(&dev->iommu_param->lock);
+	param = dev->iommu_param->sva_param;
+	dev->iommu_param->sva_param = NULL;
+	mutex_unlock(&dev->iommu_param->lock);

drivers/iommu/iommu-sva.c line 104

Quoted 5 lines

drivers/iommu/iommu-sva.c new file +5 -0

+	if (!param)
+		return -ENODEV;
+
+	if (domain->ops->sva_device_shutdown)
+		domain->ops->sva_device_shutdown(dev, param);

seems a little mismatch here, do you need pass the param. I don't think there is anything else model specific iommu driver need to do for the param.

Hi Jacob,

Thanks for reviewing this

On 16/05/18 21:41, Jacob Pan wrote:
[...]

Quoted 1 line

seems the min_pasid never gets used. do you really need it?

Yes, the SMMU sets it to 1 in patch 28/40, because it needs to reserve PASID 0

[...]

Quoted 1 line

should it be !features?

This checks if the user sets any unsupported bit in features. No feature is supported right now, but patch 09/40 adds IOMMU_SVA_FEAT_IOPF, and changes this line to "features & ~IOMMU_SVA_FEAT_IOPF"

Quoted 4 lines

Quoted 4 lines

drivers/iommu/iommu-sva.c new file +4 -0

+	mutex_lock(&dev->iommu_param->lock);
+	param = dev->iommu_param->sva_param;
+	dev->iommu_param->sva_param = NULL;
+	mutex_unlock(&dev->iommu_param->lock);

Quoted 8 lines

drivers/iommu/iommu-sva.c line 104

Quoted 5 lines

drivers/iommu/iommu-sva.c new file +5 -0

+	if (!param)
+		return -ENODEV;
+
+	if (domain->ops->sva_device_shutdown)
+		domain->ops->sva_device_shutdown(dev, param);  

seems a little mismatch here, do you need pass the param. I don't think there is anything else model specific iommu driver need to do for the param.

SMMU doesn't use it, but maybe it would remind other IOMMU driver which features were enabled, so they don't have to keep track of that themselves? I can remove it if it isn't useful

Thanks,
Jean

Shared Virtual Addressing (SVA) provides a way for device drivers to bind process address spaces to devices. This requires the IOMMU to support page table format and features compatible with the CPUs, and usually requires the system to support I/O Page Faults (IOPF) and Process Address Space ID (PASID). When all of these are available, DMA can access virtual addresses of a process. A PASID is allocated for each process, and the device driver programs it into the device in an implementation-specific way.

Add a new API for sharing process page tables with devices. Introduce two IOMMU operations, sva_device_init() and sva_device_shutdown(), that prepare the IOMMU driver for SVA. For example allocate PASID tables and fault queues. Subsequent patches will implement the bind() and unbind() operations.

Support for I/O Page Faults will be added in a later patch using a new feature bit (IOMMU_SVA_FEAT_IOPF). With the current API users must pin down all shared mappings. Other feature bits that may be added in the future are IOMMU_SVA_FEAT_PRIVATE, to support private PASID address spaces, and IOMMU_SVA_FEAT_NO_PASID, to bind the whole device address space to a process.

Signed-off-by Jean-Philippe Brucker <[email protected]>

---
v1->v2:
* Add sva_param structure to iommu_param
* CONFIG option is only selectable by IOMMU drivers

---
 drivers/iommu/Kconfig     |   4 ++
 drivers/iommu/Makefile    |   1 +
 drivers/iommu/iommu-sva.c | 110 ++++++++++++++++++++++++++++++++++++++
 include/linux/iommu.h     |  32 +++++++++++
 4 files changed, 147 insertions(+)
 create mode 100644 drivers/iommu/iommu-sva.c

drivers/iommu/Kconfig +4 -0

index 7564237f788d..cca8e06903c7 100644
@@ -74,6 +74,10 @@ config IOMMU_DMA
 	select IOMMU_IOVA
 	select NEED_SG_DMA_LENGTH
 
+config IOMMU_SVA
+	bool
+	select IOMMU_API
+
 config FSL_PAMU
 	bool "Freescale IOMMU support"
 	depends on PCI

drivers/iommu/Makefile +1 -0

index 1fb695854809..1dbcc89ebe4c 100644
@@ -3,6 +3,7 @@ obj-$(CONFIG_IOMMU_API) += iommu.o
 obj-$(CONFIG_IOMMU_API) += iommu-traces.o
 obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
 obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
+obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o

drivers/iommu/iommu-sva.c new file +22 -0

new file mode 100644
index 000000000000..8b4afb7c63ae
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Manage PASIDs and bind process address spaces to devices.
+ *
+ * Copyright (C) 2018 ARM Ltd.
+ */
+
+#include <linux/iommu.h>
+#include <linux/slab.h>
+
+/**
+ * iommu_sva_device_init() - Initialize Shared Virtual Addressing for a device
+ * @dev: the device
+ * @features: bitmask of features that need to be initialized
+ * @max_pasid: max PASID value supported by the device
+ *
+ * Users of the bind()/unbind() API must call this function to initialize all
+ * features required for SVA.
+ *
+ * The device must support multiple address spaces (e.g. PCI PASID). By default
+ * the PASID allocated during bind() is limited by the IOMMU capacity, and by
+ * the device PASID width defined in the PCI capability or in the firmware

drivers/iommu/iommu-sva.c new file +15 -0

+{
+	int ret;
+	struct iommu_sva_param *param;
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (!domain || !domain->ops->sva_device_init)
+		return -ENODEV;
+
+	if (features)
+		return -EINVAL;
+
+	param = kzalloc(sizeof(*param), GFP_KERNEL);
+	if (!param)
+		return -ENOMEM;
+

drivers/iommu/iommu-sva.c new file +21 -0

+
+	mutex_lock(&dev->iommu_param->lock);
+	if (dev->iommu_param->sva_param)
+		ret = -EEXIST;
+	else
+		dev->iommu_param->sva_param = param;
+	mutex_unlock(&dev->iommu_param->lock);
+	if (ret)
+		goto err_device_shutdown;
+
+	return 0;
+
+err_device_shutdown:
+	if (domain->ops->sva_device_shutdown)
+		domain->ops->sva_device_shutdown(dev, param);
+
+err_free_param:
+	kfree(param);
+
+	return ret;
+}

drivers/iommu/iommu-sva.c new file +22 -0

+{
+	struct iommu_sva_param *param;
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (!domain)
+		return -ENODEV;
+
+	mutex_lock(&dev->iommu_param->lock);
+	param = dev->iommu_param->sva_param;
+	dev->iommu_param->sva_param = NULL;
+	mutex_unlock(&dev->iommu_param->lock);
+	if (!param)
+		return -ENODEV;
+
+	if (domain->ops->sva_device_shutdown)
+		domain->ops->sva_device_shutdown(dev, param);
+
+	kfree(param);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_device_shutdown);

include/linux/iommu.h +0 -0

index 0933f726d2e6..2efe7738bedb 100644

include/linux/iommu.h +15 -0

  * @map: map a physically contiguous memory region to an iommu domain
  * @unmap: unmap a physically contiguous memory region from an iommu domain
  * @map_sg: map a scatter-gather list of physically contiguous memory chunks

@@ -256,6 +264,10 @@ struct iommu_ops {
 
 	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
 	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
+	int (*sva_device_init)(struct device *dev,
+			       struct iommu_sva_param *param);
+	void (*sva_device_shutdown)(struct device *dev,
+				    struct iommu_sva_param *param);
 	int (*map)(struct iommu_domain *domain, unsigned long iova,
 		   phys_addr_t paddr, size_t size, int prot);
 	size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,

@@ -413,6 +425,7 @@ struct iommu_fault_param {
  * struct iommu_param - collection of per-device IOMMU data
  *
  * @fault_param: IOMMU detected device fault reporting data
+ * @sva_param: SVA parameters
  *
  * TODO: migrate other per device data pointers under iommu_dev_data, e.g.
  *	struct iommu_group	*iommu_group;

@@ -421,6 +434,7 @@ struct iommu_fault_param {
 struct iommu_param {
 	struct mutex lock;
 	struct iommu_fault_param *fault_param;
+	struct iommu_sva_param *sva_param;
 };
 
 int  iommu_device_register(struct iommu_device *iommu);

@@ -920,4 +934,22 @@ static inline int iommu_sva_invalidate(struct iommu_domain *domain,
 
 #endif /* CONFIG_IOMMU_API */
 
+#ifdef CONFIG_IOMMU_SVA
+extern int iommu_sva_device_init(struct device *dev, unsigned long features,
+				 unsigned int max_pasid);
+extern int iommu_sva_device_shutdown(struct device *dev);
+#else /* CONFIG_IOMMU_SVA */
+static inline int iommu_sva_device_init(struct device *dev,
+					unsigned long features,
+					unsigned int max_pasid)
+{

drivers/iommu/iommu-sva.c line 32

Quoted 2 lines

drivers/iommu/iommu-sva.c new file +2 -0

+int iommu_sva_device_init(struct device *dev, unsigned long features,
+			  unsigned int max_pasid)

what about min_pasid?

drivers/iommu/iommu-sva.c line 57

Quoted 7 lines

drivers/iommu/iommu-sva.c new file +7 -0

+	/*
+	 * IOMMU driver updates the limits depending on the IOMMU and device
+	 * capabilities.
+	 */
+	ret = domain->ops->sva_device_init(dev, param);
+	if (ret)
+		goto err_free_param;

So you are likely to call sva_device_init even if it was already called (ie. dev->iommu_param->sva_param is already set). Can't you test whether dev->iommu_param->sva_param is already set first?

drivers/iommu/iommu-sva.c line 88

Quoted 8 lines

drivers/iommu/iommu-sva.c new file +8 -0

+/**
+ * iommu_sva_device_shutdown() - Shutdown Shared Virtual Addressing for a device
+ * @dev: the device
+ *
+ * Disable SVA. Device driver should ensure that the device isn't performing any
+ * DMA while this function is running.
+ */
+int iommu_sva_device_shutdown(struct device *dev)

Not sure the returned value is required for a shutdown operation.

Quoted 1 line

+struct iommu_sva_param {

What are the feature values?

include/linux/iommu.h struct page_response_msg { · line 228

Quoted 13 lines

include/linux/iommu.h +6 -0

+	unsigned long features;
+	unsigned int min_pasid;
+	unsigned int max_pasid;
+};
+
 /**
  * struct iommu_ops - iommu ops and capabilities
  * @capable: check capability

@@ -219,6 +225,8 @@ struct page_response_msg {
  * @domain_free: free iommu domain
  * @attach_dev: attach device to an iommu domain
  * @detach_dev: detach device from an iommu domain
+ * @sva_device_init: initialize Shared Virtual Adressing for a device

Addressing

include/linux/iommu.h struct page_response_msg { · line 229

Quoted 1 line

include/linux/iommu.h +1 -0

+ * @sva_device_shutdown: shutdown Shared Virtual Adressing for a device

Addressing

Hi Eric,

Thanks for reviewing

Quoted 3 lines · Auger Eric

drivers/iommu/iommu-sva.c line 32

Quoted 2 lines

drivers/iommu/iommu-sva.c new file +2 -0

+int iommu_sva_device_init(struct device *dev, unsigned long features,
+			  unsigned int max_pasid)

what about min_pasid?

No one asked for it... The max_pasid parameter is here for drivers that have vendor-specific PASID size limits, such as AMD KFD (see kfd_iommu_device_init and
https://patchwork.kernel.org/patch/9989307/#21389571). But in most cases the PASID size will only depend on the PCI PASID capability and the IOMMU limits, both known by the IOMMU driver, so device drivers won't have to set max_pasid.

IOMMU drivers need to set min_pasid in the sva_device_init callback because it may be either 1 (e.g. Arm where PASID #0 is reserved) or 0 (Intel Vt-d rev2), but at the moment I can't see a reason for device drivers to override min_pasid

Quoted 11 lines

drivers/iommu/iommu-sva.c line 57

Quoted 8 lines

drivers/iommu/iommu-sva.c new file +7 -0

+	/*
+	 * IOMMU driver updates the limits depending on the IOMMU and device
+	 * capabilities.
+	 */
+	ret = domain->ops->sva_device_init(dev, param);
+	if (ret)
+		goto err_free_param;

So you are likely to call sva_device_init even if it was already called (ie. dev->iommu_param->sva_param is already set). Can't you test whether dev->iommu_param->sva_param is already set first?

Right, that's probably better

Quoted 9 lines

drivers/iommu/iommu-sva.c line 88

Quoted 8 lines

drivers/iommu/iommu-sva.c new file +8 -0

+/**
+ * iommu_sva_device_shutdown() - Shutdown Shared Virtual Addressing for a device
+ * @dev: the device
+ *
+ * Disable SVA. Device driver should ensure that the device isn't performing any
+ * DMA while this function is running.
+ */
+int iommu_sva_device_shutdown(struct device *dev)

Not sure the returned value is required for a shutdown operation.

I don't know either. I like them because they help me debug, but are otherwise rather useless if we don't describe precise semantics. The caller cannot do anything with it. Given that the corresponding IOMMU op is already void, I can change this function to void as well

Quoted 2 lines

Quoted 1 line

+struct iommu_sva_param {

What are the feature values?

At the moment only IOMMU_SVA_FEAT_IOPF, introduced by patch 09

Quoted 14 lines

include/linux/iommu.h struct page_response_msg { · line 228

Quoted 13 lines

include/linux/iommu.h +6 -0

+	unsigned long features;
+	unsigned int min_pasid;
+	unsigned int max_pasid;
+};
+
  /**
   * struct iommu_ops - iommu ops and capabilities
   * @capable: check capability

@@ -219,6 +225,8 @@ struct page_response_msg {
   * @domain_free: free iommu domain
   * @attach_dev: attach device to an iommu domain
   * @detach_dev: detach device from an iommu domain
+ * @sva_device_init: initialize Shared Virtual Adressing for a device

Addressing

Quoted 2 lines

include/linux/iommu.h struct page_response_msg { · line 229

Quoted 1 line

include/linux/iommu.h +1 -0

+ * @sva_device_shutdown: shutdown Shared Virtual Adressing for a device

Addressing

Nice catch

Thanks,
Jean

Am 06.09.2018 um 13:09 schrieb Jean-Philippe Brucker:

Quoted 16 lines

Hi Eric,

Thanks for reviewing

Quoted 3 lines · Auger Eric

Quoted 2 lines

+int iommu_sva_device_init(struct device *dev, unsigned long features, +              unsigned int max_pasid)

what about min_pasid?

No one asked for it... The max_pasid parameter is here for drivers that have vendor-specific PASID size limits, such as AMD KFD (see kfd_iommu_device_init and
https://patchwork.kernel.org/patch/9989307/#21389571). But in most cases the PASID size will only depend on the PCI PASID capability and the IOMMU limits, both known by the IOMMU driver, so device drivers won't have to set max_pasid.

Quoted 4 lines

IOMMU drivers need to set min_pasid in the sva_device_init callback because it may be either 1 (e.g. Arm where PASID #0 is reserved) or 0 (Intel Vt-d rev2), but at the moment I can't see a reason for device drivers to override min_pasid

Sorry to ruin your day, but if I'm not completely mistaken PASID zero is reserved in the AMD KFD as well.

Quoted 17 lines

Am 06.09.2018 um 13:09 schrieb Jean-Philippe Brucker:

Quoted 16 lines

Hi Eric,

Thanks for reviewing

Quoted 3 lines · Auger Eric

Quoted 2 lines

+int iommu_sva_device_init(struct device *dev, unsigned long features, +              unsigned int max_pasid)

what about min_pasid?

No one asked for it... The max_pasid parameter is here for drivers that have vendor-specific PASID size limits, such as AMD KFD (see kfd_iommu_device_init and
https://patchwork.kernel.org/patch/9989307/#21389571). But in most cases the PASID size will only depend on the PCI PASID capability and the IOMMU limits, both known by the IOMMU driver, so device drivers won't have to set max_pasid.

Quoted 6 lines

Quoted 4 lines

IOMMU drivers need to set min_pasid in the sva_device_init callback because it may be either 1 (e.g. Arm where PASID #0 is reserved) or 0 (Intel Vt-d rev2), but at the moment I can't see a reason for device drivers to override min_pasid

Sorry to ruin your day, but if I'm not completely mistaken PASID zero is reserved in the AMD KFD as well.

Heh, fair enough. I'll add the min_pasid parameter

Thanks,
Jean

I will take this as an opportunity to summarize some of the requirements we have for PASID management from the amdgpu driver point of view:

1. We need to be able to allocate PASID between 1 and some maximum. Zero is reserved as far as I know, but we don't necessary need a minimum.

2. We need to be able to allocate PASIDs without a process address space backing it. E.g. our hardware uses PASIDs even without Shared Virtual Addressing enabled to distinct clients from each other.
        Would be a pity if we need to still have a separate PASID
handling because the system wide is only available when IOMMU is turned on.

3. Even after destruction of a process address space we need some grace period before a PASID is reused because it can be that the specific PASID is still in some hardware queues etc...
        At bare minimum all device drivers using process binding need
to explicitly note to the core when they are done with a PASID.

4. It would be nice to have to be able to set a "void *" for each PASID/device combination while binding to a process which then can be queried later on based on the PASID.
        E.g. when you have a per PASID/device structure around anyway,
just add an extra field.

5. It would be nice to have to allocate multiple PASIDs for the same process address space.
        E.g. some teams at AMD want to use a separate GPU address space
for their userspace client library. I'm still trying to avoid that, but it is perfectly possible that we are going to need that.

        Additional to that it is sometimes quite useful for debugging
to isolate where exactly an incorrect access (segfault) is coming from.

Let me know if there are some problems with that, especially I want to know if there is pushback on #5 so that I can forward that :)

Thanks,
Christian.

Quoted 3 lines

Thanks,
Jean

Quoted 2 lines

I will take this as an opportunity to summarize some of the requirements we have for PASID management from the amdgpu driver point of view:

That's incredibly useful, thanks :)

Quoted 2 lines

1. We need to be able to allocate PASID between 1 and some maximum. Zero is reserved as far as I know, but we don't necessary need a minimum.

Should be fine. The PASID range is restricted by the PCI PASID capability, firmware description (for non-PCI devices), the IOMMU capacity, and what the device driver passes to iommu_sva_device_init. Not all IOMMUs reserve PASID 0 (AMD IOMMU without GIoSup doesn't, if I'm not mistaken), so the KFD driver will need to pass min_pasid=1 to make sure that 0 isn't allocated.

Quoted 5 lines

2. We need to be able to allocate PASIDs without a process address space backing it. E.g. our hardware uses PASIDs even without Shared Virtual Addressing enabled to distinct clients from each other.
        Would be a pity if we need to still have a separate PASID
handling because the system wide is only available when IOMMU is turned on.

I'm still not sure about this one. From my point of view we shouldn't add to the IOMMU subsystem helpers for devices without an IOMMU.

iommu-sva expects everywhere that the device has an iommu_domain, it's the first thing we check on entry. Bypassing all of this would call idr_alloc() directly, and wouldn't have any code in common with the current iommu-sva. So it seems like you need a layer on top of iommu-sva calling idr_alloc() when an IOMMU isn't present, but I don't think it should be in drivers/iommu/

Quoted 5 lines

3. Even after destruction of a process address space we need some grace period before a PASID is reused because it can be that the specific PASID is still in some hardware queues etc...
        At bare minimum all device drivers using process binding need
to explicitly note to the core when they are done with a PASID.

Right, much of the horribleness in iommu-sva deals with this:

The process dies, iommu-sva is notified and calls the mm_exit() function passed by the device driver to iommu_sva_device_init(). In mm_exit() the device driver needs to clear any reference to the PASID in hardware and in its own structures. When the device driver returns from mm_exit(), it effectively tells the core that it has finished using the PASID, and iommu-sva can reuse the PASID for another process. mm_exit() is allowed to block, so the device driver has time to clean up and flush the queues.

If the device driver finishes using the PASID before the process exits, it just calls unbind().

Quoted 6 lines

4. It would be nice to have to be able to set a "void *" for each PASID/device combination while binding to a process which then can be queried later on based on the PASID.
        E.g. when you have a per PASID/device structure around anyway,
just add an extra field.

iommu_sva_bind_device() takes a "drvdata" pointer that is stored internally for the PASID/device combination (iommu_bond). It is passed to mm_exit(), but I haven't added anything for the device driver to query it back.

Quoted 5 lines

5. It would be nice to have to allocate multiple PASIDs for the same process address space.
        E.g. some teams at AMD want to use a separate GPU address space
for their userspace client library. I'm still trying to avoid that, but it is perfectly possible that we are going to need that.

Two PASIDs pointing to the same process pgd? At first glance it seems feasible, maybe with a flag passed to bind() and a few changes to internal structures. It will duplicate ATC invalidation commands for each process address space change (munmap etc) so you might take a performance hit.

Intel's SVM code has the SVM_FLAG_PRIVATE_PASID which seems similar to what you describe, but I don't plan to support it in this series (the io_mm model is already pretty complicated). I think it can be added without too much effort in a future series, though with a different flag name since we'd like to use "private PASID" for something else
(https://www.spinics.net/lists/dri-devel/msg177007.html).

Thanks,
Jean

Quoted 11 lines

        Additional to that it is sometimes quite useful for debugging
to isolate where exactly an incorrect access (segfault) is coming from.

Let me know if there are some problems with that, especially I want to know if there is pushback on #5 so that I can forward that :)

Thanks,
Christian.

Quoted 2 lines

Thanks,
Jean

Am 07.09.2018 um 17:45 schrieb Jean-Philippe Brucker:

Quoted 9 lines · Christian König

Quoted 3 lines

I will take this as an opportunity to summarize some of the requirements we have for PASID management from the amdgpu driver point of view:

That's incredibly useful, thanks :)

Quoted 3 lines

1. We need to be able to allocate PASID between 1 and some maximum. Zero is reserved as far as I know, but we don't necessary need a minimum.

[...]

Quoted 6 lines

Quoted 6 lines

2. We need to be able to allocate PASIDs without a process address space backing it. E.g. our hardware uses PASIDs even without Shared Virtual Addressing enabled to distinct clients from each other. Would be a pity if we need to still have a separate PASID handling because the system wide is only available when IOMMU is turned on.

[...]

I agree on that.

Quoted 6 lines

iommu-sva expects everywhere that the device has an iommu_domain, it's the first thing we check on entry. Bypassing all of this would call idr_alloc() directly, and wouldn't have any code in common with the current iommu-sva. So it seems like you need a layer on top of iommu-sva calling idr_alloc() when an IOMMU isn't present, but I don't think it should be in drivers/iommu/

In this case I question if the PASID handling should be under drivers/iommu at all.

See I can have a mix of VM context which are bound to processes (some few) and VM contexts which are standalone and doesn't care for a process address space. But for each VM context I need a distinct PASID for the hardware to work.

I can live if we say if IOMMU is completely disabled we use a simple ida to allocate them, but when IOMMU is enabled I certainly need a way to reserve a PASID without an associated process.

Quoted 17 lines

Quoted 6 lines

3. Even after destruction of a process address space we need some grace period before a PASID is reused because it can be that the specific PASID is still in some hardware queues etc... At bare minimum all device drivers using process binding
need to explicitly note to the core when they are done with a PASID.

Right, much of the horribleness in iommu-sva deals with this:

The process dies, iommu-sva is notified and calls the mm_exit() function passed by the device driver to iommu_sva_device_init(). In mm_exit() the device driver needs to clear any reference to the PASID in hardware and in its own structures. When the device driver returns from mm_exit(), it effectively tells the core that it has finished using the PASID, and iommu-sva can reuse the PASID for another process. mm_exit() is allowed to block, so the device driver has time to clean up and flush the queues.

Quoted 2 lines

If the device driver finishes using the PASID before the process exits, it just calls unbind().

Exactly that's what Michal Hocko is probably going to not like at all.

Can we have a different approach where each driver is informed by the mm_exit(), but needs to explicitly call unbind() before a PASID is reused?

During that teardown transition it would be ideal if that PASID only points to a dummy root page directory with only invalid entries.

Quoted 10 lines

Quoted 6 lines

4. It would be nice to have to be able to set a "void *" for each PASID/device combination while binding to a process which then can be queried later on based on the PASID.
        E.g. when you have a per PASID/device structure around
anyway, just add an extra field.

iommu_sva_bind_device() takes a "drvdata" pointer that is stored internally for the PASID/device combination (iommu_bond). It is passed to mm_exit(), but I haven't added anything for the device driver to query it back.

Nice! Looks like all we need additionally is a function to retrieve that based on the PASID.

Quoted 12 lines

Quoted 6 lines

5. It would be nice to have to allocate multiple PASIDs for the same process address space.
        E.g. some teams at AMD want to use a separate GPU
address space for their userspace client library. I'm still trying to avoid that, but it is perfectly possible that we are going to need that.

Two PASIDs pointing to the same process pgd? At first glance it seems feasible, maybe with a flag passed to bind() and a few changes to internal structures. It will duplicate ATC invalidation commands for each process address space change (munmap etc) so you might take a performance hit.

Quoted 7 lines

Intel's SVM code has the SVM_FLAG_PRIVATE_PASID which seems similar to what you describe, but I don't plan to support it in this series (the io_mm model is already pretty complicated). I think it can be added without too much effort in a future series, though with a different flag name since we'd like to use "private PASID" for something else
(https://www.spinics.net/lists/dri-devel/msg177007.html).

To be honest I hoped that you would say: No never! So that I have a good argument to pushback on such requirements :)

But if it's doable it would be at least nice to have for debugging.

Thanks a lot for working on that,
Christian.

Quoted 17 lines

Thanks,
Jean

Quoted 13 lines

        Additional to that it is sometimes quite useful for
debugging to isolate where exactly an incorrect access (segfault) is coming from.

Let me know if there are some problems with that, especially I want to know if there is pushback on #5 so that I can forward that :)

Thanks,
Christian.

Quoted 2 lines

Thanks,
Jean

Quoted 10 lines · On Fri, 7 Sep 2018 20:02:54 +0200 Christian König <[email protected]>…

[SNIP]

Quoted 6 lines

iommu-sva expects everywhere that the device has an iommu_domain, it's the first thing we check on entry. Bypassing all of this would call idr_alloc() directly, and wouldn't have any code in common with the current iommu-sva. So it seems like you need a layer on top of iommu-sva calling idr_alloc() when an IOMMU isn't present, but I don't think it should be in drivers/iommu/

In this case I question if the PASID handling should be under drivers/iommu at all.

Quoted 9 lines

See I can have a mix of VM context which are bound to processes (some few) and VM contexts which are standalone and doesn't care for a process address space. But for each VM context I need a distinct PASID for the hardware to work.

I can live if we say if IOMMU is completely disabled we use a simple ida to allocate them, but when IOMMU is enabled I certainly need a way to reserve a PASID without an associated process.

VT-d would also have such requirement. There is a virtual command register for allocate and free PASID for VM use. When that PASID allocation request gets propagated to the host IOMMU driver, we need to allocate PASID w/o mm.

If the PASID allocation is done via VFIO, can we have FD to track PASID life cycle instead of mm_exit()? i.e. all FDs get closed before mm_exit, I assume?

Quoted 25 lines

Quoted 17 lines

Quoted 6 lines

3. Even after destruction of a process address space we need some grace period before a PASID is reused because it can be that the specific PASID is still in some hardware queues etc... At bare minimum all device drivers using process binding
need to explicitly note to the core when they are done with a PASID.

Right, much of the horribleness in iommu-sva deals with this:

The process dies, iommu-sva is notified and calls the mm_exit() function passed by the device driver to iommu_sva_device_init(). In mm_exit() the device driver needs to clear any reference to the PASID in hardware and in its own structures. When the device driver returns from mm_exit(), it effectively tells the core that it has finished using the PASID, and iommu-sva can reuse the PASID for another process. mm_exit() is allowed to block, so the device driver has time to clean up and flush the queues.

Quoted 2 lines

If the device driver finishes using the PASID before the process exits, it just calls unbind().

Exactly that's what Michal Hocko is probably going to not like at all.

Can we have a different approach where each driver is informed by the mm_exit(), but needs to explicitly call unbind() before a PASID is reused?

Quoted 3 lines

During that teardown transition it would be ideal if that PASID only points to a dummy root page directory with only invalid entries.

I guess this can be vendor specific, In VT-d I plan to mark PASID entry not present and disable fault reporting while draining remaining activities.

Yes, exactly. I just need a PASID which is never used by the OS for a process and we can easily give that back when the last FD reference is closed.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

The process dies, iommu-sva is notified and calls the mm_exit() function passed by the device driver to iommu_sva_device_init(). In mm_exit() the device driver needs to clear any reference to the PASID in hardware and in its own structures. When the device driver returns from mm_exit(), it effectively tells the core that it has finished using the PASID, and iommu-sva can reuse the PASID for another process. mm_exit() is allowed to block, so the device driver has time to clean up and flush the queues.

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

If the device driver finishes using the PASID before the process exits, it just calls unbind().

Exactly that's what Michal Hocko is probably going to not like at all.

Can we have a different approach where each driver is informed by the mm_exit(), but needs to explicitly call unbind() before a PASID is reused?

Quoted 6 lines

Quoted 3 lines

During that teardown transition it would be ideal if that PASID only points to a dummy root page directory with only invalid entries.

I guess this can be vendor specific, In VT-d I plan to mark PASID entry not present and disable fault reporting while draining remaining activities.

Sounds good to me.

Point is at least in the case where the process was killed by the OOM killer we should not block in mm_exit().

Instead operations issued by the process to a device driver which uses SVA needs to be terminated as soon as possible to make sure that the OOM killer can advance.

Quoted 3 lines

Yes, exactly. I just need a PASID which is never used by the OS for a process and we can easily give that back when the last FD reference is closed.

Alright, iommu-sva can get its PASID from this external allocator as well, as long as it has an interface similar to idr. Where would it go, drivers/base/, mm/, kernel/...?

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

The process dies, iommu-sva is notified and calls the mm_exit() function passed by the device driver to iommu_sva_device_init(). In mm_exit() the device driver needs to clear any reference to the PASID in hardware and in its own structures. When the device driver returns from mm_exit(), it effectively tells the core that it has finished using the PASID, and iommu-sva can reuse the PASID for another process. mm_exit() is allowed to block, so the device driver has time to clean up and flush the queues.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

If the device driver finishes using the PASID before the process exits, it just calls unbind().

Exactly that's what Michal Hocko is probably going to not like at all.

Can we have a different approach where each driver is informed by the mm_exit(), but needs to explicitly call unbind() before a PASID is reused?

It's awful from the IOMMU driver perspective. In addition to "enabled" and "disabled" PASID states, you add "disabled but DMA still running normally". Between that new state and "disabled", the IOMMU will be flooded by translation faults (non-recoverable ones), which it needs to ignore instead of reporting to the kernel. Not all IOMMUs can deal with this in hardware (SMMU and VT-d can quiesce translation faults per-PASID, but I don't think AMD IOMMU can.) Some drivers will have to filter fault events themselves, depending on the PASID state.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

During that teardown transition it would be ideal if that PASID only points to a dummy root page directory with only invalid entries.

I guess this can be vendor specific, In VT-d I plan to mark PASID entry not present and disable fault reporting while draining remaining activities.

Quoted 8 lines

Sounds good to me.

Point is at least in the case where the process was killed by the OOM killer we should not block in mm_exit().

Instead operations issued by the process to a device driver which uses SVA needs to be terminated as soon as possible to make sure that the OOM killer can advance.

I don't see how we're preventing the OOM killer from advancing, so I'm looking for a stronger argument that justifies adding this complexity to IOMMU drivers. Time limit of the release MMU notifier, locking requirement, a concrete example where things break, even a comment somewhere in mm/ would do...

In my tests I can't manage to disturb the OOM killer, but I could be missing some special case. Even if the mm_exit() callback (unrealistically) sleeps for 60 seconds,

the OOM killer isn't affected
because oom_reap_task_mm() wipes the victim's address space in another thread, either before or while the release notifier is running.

Thanks,
Jean

From: Jacob Pan [mailto:jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]]
Sent: Saturday, September 8, 2018 5:25 AM

Quoted 14 lines

Quoted 6 lines

iommu-sva expects everywhere that the device has an iommu_domain, it's the first thing we check on entry. Bypassing all of this would call idr_alloc() directly, and wouldn't have any code in common with the current iommu-sva. So it seems like you need a layer on top of iommu-sva calling idr_alloc() when an IOMMU isn't present, but I don't think it should be in drivers/iommu/

In this case I question if the PASID handling should be under drivers/iommu at all.

See I can have a mix of VM context which are bound to processes (some few) and VM contexts which are standalone and doesn't care for a process address space. But for each VM context I need a distinct PASID for the hardware to work.

Quoted 5 lines

I can live if we say if IOMMU is completely disabled we use a simple ida to allocate them, but when IOMMU is enabled I certainly need a way to reserve a PASID without an associated process.

VT-d would also have such requirement. There is a virtual command register for allocate and free PASID for VM use. When that PASID allocation request gets propagated to the host IOMMU driver, we need to allocate PASID w/o mm.

From: Christian König
Sent: Friday, September 7, 2018 4:56 PM

5. It would be nice to have to allocate multiple PASIDs for the same process address space.
        E.g. some teams at AMD want to use a separate GPU address space
for their userspace client library. I'm still trying to avoid that, but it is perfectly possible that we are going to need that.
        Additional to that it is sometimes quite useful for debugging
to isolate where exactly an incorrect access (segfault) is coming from.

Let me know if there are some problems with that, especially I want to know if there is pushback on #5 so that I can forward that :)

Add bind() and unbind() operations to the IOMMU API. Bind() returns a PASID that drivers can program in hardware, to let their devices access an mm. This patch only adds skeletons for the device driver API, most of the implementation is still missing.

IOMMU groups with more than one device aren't supported for SVA at the moment. There may be P2P traffic between devices within a group, which cannot be seen by an IOMMU (note that supporting PASID doesn't add any form of isolation with regard to P2P). Supporting groups would require calling bind() for all bound processes every time a device is added to a group, to perform sanity checks (e.g. ensure that new devices support PASIDs at least as big as those already allocated in the group).

It also
means making sure that reserved ranges (IOMMU_RESV_*) of all devices are carved out of processes. This is already tricky with single devices, but becomes very difficult with groups. Since SVA-capable devices are expected to be cleanly isolated, and since we don't have any way to test groups or hot-plug, we only allow singular groups for now.

Signed-off-by Jean-Philippe Brucker <[email protected]>

---
v1->v2: remove iommu_sva_bind/unbind_group

---
 drivers/iommu/iommu-sva.c | 27 +++++++++++++
 drivers/iommu/iommu.c     | 83 +++++++++++++++++++++++++++++++++++++++
 include/linux/iommu.h     | 37 +++++++++++++++++
 3 files changed, 147 insertions(+)

drivers/iommu/iommu-sva.c +27 -0

index 8b4afb7c63ae..8d98f9c09864 100644
@@ -93,6 +93,8 @@ int iommu_sva_device_shutdown(struct device *dev)
 	if (!domain)
 		return -ENODEV;
 
+	__iommu_sva_unbind_dev_all(dev);
+
 	mutex_lock(&dev->iommu_param->lock);
 	param = dev->iommu_param->sva_param;
 	dev->iommu_param->sva_param = NULL;

@@ -108,3 +110,28 @@ int iommu_sva_device_shutdown(struct device *dev)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_sva_device_shutdown);
+
+int __iommu_sva_bind_device(struct device *dev, struct mm_struct *mm,
+			    int *pasid, unsigned long flags, void *drvdata)
+{
+	return -ENOSYS; /* TODO */
+}
+EXPORT_SYMBOL_GPL(__iommu_sva_bind_device);
+
+int __iommu_sva_unbind_device(struct device *dev, int pasid)
+{
+	return -ENOSYS; /* TODO */
+}
+EXPORT_SYMBOL_GPL(__iommu_sva_unbind_device);
+
+/**
+ * __iommu_sva_unbind_dev_all() - Detach all address spaces from this device
+ * @dev: the device
+ *
+ * When detaching @device from a domain, IOMMU drivers should use this helper.
+ */
+void __iommu_sva_unbind_dev_all(struct device *dev)
+{
+	/* TODO */
+}
+EXPORT_SYMBOL_GPL(__iommu_sva_unbind_dev_all);

drivers/iommu/iommu.c +83 -0

index 9e28d88c8074..bd2819deae5b 100644
@@ -2261,3 +2261,86 @@ int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
+
+/**
+ * iommu_sva_bind_device() - Bind a process address space to a device
+ * @dev: the device
+ * @mm: the mm to bind, caller must hold a reference to it
+ * @pasid: valid address where the PASID will be stored
+ * @flags: bond properties
+ * @drvdata: private data passed to the mm exit handler
+ *
+ * Create a bond between device and task, allowing the device to access the mm
+ * using the returned PASID. If unbind() isn't called first, a subsequent bind()
+ * for the same device and mm fails with -EEXIST.
+ *
+ * iommu_sva_device_init() must be called first, to initialize the required SVA
+ * features. @flags is a subset of these features.
+ *
+ * The caller must pin down using get_user_pages*() all mappings shared with the
+ * device. mlock() isn't sufficient, as it doesn't prevent minor page faults
+ * (e.g. copy-on-write).
+ *
+ * On success, 0 is returned and @pasid contains a valid ID. Otherwise, an error
+ * is returned.
+ */
+int iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, int *pasid,
+			  unsigned long flags, void *drvdata)
+{
+	int ret = -EINVAL;
+	struct iommu_group *group;
+
+	if (!pasid)
+		return -EINVAL;
+
+	group = iommu_group_get(dev);
+	if (!group)
+		return -ENODEV;
+
+	/* Ensure device count and domain don't change while we're binding */
+	mutex_lock(&group->mutex);
+	if (iommu_group_device_count(group) != 1)
+		goto out_unlock;
+
+	ret = __iommu_sva_bind_device(dev, mm, pasid, flags, drvdata);
+
+out_unlock:
+	mutex_unlock(&group->mutex);
+	iommu_group_put(group);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
+
+/**
+ * iommu_sva_unbind_device() - Remove a bond created with iommu_sva_bind_device
+ * @dev: the device
+ * @pasid: the pasid returned by bind()
+ *
+ * Remove bond between device and address space identified by @pasid. Users
+ * should not call unbind() if the corresponding mm exited (as the PASID might
+ * have been reallocated for another process).
+ *
+ * The device must not be issuing any more transaction for this PASID. All
+ * outstanding page requests for this PASID must have been flushed to the IOMMU.
+ *
+ * Returns 0 on success, or an error value
+ */
+int iommu_sva_unbind_device(struct device *dev, int pasid)
+{
+	int ret = -EINVAL;
+	struct iommu_group *group;
+
+	group = iommu_group_get(dev);
+	if (!group)
+		return -ENODEV;
+
+	mutex_lock(&group->mutex);
+	ret = __iommu_sva_unbind_device(dev, pasid);
+	mutex_unlock(&group->mutex);
+
+	iommu_group_put(group);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);

include/linux/iommu.h +37 -0

index 2efe7738bedb..da59c20c4f12 100644
@@ -613,6 +613,10 @@ void iommu_fwspec_free(struct device *dev);
 int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
 const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
 
+extern int iommu_sva_bind_device(struct device *dev, struct mm_struct *mm,
+				int *pasid, unsigned long flags, void *drvdata);
+extern int iommu_sva_unbind_device(struct device *dev, int pasid);
+
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};

@@ -932,12 +936,29 @@ static inline int iommu_sva_invalidate(struct iommu_domain *domain,
 	return -EINVAL;
 }
 
+static inline int iommu_sva_bind_device(struct device *dev,
+					struct mm_struct *mm, int *pasid,
+					unsigned long flags, void *drvdata)
+{
+	return -ENODEV;
+}
+
+static inline int iommu_sva_unbind_device(struct device *dev, int pasid)
+{
+	return -ENODEV;
+}
+
 #endif /* CONFIG_IOMMU_API */
 
 #ifdef CONFIG_IOMMU_SVA
 extern int iommu_sva_device_init(struct device *dev, unsigned long features,
 				 unsigned int max_pasid);
 extern int iommu_sva_device_shutdown(struct device *dev);
+extern int __iommu_sva_bind_device(struct device *dev, struct mm_struct *mm,
+				   int *pasid, unsigned long flags,
+				   void *drvdata);
+extern int __iommu_sva_unbind_device(struct device *dev, int pasid);
+extern void __iommu_sva_unbind_dev_all(struct device *dev);
 #else /* CONFIG_IOMMU_SVA */
 static inline int iommu_sva_device_init(struct device *dev,
 					unsigned long features,

@@ -950,6 +971,22 @@ static inline int iommu_sva_device_shutdown(struct device *dev)
 {
 	return -ENODEV;
 }
+
+static inline int __iommu_sva_bind_device(struct device *dev,
+					  struct mm_struct *mm, int *pasid,
+					  unsigned long flags, void *drvdata)
+{
+	return -ENODEV;
+}
+
+static inline int __iommu_sva_unbind_device(struct device *dev, int pasid)
+{
+	return -ENODEV;
+}
+
+static inline void __iommu_sva_unbind_dev_all(struct device *dev)
+{
+}
 #endif /* CONFIG_IOMMU_SVA */
 
 #endif /* __LINUX_IOMMU_H */

Quoted 5 lines · On Fri, 11 May 2018 20:06:03 +0100 Jean-Philippe Brucker…

Add bind() and unbind() operations to the IOMMU API. Bind() returns a PASID that drivers can program in hardware, to let their devices access an mm. This patch only adds skeletons for the device driver API, most of the implementation is still missing.

Quoted 7 lines

IOMMU groups with more than one device aren't supported for SVA at the moment. There may be P2P traffic between devices within a group, which cannot be seen by an IOMMU (note that supporting PASID doesn't add any form of isolation with regard to P2P). Supporting groups would require calling bind() for all bound processes every time a device is added to a group, to perform sanity checks (e.g. ensure that new devices support PASIDs at least as big as those already allocated in the group).

Is it worth adding an explicit comment on this reasoning (or a minimal subset of it) at the check for the number of devices in the group?
It's well laid out here, but might not be so obvious if someone is reading the code in the future.

Add bind() and unbind() operations to the IOMMU API. Bind() returns a PASID that drivers can program in hardware, to let their devices access an mm. This patch only adds skeletons for the device driver API, most of the implementation is still missing.

IOMMU groups with more than one device aren't supported for SVA at the moment. There may be P2P traffic between devices within a group, which cannot be seen by an IOMMU (note that supporting PASID doesn't add any form of isolation with regard to P2P). Supporting groups would require calling bind() for all bound processes every time a device is added to a group, to perform sanity checks (e.g. ensure that new devices support PASIDs at least as big as those already allocated in the group). It also means making sure that reserved ranges (IOMMU_RESV_*) of all devices are carved out of processes. This is already tricky with single devices, but becomes very difficult with groups. Since SVA-capable devices are expected to be cleanly isolated, and since we don't have any way to test groups or hot-plug, we only allow singular groups for now.

Signed-off-by Jean-Philippe Brucker <[email protected]>

---
v1->v2: remove iommu_sva_bind/unbind_group

---
 drivers/iommu/iommu-sva.c | 27 +++++++++++++
 drivers/iommu/iommu.c     | 83 +++++++++++++++++++++++++++++++++++++++
 include/linux/iommu.h     | 37 +++++++++++++++++
 3 files changed, 147 insertions(+)

drivers/iommu/iommu-sva.c +27 -0

index 8b4afb7c63ae..8d98f9c09864 100644
@@ -93,6 +93,8 @@ int iommu_sva_device_shutdown(struct device *dev)
 	if (!domain)
 		return -ENODEV;
 
+	__iommu_sva_unbind_dev_all(dev);
+
 	mutex_lock(&dev->iommu_param->lock);
 	param = dev->iommu_param->sva_param;
 	dev->iommu_param->sva_param = NULL;

@@ -108,3 +110,28 @@ int iommu_sva_device_shutdown(struct device *dev)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_sva_device_shutdown);
+
+int __iommu_sva_bind_device(struct device *dev, struct mm_struct *mm,
+			    int *pasid, unsigned long flags, void *drvdata)
+{
+	return -ENOSYS; /* TODO */
+}
+EXPORT_SYMBOL_GPL(__iommu_sva_bind_device);
+
+int __iommu_sva_unbind_device(struct device *dev, int pasid)
+{
+	return -ENOSYS; /* TODO */
+}
+EXPORT_SYMBOL_GPL(__iommu_sva_unbind_device);
+
+/**
+ * __iommu_sva_unbind_dev_all() - Detach all address spaces from this device
+ * @dev: the device
+ *
+ * When detaching @device from a domain, IOMMU drivers should use this helper.
+ */
+void __iommu_sva_unbind_dev_all(struct device *dev)
+{
+	/* TODO */
+}
+EXPORT_SYMBOL_GPL(__iommu_sva_unbind_dev_all);

drivers/iommu/iommu.c +5 -0

index 9e28d88c8074..bd2819deae5b 100644
@@ -2261,3 +2261,86 @@ int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
+
+/**
+ * iommu_sva_bind_device() - Bind a process address space to a device
+ * @dev: the device
+ * @mm: the mm to bind, caller must hold a reference to it

drivers/iommu/iommu.c +15 -0

+ *
+ * The caller must pin down using get_user_pages*() all mappings shared with the
+ * device. mlock() isn't sufficient, as it doesn't prevent minor page faults
+ * (e.g. copy-on-write).
+ *
+ * On success, 0 is returned and @pasid contains a valid ID. Otherwise, an error
+ * is returned.
+ */
+int iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, int *pasid,
+			  unsigned long flags, void *drvdata)
+{
+	int ret = -EINVAL;
+	struct iommu_group *group;
+
+	if (!pasid)

drivers/iommu/iommu.c +16 -0

+
+	ret = __iommu_sva_bind_device(dev, mm, pasid, flags, drvdata);
+
+out_unlock:
+	mutex_unlock(&group->mutex);
+	iommu_group_put(group);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
+
+/**
+ * iommu_sva_unbind_device() - Remove a bond created with iommu_sva_bind_device
+ * @dev: the device
+ * @pasid: the pasid returned by bind()
+ *

drivers/iommu/iommu.c +17 -0

+{
+	int ret = -EINVAL;
+	struct iommu_group *group;
+
+	group = iommu_group_get(dev);
+	if (!group)
+		return -ENODEV;
+
+	mutex_lock(&group->mutex);
+	ret = __iommu_sva_unbind_device(dev, pasid);
+	mutex_unlock(&group->mutex);
+
+	iommu_group_put(group);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);

include/linux/iommu.h +30 -0

index 2efe7738bedb..da59c20c4f12 100644
@@ -613,6 +613,10 @@ void iommu_fwspec_free(struct device *dev);
 int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
 const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
 
+extern int iommu_sva_bind_device(struct device *dev, struct mm_struct *mm,
+				int *pasid, unsigned long flags, void *drvdata);
+extern int iommu_sva_unbind_device(struct device *dev, int pasid);
+
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};

@@ -932,12 +936,29 @@ static inline int iommu_sva_invalidate(struct iommu_domain *domain,
 	return -EINVAL;
 }
 
+static inline int iommu_sva_bind_device(struct device *dev,
+					struct mm_struct *mm, int *pasid,
+					unsigned long flags, void *drvdata)
+{
+	return -ENODEV;
+}
+
+static inline int iommu_sva_unbind_device(struct device *dev, int pasid)
+{
+	return -ENODEV;
+}
+
 #endif /* CONFIG_IOMMU_API */
 
 #ifdef CONFIG_IOMMU_SVA
 extern int iommu_sva_device_init(struct device *dev, unsigned long features,
 				 unsigned int max_pasid);
 extern int iommu_sva_device_shutdown(struct device *dev);
+extern int __iommu_sva_bind_device(struct device *dev, struct mm_struct *mm,
+				   int *pasid, unsigned long flags,
+				   void *drvdata);
+extern int __iommu_sva_unbind_device(struct device *dev, int pasid);
+extern void __iommu_sva_unbind_dev_all(struct device *dev);
 #else /* CONFIG_IOMMU_SVA */
 static inline int iommu_sva_device_init(struct device *dev,
 					unsigned long features,

@@ -950,6 +971,22 @@ static inline int iommu_sva_device_shutdown(struct device *dev)
 {
 	return -ENODEV;
 }
+
+static inline int __iommu_sva_bind_device(struct device *dev,
+					  struct mm_struct *mm, int *pasid,
+					  unsigned long flags, void *drvdata)
+{
+	return -ENODEV;
+}
+
+static inline int __iommu_sva_unbind_device(struct device *dev, int pasid)

Quoted 6 lines

+/**
+ * iommu_sva_bind_device() - Bind a process address space to a device
+ * @dev: the device
+ * @mm: the mm to bind, caller must hold a reference to it
+ * @pasid: valid address where the PASID will be stored
+ * @flags: bond properties

Quoted 8 lines

+ * @drvdata: private data passed to the mm exit handler
+ *
+ * Create a bond between device and task, allowing the device to access the mm
+ * using the returned PASID. If unbind() isn't called first, a subsequent bind()
+ * for the same device and mm fails with -EEXIST.
+ *
+ * iommu_sva_device_init() must be called first, to initialize the required SVA
+ * features. @flags is a subset of these features.

@flags must be a subset of these features?

don't you want to check flags is a subset of
dev->iommu_param->sva_param->features?

Quoted 8 lines

+/**
+ * iommu_sva_unbind_device() - Remove a bond created with iommu_sva_bind_device
+ * @dev: the device
+ * @pasid: the pasid returned by bind()
+ *
+ * Remove bond between device and address space identified by @pasid. Users
+ * should not call unbind() if the corresponding mm exited (as the PASID might
+ * have been reallocated for another process).

Quoted 7 lines

+ *
+ * The device must not be issuing any more transaction for this PASID. All
+ * outstanding page requests for this PASID must have been flushed to the IOMMU.
+ *
+ * Returns 0 on success, or an error value
+ */
+int iommu_sva_unbind_device(struct device *dev, int pasid)

returned value needed?

Allocate IOMMU mm structures and binding them to devices. Four operations are added to IOMMU drivers:

* mm_alloc(): to create an io_mm structure and perform architecture- specific operations required to grab the process (for instance on
ARM, pin down the CPU ASID so that the process doesn't get assigned a new ASID on rollover).

There is a single valid io_mm structure per Linux mm. Future
extensions may also use io_mm for kernel-managed address spaces, populated with map()/unmap() calls instead of bound to process address spaces. This patch focuses on "shared" io_mm.

* mm_attach(): attach an mm to a device. The IOMMU driver checks that the device is capable of sharing an address space, and writes the PASID table entry to install the pgd.

Some IOMMU drivers will have a single PASID table per domain, for convenience. Other can implement it differently but to help these drivers, mm_attach and mm_detach take 'attach_domain' and 'detach_domain' parameters, that tell whether they need to set and
clear the PASID entry or only send the required TLB invalidations.

* mm_detach(): detach an mm from a device. The IOMMU driver removes the PASID table entry and invalidates the IOTLBs.

* mm_free(): free a structure allocated by mm_alloc(), and let arch release the process.

mm_attach and mm_detach operations are serialized with a spinlock. When trying to optimize this code, we should at least prevent concurrent attach()/detach() on the same domain (so multi-level PASID table code can allocate tables lazily). mm_alloc() can sleep, but mm_free must not (because we'll have to call it from call_srcu later on).

At the moment we use an IDR for allocating PASIDs and retrieving contexts. We also use a single spinlock. These can be refined and optimized later (a custom allocator will be needed for top-down PASID allocation).

Keeping track of address spaces requires the use of MMU notifiers. Handling process exit with regard to unbind() is tricky, so it is left for another patch and we explicitly fail mm_alloc() for the moment.

Signed-off-by Jean-Philippe Brucker <[email protected]>

---
v1->v2: sanity-check of flags

---
 drivers/iommu/iommu-sva.c | 380
+++++++++++++++++++++++++++++++++++++- drivers/iommu/iommu.c     |
1 + include/linux/iommu.h     |  28 +++
 3 files changed, 406 insertions(+), 3 deletions(-)

drivers/iommu/iommu-sva.c +8 -0

index 8d98f9c09864..6ac679c48f3c 100644
@@ -5,8 +5,298 @@
  * Copyright (C) 2018 ARM Ltd.
  */
 
+#include <linux/idr.h>
 #include <linux/iommu.h>
+#include <linux/sched/mm.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
+
+/**
+ * DOC: io_mm model
+ *
+ * The io_mm keeps track of process address spaces shared between

CPU and IOMMU.
+ * The following example illustrates the relation between structures
+ * iommu_domain, io_mm and iommu_bond. An iommu_bond is a link between io_mm and
+ * device. A device can have multiple io_mm and an io_mm may be bound to
+ * multiple devices.
+ * ___________________________

+ *             |  IOMMU domain A           |
+ *             |  ________________         |
+ *             | |  IOMMU group   |        +------- io_pgtables
+ *             | |                |        |
+ *             | |   dev 00:00.0 ----+------- bond --- io_mm X
+ *             | |________________|   \    |
+ *             |                       '----- bond ---.
+ *             |___________________________|           \
+ *              ___________________________             \
+ *             |  IOMMU domain B           |           io_mm Y
+ *             |  ________________         |           / /
+ *             | |  IOMMU group   |        |          / /
+ *             | |                |        |         / /
+ *             | |   dev 00:01.0 ------------ bond -' /
+ *             | |   dev 00:01.1 ------------ bond --'
+ *             | |________________|        |
+ *             |                           +------- io_pgtables
+ *             |___________________________|

+ *
+ * In this example, device 00:00.0 is in domain A, devices 00:01.* are in domain
+ * B. All devices within the same domain access the same address spaces. Device
+ * 00:00.0 accesses address spaces X and Y, each corresponding to an mm_struct.
+ * Devices 00:01.* only access address space Y. In addition each
+ * IOMMU_DOMAIN_DMA domain has a private address space, io_pgtable, that is
+ * managed with iommu_map()/iommu_unmap(), and isn't shared with the CPU MMU.
+ *
+ * To obtain the above configuration, users would for instance issue the
+ * following calls:
+ *

+ *     iommu_sva_bind_device(dev 00:00.0, mm X, ...) -> PASID 1
+ *     iommu_sva_bind_device(dev 00:00.0, mm Y, ...) -> PASID 2
+ *     iommu_sva_bind_device(dev 00:01.0, mm Y, ...) -> PASID 2
+ *     iommu_sva_bind_device(dev 00:01.1, mm Y, ...) -> PASID 2

+ *
+ * A single Process Address Space ID (PASID) is allocated for each mm. In the
+ * example, devices use PASID 1 to read/write into address space X and PASID 2
+ * to read/write into address space Y.
+ *
+ * Hardware tables describing this configuration in the IOMMU would typically
+ * look like this:
+ *
+ * PASID tables
+ * of domain A
+ * .->+--------+

+ *                             / 0 |        |-------> io_pgtable
+ *                            /    +--------+
+ *            Device tables  /   1 |        |-------> pgd X
+ *              +--------+  /      +--------+
+ *      00:00.0 |      A |-'     2 |        |--.
+ *              +--------+         +--------+   \
+ *              :        :       3 |        |    \
+ *              +--------+         +--------+     --> pgd Y
+ *      00:01.0 |      B |--.                    /
+ *              +--------+   \                  |
+ *      00:01.1 |      B |----+   PASID tables  |
+ *              +--------+     \   of domain B  |
+ *                              '->+--------+   |
+ *                               0 |        |-- | --> io_pgtable
+ *                                 +--------+   |
+ *                               1 |        |   |
+ *                                 +--------+   |
+ *                               2 |        |---'
+ *                                 +--------+
+ *                               3 |        |
+ *                                 +--------+

+ *

+ * With this model, a single call binds all devices in a given domain to an
+ * address space. Other devices in the domain will get the same bond implicitly.
+ * However, users must issue one bind() for each device, because IOMMUs may
+ * implement SVA differently. Furthermore, mandating one bind() per device
+ * allows the driver to perform sanity-checks on device capabilities.
+ *
+ * On Arm and AMD IOMMUs, entry 0 of the PASID table can be used to hold
+ * non-PASID translations. In this case PASID 0 is reserved and entry 0 points
+ * to the io_pgtable base. On Intel IOMMU, the io_pgtable base would be held in
+ * the device table and PASID 0 would be available to the allocator.
+ */

drivers/iommu/iommu-sva.c +15 -0

+
+struct iommu_bond {
+	struct io_mm		*io_mm;
+	struct device		*dev;
+	struct iommu_domain	*domain;
+
+	struct list_head	mm_head;
+	struct list_head	dev_head;
+	struct list_head	domain_head;
+
+	void			*drvdata;
+};
+
+/*
+ * Because we're using an IDR, PASIDs are limited to 31 bits (the

sign bit is
+ * used for returning errors). In practice implementations will use at most 20
+ * bits, which is the PCI limit.
+ */

drivers/iommu/iommu-sva.c +5 -0

+static DEFINE_IDR(iommu_pasid_idr);
+
+/*
+ * For the moment this is an all-purpose lock. It serializes
+ * access/modifications to bonds, access/modifications to the PASID

IDR, and
+ * changes to io_mm refcount as well.
+ */

drivers/iommu/iommu-sva.c +22 -0

+static DEFINE_SPINLOCK(iommu_sva_lock);
+
+static struct io_mm *
+io_mm_alloc(struct iommu_domain *domain, struct device *dev,
+	    struct mm_struct *mm, unsigned long flags)
+{
+	int ret;
+	int pasid;
+	struct io_mm *io_mm;
+	struct iommu_sva_param *param = dev->iommu_param->sva_param;
+
+	if (!domain->ops->mm_alloc || !domain->ops->mm_free)
+		return ERR_PTR(-ENODEV);
+
+	io_mm = domain->ops->mm_alloc(domain, mm, flags);
+	if (IS_ERR(io_mm))
+		return io_mm;
+	if (!io_mm)
+		return ERR_PTR(-ENOMEM);
+
+	/*
+	 * The mm must not be freed until after the driver frees the

io_mm

drivers/iommu/iommu-sva.c +1 -0

+	 * (which may involve unpinning the CPU ASID for instance,

requiring a

drivers/iommu/iommu-sva.c +57 -0

+	 * valid mm struct.)
+	 */
+	mmgrab(mm);
+
+	io_mm->flags		= flags;
+	io_mm->mm		= mm;
+	io_mm->release		= domain->ops->mm_free;
+	INIT_LIST_HEAD(&io_mm->devices);
+
+	idr_preload(GFP_KERNEL);
+	spin_lock(&iommu_sva_lock);
+	pasid = idr_alloc(&iommu_pasid_idr, io_mm, param->min_pasid,
+			  param->max_pasid + 1, GFP_ATOMIC);
+	io_mm->pasid = pasid;
+	spin_unlock(&iommu_sva_lock);
+	idr_preload_end();
+
+	if (pasid < 0) {
+		ret = pasid;
+		goto err_free_mm;
+	}
+
+	/* TODO: keep track of mm. For the moment, abort. */
+	ret = -ENOSYS;
+	spin_lock(&iommu_sva_lock);
+	idr_remove(&iommu_pasid_idr, io_mm->pasid);
+	spin_unlock(&iommu_sva_lock);
+
+err_free_mm:
+	domain->ops->mm_free(io_mm);
+	mmdrop(mm);
+
+	return ERR_PTR(ret);
+}
+
+static void io_mm_free(struct io_mm *io_mm)
+{
+	struct mm_struct *mm = io_mm->mm;
+
+	io_mm->release(io_mm);
+	mmdrop(mm);
+}
+
+static void io_mm_release(struct kref *kref)
+{
+	struct io_mm *io_mm;
+
+	io_mm = container_of(kref, struct io_mm, kref);
+	WARN_ON(!list_empty(&io_mm->devices));
+
+	idr_remove(&iommu_pasid_idr, io_mm->pasid);
+
+	io_mm_free(io_mm);
+}
+
+/*
+ * Returns non-zero if a reference to the io_mm was successfully

taken.
+ * Returns zero if the io_mm is being freed and should not be used.
+ */

drivers/iommu/iommu-sva.c +21 -0

+static int io_mm_get_locked(struct io_mm *io_mm)
+{
+	if (io_mm)
+		return kref_get_unless_zero(&io_mm->kref);
+
+	return 0;
+}
+
+static void io_mm_put_locked(struct io_mm *io_mm)
+{
+	kref_put(&io_mm->kref, io_mm_release);
+}
+
+static void io_mm_put(struct io_mm *io_mm)
+{
+	spin_lock(&iommu_sva_lock);
+	io_mm_put_locked(io_mm);
+	spin_unlock(&iommu_sva_lock);
+}
+
+static int io_mm_attach(struct iommu_domain *domain, struct device

*dev,

drivers/iommu/iommu-sva.c +26 -0

+			struct io_mm *io_mm, void *drvdata)
+{
+	int ret;
+	bool attach_domain = true;
+	int pasid = io_mm->pasid;
+	struct iommu_bond *bond, *tmp;
+	struct iommu_sva_param *param = dev->iommu_param->sva_param;
+
+	if (!domain->ops->mm_attach || !domain->ops->mm_detach)
+		return -ENODEV;
+
+	if (pasid > param->max_pasid || pasid < param->min_pasid)
+		return -ERANGE;
+
+	bond = kzalloc(sizeof(*bond), GFP_KERNEL);
+	if (!bond)
+		return -ENOMEM;
+
+	bond->domain		= domain;
+	bond->io_mm		= io_mm;
+	bond->dev		= dev;
+	bond->drvdata		= drvdata;
+
+	spin_lock(&iommu_sva_lock);
+	/*
+	 * Check if this io_mm is already bound to the domain. In

which case the

drivers/iommu/iommu-sva.c +1 -0

+	 * IOMMU driver doesn't have to install the PASID table

entry.

drivers/iommu/iommu-sva.c +9 -0

+	 */
+	list_for_each_entry(tmp, &domain->mm_list, domain_head) {
+		if (tmp->io_mm == io_mm) {
+			attach_domain = false;
+			break;
+		}
+	}
+
+	ret = domain->ops->mm_attach(domain, dev, io_mm,

attach_domain);

drivers/iommu/iommu-sva.c +22 -0

+	if (ret) {
+		kfree(bond);
+		spin_unlock(&iommu_sva_lock);
+		return ret;
+	}
+
+	list_add(&bond->mm_head, &io_mm->devices);
+	list_add(&bond->domain_head, &domain->mm_list);
+	list_add(&bond->dev_head, &param->mm_list);
+	spin_unlock(&iommu_sva_lock);
+
+	return 0;
+}
+
+static void io_mm_detach_locked(struct iommu_bond *bond)
+{
+	struct iommu_bond *tmp;
+	bool detach_domain = true;
+	struct iommu_domain *domain = bond->domain;
+
+	list_for_each_entry(tmp, &domain->mm_list, domain_head) {
+		if (tmp->io_mm == bond->io_mm && tmp->dev !=

bond->dev) {

drivers/iommu/iommu-sva.c +6 -0

+			detach_domain = false;
+			break;
+		}
+	}
+
+	domain->ops->mm_detach(domain, bond->dev, bond->io_mm,

detach_domain); +

drivers/iommu/iommu-sva.c +7 -0

+	list_del(&bond->mm_head);
+	list_del(&bond->domain_head);
+	list_del(&bond->dev_head);
+	io_mm_put_locked(bond->io_mm);
+
+	kfree(bond);
+}
 
 /**
  * iommu_sva_device_init() - Initialize Shared Virtual Addressing

for a device @@ -47,6 +337,7 @@ int iommu_sva_device_init(struct device *dev, unsigned long features,

drivers/iommu/iommu-sva.c +1 -0

 	param->features		= features;
 	param->max_pasid	= max_pasid;
+	INIT_LIST_HEAD(&param->mm_list);
 
 	/*
 	 * IOMMU driver updates the limits depending on the IOMMU

and device @@ -114,13 +405,87 @@ EXPORT_SYMBOL_GPL(iommu_sva_device_shutdown); int __iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, int *pasid, unsigned long flags, void *drvdata) {

drivers/iommu/iommu-sva.c +12 -1

-	return -ENOSYS; /* TODO */
+	int i, ret = 0;
+	struct io_mm *io_mm = NULL;
+	struct iommu_domain *domain;
+	struct iommu_bond *bond = NULL, *tmp;
+	struct iommu_sva_param *param = dev->iommu_param->sva_param;
+
+	domain = iommu_get_domain_for_dev(dev);
+	if (!domain)
+		return -EINVAL;
+
+	/*
+	 * The device driver does not call sva_device_init/shutdown

and

drivers/iommu/iommu-sva.c +1 -0

+	 * bind/unbind concurrently, so no need to take the param

lock.

drivers/iommu/iommu-sva.c +9 -0

+	 */
+	if (WARN_ON_ONCE(!param) || (flags & ~param->features))
+		return -EINVAL;
+
+	/* If an io_mm already exists, use it */
+	spin_lock(&iommu_sva_lock);
+	idr_for_each_entry(&iommu_pasid_idr, io_mm, i) {
+		if (io_mm->mm == mm && io_mm_get_locked(io_mm)) {
+			/* ... Unless it's already bound to this

device */

drivers/iommu/iommu-sva.c +1 -0

+			list_for_each_entry(tmp, &io_mm->devices,

mm_head) {

drivers/iommu/iommu-sva.c +63 -2

+				if (tmp->dev == dev) {
+					bond = tmp;
+					io_mm_put_locked(io_mm);
+					break;
+				}
+			}
+			break;
+		}
+	}
+	spin_unlock(&iommu_sva_lock);
+
+	if (bond)
+		return -EEXIST;
+
+	/* Require identical features within an io_mm for now */
+	if (io_mm && (flags != io_mm->flags)) {
+		io_mm_put(io_mm);
+		return -EDOM;
+	}
+
+	if (!io_mm) {
+		io_mm = io_mm_alloc(domain, dev, mm, flags);
+		if (IS_ERR(io_mm))
+			return PTR_ERR(io_mm);
+	}
+
+	ret = io_mm_attach(domain, dev, io_mm, drvdata);
+	if (ret)
+		io_mm_put(io_mm);
+	else
+		*pasid = io_mm->pasid;
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__iommu_sva_bind_device);
 
 int __iommu_sva_unbind_device(struct device *dev, int pasid)
 {
-	return -ENOSYS; /* TODO */
+	int ret = -ESRCH;
+	struct iommu_domain *domain;
+	struct iommu_bond *bond = NULL;
+	struct iommu_sva_param *param = dev->iommu_param->sva_param;
+
+	domain = iommu_get_domain_for_dev(dev);
+	if (!param || WARN_ON(!domain))
+		return -EINVAL;
+
+	spin_lock(&iommu_sva_lock);
+	list_for_each_entry(bond, &param->mm_list, dev_head) {
+		if (bond->io_mm->pasid == pasid) {
+			io_mm_detach_locked(bond);
+			ret = 0;
+			break;
+		}
+	}
+	spin_unlock(&iommu_sva_lock);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__iommu_sva_unbind_device);

@@ -132,6 +497,15 @@ EXPORT_SYMBOL_GPL(__iommu_sva_unbind_device);
  */
 void __iommu_sva_unbind_dev_all(struct device *dev)
 {
-	/* TODO */
+	struct iommu_sva_param *param;
+	struct iommu_bond *bond, *next;
+
+	param = dev->iommu_param->sva_param;
+	if (param) {
+		spin_lock(&iommu_sva_lock);
+		list_for_each_entry_safe(bond, next,
 &param->mm_list, dev_head)
+			io_mm_detach_locked(bond);
+		spin_unlock(&iommu_sva_lock);
+	}
 }

EXPORT_SYMBOL_GPL(__iommu_sva_unbind_dev_all);

drivers/iommu/iommu.c +1 -0

index bd2819deae5b..333801e1519c 100644
@@ -1463,6 +1463,7 @@ static struct iommu_domain
 *__iommu_domain_alloc(struct bus_type *bus, domain->type = type;
 	/* Assume all sizes by default; the driver may override this
 later */ domain->pgsize_bitmap  = bus->iommu_ops->pgsize_bitmap;
+	INIT_LIST_HEAD(&domain->mm_list);
 
 	return domain;
 }

include/linux/iommu.h +15 -0

index da59c20c4f12..d5f21719a5a0 100644
@@ -100,6 +100,20 @@ struct iommu_domain {
 	void *handler_token;
 	struct iommu_domain_geometry geometry;
 	void *iova_cookie;
+
+	struct list_head mm_list;
+};
+
+struct io_mm {
+	int			pasid;
+	/* IOMMU_SVA_FEAT_* */
+	unsigned long		flags;
+	struct list_head	devices;
+	struct kref		kref;
+	struct mm_struct	*mm;
+
+	/* Release callback for this mm */
+	void (*release)(struct io_mm *io_mm);
 };
 
 enum iommu_cap {

@@ -216,6 +230,7 @@ struct iommu_sva_param {
 	unsigned long features;
 	unsigned int min_pasid;
 	unsigned int max_pasid;
+	struct list_head mm_list;
 };
 
 /**

@@ -227,6 +242,11 @@ struct iommu_sva_param {
  * @detach_dev: detach device from an iommu domain
  * @sva_device_init: initialize Shared Virtual Adressing for a device
  * @sva_device_shutdown: shutdown Shared Virtual Adressing for a

device
+ * @mm_alloc: allocate io_mm
+ * @mm_free: free io_mm
+ * @mm_attach: attach io_mm to a device. Install PASID entry if necessary
+ * @mm_detach: detach io_mm from a device. Remove PASID entry and
+ * flush associated TLB entries.
* @map: map a physically contiguous memory region to an iommu domain
* @unmap: unmap a physically contiguous memory region from an iommu
domain
* @map_sg: map a scatter-gather list of physically contiguous
memory chunks @@ -268,6 +288,14 @@ struct iommu_ops {

include/linux/iommu.h +5 -0

 			       struct iommu_sva_param *param);
 	void (*sva_device_shutdown)(struct device *dev,
 				    struct iommu_sva_param *param);
+	struct io_mm *(*mm_alloc)(struct iommu_domain *domain,
+				  struct mm_struct *mm,
+				  unsigned long flags);
+	void (*mm_free)(struct io_mm *io_mm);
+	int (*mm_attach)(struct iommu_domain *domain, struct device

*dev,

include/linux/iommu.h +2 -0

+			 struct io_mm *io_mm, bool attach_domain);
+	void (*mm_detach)(struct iommu_domain *domain, struct device

*dev,

include/linux/iommu.h +1 -0

+			  struct io_mm *io_mm, bool detach_domain);
 	int (*map)(struct iommu_domain *domain, unsigned long iova,
 		   phys_addr_t paddr, size_t size, int prot);
 	size_t (*unmap)(struct iommu_domain *domain, unsigned long

iova,

On Fri, 11 May 2018 20:06:04 +0100
I am a little confused about domain vs. pasid relationship. If each domain represents a address space, should there be a domain for each pasid?

Quoted 4 lines · Jacob Pan

On Fri, 11 May 2018 20:06:04 +0100
I am a little confused about domain vs. pasid relationship. If each domain represents a address space, should there be a domain for each pasid?

I don't think there is a formal definition, but from previous discussion the consensus seems to be: domains are a collection of devices that have the same virtual address spaces (one or many).

Keeping that definition makes things easier, in my opinion. Some time ago, I did try to represent PASIDs using "subdomains" (introducing a hierarchy of struct iommu_domain), but it required invasive changes in the IOMMU subsystem and probably all over the tree.

You do need some kind of "root domain" for each device, so that "iommu_get_domain_for_dev()" still makes sense. That root domain doesn't have a single address space but a collection of subdomains. If you need this anyway, representing a PASID with an iommu_domain doesn't seem preferable than using a different structure (io_mm), because they don't have anything in common.

Thanks,
Jean

Quoted 15 lines · On Thu, 17 May 2018 11:02:42 +0100 Jean-Philippe Brucker…

Quoted 4 lines · Jacob Pan

On Fri, 11 May 2018 20:06:04 +0100
I am a little confused about domain vs. pasid relationship. If each domain represents a address space, should there be a domain for each pasid?

I don't think there is a formal definition, but from previous discussion the consensus seems to be: domains are a collection of devices that have the same virtual address spaces (one or many).

Keeping that definition makes things easier, in my opinion. Some time ago, I did try to represent PASIDs using "subdomains" (introducing a hierarchy of struct iommu_domain), but it required invasive changes in the IOMMU subsystem and probably all over the tree.

Quoted 7 lines

You do need some kind of "root domain" for each device, so that "iommu_get_domain_for_dev()" still makes sense. That root domain doesn't have a single address space but a collection of subdomains. If you need this anyway, representing a PASID with an iommu_domain doesn't seem preferable than using a different structure (io_mm), because they don't have anything in common.

My main concern is the PASID table storage. If PASID table storage is tied to domain, it is ok to scale up, i.e. multiple devices in a domain share a single PASID table. But to scale down, e.g. further partition device with VFIO mdev for assignment, each mdev may get its own domain via vfio. But there is no IOMMU storage for PASID table at mdev device level. Perhaps we do need root domain or some parent-child relationship to locate PASID table.

Interesting, I hadn't thought about this use-case before. At first I thought you were talking about mdev devices assigned to VMs, but I think you're referring to mdevs assigned to userspace drivers instead? Out of curiosity, is it only theoretical or does someone actually need this?

Quoted 4 lines

Interesting, I hadn't thought about this use-case before. At first I thought you were talking about mdev devices assigned to VMs, but I think you're referring to mdevs assigned to userspace drivers instead? Out of curiosity, is it only theoretical or does someone actually need this?

There has been some non upstreamed efforts to have mdev and produce userspace drivers. Huawei is using it on what they call "wrapdrive" for crypto devices and we did a proof of concept for ethernet interfaces. At the time we choose not to involve the IOMMU for the reason you mentioned, but having it there would be good.

Quoted 10 lines · Ilias Apalodimas

Quoted 4 lines

Interesting, I hadn't thought about this use-case before. At first I thought you were talking about mdev devices assigned to VMs, but I think you're referring to mdevs assigned to userspace drivers instead? Out of curiosity, is it only theoretical or does someone actually need this?

There has been some non upstreamed efforts to have mdev and produce userspace drivers. Huawei is using it on what they call "wrapdrive" for crypto devices and we did a proof of concept for ethernet interfaces. At the time we choose not to involve the IOMMU for the reason you mentioned, but having it there would be good.

I'm guessing there were good reasons to do it that way but I wonder, is it not simpler to just have the kernel driver create a /dev/foo, with a standard ioctl/mmap/poll interface? Here VFIO adds a layer of indirection, and since the mediating driver has to implement these operations already, what is gained?

Thanks,
Jean

Quoted 12 lines · Jean-Philippe Brucker

Quoted 10 lines · Ilias Apalodimas

Quoted 4 lines

Interesting, I hadn't thought about this use-case before. At first I thought you were talking about mdev devices assigned to VMs, but I think you're referring to mdevs assigned to userspace drivers instead? Out of curiosity, is it only theoretical or does someone actually need this?

There has been some non upstreamed efforts to have mdev and produce userspace drivers. Huawei is using it on what they call "wrapdrive" for crypto devices and we did a proof of concept for ethernet interfaces. At the time we choose not to involve the IOMMU for the reason you mentioned, but having it there would be good.

Quoted 5 lines

I'm guessing there were good reasons to do it that way but I wonder, is it not simpler to just have the kernel driver create a /dev/foo, with a standard ioctl/mmap/poll interface? Here VFIO adds a layer of indirection, and since the mediating driver has to implement these operations already, what is gained?

The best reason i can come up with is "common code". You already have one API doing that for you so we replicate it in a /dev file?
The mdev approach still needs extentions to support what we tried to do (i.e mdev bus might need yo have access on iommu_ops), but as far as i undestand it's a possible case.

Quoted 3 lines

Thanks,
Jean

Date: Fri, 25 May 2018 09:39:59 +0100
From: Jonathan Cameron <[email protected]>
To: Ilias Apalodimas <[email protected]>
CC: Jean-Philippe Brucker <[email protected]>,
"[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Will Deacon <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "tn-nYOzD4b6Jr9Wk0Htik3J/[email protected]" <tn-nYOzD4b6Jr9Wk0Htik3J/[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "rfranz-YGCgFSpz5w/[email protected]" <rfranz-YGCgFSpz5w/[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" <linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org" <iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>, Robin Murphy <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]" <baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]>, liguozhu-C8/M+/[email protected], [email protected]
Subject: Re: [PATCH v2 03/40] iommu/sva: Manage process address spaces
Message-ID: <[email protected]>

+CC Kenneth Lee

Quoted 23 lines · On Fri, 25 May 2018 09:33:11 +0300 Ilias Apalodimas…

Quoted 12 lines · Jean-Philippe Brucker

Quoted 10 lines · Ilias Apalodimas

Quoted 4 lines

Interesting, I hadn't thought about this use-case before. At first I thought you were talking about mdev devices assigned to VMs, but I think you're referring to mdevs assigned to userspace drivers instead? Out of curiosity, is it only theoretical or does someone actually need this?

There has been some non upstreamed efforts to have mdev and produce userspace drivers. Huawei is using it on what they call "wrapdrive" for crypto devices and we did a proof of concept for ethernet interfaces. At the time we choose not to involve the IOMMU for the reason you mentioned, but having it there would be good.

Quoted 5 lines

I'm guessing there were good reasons to do it that way but I wonder, is it not simpler to just have the kernel driver create a /dev/foo, with a standard ioctl/mmap/poll interface? Here VFIO adds a layer of indirection, and since the mediating driver has to implement these operations already, what is gained?

The best reason i can come up with is "common code". You already have one API doing that for you so we replicate it in a /dev file?
The mdev approach still needs extentions to support what we tried to do (i.e mdev bus might need yo have access on iommu_ops), but as far as i undestand it's a possible case.

Quoted 3 lines

Quoted 3 lines

Thanks,
Jean

Date: Fri, 25 May 2018 09:39:59 +0100
From: Jonathan Cameron <[email protected]>
To: Ilias Apalodimas <[email protected]>
CC: Jean-Philippe Brucker <[email protected]>,
"[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Will Deacon <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Jacob Pan <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Robin Murphy <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, [email protected], [email protected]
Subject: Re: [PATCH v2 03/40] iommu/sva: Manage process address spaces
Message-ID: <[email protected]>

+CC Kenneth Lee

Quoted 23 lines · On Fri, 25 May 2018 09:33:11 +0300 Ilias Apalodimas…

Quoted 12 lines · Jean-Philippe Brucker

Quoted 10 lines · Ilias Apalodimas

Quoted 4 lines

Interesting, I hadn't thought about this use-case before. At first I thought you were talking about mdev devices assigned to VMs, but I think you're referring to mdevs assigned to userspace drivers instead? Out of curiosity, is it only theoretical or does someone actually need this?

There has been some non upstreamed efforts to have mdev and produce userspace drivers. Huawei is using it on what they call "wrapdrive" for crypto devices and we did a proof of concept for ethernet interfaces. At the time we choose not to involve the IOMMU for the reason you mentioned, but having it there would be good.

Quoted 5 lines

I'm guessing there were good reasons to do it that way but I wonder, is it not simpler to just have the kernel driver create a /dev/foo, with a standard ioctl/mmap/poll interface? Here VFIO adds a layer of indirection, and since the mediating driver has to implement these operations already, what is gained?

The best reason i can come up with is "common code". You already have one API doing that for you so we replicate it in a /dev file?
The mdev approach still needs extentions to support what we tried to do (i.e mdev bus might need yo have access on iommu_ops), but as far as i undestand it's a possible case.

Quoted 3 lines

Quoted 3 lines

Thanks,
Jean

Date: Fri, 25 May 2018 09:39:59 +0100
From: Jonathan Cameron <[email protected]>
To: Ilias Apalodimas <[email protected]>
CC: Jean-Philippe Brucker <[email protected]>,
"xieyisheng1 at huawei.com" <[email protected]>, "kvm at vger.kernel.org" <[email protected]>, "linux-pci at vger.kernel.org" <[email protected]>, "xuzaibo at huawei.com" <[email protected]>, Will Deacon <[email protected]>, "okaya at codeaurora.org" <[email protected]>, "linux-mm at kvack.org" <[email protected]>, "yi.l.liu at intel.com" <[email protected]>, "ashok.raj at intel.com" <[email protected]>, "tn at semihalf.com" <[email protected]>, "joro at 8bytes.org" <[email protected]>, "robdclark at gmail.com" <[email protected]>, "bharatku at xilinx.com" <[email protected]>, "linux-acpi at vger.kernel.org" <[email protected]>, "liudongdong3 at huawei.com" <[email protected]>, "rfranz at cavium.com" <[email protected]>, "devicetree at vger.kernel.org" <[email protected]>, "kevin.tian at intel.com" <[email protected]>, Jacob Pan <[email protected]>, "alex.williamson at redhat.com" <[email protected]>, "rgummal at xilinx.com" <[email protected]>, "thunder.leizhen at huawei.com" <[email protected]>, "linux-arm-kernel at lists.infradead.org" <[email protected]>, "shunyong.yang at hxt-semitech.com" <[email protected]>, "dwmw2 at infradead.org" <[email protected]>, "liubo95 at huawei.com" <[email protected]>, "jcrouse at codeaurora.org" <[email protected]>,
"iommu at lists.linux-foundation.org" <[email protected]>, Robin Murphy <[email protected]>, "christian.koenig at amd.com" <[email protected]>, "nwatters at codeaurora.org" <[email protected]>, "baolu.lu at linux.intel.com" <[email protected]>, liguozhu at hisilicon.com, kenneth-lee-2012 at foxmail.com
Subject: Re: [PATCH v2 03/40] iommu/sva: Manage process address spaces
Message-ID: <[email protected]>

+CC Kenneth Lee

Quoted 23 lines · On Fri, 25 May 2018 09:33:11 +0300 Ilias Apalodimas…

Quoted 12 lines · Jean-Philippe Brucker

Quoted 10 lines · Ilias Apalodimas

Quoted 4 lines

Interesting, I hadn't thought about this use-case before. At first I thought you were talking about mdev devices assigned to VMs, but I think you're referring to mdevs assigned to userspace drivers instead? Out of curiosity, is it only theoretical or does someone actually need this?

There has been some non upstreamed efforts to have mdev and produce userspace drivers. Huawei is using it on what they call "wrapdrive" for crypto devices and we did a proof of concept for ethernet interfaces. At the time we choose not to involve the IOMMU for the reason you mentioned, but having it there would be good.

Quoted 5 lines

I'm guessing there were good reasons to do it that way but I wonder, is it not simpler to just have the kernel driver create a /dev/foo, with a standard ioctl/mmap/poll interface? Here VFIO adds a layer of indirection, and since the mediating driver has to implement these operations already, what is gained?

The best reason i can come up with is "common code". You already have one API doing that for you so we replicate it in a /dev file?
The mdev approach still needs extentions to support what we tried to do (i.e mdev bus might need yo have access on iommu_ops), but as far as i undestand it's a possible case.

Quoted 3 lines

Quoted 3 lines

Thanks,
Jean

Date: Sat, 26 May 2018 10:24:45 +0800
From: Kenneth Lee <[email protected]>
To: Jonathan Cameron <[email protected]>
Cc: Ilias Apalodimas <[email protected]>, Jean-Philippe Brucker
<[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>,
"[email protected]" <[email protected]>, Will Deacon <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "tn-nYOzD4b6Jr9Wk0Htik3J/[email protected]" <tn-nYOzD4b6Jr9Wk0Htik3J/[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "rfranz-YGCgFSpz5w/[email protected]" <rfranz-YGCgFSpz5w/[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org"
<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>, "[email protected]" <[email protected]>, "[email protected]"
<[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org" <iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>,
Robin Murphy <[email protected]>, "[email protected]"
<[email protected]>, "[email protected]"
<[email protected]>, "baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]"
<baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]>, liguozhu-C8/M+/[email protected]
Subject: Re: [PATCH v2 03/40] iommu/sva: Manage process address spaces
Message-ID: <20180526022445.GA6069@kllp05>

Quoted 34 lines · Jonathan Cameron

Date: Fri, 25 May 2018 09:39:59 +0100
From: Jonathan Cameron <[email protected]>
To: Ilias Apalodimas <[email protected]>
CC: Jean-Philippe Brucker <[email protected]>,
"[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Will Deacon <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "tn-nYOzD4b6Jr9Wk0Htik3J/[email protected]" <tn-nYOzD4b6Jr9Wk0Htik3J/[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "rfranz-YGCgFSpz5w/[email protected]" <rfranz-YGCgFSpz5w/[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" <linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org" <iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>, Robin Murphy <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]" <baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]>, liguozhu-C8/M+/[email protected], [email protected]
Subject: Re: [PATCH v2 03/40] iommu/sva: Manage process address spaces
Message-ID: <[email protected]>

Quoted 28 lines

+CC Kenneth Lee

Quoted 23 lines · On Fri, 25 May 2018 09:33:11 +0300 Ilias Apalodimas…

Quoted 12 lines · Jean-Philippe Brucker

Quoted 10 lines · Ilias Apalodimas

Quoted 4 lines

Interesting, I hadn't thought about this use-case before. At first I thought you were talking about mdev devices assigned to VMs, but I think you're referring to mdevs assigned to userspace drivers instead? Out of curiosity, is it only theoretical or does someone actually need this?

There has been some non upstreamed efforts to have mdev and produce userspace drivers. Huawei is using it on what they call "wrapdrive" for crypto devices and we did a proof of concept for ethernet interfaces. At the time we choose not to involve the IOMMU for the reason you mentioned, but having it there would be good.

Quoted 5 lines

I'm guessing there were good reasons to do it that way but I wonder, is it not simpler to just have the kernel driver create a /dev/foo, with a standard ioctl/mmap/poll interface? Here VFIO adds a layer of indirection, and since the mediating driver has to implement these operations already, what is gained?

The best reason i can come up with is "common code". You already have one API doing that for you so we replicate it in a /dev file?
The mdev approach still needs extentions to support what we tried to do (i.e mdev bus might need yo have access on iommu_ops), but as far as i undestand it's a possible case.

Hi, Jean, Please allow me to share my understanding here:
https://zhuanlan.zhihu.com/p/35489035

The reason we do not use the /dev/foo scheme is that the devices to be shared are programmable accelerators. We cannot fix up the kernel driver for them.

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

Thanks,
Jean

Signature · 1 line

			-Kenneth Lee (Hisilicon)

Date: Sat, 26 May 2018 10:24:45 +0800
From: Kenneth Lee <[email protected]>
To: Jonathan Cameron <[email protected]>
Cc: Ilias Apalodimas <[email protected]>, Jean-Philippe Brucker
<[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Will Deacon <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Jacob Pan <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Robin Murphy <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, [email protected]
Subject: Re: [PATCH v2 03/40] iommu/sva: Manage process address spaces
Message-ID: <20180526022445.GA6069@kllp05>

Quoted 34 lines · Jonathan Cameron

Date: Fri, 25 May 2018 09:39:59 +0100
From: Jonathan Cameron <[email protected]>
To: Ilias Apalodimas <[email protected]>
CC: Jean-Philippe Brucker <[email protected]>,
"[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Will Deacon <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Jacob Pan <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, Robin Murphy <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, [email protected], [email protected]
Subject: Re: [PATCH v2 03/40] iommu/sva: Manage process address spaces
Message-ID: <[email protected]>

Quoted 28 lines

+CC Kenneth Lee

Quoted 23 lines · On Fri, 25 May 2018 09:33:11 +0300 Ilias Apalodimas…

Quoted 12 lines · Jean-Philippe Brucker

Quoted 10 lines · Ilias Apalodimas

Quoted 4 lines

Interesting, I hadn't thought about this use-case before. At first I thought you were talking about mdev devices assigned to VMs, but I think you're referring to mdevs assigned to userspace drivers instead? Out of curiosity, is it only theoretical or does someone actually need this?

There has been some non upstreamed efforts to have mdev and produce userspace drivers. Huawei is using it on what they call "wrapdrive" for crypto devices and we did a proof of concept for ethernet interfaces. At the time we choose not to involve the IOMMU for the reason you mentioned, but having it there would be good.

Quoted 5 lines

I'm guessing there were good reasons to do it that way but I wonder, is it not simpler to just have the kernel driver create a /dev/foo, with a standard ioctl/mmap/poll interface? Here VFIO adds a layer of indirection, and since the mediating driver has to implement these operations already, what is gained?

The best reason i can come up with is "common code". You already have one API doing that for you so we replicate it in a /dev file?
The mdev approach still needs extentions to support what we tried to do (i.e mdev bus might need yo have access on iommu_ops), but as far as i undestand it's a possible case.

Hi, Jean, Please allow me to share my understanding here:
https://zhuanlan.zhihu.com/p/35489035