Re: [PATCH v2 02/40] iommu/sva: Bind process address spaces to devices

[Jean-Philippe Brucker]

On 05/09/2018 12:29, Auger Eric wrote:
>> +/**
>> + * 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.
> @flags must be a subset of these features?

Ok

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

Yes, that will be in next version

>> +/**
>> + * 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)
> returned value needed?

I'd rather keep this one, since it already pointed out some of my bugs
during regression testing.

Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 02/40] iommu/sva: Bind process address spaces to devices

[Auger Eric]

Hi Jean-Philippe,

On 05/11/2018 09:06 PM, Jean-Philippe Brucker wrote:
> 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 
> 
> ---
> 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(+)
> 
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 8b4afb7c63ae..8d98f9c09864 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -93,6 +93,8 @@ int iommu_sva_device_shutdown(struct device *dev)
>   if (!domain)
>   return -ENODEV;
>  
> + __iommu_sva_unbind_dev_all(dev);
> +
>   mutex_lock(>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);
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 9e28d88c8074..bd2819deae5b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -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.
@flags must be 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(>mutex);
> + if (iommu_group_device_count(group) != 1)
> + goto out_unlock;
don't you want to check flags is a subset of
dev->iommu_param->sva_param->features?
> +
> + ret = __iommu_sva_bind_device(dev, mm, pasid, flags, drvdata);

Re: [PATCH v2 02/40] iommu/sva: Bind process address spaces to devices

[Jean-Philippe Brucker]

On 17/05/18 14:10, Jonathan Cameron wrote:
> On Fri, 11 May 2018 20:06:03 +0100
> Jean-Philippe Brucker  wrote:
> 
>> 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).
> 
> 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.

Sure, I'll add something

Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 02/40] iommu/sva: Bind process address spaces to devices

[Jonathan Cameron]

On Fri, 11 May 2018 20:06:03 +0100
Jean-Philippe Brucker  wrote:

> 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).

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.

>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 

Otherwise, looks good to me.

> 
> ---
> 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(+)
> 
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 8b4afb7c63ae..8d98f9c09864 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -93,6 +93,8 @@ int iommu_sva_device_shutdown(struct device *dev)
>   if (!domain)
>   return -ENODEV;
>  
> + __iommu_sva_unbind_dev_all(dev);
> +
>   mutex_lock(>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);
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 9e28d88c8074..bd2819deae5b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -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 

[PATCH v2 02/40] iommu/sva: Bind process address spaces to devices

[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.

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 

---
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(+)

diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 8b4afb7c63ae..8d98f9c09864 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -93,6 +93,8 @@ int iommu_sva_device_shutdown(struct device *dev)
if (!domain)
return -ENODEV;
 
+   __iommu_sva_unbind_dev_all(dev);
+
mutex_lock(>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);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 9e28d88c8074..bd2819deae5b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -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(>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(>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
+