Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()

[Lu Baolu]

Hi Jacob,

On 7/16/20 12:01 AM, Jacob Pan wrote:
> On Wed, 15 Jul 2020 08:47:36 +0800
> Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
>
> > Hi Jacob,
> >
> > On 7/15/20 12:39 AM, Jacob Pan wrote:
> > > On Tue, 14 Jul 2020 13:57:01 +0800
> > > Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>  wrote:
> > >    
> > > > This adds two new aux-domain APIs for a use case like vfio/mdev
> > > > where sub-devices derived from an aux-domain capable device are
> > > > created and put in an iommu_group.
> > > >
> > > > /**
> > > >    * iommu_aux_attach_group - attach an aux-domain to an iommu_group
> > > > which
> > > >    *                          contains sub-devices (for example
> > > > mdevs) derived
> > > >    *                          from @dev.
> > > >    * @domain: an aux-domain;
> > > >    * @group:  an iommu_group which contains sub-devices derived from
> > > > @dev;
> > > >    * @dev:    the physical device which supports IOMMU_DEV_FEAT_AUX.
> > > >    *
> > > >    * Returns 0 on success, or an error value.
> > > >    */
> > > > int iommu_aux_attach_group(struct iommu_domain *domain,
> > > >                              struct iommu_group *group,
> > > >                              struct device *dev)
> > > >
> > > > /**
> > > >    * iommu_aux_detach_group - detach an aux-domain from an
> > > > iommu_group *
> > > >    * @domain: an aux-domain;
> > > >    * @group:  an iommu_group which contains sub-devices derived from
> > > > @dev;
> > > >    * @dev:    the physical device which supports IOMMU_DEV_FEAT_AUX.
> > > >    *
> > > >    * @domain must have been attached to @group via
> > > > iommu_aux_attach_group(). */
> > > > void iommu_aux_detach_group(struct iommu_domain *domain,
> > > >                               struct iommu_group *group,
> > > >                               struct device *dev)
> > > >
> > > > It also adds a flag in the iommu_group data structure to identify
> > > > an iommu_group with aux-domain attached from those normal ones.
> > > >
> > > > Signed-off-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > > > ---
> > > >    drivers/iommu/iommu.c | 58
> > > > +++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h |
> > > > 17 +++++++++++++ 2 files changed, 75 insertions(+)
> > > >
> > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > index e1fdd3531d65..cad5a19ebf22 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 {
> > > > @@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct iommu_domain
> > > > *domain, struct device *dev) }
> > > >    EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
> > > >    
> > > > +/**
> > > > + * iommu_aux_attach_group - attach an aux-domain to an iommu_group
> > > > which
> > > > + *                          contains sub-devices (for example
> > > > mdevs) derived
> > > > + *                          from @dev.
> > > > + * @domain: an aux-domain;
> > > > + * @group:  an iommu_group which contains sub-devices derived from
> > > > @dev;
> > > > + * @dev:    the physical device which supports IOMMU_DEV_FEAT_AUX.
> > > > + *
> > > > + * Returns 0 on success, or an error value.
> > > > + */
> > > > +int iommu_aux_attach_group(struct iommu_domain *domain,
> > > > +			   struct iommu_group *group, struct
> > > > device *dev) +{
> > > > +	int ret = -EBUSY;
> > > > +
> > > > +	mutex_lock(&group->mutex);
> > > > +	if (group->domain)
> > > > +		goto out_unlock;
> > > > +
> > > Perhaps I missed something but are we assuming only one mdev per
> > > mdev group? That seems to change the logic where vfio does:
> > > iommu_group_for_each_dev()
> > > 	iommu_aux_attach_device()
> > >    
> >
> > It has been changed in PATCH 4/4:
> >
> > static int vfio_iommu_attach_group(struct vfio_domain *domain,
> >                                      struct vfio_group *group)
> > {
> >           if (group->mdev_group)
> >                   return iommu_aux_attach_group(domain->domain,
> >                                                 group->iommu_group,
> >                                                 group->iommu_device);
> >           else
> >                   return iommu_attach_group(domain->domain,
> > group->iommu_group);
> > }
> >
> > So, for both normal domain and aux-domain, we use the same concept:
> > attach a domain to a group.
> I get that, but don't you have to attach all the devices within the

This iommu_group includes only mediated devices derived from an
IOMMU_DEV_FEAT_AUX-capable device. Different from iommu_attach_group(),
iommu_aux_attach_group() doesn't need to attach the domain to each
device in group, instead it only needs to attach the domain to the
physical device where the mdev's were created from.

> group? Here you see the group already has a domain and exit.

If the (group->domain) has been set, that means a domain has already
attached to the group, so it returns -EBUSY.

Best regards,
baolu