* [PATCH v2 0/2] iommu_aux_at(de)tach_device() enhancement
@ 2020-07-07 1:39 Lu Baolu
2020-07-07 1:39 ` [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension Lu Baolu
2020-07-07 1:39 ` [PATCH v2 2/2] iommu: Add aux_domain_attached flag to iommu_group Lu Baolu
0 siblings, 2 replies; 8+ messages in thread
From: Lu Baolu @ 2020-07-07 1:39 UTC (permalink / raw)
To: Joerg Roedel, Alex Williamson, Robin Murphy
Cc: Kevin Tian, Dave Jiang, Ashok Raj, kvm, Cornelia Huck,
linux-kernel, iommu
This series aims to enhance the iommu_aux_at(de)tach_device() api's
so that some generic iommu api's like iommu_get_domain_for_dev() could
also work for vfio/mdev device.
The initial version of this series was post at
https://lkml.org/lkml/2020/6/26/1118
This version is evolved according to Robin's feedback.
Your comments are very appreciated.
Best regards,
baolu
Lu Baolu (2):
iommu: iommu_aux_at(de)tach_device() extension
iommu: Add aux_domain_attached flag to iommu_group
drivers/iommu/iommu.c | 98 +++++++++++++++++++++++++++++----
drivers/vfio/vfio_iommu_type1.c | 5 +-
include/linux/iommu.h | 12 ++--
3 files changed, 99 insertions(+), 16 deletions(-)
--
2.17.1
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
2020-07-07 1:39 [PATCH v2 0/2] iommu_aux_at(de)tach_device() enhancement Lu Baolu
@ 2020-07-07 1:39 ` Lu Baolu
2020-07-07 21:04 ` Alex Williamson
2020-07-09 6:52 ` Lu Baolu
2020-07-07 1:39 ` [PATCH v2 2/2] iommu: Add aux_domain_attached flag to iommu_group Lu Baolu
1 sibling, 2 replies; 8+ messages in thread
From: Lu Baolu @ 2020-07-07 1:39 UTC (permalink / raw)
To: Joerg Roedel, Alex Williamson, Robin Murphy
Cc: Kevin Tian, Dave Jiang, Ashok Raj, kvm, Cornelia Huck,
linux-kernel, iommu
The hardware assistant vfio mediated device is a use case of iommu
aux-domain. The interactions between vfio/mdev and iommu during mdev
creation and passthr are:
- Create a group for mdev with iommu_group_alloc();
- Add the device to the group with
group = iommu_group_alloc();
if (IS_ERR(group))
return PTR_ERR(group);
ret = iommu_group_add_device(group, &mdev->dev);
if (!ret)
dev_info(&mdev->dev, "MDEV: group_id = %d\n",
iommu_group_id(group));
- Allocate an aux-domain
iommu_domain_alloc()
- Attach the aux-domain to the physical device from which the mdev is
created.
iommu_aux_attach_device()
In the whole process, an iommu group was allocated for the mdev and an
iommu domain was attached to the group, but the group->domain leaves
NULL. As the result, iommu_get_domain_for_dev() doesn't work anymore.
The iommu_get_domain_for_dev() is a necessary interface for device
drivers that want to support aux-domain. For example,
struct iommu_domain *domain;
struct device *dev = mdev_dev(mdev);
unsigned long pasid;
domain = iommu_get_domain_for_dev(dev);
if (!domain)
return -ENODEV;
pasid = iommu_aux_get_pasid(domain, dev->parent);
if (pasid == IOASID_INVALID)
return -EINVAL;
/* Program the device context with the PASID value */
....
This extends iommu_aux_at(de)tach_device() so that the users could pass
in an optional device pointer (struct device for vfio/mdev for example),
and the necessary check and data link could be done.
Fixes: a3a195929d40b ("iommu: Add APIs for multiple domains per device")
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/iommu.c | 86 +++++++++++++++++++++++++++++----
drivers/vfio/vfio_iommu_type1.c | 5 +-
include/linux/iommu.h | 12 +++--
3 files changed, 87 insertions(+), 16 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 1ed1e14a1f0c..435835058209 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2723,26 +2723,92 @@ EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
* This should make us safe against a device being attached to a guest as a
* whole while there are still pasid users on it (aux and sva).
*/
-int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
+int iommu_aux_attach_device(struct iommu_domain *domain,
+ struct device *phys_dev, struct device *dev)
{
- int ret = -ENODEV;
+ struct iommu_group *group;
+ int ret;
- if (domain->ops->aux_attach_dev)
- ret = domain->ops->aux_attach_dev(domain, dev);
+ if (!domain->ops->aux_attach_dev ||
+ !iommu_dev_feature_enabled(phys_dev, IOMMU_DEV_FEAT_AUX))
+ return -ENODEV;
- if (!ret)
- trace_attach_device_to_domain(dev);
+ /* Bare use only. */
+ if (!dev) {
+ ret = domain->ops->aux_attach_dev(domain, phys_dev);
+ if (!ret)
+ trace_attach_device_to_domain(phys_dev);
+
+ return ret;
+ }
+
+ /*
+ * The caller has created a made-up device (for example, vfio/mdev)
+ * and allocated an iommu_group for user level direct assignment.
+ * Make sure that the group has only single device and hasn't been
+ * attached by any other domain.
+ */
+ group = iommu_group_get(dev);
+ if (!group)
+ return -ENODEV;
+
+ /*
+ * Lock the group to make sure the device-count doesn't change while
+ * we are attaching.
+ */
+ mutex_lock(&group->mutex);
+ ret = -EINVAL;
+ if ((iommu_group_device_count(group) != 1) || group->domain)
+ goto out_unlock;
+
+ ret = -EBUSY;
+ if (group->default_domain && group->domain != group->default_domain)
+ goto out_unlock;
+
+ ret = domain->ops->aux_attach_dev(domain, phys_dev);
+ if (!ret) {
+ trace_attach_device_to_domain(phys_dev);
+ group->domain = domain;
+ }
+
+out_unlock:
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
return ret;
}
EXPORT_SYMBOL_GPL(iommu_aux_attach_device);
-void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev)
+void iommu_aux_detach_device(struct iommu_domain *domain,
+ struct device *phys_dev, struct device *dev)
{
- if (domain->ops->aux_detach_dev) {
- domain->ops->aux_detach_dev(domain, dev);
- trace_detach_device_from_domain(dev);
+ struct iommu_group *group;
+
+ if (WARN_ON_ONCE(!domain->ops->aux_detach_dev))
+ return;
+
+ if (!dev) {
+ domain->ops->aux_detach_dev(domain, phys_dev);
+ trace_detach_device_from_domain(phys_dev);
+
+ return;
}
+
+ group = iommu_group_get(dev);
+ if (!group)
+ return;
+
+ mutex_lock(&group->mutex);
+ if (WARN_ON(iommu_group_device_count(group) != 1))
+ goto out_unlock;
+
+ domain->ops->aux_detach_dev(domain, phys_dev);
+ group->domain = NULL;
+ trace_detach_device_from_domain(phys_dev);
+
+out_unlock:
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
}
EXPORT_SYMBOL_GPL(iommu_aux_detach_device);
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 5e556ac9102a..d3be45dfa58e 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1635,7 +1635,8 @@ static int vfio_mdev_attach_domain(struct device *dev, void *data)
iommu_device = vfio_mdev_get_iommu_device(dev);
if (iommu_device) {
if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
- return iommu_aux_attach_device(domain, iommu_device);
+ return iommu_aux_attach_device(domain,
+ iommu_device, dev);
else
return iommu_attach_device(domain, iommu_device);
}
@@ -1651,7 +1652,7 @@ static int vfio_mdev_detach_domain(struct device *dev, void *data)
iommu_device = vfio_mdev_get_iommu_device(dev);
if (iommu_device) {
if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
- iommu_aux_detach_device(domain, iommu_device);
+ iommu_aux_detach_device(domain, iommu_device, dev);
else
iommu_detach_device(domain, iommu_device);
}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5657d4fef9f2..7da5e67bf7dc 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -632,8 +632,10 @@ bool iommu_dev_has_feature(struct device *dev, enum iommu_dev_features f);
int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features f);
-int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev);
-void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev);
+int iommu_aux_attach_device(struct iommu_domain *domain,
+ struct device *phys_dev, struct device *dev);
+void iommu_aux_detach_device(struct iommu_domain *domain,
+ struct device *phys_dev, struct device *dev);
int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev);
struct iommu_sva *iommu_sva_bind_device(struct device *dev,
@@ -1007,13 +1009,15 @@ iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
}
static inline int
-iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
+iommu_aux_attach_device(struct iommu_domain *domain,
+ struct device *phys_dev, struct device *dev)
{
return -ENODEV;
}
static inline void
-iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev)
+iommu_aux_detach_device(struct iommu_domain *domain,
+ struct device *phys_dev, struct device *dev)
{
}
--
2.17.1
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] iommu: Add aux_domain_attached flag to iommu_group
2020-07-07 1:39 [PATCH v2 0/2] iommu_aux_at(de)tach_device() enhancement Lu Baolu
2020-07-07 1:39 ` [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension Lu Baolu
@ 2020-07-07 1:39 ` Lu Baolu
1 sibling, 0 replies; 8+ messages in thread
From: Lu Baolu @ 2020-07-07 1:39 UTC (permalink / raw)
To: Joerg Roedel, Alex Williamson, Robin Murphy
Cc: Kevin Tian, Dave Jiang, Ashok Raj, kvm, Cornelia Huck,
linux-kernel, iommu
The normal domain at(de)tach is parallel with aux-domain at(de)tach. In
another word, once an iommu_group is attached through the normal domain
attach api's, it should not go through the aux-domain at(de)tach api's
until the domain is detached. And, vice versa.
Currently, we prohibit an iommu_group to go through aux-domain api's if
group->domain != NULL; but we don't check aux-domain attachment in the
normal attach api's. This marks an iommu_group after an aux-domain is
attached, so that normal domain at(de)tach api's should never be used
after that.
Cc: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/iommu.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 435835058209..3e7489ea2010 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -45,6 +45,7 @@ struct iommu_group {
struct iommu_domain *default_domain;
struct iommu_domain *domain;
struct list_head entry;
+ unsigned int aux_domain_attached:1;
};
struct group_device {
@@ -2074,6 +2075,9 @@ static int __iommu_attach_group(struct iommu_domain *domain,
{
int ret;
+ if (group->aux_domain_attached)
+ return -EINVAL;
+
if (group->default_domain && group->domain != group->default_domain)
return -EBUSY;
@@ -2111,6 +2115,9 @@ static void __iommu_detach_group(struct iommu_domain *domain,
{
int ret;
+ if (WARN_ON(group->aux_domain_attached))
+ return;
+
if (!group->default_domain) {
__iommu_group_for_each_dev(group, domain,
iommu_group_do_detach_device);
@@ -2769,6 +2776,7 @@ int iommu_aux_attach_device(struct iommu_domain *domain,
if (!ret) {
trace_attach_device_to_domain(phys_dev);
group->domain = domain;
+ group->aux_domain_attached = true;
}
out_unlock:
@@ -2802,8 +2810,12 @@ void iommu_aux_detach_device(struct iommu_domain *domain,
if (WARN_ON(iommu_group_device_count(group) != 1))
goto out_unlock;
+ if (WARN_ON(!group->aux_domain_attached))
+ goto out_unlock;
+
domain->ops->aux_detach_dev(domain, phys_dev);
group->domain = NULL;
+ group->aux_domain_attached = false;
trace_detach_device_from_domain(phys_dev);
out_unlock:
--
2.17.1
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
2020-07-07 1:39 ` [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension Lu Baolu
@ 2020-07-07 21:04 ` Alex Williamson
2020-07-08 2:53 ` Lu Baolu
2020-07-09 6:52 ` Lu Baolu
1 sibling, 1 reply; 8+ messages in thread
From: Alex Williamson @ 2020-07-07 21:04 UTC (permalink / raw)
To: Lu Baolu
Cc: Kevin Tian, Dave Jiang, Ashok Raj, kvm, Cornelia Huck,
linux-kernel, iommu, Robin Murphy
On Tue, 7 Jul 2020 09:39:56 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:
> The hardware assistant vfio mediated device is a use case of iommu
> aux-domain. The interactions between vfio/mdev and iommu during mdev
> creation and passthr are:
>
> - Create a group for mdev with iommu_group_alloc();
> - Add the device to the group with
> group = iommu_group_alloc();
> if (IS_ERR(group))
> return PTR_ERR(group);
>
> ret = iommu_group_add_device(group, &mdev->dev);
> if (!ret)
> dev_info(&mdev->dev, "MDEV: group_id = %d\n",
> iommu_group_id(group));
> - Allocate an aux-domain
> iommu_domain_alloc()
> - Attach the aux-domain to the physical device from which the mdev is
> created.
> iommu_aux_attach_device()
>
> In the whole process, an iommu group was allocated for the mdev and an
> iommu domain was attached to the group, but the group->domain leaves
> NULL. As the result, iommu_get_domain_for_dev() doesn't work anymore.
>
> The iommu_get_domain_for_dev() is a necessary interface for device
> drivers that want to support aux-domain. For example,
>
> struct iommu_domain *domain;
> struct device *dev = mdev_dev(mdev);
> unsigned long pasid;
>
> domain = iommu_get_domain_for_dev(dev);
> if (!domain)
> return -ENODEV;
>
> pasid = iommu_aux_get_pasid(domain, dev->parent);
How did we know this was an aux domain? ie. How did we know we could
use it with iommu_aux_get_pasid()?
Why did we assume the parent device is the iommu device for the aux
domain? Should that level of detail be already known by the aux domain?
Nits - The iomu device of an mdev device is found via
mdev_get_iommu_device(dev), it should not be assumed to be the parent.
The parent of an mdev device is found via mdev_parent_dev(mdev).
The leaps in logic here make me wonder if we should instead be exposing
more of an aux domain API rather than blurring the differences between
these domains. Thanks,
Alex
> if (pasid == IOASID_INVALID)
> return -EINVAL;
>
> /* Program the device context with the PASID value */
> ....
>
> This extends iommu_aux_at(de)tach_device() so that the users could pass
> in an optional device pointer (struct device for vfio/mdev for example),
> and the necessary check and data link could be done.
>
> Fixes: a3a195929d40b ("iommu: Add APIs for multiple domains per device")
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> drivers/iommu/iommu.c | 86 +++++++++++++++++++++++++++++----
> drivers/vfio/vfio_iommu_type1.c | 5 +-
> include/linux/iommu.h | 12 +++--
> 3 files changed, 87 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 1ed1e14a1f0c..435835058209 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2723,26 +2723,92 @@ EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
> * This should make us safe against a device being attached to a guest as a
> * whole while there are still pasid users on it (aux and sva).
> */
> -int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
> +int iommu_aux_attach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev)
> {
> - int ret = -ENODEV;
> + struct iommu_group *group;
> + int ret;
>
> - if (domain->ops->aux_attach_dev)
> - ret = domain->ops->aux_attach_dev(domain, dev);
> + if (!domain->ops->aux_attach_dev ||
> + !iommu_dev_feature_enabled(phys_dev, IOMMU_DEV_FEAT_AUX))
> + return -ENODEV;
>
> - if (!ret)
> - trace_attach_device_to_domain(dev);
> + /* Bare use only. */
> + if (!dev) {
> + ret = domain->ops->aux_attach_dev(domain, phys_dev);
> + if (!ret)
> + trace_attach_device_to_domain(phys_dev);
> +
> + return ret;
> + }
> +
> + /*
> + * The caller has created a made-up device (for example, vfio/mdev)
> + * and allocated an iommu_group for user level direct assignment.
> + * Make sure that the group has only single device and hasn't been
> + * attached by any other domain.
> + */
> + group = iommu_group_get(dev);
> + if (!group)
> + return -ENODEV;
> +
> + /*
> + * Lock the group to make sure the device-count doesn't change while
> + * we are attaching.
> + */
> + mutex_lock(&group->mutex);
> + ret = -EINVAL;
> + if ((iommu_group_device_count(group) != 1) || group->domain)
> + goto out_unlock;
> +
> + ret = -EBUSY;
> + if (group->default_domain && group->domain != group->default_domain)
> + goto out_unlock;
> +
> + ret = domain->ops->aux_attach_dev(domain, phys_dev);
> + if (!ret) {
> + trace_attach_device_to_domain(phys_dev);
> + group->domain = domain;
> + }
> +
> +out_unlock:
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
>
> return ret;
> }
> EXPORT_SYMBOL_GPL(iommu_aux_attach_device);
>
> -void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev)
> +void iommu_aux_detach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev)
> {
> - if (domain->ops->aux_detach_dev) {
> - domain->ops->aux_detach_dev(domain, dev);
> - trace_detach_device_from_domain(dev);
> + struct iommu_group *group;
> +
> + if (WARN_ON_ONCE(!domain->ops->aux_detach_dev))
> + return;
> +
> + if (!dev) {
> + domain->ops->aux_detach_dev(domain, phys_dev);
> + trace_detach_device_from_domain(phys_dev);
> +
> + return;
> }
> +
> + group = iommu_group_get(dev);
> + if (!group)
> + return;
> +
> + mutex_lock(&group->mutex);
> + if (WARN_ON(iommu_group_device_count(group) != 1))
> + goto out_unlock;
> +
> + domain->ops->aux_detach_dev(domain, phys_dev);
> + group->domain = NULL;
> + trace_detach_device_from_domain(phys_dev);
> +
> +out_unlock:
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
> }
> EXPORT_SYMBOL_GPL(iommu_aux_detach_device);
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 5e556ac9102a..d3be45dfa58e 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1635,7 +1635,8 @@ static int vfio_mdev_attach_domain(struct device *dev, void *data)
> iommu_device = vfio_mdev_get_iommu_device(dev);
> if (iommu_device) {
> if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
> - return iommu_aux_attach_device(domain, iommu_device);
> + return iommu_aux_attach_device(domain,
> + iommu_device, dev);
> else
> return iommu_attach_device(domain, iommu_device);
> }
> @@ -1651,7 +1652,7 @@ static int vfio_mdev_detach_domain(struct device *dev, void *data)
> iommu_device = vfio_mdev_get_iommu_device(dev);
> if (iommu_device) {
> if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
> - iommu_aux_detach_device(domain, iommu_device);
> + iommu_aux_detach_device(domain, iommu_device, dev);
> else
> iommu_detach_device(domain, iommu_device);
> }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 5657d4fef9f2..7da5e67bf7dc 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -632,8 +632,10 @@ bool iommu_dev_has_feature(struct device *dev, enum iommu_dev_features f);
> int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
> int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
> bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features f);
> -int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev);
> -void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev);
> +int iommu_aux_attach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev);
> +void iommu_aux_detach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev);
> int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev);
>
> struct iommu_sva *iommu_sva_bind_device(struct device *dev,
> @@ -1007,13 +1009,15 @@ iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
> }
>
> static inline int
> -iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
> +iommu_aux_attach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev)
> {
> return -ENODEV;
> }
>
> static inline void
> -iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev)
> +iommu_aux_detach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev)
> {
> }
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
2020-07-07 21:04 ` Alex Williamson
@ 2020-07-08 2:53 ` Lu Baolu
2020-07-08 19:07 ` Alex Williamson
0 siblings, 1 reply; 8+ messages in thread
From: Lu Baolu @ 2020-07-08 2:53 UTC (permalink / raw)
To: Alex Williamson
Cc: Kevin Tian, Dave Jiang, Ashok Raj, kvm, Cornelia Huck,
linux-kernel, iommu, Robin Murphy
Hi Alex,
Thanks a lot for your comments. Please check my reply inline.
On 7/8/20 5:04 AM, Alex Williamson wrote:
> On Tue, 7 Jul 2020 09:39:56 +0800
> Lu Baolu<baolu.lu@linux.intel.com> wrote:
>
>> The hardware assistant vfio mediated device is a use case of iommu
>> aux-domain. The interactions between vfio/mdev and iommu during mdev
>> creation and passthr are:
>>
>> - Create a group for mdev with iommu_group_alloc();
>> - Add the device to the group with
>> group = iommu_group_alloc();
>> if (IS_ERR(group))
>> return PTR_ERR(group);
>>
>> ret = iommu_group_add_device(group, &mdev->dev);
>> if (!ret)
>> dev_info(&mdev->dev, "MDEV: group_id = %d\n",
>> iommu_group_id(group));
>> - Allocate an aux-domain
>> iommu_domain_alloc()
>> - Attach the aux-domain to the physical device from which the mdev is
>> created.
>> iommu_aux_attach_device()
>>
>> In the whole process, an iommu group was allocated for the mdev and an
>> iommu domain was attached to the group, but the group->domain leaves
>> NULL. As the result, iommu_get_domain_for_dev() doesn't work anymore.
>>
>> The iommu_get_domain_for_dev() is a necessary interface for device
>> drivers that want to support aux-domain. For example,
>>
>> struct iommu_domain *domain;
>> struct device *dev = mdev_dev(mdev);
>> unsigned long pasid;
>>
>> domain = iommu_get_domain_for_dev(dev);
>> if (!domain)
>> return -ENODEV;
>>
>> pasid = iommu_aux_get_pasid(domain, dev->parent);
> How did we know this was an aux domain? ie. How did we know we could
> use it with iommu_aux_get_pasid()?
Yes. It's a bit confusing if iommu_get_domain_for_dev() is reused here
for aux-domain.
>
> Why did we assume the parent device is the iommu device for the aux
> domain? Should that level of detail be already known by the aux domain?
>
> Nits - The iomu device of an mdev device is found via
> mdev_get_iommu_device(dev), it should not be assumed to be the parent.
> The parent of an mdev device is found via mdev_parent_dev(mdev).
My bad. The driver should use mdev_get_iommu_device() instead.
>
> The leaps in logic here make me wonder if we should instead be exposing
> more of an aux domain API rather than blurring the differences between
> these domains. Thanks,
How about add below API?
/**
* iommu_aux_get_domain_for_dev - get aux domain for a device
* @dev: the accessory device
*
* The caller should pass a valid @dev to iommu_aux_attach_device() before
* calling this api. Return an attached aux-domain, or NULL otherwise.
*/
struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev)
{
struct iommu_domain *domain = NULL;
struct iommu_group *group;
group = iommu_group_get(dev);
if (!group)
return NULL;
if (group->aux_domain_attached)
domain = group->domain;
iommu_group_put(group);
return domain;
}
EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);
Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
2020-07-08 2:53 ` Lu Baolu
@ 2020-07-08 19:07 ` Alex Williamson
2020-07-09 0:37 ` Lu Baolu
0 siblings, 1 reply; 8+ messages in thread
From: Alex Williamson @ 2020-07-08 19:07 UTC (permalink / raw)
To: Lu Baolu
Cc: Kevin Tian, Dave Jiang, Ashok Raj, kvm, Cornelia Huck,
linux-kernel, iommu, Robin Murphy
On Wed, 8 Jul 2020 10:53:12 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:
> Hi Alex,
>
> Thanks a lot for your comments. Please check my reply inline.
>
> On 7/8/20 5:04 AM, Alex Williamson wrote:
> > On Tue, 7 Jul 2020 09:39:56 +0800
> > Lu Baolu<baolu.lu@linux.intel.com> wrote:
> >
> >> The hardware assistant vfio mediated device is a use case of iommu
> >> aux-domain. The interactions between vfio/mdev and iommu during mdev
> >> creation and passthr are:
> >>
> >> - Create a group for mdev with iommu_group_alloc();
> >> - Add the device to the group with
> >> group = iommu_group_alloc();
> >> if (IS_ERR(group))
> >> return PTR_ERR(group);
> >>
> >> ret = iommu_group_add_device(group, &mdev->dev);
> >> if (!ret)
> >> dev_info(&mdev->dev, "MDEV: group_id = %d\n",
> >> iommu_group_id(group));
> >> - Allocate an aux-domain
> >> iommu_domain_alloc()
> >> - Attach the aux-domain to the physical device from which the mdev is
> >> created.
> >> iommu_aux_attach_device()
> >>
> >> In the whole process, an iommu group was allocated for the mdev and an
> >> iommu domain was attached to the group, but the group->domain leaves
> >> NULL. As the result, iommu_get_domain_for_dev() doesn't work anymore.
> >>
> >> The iommu_get_domain_for_dev() is a necessary interface for device
> >> drivers that want to support aux-domain. For example,
> >>
> >> struct iommu_domain *domain;
> >> struct device *dev = mdev_dev(mdev);
> >> unsigned long pasid;
> >>
> >> domain = iommu_get_domain_for_dev(dev);
> >> if (!domain)
> >> return -ENODEV;
> >>
> >> pasid = iommu_aux_get_pasid(domain, dev->parent);
> > How did we know this was an aux domain? ie. How did we know we could
> > use it with iommu_aux_get_pasid()?
>
> Yes. It's a bit confusing if iommu_get_domain_for_dev() is reused here
> for aux-domain.
>
> >
> > Why did we assume the parent device is the iommu device for the aux
> > domain? Should that level of detail be already known by the aux domain?
> >
> > Nits - The iomu device of an mdev device is found via
> > mdev_get_iommu_device(dev), it should not be assumed to be the parent.
> > The parent of an mdev device is found via mdev_parent_dev(mdev).
>
> My bad. The driver should use mdev_get_iommu_device() instead.
>
> >
> > The leaps in logic here make me wonder if we should instead be exposing
> > more of an aux domain API rather than blurring the differences between
> > these domains. Thanks,
>
> How about add below API?
>
> /**
> * iommu_aux_get_domain_for_dev - get aux domain for a device
> * @dev: the accessory device
> *
> * The caller should pass a valid @dev to iommu_aux_attach_device() before
> * calling this api. Return an attached aux-domain, or NULL otherwise.
That's not necessarily the caller's responsibility, that might happen
elsewhere, this function simply returns an aux domain for the device if
it's attached to one.
> */
> struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev)
> {
> struct iommu_domain *domain = NULL;
> struct iommu_group *group;
>
> group = iommu_group_get(dev);
> if (!group)
> return NULL;
>
> if (group->aux_domain_attached)
> domain = group->domain;
>
> iommu_group_put(group);
>
> return domain;
> }
> EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);
For your example use case, this seems more clear to me. Thanks,
Alex
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
2020-07-08 19:07 ` Alex Williamson
@ 2020-07-09 0:37 ` Lu Baolu
0 siblings, 0 replies; 8+ messages in thread
From: Lu Baolu @ 2020-07-09 0:37 UTC (permalink / raw)
To: Alex Williamson
Cc: Kevin Tian, Dave Jiang, Ashok Raj, kvm, Cornelia Huck,
linux-kernel, iommu, Robin Murphy
Hi Alex,
On 7/9/20 3:07 AM, Alex Williamson wrote:
> On Wed, 8 Jul 2020 10:53:12 +0800
> Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> Hi Alex,
>>
>> Thanks a lot for your comments. Please check my reply inline.
>>
>> On 7/8/20 5:04 AM, Alex Williamson wrote:
>>> On Tue, 7 Jul 2020 09:39:56 +0800
>>> Lu Baolu<baolu.lu@linux.intel.com> wrote:
>>>
>>>> The hardware assistant vfio mediated device is a use case of iommu
>>>> aux-domain. The interactions between vfio/mdev and iommu during mdev
>>>> creation and passthr are:
>>>>
>>>> - Create a group for mdev with iommu_group_alloc();
>>>> - Add the device to the group with
>>>> group = iommu_group_alloc();
>>>> if (IS_ERR(group))
>>>> return PTR_ERR(group);
>>>>
>>>> ret = iommu_group_add_device(group, &mdev->dev);
>>>> if (!ret)
>>>> dev_info(&mdev->dev, "MDEV: group_id = %d\n",
>>>> iommu_group_id(group));
>>>> - Allocate an aux-domain
>>>> iommu_domain_alloc()
>>>> - Attach the aux-domain to the physical device from which the mdev is
>>>> created.
>>>> iommu_aux_attach_device()
>>>>
>>>> In the whole process, an iommu group was allocated for the mdev and an
>>>> iommu domain was attached to the group, but the group->domain leaves
>>>> NULL. As the result, iommu_get_domain_for_dev() doesn't work anymore.
>>>>
>>>> The iommu_get_domain_for_dev() is a necessary interface for device
>>>> drivers that want to support aux-domain. For example,
>>>>
>>>> struct iommu_domain *domain;
>>>> struct device *dev = mdev_dev(mdev);
>>>> unsigned long pasid;
>>>>
>>>> domain = iommu_get_domain_for_dev(dev);
>>>> if (!domain)
>>>> return -ENODEV;
>>>>
>>>> pasid = iommu_aux_get_pasid(domain, dev->parent);
>>> How did we know this was an aux domain? ie. How did we know we could
>>> use it with iommu_aux_get_pasid()?
>>
>> Yes. It's a bit confusing if iommu_get_domain_for_dev() is reused here
>> for aux-domain.
>>
>>>
>>> Why did we assume the parent device is the iommu device for the aux
>>> domain? Should that level of detail be already known by the aux domain?
>>>
>>> Nits - The iomu device of an mdev device is found via
>>> mdev_get_iommu_device(dev), it should not be assumed to be the parent.
>>> The parent of an mdev device is found via mdev_parent_dev(mdev).
>>
>> My bad. The driver should use mdev_get_iommu_device() instead.
>>
>>>
>>> The leaps in logic here make me wonder if we should instead be exposing
>>> more of an aux domain API rather than blurring the differences between
>>> these domains. Thanks,
>>
>> How about add below API?
>>
>> /**
>> * iommu_aux_get_domain_for_dev - get aux domain for a device
>> * @dev: the accessory device
>> *
>> * The caller should pass a valid @dev to iommu_aux_attach_device() before
>> * calling this api. Return an attached aux-domain, or NULL otherwise.
>
> That's not necessarily the caller's responsibility, that might happen
> elsewhere, this function simply returns an aux domain for the device if
> it's attached to one.
Yes. Fair enough. This piece of comments will be removed.
>
>> */
>> struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev)
>> {
>> struct iommu_domain *domain = NULL;
>> struct iommu_group *group;
>>
>> group = iommu_group_get(dev);
>> if (!group)
>> return NULL;
>>
>> if (group->aux_domain_attached)
>> domain = group->domain;
>>
>> iommu_group_put(group);
>>
>> return domain;
>> }
>> EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);
>
> For your example use case, this seems more clear to me. Thanks,
>
Okay, thank you!
> Alex
Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
2020-07-07 1:39 ` [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension Lu Baolu
2020-07-07 21:04 ` Alex Williamson
@ 2020-07-09 6:52 ` Lu Baolu
1 sibling, 0 replies; 8+ messages in thread
From: Lu Baolu @ 2020-07-09 6:52 UTC (permalink / raw)
To: Joerg Roedel, Alex Williamson, Robin Murphy
Cc: Kevin Tian, Dave Jiang, Ashok Raj, kvm, Cornelia Huck,
linux-kernel, iommu
On 2020/7/7 9:39, Lu Baolu wrote:
> The hardware assistant vfio mediated device is a use case of iommu
> aux-domain. The interactions between vfio/mdev and iommu during mdev
> creation and passthr are:
>
> - Create a group for mdev with iommu_group_alloc();
> - Add the device to the group with
> group = iommu_group_alloc();
> if (IS_ERR(group))
> return PTR_ERR(group);
>
> ret = iommu_group_add_device(group, &mdev->dev);
> if (!ret)
> dev_info(&mdev->dev, "MDEV: group_id = %d\n",
> iommu_group_id(group));
> - Allocate an aux-domain
> iommu_domain_alloc()
> - Attach the aux-domain to the physical device from which the mdev is
> created.
> iommu_aux_attach_device()
>
> In the whole process, an iommu group was allocated for the mdev and an
> iommu domain was attached to the group, but the group->domain leaves
> NULL. As the result, iommu_get_domain_for_dev() doesn't work anymore.
>
> The iommu_get_domain_for_dev() is a necessary interface for device
> drivers that want to support aux-domain. For example,
>
> struct iommu_domain *domain;
> struct device *dev = mdev_dev(mdev);
> unsigned long pasid;
>
> domain = iommu_get_domain_for_dev(dev);
> if (!domain)
> return -ENODEV;
>
> pasid = iommu_aux_get_pasid(domain, dev->parent);
> if (pasid == IOASID_INVALID)
> return -EINVAL;
>
> /* Program the device context with the PASID value */
> ....
>
> This extends iommu_aux_at(de)tach_device() so that the users could pass
> in an optional device pointer (struct device for vfio/mdev for example),
> and the necessary check and data link could be done.
>
> Fixes: a3a195929d40b ("iommu: Add APIs for multiple domains per device")
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> drivers/iommu/iommu.c | 86 +++++++++++++++++++++++++++++----
> drivers/vfio/vfio_iommu_type1.c | 5 +-
> include/linux/iommu.h | 12 +++--
> 3 files changed, 87 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 1ed1e14a1f0c..435835058209 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2723,26 +2723,92 @@ EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
> * This should make us safe against a device being attached to a guest as a
> * whole while there are still pasid users on it (aux and sva).
> */
> -int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
> +int iommu_aux_attach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev)
I hit a lock issue during internal test. Will fix it in the next
version.
Best regards,
baolu
> {
> - int ret = -ENODEV;
> + struct iommu_group *group;
> + int ret;
>
> - if (domain->ops->aux_attach_dev)
> - ret = domain->ops->aux_attach_dev(domain, dev);
> + if (!domain->ops->aux_attach_dev ||
> + !iommu_dev_feature_enabled(phys_dev, IOMMU_DEV_FEAT_AUX))
> + return -ENODEV;
>
> - if (!ret)
> - trace_attach_device_to_domain(dev);
> + /* Bare use only. */
> + if (!dev) {
> + ret = domain->ops->aux_attach_dev(domain, phys_dev);
> + if (!ret)
> + trace_attach_device_to_domain(phys_dev);
> +
> + return ret;
> + }
> +
> + /*
> + * The caller has created a made-up device (for example, vfio/mdev)
> + * and allocated an iommu_group for user level direct assignment.
> + * Make sure that the group has only single device and hasn't been
> + * attached by any other domain.
> + */
> + group = iommu_group_get(dev);
> + if (!group)
> + return -ENODEV;
> +
> + /*
> + * Lock the group to make sure the device-count doesn't change while
> + * we are attaching.
> + */
> + mutex_lock(&group->mutex);
> + ret = -EINVAL;
> + if ((iommu_group_device_count(group) != 1) || group->domain)
> + goto out_unlock;
> +
> + ret = -EBUSY;
> + if (group->default_domain && group->domain != group->default_domain)
> + goto out_unlock;
> +
> + ret = domain->ops->aux_attach_dev(domain, phys_dev);
> + if (!ret) {
> + trace_attach_device_to_domain(phys_dev);
> + group->domain = domain;
> + }
> +
> +out_unlock:
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
>
> return ret;
> }
> EXPORT_SYMBOL_GPL(iommu_aux_attach_device);
>
> -void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev)
> +void iommu_aux_detach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev)
> {
> - if (domain->ops->aux_detach_dev) {
> - domain->ops->aux_detach_dev(domain, dev);
> - trace_detach_device_from_domain(dev);
> + struct iommu_group *group;
> +
> + if (WARN_ON_ONCE(!domain->ops->aux_detach_dev))
> + return;
> +
> + if (!dev) {
> + domain->ops->aux_detach_dev(domain, phys_dev);
> + trace_detach_device_from_domain(phys_dev);
> +
> + return;
> }
> +
> + group = iommu_group_get(dev);
> + if (!group)
> + return;
> +
> + mutex_lock(&group->mutex);
> + if (WARN_ON(iommu_group_device_count(group) != 1))
> + goto out_unlock;
> +
> + domain->ops->aux_detach_dev(domain, phys_dev);
> + group->domain = NULL;
> + trace_detach_device_from_domain(phys_dev);
> +
> +out_unlock:
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
> }
> EXPORT_SYMBOL_GPL(iommu_aux_detach_device);
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 5e556ac9102a..d3be45dfa58e 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1635,7 +1635,8 @@ static int vfio_mdev_attach_domain(struct device *dev, void *data)
> iommu_device = vfio_mdev_get_iommu_device(dev);
> if (iommu_device) {
> if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
> - return iommu_aux_attach_device(domain, iommu_device);
> + return iommu_aux_attach_device(domain,
> + iommu_device, dev);
> else
> return iommu_attach_device(domain, iommu_device);
> }
> @@ -1651,7 +1652,7 @@ static int vfio_mdev_detach_domain(struct device *dev, void *data)
> iommu_device = vfio_mdev_get_iommu_device(dev);
> if (iommu_device) {
> if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
> - iommu_aux_detach_device(domain, iommu_device);
> + iommu_aux_detach_device(domain, iommu_device, dev);
> else
> iommu_detach_device(domain, iommu_device);
> }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 5657d4fef9f2..7da5e67bf7dc 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -632,8 +632,10 @@ bool iommu_dev_has_feature(struct device *dev, enum iommu_dev_features f);
> int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
> int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
> bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features f);
> -int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev);
> -void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev);
> +int iommu_aux_attach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev);
> +void iommu_aux_detach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev);
> int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev);
>
> struct iommu_sva *iommu_sva_bind_device(struct device *dev,
> @@ -1007,13 +1009,15 @@ iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
> }
>
> static inline int
> -iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
> +iommu_aux_attach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev)
> {
> return -ENODEV;
> }
>
> static inline void
> -iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev)
> +iommu_aux_detach_device(struct iommu_domain *domain,
> + struct device *phys_dev, struct device *dev)
> {
> }
>
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-09 6:52 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07 1:39 [PATCH v2 0/2] iommu_aux_at(de)tach_device() enhancement Lu Baolu
2020-07-07 1:39 ` [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension Lu Baolu
2020-07-07 21:04 ` Alex Williamson
2020-07-08 2:53 ` Lu Baolu
2020-07-08 19:07 ` Alex Williamson
2020-07-09 0:37 ` Lu Baolu
2020-07-09 6:52 ` Lu Baolu
2020-07-07 1:39 ` [PATCH v2 2/2] iommu: Add aux_domain_attached flag to iommu_group Lu Baolu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).