| Message ID | 20200714055703.5510-4-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu aux-domain APIs extensions | expand |
On Tue, 14 Jul 2020 13:57:02 +0800 Lu Baolu <baolu.lu@linux.intel.com> wrote: > The device driver needs an API to get its aux-domain. A typical usage > scenario is: > > unsigned long pasid; > struct iommu_domain *domain; > struct device *dev = mdev_dev(mdev); > struct device *iommu_device = vfio_mdev_get_iommu_device(dev); > > domain = iommu_aux_get_domain_for_dev(dev); > if (!domain) > return -ENODEV; > > pasid = iommu_aux_get_pasid(domain, iommu_device); > if (pasid <= 0) > return -EINVAL; > > /* Program the device context */ > .... > > This adds an API for such use case. > > Suggested-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > drivers/iommu/iommu.c | 18 ++++++++++++++++++ > include/linux/iommu.h | 7 +++++++ > 2 files changed, 25 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index cad5a19ebf22..434bf42b6b9b 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain, > } > EXPORT_SYMBOL_GPL(iommu_aux_detach_group); > > +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; Why wouldn't the aux domain flag be on the domain itself rather than the group? Then if we wanted sanity checking in patch 1/ we'd only need to test the flag on the object we're provided. If we had such a flag, we could create an iommu_domain_is_aux() function and then simply use iommu_get_domain_for_dev() and test that it's an aux domain in the example use case. It seems like that would resolve the jump from a domain to an aux-domain just as well as adding this separate iommu_aux_get_domain_for_dev() interface. The is_aux test might also be useful in other cases too. Thanks, Alex > + > + iommu_group_put(group); > + > + return domain; > +} > +EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev); > + > /** > * iommu_sva_bind_device() - Bind a process address space to a device > * @dev: the device > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 9506551139ab..cda6cef7579e 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -639,6 +639,7 @@ int iommu_aux_attach_group(struct iommu_domain *domain, > struct iommu_group *group, struct device *dev); > void iommu_aux_detach_group(struct iommu_domain *domain, > struct iommu_group *group, struct device *dev); > +struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev); > > struct iommu_sva *iommu_sva_bind_device(struct device *dev, > struct mm_struct *mm, > @@ -1040,6 +1041,12 @@ iommu_aux_detach_group(struct iommu_domain *domain, > { > } > > +static inline struct iommu_domain * > +iommu_aux_get_domain_for_dev(struct device *dev) > +{ > + return NULL; > +} > + > static inline struct iommu_sva * > iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata) > {
> From: Alex Williamson <alex.williamson@redhat.com> > Sent: Thursday, July 30, 2020 4:25 AM > > On Tue, 14 Jul 2020 13:57:02 +0800 > Lu Baolu <baolu.lu@linux.intel.com> wrote: > > > The device driver needs an API to get its aux-domain. A typical usage > > scenario is: > > > > unsigned long pasid; > > struct iommu_domain *domain; > > struct device *dev = mdev_dev(mdev); > > struct device *iommu_device = vfio_mdev_get_iommu_device(dev); > > > > domain = iommu_aux_get_domain_for_dev(dev); > > if (!domain) > > return -ENODEV; > > > > pasid = iommu_aux_get_pasid(domain, iommu_device); > > if (pasid <= 0) > > return -EINVAL; > > > > /* Program the device context */ > > .... > > > > This adds an API for such use case. > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com> > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > > --- > > drivers/iommu/iommu.c | 18 ++++++++++++++++++ > > include/linux/iommu.h | 7 +++++++ > > 2 files changed, 25 insertions(+) > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > index cad5a19ebf22..434bf42b6b9b 100644 > > --- a/drivers/iommu/iommu.c > > +++ b/drivers/iommu/iommu.c > > @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct > iommu_domain *domain, > > } > > EXPORT_SYMBOL_GPL(iommu_aux_detach_group); > > > > +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; > > Why wouldn't the aux domain flag be on the domain itself rather than > the group? Then if we wanted sanity checking in patch 1/ we'd only > need to test the flag on the object we're provided. > > If we had such a flag, we could create an iommu_domain_is_aux() > function and then simply use iommu_get_domain_for_dev() and test that > it's an aux domain in the example use case. It seems like that would IOMMU layer manages domains per parent device. Here given a dev (of mdev), we need a way to find its associated domain under its parent device. And we cannot simply use iommu_get_domain_for_dev on the parent device of the mdev, as it will give us the primary domain of parent device. Thanks Kevin > resolve the jump from a domain to an aux-domain just as well as adding > this separate iommu_aux_get_domain_for_dev() interface. The is_aux > test might also be useful in other cases too. Thanks, > > Alex > > > + > > + iommu_group_put(group); > > + > > + return domain; > > +} > > +EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev); > > + > > /** > > * iommu_sva_bind_device() - Bind a process address space to a device > > * @dev: the device > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > index 9506551139ab..cda6cef7579e 100644 > > --- a/include/linux/iommu.h > > +++ b/include/linux/iommu.h > > @@ -639,6 +639,7 @@ int iommu_aux_attach_group(struct > iommu_domain *domain, > > struct iommu_group *group, struct device *dev); > > void iommu_aux_detach_group(struct iommu_domain *domain, > > struct iommu_group *group, struct device *dev); > > +struct iommu_domain *iommu_aux_get_domain_for_dev(struct device > *dev); > > > > struct iommu_sva *iommu_sva_bind_device(struct device *dev, > > struct mm_struct *mm, > > @@ -1040,6 +1041,12 @@ iommu_aux_detach_group(struct > iommu_domain *domain, > > { > > } > > > > +static inline struct iommu_domain * > > +iommu_aux_get_domain_for_dev(struct device *dev) > > +{ > > + return NULL; > > +} > > + > > static inline struct iommu_sva * > > iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void > *drvdata) > > {
On Wed, 29 Jul 2020 23:49:20 +0000 "Tian, Kevin" <kevin.tian@intel.com> wrote: > > From: Alex Williamson <alex.williamson@redhat.com> > > Sent: Thursday, July 30, 2020 4:25 AM > > > > On Tue, 14 Jul 2020 13:57:02 +0800 > > Lu Baolu <baolu.lu@linux.intel.com> wrote: > > > > > The device driver needs an API to get its aux-domain. A typical usage > > > scenario is: > > > > > > unsigned long pasid; > > > struct iommu_domain *domain; > > > struct device *dev = mdev_dev(mdev); > > > struct device *iommu_device = vfio_mdev_get_iommu_device(dev); > > > > > > domain = iommu_aux_get_domain_for_dev(dev); > > > if (!domain) > > > return -ENODEV; > > > > > > pasid = iommu_aux_get_pasid(domain, iommu_device); > > > if (pasid <= 0) > > > return -EINVAL; > > > > > > /* Program the device context */ > > > .... > > > > > > This adds an API for such use case. > > > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com> > > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > > > --- > > > drivers/iommu/iommu.c | 18 ++++++++++++++++++ > > > include/linux/iommu.h | 7 +++++++ > > > 2 files changed, 25 insertions(+) > > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > > index cad5a19ebf22..434bf42b6b9b 100644 > > > --- a/drivers/iommu/iommu.c > > > +++ b/drivers/iommu/iommu.c > > > @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct > > iommu_domain *domain, > > > } > > > EXPORT_SYMBOL_GPL(iommu_aux_detach_group); > > > > > > +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; > > > > Why wouldn't the aux domain flag be on the domain itself rather than > > the group? Then if we wanted sanity checking in patch 1/ we'd only > > need to test the flag on the object we're provided. > > > > If we had such a flag, we could create an iommu_domain_is_aux() > > function and then simply use iommu_get_domain_for_dev() and test that > > it's an aux domain in the example use case. It seems like that would > > IOMMU layer manages domains per parent device. Here given a Is this the IOMMU layer or the VT-d driver? I don't see any notion of managing domains relative to a parent in the IOMMU layer. Please point to something more specific if I'm wrong here. > dev (of mdev), we need a way to find its associated domain under its > parent device. And we cannot simply use iommu_get_domain_for_dev > on the parent device of the mdev, as it will give us the primary domain > of parent device. Not the parent device of the mdev, but the mdev_dev(mdev) device. Isn't that what this series is enabling, being able to return the domain from the group that contains the mdev_dev? We shouldn't need to leave breadcrumbs on the group to know about the domain, the domain itself should be the source of knowledge, or provide a mechanism/ops to learn that knowledge. Thanks, Alex > > resolve the jump from a domain to an aux-domain just as well as adding > > this separate iommu_aux_get_domain_for_dev() interface. The is_aux > > test might also be useful in other cases too. Thanks, > > > > Alex > > > > > + > > > + iommu_group_put(group); > > > + > > > + return domain; > > > +} > > > +EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev); > > > + > > > /** > > > * iommu_sva_bind_device() - Bind a process address space to a device > > > * @dev: the device > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > > index 9506551139ab..cda6cef7579e 100644 > > > --- a/include/linux/iommu.h > > > +++ b/include/linux/iommu.h > > > @@ -639,6 +639,7 @@ int iommu_aux_attach_group(struct > > iommu_domain *domain, > > > struct iommu_group *group, struct device *dev); > > > void iommu_aux_detach_group(struct iommu_domain *domain, > > > struct iommu_group *group, struct device *dev); > > > +struct iommu_domain *iommu_aux_get_domain_for_dev(struct device > > *dev); > > > > > > struct iommu_sva *iommu_sva_bind_device(struct device *dev, > > > struct mm_struct *mm, > > > @@ -1040,6 +1041,12 @@ iommu_aux_detach_group(struct > > iommu_domain *domain, > > > { > > > } > > > > > > +static inline struct iommu_domain * > > > +iommu_aux_get_domain_for_dev(struct device *dev) > > > +{ > > > + return NULL; > > > +} > > > + > > > static inline struct iommu_sva * > > > iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void > > *drvdata) > > > { >
> From: Alex Williamson <alex.williamson@redhat.com> > Sent: Friday, July 31, 2020 4:17 AM > > On Wed, 29 Jul 2020 23:49:20 +0000 > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > From: Alex Williamson <alex.williamson@redhat.com> > > > Sent: Thursday, July 30, 2020 4:25 AM > > > > > > On Tue, 14 Jul 2020 13:57:02 +0800 > > > Lu Baolu <baolu.lu@linux.intel.com> wrote: > > > > > > > The device driver needs an API to get its aux-domain. A typical usage > > > > scenario is: > > > > > > > > unsigned long pasid; > > > > struct iommu_domain *domain; > > > > struct device *dev = mdev_dev(mdev); > > > > struct device *iommu_device = vfio_mdev_get_iommu_device(dev); > > > > > > > > domain = iommu_aux_get_domain_for_dev(dev); > > > > if (!domain) > > > > return -ENODEV; > > > > > > > > pasid = iommu_aux_get_pasid(domain, iommu_device); > > > > if (pasid <= 0) > > > > return -EINVAL; > > > > > > > > /* Program the device context */ > > > > .... > > > > > > > > This adds an API for such use case. > > > > > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com> > > > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > > > > --- > > > > drivers/iommu/iommu.c | 18 ++++++++++++++++++ > > > > include/linux/iommu.h | 7 +++++++ > > > > 2 files changed, 25 insertions(+) > > > > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > > > index cad5a19ebf22..434bf42b6b9b 100644 > > > > --- a/drivers/iommu/iommu.c > > > > +++ b/drivers/iommu/iommu.c > > > > @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct > > > iommu_domain *domain, > > > > } > > > > EXPORT_SYMBOL_GPL(iommu_aux_detach_group); > > > > > > > > +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; > > > > > > Why wouldn't the aux domain flag be on the domain itself rather than > > > the group? Then if we wanted sanity checking in patch 1/ we'd only > > > need to test the flag on the object we're provided. > > > > > > If we had such a flag, we could create an iommu_domain_is_aux() > > > function and then simply use iommu_get_domain_for_dev() and test that > > > it's an aux domain in the example use case. It seems like that would > > > > IOMMU layer manages domains per parent device. Here given a > > Is this the IOMMU layer or the VT-d driver? I don't see any notion of > managing domains relative to a parent in the IOMMU layer. Please point > to something more specific if I'm wrong here. it's maintained in VT-d driver (include/linux/intel-iommu.h) struct device_domain_info { struct list_head link; /* link to domain siblings */ struct list_head global; /* link to global list */ struct list_head table; /* link to pasid table */ struct list_head auxiliary_domains; /* auxiliary domains * attached to this device */ ... > > > dev (of mdev), we need a way to find its associated domain under its > > parent device. And we cannot simply use iommu_get_domain_for_dev > > on the parent device of the mdev, as it will give us the primary domain > > of parent device. > > Not the parent device of the mdev, but the mdev_dev(mdev) device. > Isn't that what this series is enabling, being able to return the > domain from the group that contains the mdev_dev? We shouldn't need to > leave breadcrumbs on the group to know about the domain, the domain > itself should be the source of knowledge, or provide a mechanism/ops to > learn that knowledge. Thanks, > > Alex It's the tradeoff between leaving breadcrumb in domain or in group. Today the domain has no knowledge of mdev. It just includes a list of physical devices which are attached to the domain (either due to the device is assigned in a whole or as the parent device of an assigned mdev). Then we have two choices. One is to save the mdev_dev info in device_domain_info and maintain a mapping between mdev_dev and related aux domain. The other is to record the domain info directly in group. Earlier we choose the latter one as it looks simpler. If you prefer to the former one, we can think more and have a try. Thanks Kevin
> From: Tian, Kevin > Sent: Friday, July 31, 2020 8:26 AM > > > From: Alex Williamson <alex.williamson@redhat.com> > > Sent: Friday, July 31, 2020 4:17 AM > > > > On Wed, 29 Jul 2020 23:49:20 +0000 > > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > > > From: Alex Williamson <alex.williamson@redhat.com> > > > > Sent: Thursday, July 30, 2020 4:25 AM > > > > > > > > On Tue, 14 Jul 2020 13:57:02 +0800 > > > > Lu Baolu <baolu.lu@linux.intel.com> wrote: > > > > > > > > > The device driver needs an API to get its aux-domain. A typical usage > > > > > scenario is: > > > > > > > > > > unsigned long pasid; > > > > > struct iommu_domain *domain; > > > > > struct device *dev = mdev_dev(mdev); > > > > > struct device *iommu_device = > vfio_mdev_get_iommu_device(dev); > > > > > > > > > > domain = iommu_aux_get_domain_for_dev(dev); > > > > > if (!domain) > > > > > return -ENODEV; > > > > > > > > > > pasid = iommu_aux_get_pasid(domain, iommu_device); > > > > > if (pasid <= 0) > > > > > return -EINVAL; > > > > > > > > > > /* Program the device context */ > > > > > .... > > > > > > > > > > This adds an API for such use case. > > > > > > > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com> > > > > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > > > > > --- > > > > > drivers/iommu/iommu.c | 18 ++++++++++++++++++ > > > > > include/linux/iommu.h | 7 +++++++ > > > > > 2 files changed, 25 insertions(+) > > > > > > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > > > > index cad5a19ebf22..434bf42b6b9b 100644 > > > > > --- a/drivers/iommu/iommu.c > > > > > +++ b/drivers/iommu/iommu.c > > > > > @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct > > > > iommu_domain *domain, > > > > > } > > > > > EXPORT_SYMBOL_GPL(iommu_aux_detach_group); > > > > > > > > > > +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; > > > > > > > > Why wouldn't the aux domain flag be on the domain itself rather than > > > > the group? Then if we wanted sanity checking in patch 1/ we'd only > > > > need to test the flag on the object we're provided. > > > > > > > > If we had such a flag, we could create an iommu_domain_is_aux() > > > > function and then simply use iommu_get_domain_for_dev() and test > that > > > > it's an aux domain in the example use case. It seems like that would > > > > > > IOMMU layer manages domains per parent device. Here given a > > > > Is this the IOMMU layer or the VT-d driver? I don't see any notion of > > managing domains relative to a parent in the IOMMU layer. Please point > > to something more specific if I'm wrong here. > > it's maintained in VT-d driver (include/linux/intel-iommu.h) > > struct device_domain_info { > struct list_head link; /* link to domain siblings */ > struct list_head global; /* link to global list */ > struct list_head table; /* link to pasid table */ > struct list_head auxiliary_domains; /* auxiliary domains > * attached to this device > */ > ... > > > > > > dev (of mdev), we need a way to find its associated domain under its > > > parent device. And we cannot simply use iommu_get_domain_for_dev > > > on the parent device of the mdev, as it will give us the primary domain > > > of parent device. > > > > Not the parent device of the mdev, but the mdev_dev(mdev) device. > > Isn't that what this series is enabling, being able to return the > > domain from the group that contains the mdev_dev? We shouldn't need > to > > leave breadcrumbs on the group to know about the domain, the domain > > itself should be the source of knowledge, or provide a mechanism/ops to > > learn that knowledge. Thanks, > > > > Alex > > It's the tradeoff between leaving breadcrumb in domain or in group. > Today the domain has no knowledge of mdev. It just includes a list > of physical devices which are attached to the domain (either due to > the device is assigned in a whole or as the parent device of an assigned > mdev). Then we have two choices. One is to save the mdev_dev info > in device_domain_info and maintain a mapping between mdev_dev > and related aux domain. The other is to record the domain info directly > in group. Earlier we choose the latter one as it looks simpler. If you > prefer to the former one, we can think more and have a try. > Please skip this comment. I have a wrong understanding of the problem and is discussing with Baolu. He will reply with our conclusion later. Thanks Kevin
Hi Alex, On 2020/7/30 4:25, Alex Williamson wrote: > On Tue, 14 Jul 2020 13:57:02 +0800 > Lu Baolu<baolu.lu@linux.intel.com> wrote: > >> The device driver needs an API to get its aux-domain. A typical usage >> scenario is: >> >> unsigned long pasid; >> struct iommu_domain *domain; >> struct device *dev = mdev_dev(mdev); >> struct device *iommu_device = vfio_mdev_get_iommu_device(dev); >> >> domain = iommu_aux_get_domain_for_dev(dev); >> if (!domain) >> return -ENODEV; >> >> pasid = iommu_aux_get_pasid(domain, iommu_device); >> if (pasid <= 0) >> return -EINVAL; >> >> /* Program the device context */ >> .... >> >> This adds an API for such use case. >> >> Suggested-by: Alex Williamson<alex.williamson@redhat.com> >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> >> --- >> drivers/iommu/iommu.c | 18 ++++++++++++++++++ >> include/linux/iommu.h | 7 +++++++ >> 2 files changed, 25 insertions(+) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index cad5a19ebf22..434bf42b6b9b 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain, >> } >> EXPORT_SYMBOL_GPL(iommu_aux_detach_group); >> >> +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; > Why wouldn't the aux domain flag be on the domain itself rather than > the group? Then if we wanted sanity checking in patch 1/ we'd only > need to test the flag on the object we're provided. Agreed. Given that a group may contain both non-aux and aux devices, adding such flag in iommu_group doesn't make sense. > > If we had such a flag, we could create an iommu_domain_is_aux() > function and then simply use iommu_get_domain_for_dev() and test that > it's an aux domain in the example use case. It seems like that would > resolve the jump from a domain to an aux-domain just as well as adding > this separate iommu_aux_get_domain_for_dev() interface. The is_aux > test might also be useful in other cases too. Let's rehearsal our use case. unsigned long pasid; struct iommu_domain *domain; struct device *dev = mdev_dev(mdev); struct device *iommu_device = vfio_mdev_get_iommu_device(dev); [1] domain = iommu_get_domain_for_dev(dev); if (!domain) return -ENODEV; [2] pasid = iommu_aux_get_pasid(domain, iommu_device); if (pasid <= 0) return -EINVAL; /* Program the device context */ .... The reason why I add this iommu_aux_get_domain_for_dev() is that we need to make sure the domain got at [1] is valid to be used at [2]. https://lore.kernel.org/linux-iommu/20200707150408.474d81f1@x1.home/ When calling into iommu_aux_get_pasid(), the iommu driver should make sure that @domain is a valid aux-domain for @iommu_device. Hence, for our use case, it seems that there's no need for a is_aux_domain() api. Anyway, I'm not against adding a new is_aux_domain() api if there's a need elsewhere. Best regards, baolu
On Fri, 31 Jul 2020 14:30:03 +0800 Lu Baolu <baolu.lu@linux.intel.com> wrote: > Hi Alex, > > On 2020/7/30 4:25, Alex Williamson wrote: > > On Tue, 14 Jul 2020 13:57:02 +0800 > > Lu Baolu<baolu.lu@linux.intel.com> wrote: > > > >> The device driver needs an API to get its aux-domain. A typical usage > >> scenario is: > >> > >> unsigned long pasid; > >> struct iommu_domain *domain; > >> struct device *dev = mdev_dev(mdev); > >> struct device *iommu_device = vfio_mdev_get_iommu_device(dev); > >> > >> domain = iommu_aux_get_domain_for_dev(dev); > >> if (!domain) > >> return -ENODEV; > >> > >> pasid = iommu_aux_get_pasid(domain, iommu_device); > >> if (pasid <= 0) > >> return -EINVAL; > >> > >> /* Program the device context */ > >> .... > >> > >> This adds an API for such use case. > >> > >> Suggested-by: Alex Williamson<alex.williamson@redhat.com> > >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> > >> --- > >> drivers/iommu/iommu.c | 18 ++++++++++++++++++ > >> include/linux/iommu.h | 7 +++++++ > >> 2 files changed, 25 insertions(+) > >> > >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > >> index cad5a19ebf22..434bf42b6b9b 100644 > >> --- a/drivers/iommu/iommu.c > >> +++ b/drivers/iommu/iommu.c > >> @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain, > >> } > >> EXPORT_SYMBOL_GPL(iommu_aux_detach_group); > >> > >> +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; > > Why wouldn't the aux domain flag be on the domain itself rather than > > the group? Then if we wanted sanity checking in patch 1/ we'd only > > need to test the flag on the object we're provided. > > Agreed. Given that a group may contain both non-aux and aux devices, > adding such flag in iommu_group doesn't make sense. > > > > > If we had such a flag, we could create an iommu_domain_is_aux() > > function and then simply use iommu_get_domain_for_dev() and test that > > it's an aux domain in the example use case. It seems like that would > > resolve the jump from a domain to an aux-domain just as well as adding > > this separate iommu_aux_get_domain_for_dev() interface. The is_aux > > test might also be useful in other cases too. > > Let's rehearsal our use case. > > unsigned long pasid; > struct iommu_domain *domain; > struct device *dev = mdev_dev(mdev); > struct device *iommu_device = vfio_mdev_get_iommu_device(dev); > > [1] domain = iommu_get_domain_for_dev(dev); > if (!domain) > return -ENODEV; > > [2] pasid = iommu_aux_get_pasid(domain, iommu_device); > if (pasid <= 0) > return -EINVAL; > > /* Program the device context */ > .... > > The reason why I add this iommu_aux_get_domain_for_dev() is that we need > to make sure the domain got at [1] is valid to be used at [2]. > > https://lore.kernel.org/linux-iommu/20200707150408.474d81f1@x1.home/ Yep, I thought that was a bit of a leap in logic. > When calling into iommu_aux_get_pasid(), the iommu driver should make > sure that @domain is a valid aux-domain for @iommu_device. Hence, for > our use case, it seems that there's no need for a is_aux_domain() api. > > Anyway, I'm not against adding a new is_aux_domain() api if there's a > need elsewhere. I think it could work either way, we could have an iommu_get_aux_domain_for_dev() which returns NULL if the domain is not an aux domain, or we could use iommu_get_domain_for_dev() and the caller could test the domain with iommu_is_aux_domain() if they need to confirm if it's an aux domain. The former could even be written using the latter, a wrapper of iommu_get_domain_for_dev() that checks aux property before returning. Thanks, Alex
Hi Alex, On 8/1/20 2:14 AM, Alex Williamson wrote: > On Fri, 31 Jul 2020 14:30:03 +0800 > Lu Baolu <baolu.lu@linux.intel.com> wrote: > >> Hi Alex, >> >> On 2020/7/30 4:25, Alex Williamson wrote: >>> On Tue, 14 Jul 2020 13:57:02 +0800 >>> Lu Baolu<baolu.lu@linux.intel.com> wrote: >>> >>>> The device driver needs an API to get its aux-domain. A typical usage >>>> scenario is: >>>> >>>> unsigned long pasid; >>>> struct iommu_domain *domain; >>>> struct device *dev = mdev_dev(mdev); >>>> struct device *iommu_device = vfio_mdev_get_iommu_device(dev); >>>> >>>> domain = iommu_aux_get_domain_for_dev(dev); >>>> if (!domain) >>>> return -ENODEV; >>>> >>>> pasid = iommu_aux_get_pasid(domain, iommu_device); >>>> if (pasid <= 0) >>>> return -EINVAL; >>>> >>>> /* Program the device context */ >>>> .... >>>> >>>> This adds an API for such use case. >>>> >>>> Suggested-by: Alex Williamson<alex.williamson@redhat.com> >>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> >>>> --- >>>> drivers/iommu/iommu.c | 18 ++++++++++++++++++ >>>> include/linux/iommu.h | 7 +++++++ >>>> 2 files changed, 25 insertions(+) >>>> >>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>>> index cad5a19ebf22..434bf42b6b9b 100644 >>>> --- a/drivers/iommu/iommu.c >>>> +++ b/drivers/iommu/iommu.c >>>> @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain, >>>> } >>>> EXPORT_SYMBOL_GPL(iommu_aux_detach_group); >>>> >>>> +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; >>> Why wouldn't the aux domain flag be on the domain itself rather than >>> the group? Then if we wanted sanity checking in patch 1/ we'd only >>> need to test the flag on the object we're provided. >> >> Agreed. Given that a group may contain both non-aux and aux devices, >> adding such flag in iommu_group doesn't make sense. >> >>> >>> If we had such a flag, we could create an iommu_domain_is_aux() >>> function and then simply use iommu_get_domain_for_dev() and test that >>> it's an aux domain in the example use case. It seems like that would >>> resolve the jump from a domain to an aux-domain just as well as adding >>> this separate iommu_aux_get_domain_for_dev() interface. The is_aux >>> test might also be useful in other cases too. >> >> Let's rehearsal our use case. >> >> unsigned long pasid; >> struct iommu_domain *domain; >> struct device *dev = mdev_dev(mdev); >> struct device *iommu_device = vfio_mdev_get_iommu_device(dev); >> >> [1] domain = iommu_get_domain_for_dev(dev); >> if (!domain) >> return -ENODEV; >> >> [2] pasid = iommu_aux_get_pasid(domain, iommu_device); >> if (pasid <= 0) >> return -EINVAL; >> >> /* Program the device context */ >> .... >> >> The reason why I add this iommu_aux_get_domain_for_dev() is that we need >> to make sure the domain got at [1] is valid to be used at [2]. >> >> https://lore.kernel.org/linux-iommu/20200707150408.474d81f1@x1.home/ > > Yep, I thought that was a bit of a leap in logic. > >> When calling into iommu_aux_get_pasid(), the iommu driver should make >> sure that @domain is a valid aux-domain for @iommu_device. Hence, for >> our use case, it seems that there's no need for a is_aux_domain() api. >> >> Anyway, I'm not against adding a new is_aux_domain() api if there's a >> need elsewhere. > > I think it could work either way, we could have an > iommu_get_aux_domain_for_dev() which returns NULL if the domain is not > an aux domain, or we could use iommu_get_domain_for_dev() and the > caller could test the domain with iommu_is_aux_domain() if they need to > confirm if it's an aux domain. The former could even be written using > the latter, a wrapper of iommu_get_domain_for_dev() that checks aux > property before returning. Okay. So iommu_get_domain_for_dev() and iommu_is_aux_domain() are what we wanted. The iommu_get_domain_for_dev() could be a simple wrapper of them. Thanks a lot for the guide. I will implement a new version according to the feedbacks. Best regards, baolu
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index cad5a19ebf22..434bf42b6b9b 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain, } EXPORT_SYMBOL_GPL(iommu_aux_detach_group); +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); + /** * iommu_sva_bind_device() - Bind a process address space to a device * @dev: the device diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 9506551139ab..cda6cef7579e 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -639,6 +639,7 @@ int iommu_aux_attach_group(struct iommu_domain *domain, struct iommu_group *group, struct device *dev); void iommu_aux_detach_group(struct iommu_domain *domain, struct iommu_group *group, struct device *dev); +struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev); struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, @@ -1040,6 +1041,12 @@ iommu_aux_detach_group(struct iommu_domain *domain, { } +static inline struct iommu_domain * +iommu_aux_get_domain_for_dev(struct device *dev) +{ + return NULL; +} + static inline struct iommu_sva * iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata) {
The device driver needs an API to get its aux-domain. A typical usage scenario is: unsigned long pasid; struct iommu_domain *domain; struct device *dev = mdev_dev(mdev); struct device *iommu_device = vfio_mdev_get_iommu_device(dev); domain = iommu_aux_get_domain_for_dev(dev); if (!domain) return -ENODEV; pasid = iommu_aux_get_pasid(domain, iommu_device); if (pasid <= 0) return -EINVAL; /* Program the device context */ .... This adds an API for such use case. Suggested-by: Alex Williamson <alex.williamson@redhat.com> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- drivers/iommu/iommu.c | 18 ++++++++++++++++++ include/linux/iommu.h | 7 +++++++ 2 files changed, 25 insertions(+)