* [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: Cornelia Huck, Kevin Tian, Ashok Raj, Dave Jiang, Liu Yi L, iommu, linux-kernel, kvm, Lu Baolu 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 ^ 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: Cornelia Huck, Kevin Tian, Ashok Raj, Dave Jiang, Liu Yi L, iommu, linux-kernel, kvm, Lu Baolu 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 ^ 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: Joerg Roedel, Robin Murphy, Cornelia Huck, Kevin Tian, Ashok Raj, Dave Jiang, Liu Yi L, iommu, linux-kernel, kvm 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) > { > } > ^ 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: baolu.lu, Joerg Roedel, Robin Murphy, Cornelia Huck, Kevin Tian, Ashok Raj, Dave Jiang, Liu Yi L, iommu, linux-kernel, kvm 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 ^ 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: Joerg Roedel, Robin Murphy, Cornelia Huck, Kevin Tian, Ashok Raj, Dave Jiang, Liu Yi L, iommu, linux-kernel, kvm 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 ^ 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: baolu.lu, Joerg Roedel, Robin Murphy, Cornelia Huck, Kevin Tian, Ashok Raj, Dave Jiang, Liu Yi L, iommu, linux-kernel, kvm 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 ^ 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: baolu.lu, Cornelia Huck, Kevin Tian, Ashok Raj, Dave Jiang, Liu Yi L, iommu, linux-kernel, kvm 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) > { > } > > ^ permalink raw reply [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: Cornelia Huck, Kevin Tian, Ashok Raj, Dave Jiang, Liu Yi L, iommu, linux-kernel, kvm, Lu Baolu 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-09 6:53 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).