Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs

[Lu Baolu]

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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > ---
> > > >    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