| Message ID | 20200714055703.5510-5-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu aux-domain APIs extensions | expand |
On Tue, Jul 14, 2020 at 01:57:03PM +0800, Lu Baolu wrote: > Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). > It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the > vfio_group data structure so that it could be reused in other places. This removes the last user of iommu_aux_attach_device and iommu_aux_detach_device, which can be removed now.
On Tue, 14 Jul 2020 09:25:14 +0100 Christoph Hellwig <hch@infradead.org> wrote: > On Tue, Jul 14, 2020 at 01:57:03PM +0800, Lu Baolu wrote: > > Replace iommu_aux_at(de)tach_device() with > > iommu_aux_at(de)tach_group(). It also saves the > > IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data > > structure so that it could be reused in other places. > > This removes the last user of iommu_aux_attach_device and > iommu_aux_detach_device, which can be removed now. it is still used in patch 2/4 inside iommu_aux_attach_group(), right? > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu [Jacob Pan]
Hi Christoph and Jacob, On 7/15/20 12:29 AM, Jacob Pan wrote: > On Tue, 14 Jul 2020 09:25:14 +0100 > Christoph Hellwig<hch@infradead.org> wrote: > >> On Tue, Jul 14, 2020 at 01:57:03PM +0800, Lu Baolu wrote: >>> Replace iommu_aux_at(de)tach_device() with >>> iommu_aux_at(de)tach_group(). It also saves the >>> IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data >>> structure so that it could be reused in other places. >> This removes the last user of iommu_aux_attach_device and >> iommu_aux_detach_device, which can be removed now. > it is still used in patch 2/4 inside iommu_aux_attach_group(), right? > There is a need to use this interface. For example, an aux-domain is attached to a subset of a physical device and used in the kernel. In this usage scenario, there's no need to use vfio/mdev. The device driver could just allocate an aux-domain and call iommu_aux_attach_device() to setup the iommu. Best regards, baolu
> From: Lu Baolu <baolu.lu@linux.intel.com> > Sent: Wednesday, July 15, 2020 9:00 AM > > Hi Christoph and Jacob, > > On 7/15/20 12:29 AM, Jacob Pan wrote: > > On Tue, 14 Jul 2020 09:25:14 +0100 > > Christoph Hellwig<hch@infradead.org> wrote: > > > >> On Tue, Jul 14, 2020 at 01:57:03PM +0800, Lu Baolu wrote: > >>> Replace iommu_aux_at(de)tach_device() with > >>> iommu_aux_at(de)tach_group(). It also saves the > >>> IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data > >>> structure so that it could be reused in other places. > >> This removes the last user of iommu_aux_attach_device and > >> iommu_aux_detach_device, which can be removed now. > > it is still used in patch 2/4 inside iommu_aux_attach_group(), right? > > > > There is a need to use this interface. For example, an aux-domain is > attached to a subset of a physical device and used in the kernel. In > this usage scenario, there's no need to use vfio/mdev. The device driver > could just allocate an aux-domain and call iommu_aux_attach_device() to > setup the iommu. > and here is one example usage for adding per-instance pagetables for drm/msm: https://lore.kernel.org/lkml/20200626200414.14382-5-jcrouse@codeaurora.org/ Thanks Kevin
On Tue, 14 Jul 2020 13:57:03 +0800 Lu Baolu <baolu.lu@linux.intel.com> wrote: > Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). > It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the > vfio_group data structure so that it could be reused in other places. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > drivers/vfio/vfio_iommu_type1.c | 44 ++++++--------------------------- > 1 file changed, 7 insertions(+), 37 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 5e556ac9102a..f8812e68de77 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -100,6 +100,7 @@ struct vfio_dma { > struct vfio_group { > struct iommu_group *iommu_group; > struct list_head next; > + struct device *iommu_device; > bool mdev_group; /* An mdev group */ > bool pinned_page_dirty_scope; > }; > @@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct device *dev) > return NULL; > } > > -static int vfio_mdev_attach_domain(struct device *dev, void *data) > -{ > - struct iommu_domain *domain = data; > - struct device *iommu_device; > - > - 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); > - else > - return iommu_attach_device(domain, iommu_device); > - } > - > - return -EINVAL; > -} > - > -static int vfio_mdev_detach_domain(struct device *dev, void *data) > -{ > - struct iommu_domain *domain = data; > - struct device *iommu_device; > - > - 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); > - else > - iommu_detach_device(domain, iommu_device); > - } > - > - return 0; > -} > - > static int vfio_iommu_attach_group(struct vfio_domain *domain, > struct vfio_group *group) > { > if (group->mdev_group) > - return iommu_group_for_each_dev(group->iommu_group, > - domain->domain, > - vfio_mdev_attach_domain); > + return iommu_aux_attach_group(domain->domain, > + group->iommu_group, > + group->iommu_device); No, we previously iterated all devices in the group and used the aux interface only when we have an iommu_device supporting aux. If we simply assume an mdev group only uses an aux domain we break existing users, ex. SR-IOV VF backed mdevs. Thanks, Alex > else > return iommu_attach_group(domain->domain, group->iommu_group); > } > @@ -1674,8 +1643,8 @@ static void vfio_iommu_detach_group(struct vfio_domain *domain, > struct vfio_group *group) > { > if (group->mdev_group) > - iommu_group_for_each_dev(group->iommu_group, domain->domain, > - vfio_mdev_detach_domain); > + iommu_aux_detach_group(domain->domain, group->iommu_group, > + group->iommu_device); > else > iommu_detach_group(domain->domain, group->iommu_group); > } > @@ -2007,6 +1976,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, > return 0; > } > > + group->iommu_device = iommu_device; > bus = iommu_device->bus; > } >
Hi Alex, On 7/30/20 4:32 AM, Alex Williamson wrote: > On Tue, 14 Jul 2020 13:57:03 +0800 > Lu Baolu <baolu.lu@linux.intel.com> wrote: > >> Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). >> It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the >> vfio_group data structure so that it could be reused in other places. >> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> --- >> drivers/vfio/vfio_iommu_type1.c | 44 ++++++--------------------------- >> 1 file changed, 7 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >> index 5e556ac9102a..f8812e68de77 100644 >> --- a/drivers/vfio/vfio_iommu_type1.c >> +++ b/drivers/vfio/vfio_iommu_type1.c >> @@ -100,6 +100,7 @@ struct vfio_dma { >> struct vfio_group { >> struct iommu_group *iommu_group; >> struct list_head next; >> + struct device *iommu_device; >> bool mdev_group; /* An mdev group */ >> bool pinned_page_dirty_scope; >> }; >> @@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct device *dev) >> return NULL; >> } >> >> -static int vfio_mdev_attach_domain(struct device *dev, void *data) >> -{ >> - struct iommu_domain *domain = data; >> - struct device *iommu_device; >> - >> - 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); >> - else >> - return iommu_attach_device(domain, iommu_device); >> - } >> - >> - return -EINVAL; >> -} >> - >> -static int vfio_mdev_detach_domain(struct device *dev, void *data) >> -{ >> - struct iommu_domain *domain = data; >> - struct device *iommu_device; >> - >> - 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); >> - else >> - iommu_detach_device(domain, iommu_device); >> - } >> - >> - return 0; >> -} >> - >> static int vfio_iommu_attach_group(struct vfio_domain *domain, >> struct vfio_group *group) >> { >> if (group->mdev_group) >> - return iommu_group_for_each_dev(group->iommu_group, >> - domain->domain, >> - vfio_mdev_attach_domain); >> + return iommu_aux_attach_group(domain->domain, >> + group->iommu_group, >> + group->iommu_device); > > No, we previously iterated all devices in the group and used the aux > interface only when we have an iommu_device supporting aux. If we > simply assume an mdev group only uses an aux domain we break existing > users, ex. SR-IOV VF backed mdevs. Thanks, Oh, yes. Sorry! I didn't consider the physical device backed mdevs cases. Looked into this part of code, it seems that there's a lock issue here. The group->mutex is held in iommu_group_for_each_dev() and will be acquired again in iommu_attach_device(). How about making it like: static int vfio_iommu_attach_group(struct vfio_domain *domain, struct vfio_group *group) { if (group->mdev_group) { struct device *iommu_device = group->iommu_device; if (WARN_ON(!iommu_device)) return -EINVAL; if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX)) return iommu_aux_attach_device(domain->domain, iommu_device); else return iommu_attach_device(domain->domain, iommu_device); } else { return iommu_attach_group(domain->domain, group->iommu_group); } } The caller (vfio_iommu_type1_attach_group) has guaranteed that all mdevs in an iommu group should be derived from a same physical device. Any thoughts? > > Alex Best regards, baolu > > >> else >> return iommu_attach_group(domain->domain, group->iommu_group); >> } >> @@ -1674,8 +1643,8 @@ static void vfio_iommu_detach_group(struct vfio_domain *domain, >> struct vfio_group *group) >> { >> if (group->mdev_group) >> - iommu_group_for_each_dev(group->iommu_group, domain->domain, >> - vfio_mdev_detach_domain); >> + iommu_aux_detach_group(domain->domain, group->iommu_group, >> + group->iommu_device); >> else >> iommu_detach_group(domain->domain, group->iommu_group); >> } >> @@ -2007,6 +1976,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, >> return 0; >> } >> >> + group->iommu_device = iommu_device; >> bus = iommu_device->bus; >> } >> >
> From: Lu Baolu <baolu.lu@linux.intel.com> > Sent: Tuesday, July 14, 2020 1:57 PM > > Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). > It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group > data structure so that it could be reused in other places. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > drivers/vfio/vfio_iommu_type1.c | 44 ++++++--------------------------- > 1 file changed, 7 insertions(+), 37 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 5e556ac9102a..f8812e68de77 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -100,6 +100,7 @@ struct vfio_dma { > struct vfio_group { > struct iommu_group *iommu_group; > struct list_head next; > + struct device *iommu_device; I know mdev group has only one device, so such a group has a single iommu_device. But I guess may be helpful to add a comment here or in commit message. Otherwise, it looks weird that a group structure contains a single iommu_device field instead of a list of iommu_device. Regards, Yi Liu > bool mdev_group; /* An mdev group */ > bool pinned_page_dirty_scope; > }; > @@ -1627,45 +1628,13 @@ static struct device > *vfio_mdev_get_iommu_device(struct device *dev) > return NULL; > } > > -static int vfio_mdev_attach_domain(struct device *dev, void *data) -{ > - struct iommu_domain *domain = data; > - struct device *iommu_device; > - > - 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); > - else > - return iommu_attach_device(domain, iommu_device); > - } > - > - return -EINVAL; > -} > - > -static int vfio_mdev_detach_domain(struct device *dev, void *data) -{ > - struct iommu_domain *domain = data; > - struct device *iommu_device; > - > - 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); > - else > - iommu_detach_device(domain, iommu_device); > - } > - > - return 0; > -} > - > static int vfio_iommu_attach_group(struct vfio_domain *domain, > struct vfio_group *group) > { > if (group->mdev_group) > - return iommu_group_for_each_dev(group->iommu_group, > - domain->domain, > - vfio_mdev_attach_domain); > + return iommu_aux_attach_group(domain->domain, > + group->iommu_group, > + group->iommu_device); > else > return iommu_attach_group(domain->domain, group- > >iommu_group); } @@ -1674,8 +1643,8 @@ static void > vfio_iommu_detach_group(struct vfio_domain *domain, > struct vfio_group *group) > { > if (group->mdev_group) > - iommu_group_for_each_dev(group->iommu_group, domain- > >domain, > - vfio_mdev_detach_domain); > + iommu_aux_detach_group(domain->domain, group->iommu_group, > + group->iommu_device); > else > iommu_detach_group(domain->domain, group->iommu_group); } > @@ -2007,6 +1976,7 @@ static int vfio_iommu_type1_attach_group(void > *iommu_data, > return 0; > } > > + group->iommu_device = iommu_device; > bus = iommu_device->bus; > } > > -- > 2.17.1
On Thu, 30 Jul 2020 10:41:32 +0800 Lu Baolu <baolu.lu@linux.intel.com> wrote: > Hi Alex, > > On 7/30/20 4:32 AM, Alex Williamson wrote: > > On Tue, 14 Jul 2020 13:57:03 +0800 > > Lu Baolu <baolu.lu@linux.intel.com> wrote: > > > >> Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). > >> It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the > >> vfio_group data structure so that it could be reused in other places. > >> > >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > >> --- > >> drivers/vfio/vfio_iommu_type1.c | 44 ++++++--------------------------- > >> 1 file changed, 7 insertions(+), 37 deletions(-) > >> > >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > >> index 5e556ac9102a..f8812e68de77 100644 > >> --- a/drivers/vfio/vfio_iommu_type1.c > >> +++ b/drivers/vfio/vfio_iommu_type1.c > >> @@ -100,6 +100,7 @@ struct vfio_dma { > >> struct vfio_group { > >> struct iommu_group *iommu_group; > >> struct list_head next; > >> + struct device *iommu_device; > >> bool mdev_group; /* An mdev group */ > >> bool pinned_page_dirty_scope; > >> }; > >> @@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct device *dev) > >> return NULL; > >> } > >> > >> -static int vfio_mdev_attach_domain(struct device *dev, void *data) > >> -{ > >> - struct iommu_domain *domain = data; > >> - struct device *iommu_device; > >> - > >> - 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); > >> - else > >> - return iommu_attach_device(domain, iommu_device); > >> - } > >> - > >> - return -EINVAL; > >> -} > >> - > >> -static int vfio_mdev_detach_domain(struct device *dev, void *data) > >> -{ > >> - struct iommu_domain *domain = data; > >> - struct device *iommu_device; > >> - > >> - 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); > >> - else > >> - iommu_detach_device(domain, iommu_device); > >> - } > >> - > >> - return 0; > >> -} > >> - > >> static int vfio_iommu_attach_group(struct vfio_domain *domain, > >> struct vfio_group *group) > >> { > >> if (group->mdev_group) > >> - return iommu_group_for_each_dev(group->iommu_group, > >> - domain->domain, > >> - vfio_mdev_attach_domain); > >> + return iommu_aux_attach_group(domain->domain, > >> + group->iommu_group, > >> + group->iommu_device); > > > > No, we previously iterated all devices in the group and used the aux > > interface only when we have an iommu_device supporting aux. If we > > simply assume an mdev group only uses an aux domain we break existing > > users, ex. SR-IOV VF backed mdevs. Thanks, > > Oh, yes. Sorry! I didn't consider the physical device backed mdevs > cases. > > Looked into this part of code, it seems that there's a lock issue here. > The group->mutex is held in iommu_group_for_each_dev() and will be > acquired again in iommu_attach_device(). These are two different groups. We walk the devices in the mdev's group with iommu_group_for_each_dev(), holding the mdev's group lock, but we call iommu_attach_device() with iommu_device, which results in acquiring the lock for the iommu_device's group. > How about making it like: > > static int vfio_iommu_attach_group(struct vfio_domain *domain, > struct vfio_group *group) > { > if (group->mdev_group) { > struct device *iommu_device = group->iommu_device; > > if (WARN_ON(!iommu_device)) > return -EINVAL; > > if (iommu_dev_feature_enabled(iommu_device, > IOMMU_DEV_FEAT_AUX)) > return iommu_aux_attach_device(domain->domain, > iommu_device); > else > return iommu_attach_device(domain->domain, > iommu_device); > } else { > return iommu_attach_group(domain->domain, > group->iommu_group); > } > } > > The caller (vfio_iommu_type1_attach_group) has guaranteed that all mdevs > in an iommu group should be derived from a same physical device. Have we? iommu_attach_device() will fail if the group is not singleton, but that's just encouraging us to use the _attach_group() interface where the _attach_device() interface is relegated to special cases. Ideally we'd get out of those special cases and create an _attach_group() for aux that doesn't further promote these notions. > Any thoughts? See my reply to Kevin, I'm thinking we need to provide a callback that can enlighten the IOMMU layer to be able to do _attach_group() with aux or separate IOMMU backed devices. Thanks, Alex
Hi Alex, On 7/31/20 5:17 AM, Alex Williamson wrote: > On Thu, 30 Jul 2020 10:41:32 +0800 > Lu Baolu <baolu.lu@linux.intel.com> wrote: > >> Hi Alex, >> >> On 7/30/20 4:32 AM, Alex Williamson wrote: >>> On Tue, 14 Jul 2020 13:57:03 +0800 >>> Lu Baolu <baolu.lu@linux.intel.com> wrote: >>> >>>> Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). >>>> It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the >>>> vfio_group data structure so that it could be reused in other places. >>>> >>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >>>> --- >>>> drivers/vfio/vfio_iommu_type1.c | 44 ++++++--------------------------- >>>> 1 file changed, 7 insertions(+), 37 deletions(-) >>>> >>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >>>> index 5e556ac9102a..f8812e68de77 100644 >>>> --- a/drivers/vfio/vfio_iommu_type1.c >>>> +++ b/drivers/vfio/vfio_iommu_type1.c >>>> @@ -100,6 +100,7 @@ struct vfio_dma { >>>> struct vfio_group { >>>> struct iommu_group *iommu_group; >>>> struct list_head next; >>>> + struct device *iommu_device; >>>> bool mdev_group; /* An mdev group */ >>>> bool pinned_page_dirty_scope; >>>> }; >>>> @@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct device *dev) >>>> return NULL; >>>> } >>>> >>>> -static int vfio_mdev_attach_domain(struct device *dev, void *data) >>>> -{ >>>> - struct iommu_domain *domain = data; >>>> - struct device *iommu_device; >>>> - >>>> - 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); >>>> - else >>>> - return iommu_attach_device(domain, iommu_device); >>>> - } >>>> - >>>> - return -EINVAL; >>>> -} >>>> - >>>> -static int vfio_mdev_detach_domain(struct device *dev, void *data) >>>> -{ >>>> - struct iommu_domain *domain = data; >>>> - struct device *iommu_device; >>>> - >>>> - 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); >>>> - else >>>> - iommu_detach_device(domain, iommu_device); >>>> - } >>>> - >>>> - return 0; >>>> -} >>>> - >>>> static int vfio_iommu_attach_group(struct vfio_domain *domain, >>>> struct vfio_group *group) >>>> { >>>> if (group->mdev_group) >>>> - return iommu_group_for_each_dev(group->iommu_group, >>>> - domain->domain, >>>> - vfio_mdev_attach_domain); >>>> + return iommu_aux_attach_group(domain->domain, >>>> + group->iommu_group, >>>> + group->iommu_device); >>> >>> No, we previously iterated all devices in the group and used the aux >>> interface only when we have an iommu_device supporting aux. If we >>> simply assume an mdev group only uses an aux domain we break existing >>> users, ex. SR-IOV VF backed mdevs. Thanks, >> >> Oh, yes. Sorry! I didn't consider the physical device backed mdevs >> cases. >> >> Looked into this part of code, it seems that there's a lock issue here. >> The group->mutex is held in iommu_group_for_each_dev() and will be >> acquired again in iommu_attach_device(). > > These are two different groups. We walk the devices in the mdev's > group with iommu_group_for_each_dev(), holding the mdev's group lock, > but we call iommu_attach_device() with iommu_device, which results in > acquiring the lock for the iommu_device's group. You are right. Sorry for the noise. Please ignore it. > >> How about making it like: >> >> static int vfio_iommu_attach_group(struct vfio_domain *domain, >> struct vfio_group *group) >> { >> if (group->mdev_group) { >> struct device *iommu_device = group->iommu_device; >> >> if (WARN_ON(!iommu_device)) >> return -EINVAL; >> >> if (iommu_dev_feature_enabled(iommu_device, >> IOMMU_DEV_FEAT_AUX)) >> return iommu_aux_attach_device(domain->domain, >> iommu_device); >> else >> return iommu_attach_device(domain->domain, >> iommu_device); >> } else { >> return iommu_attach_group(domain->domain, >> group->iommu_group); >> } >> } >> >> The caller (vfio_iommu_type1_attach_group) has guaranteed that all mdevs >> in an iommu group should be derived from a same physical device. > > Have we? We have done this with below. static int vfio_mdev_iommu_device(struct device *dev, void *data) { struct device **old = data, *new; new = vfio_mdev_get_iommu_device(dev); if (!new || (*old && *old != new)) return -EINVAL; *old = new; return 0; } But I agree that as a generic iommu aux-domain api, we shouldn't put this limited assumption in it. > iommu_attach_device() will fail if the group is not > singleton, but that's just encouraging us to use the _attach_group() > interface where the _attach_device() interface is relegated to special > cases. Ideally we'd get out of those special cases and create an > _attach_group() for aux that doesn't further promote these notions. Yes. Fair enough. > >> Any thoughts? > > See my reply to Kevin, I'm thinking we need to provide a callback that > can enlighten the IOMMU layer to be able to do _attach_group() with > aux or separate IOMMU backed devices. Thanks for the guide. I will check your reply. Best regards, baolu
Hi Yi, On 7/30/20 5:36 PM, Liu, Yi L wrote: >> From: Lu Baolu<baolu.lu@linux.intel.com> >> Sent: Tuesday, July 14, 2020 1:57 PM >> >> Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). >> It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group >> data structure so that it could be reused in other places. >> >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> >> --- >> drivers/vfio/vfio_iommu_type1.c | 44 ++++++--------------------------- >> 1 file changed, 7 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >> index 5e556ac9102a..f8812e68de77 100644 >> --- a/drivers/vfio/vfio_iommu_type1.c >> +++ b/drivers/vfio/vfio_iommu_type1.c >> @@ -100,6 +100,7 @@ struct vfio_dma { >> struct vfio_group { >> struct iommu_group *iommu_group; >> struct list_head next; >> + struct device *iommu_device; > I know mdev group has only one device, so such a group has a single > iommu_device. But I guess may be helpful to add a comment here or in > commit message. Otherwise, it looks weird that a group structure > contains a single iommu_device field instead of a list of iommu_device. > Right! I will add some comments if this is still needed in the next version. Best regards, baolu
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 5e556ac9102a..f8812e68de77 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -100,6 +100,7 @@ struct vfio_dma { struct vfio_group { struct iommu_group *iommu_group; struct list_head next; + struct device *iommu_device; bool mdev_group; /* An mdev group */ bool pinned_page_dirty_scope; }; @@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct device *dev) return NULL; } -static int vfio_mdev_attach_domain(struct device *dev, void *data) -{ - struct iommu_domain *domain = data; - struct device *iommu_device; - - 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); - else - return iommu_attach_device(domain, iommu_device); - } - - return -EINVAL; -} - -static int vfio_mdev_detach_domain(struct device *dev, void *data) -{ - struct iommu_domain *domain = data; - struct device *iommu_device; - - 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); - else - iommu_detach_device(domain, iommu_device); - } - - return 0; -} - static int vfio_iommu_attach_group(struct vfio_domain *domain, struct vfio_group *group) { if (group->mdev_group) - return iommu_group_for_each_dev(group->iommu_group, - domain->domain, - vfio_mdev_attach_domain); + return iommu_aux_attach_group(domain->domain, + group->iommu_group, + group->iommu_device); else return iommu_attach_group(domain->domain, group->iommu_group); } @@ -1674,8 +1643,8 @@ static void vfio_iommu_detach_group(struct vfio_domain *domain, struct vfio_group *group) { if (group->mdev_group) - iommu_group_for_each_dev(group->iommu_group, domain->domain, - vfio_mdev_detach_domain); + iommu_aux_detach_group(domain->domain, group->iommu_group, + group->iommu_device); else iommu_detach_group(domain->domain, group->iommu_group); } @@ -2007,6 +1976,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, return 0; } + group->iommu_device = iommu_device; bus = iommu_device->bus; }
Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group(). It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group data structure so that it could be reused in other places. Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- drivers/vfio/vfio_iommu_type1.c | 44 ++++++--------------------------- 1 file changed, 7 insertions(+), 37 deletions(-)