Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
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, >dev);
if (!ret)
dev_info(>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
Cc: Alex Williamson
Signed-off-by: Lu Baolu
---
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(>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(>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))
+
Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
Hi Alex,
On 7/9/20 3:07 AM, Alex Williamson wrote:
On Wed, 8 Jul 2020 10:53:12 +0800
Lu Baolu 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 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, >dev);
if (!ret)
dev_info(>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
Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
On Wed, 8 Jul 2020 10:53:12 +0800
Lu Baolu 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 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, >dev);
> >> if (!ret)
> >> dev_info(>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
Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
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 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, >dev);
if (!ret)
dev_info(>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
Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
On Tue, 7 Jul 2020 09:39:56 +0800
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, >dev);
> if (!ret)
> dev_info(>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
> Cc: Alex Williamson
> Signed-off-by: Lu Baolu
> ---
> 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(>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);
> +
[PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension
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, >dev);
if (!ret)
dev_info(>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
Cc: Alex Williamson
Signed-off-by: Lu Baolu
---
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(>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(>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);
+
