Re: [PATCH 2/4] iommu: Use group ownership to avoid driver attachment

[Baolu Lu]

On 2/13/23 10:19 PM, Jason Gunthorpe wrote:
> On Mon, Feb 13, 2023 at 03:49:39PM +0800, Lu Baolu wrote:
> > The iommu_group_store_type() requires the devices in the iommu group are
> > not bound to any device driver during the whole operation. The existing
> > code locks the device with device_lock(dev) and use device_is_bound() to
> > check whether any driver is bound to device.
> >
> > In fact, this can be achieved through the DMA ownership helpers. Replace
> > them with iommu_group_claim/release_dma_owner() helpers.
> >
> > Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/iommu/iommu.c | 27 +++++++++++++--------------
> >   1 file changed, 13 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 4f71dcd2621b..6547cb38480c 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -2807,12 +2807,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
> >   
> >   	mutex_lock(&group->mutex);
> >   
> > -	if (group->default_domain != group->domain) {
> > -		dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
> > -		ret = -EBUSY;
> > -		goto out;
> > -	}
> > -
> >   	/*
> >   	 * iommu group wasn't locked while acquiring device lock in
> >   	 * iommu_group_store_type(). So, make sure that the device count hasn't
> > @@ -2971,6 +2965,7 @@ static void iommu_group_unfreeze_dev_ops(struct iommu_group *group)
> >   static ssize_t iommu_group_store_type(struct iommu_group *group,
> >   				      const char *buf, size_t count)
> >   {
> > +	bool group_owner_claimed = false;
> >   	struct group_device *grp_dev;
> >   	struct device *dev;
> >   	int ret, req_type;
> > @@ -2992,6 +2987,14 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
> >   	else
> >   		return -EINVAL;
> >   
> > +	if (req_type != IOMMU_DOMAIN_DMA_FQ ||
> > +	    group->default_domain->type != IOMMU_DOMAIN_DMA) {
> > +		ret = iommu_group_claim_dma_owner(group, (void *)buf);
> > +		if (ret)
> > +			return ret;
> > +		group_owner_claimed = true;
> > +	}
>
> I don't get it, this should be done unconditionally. If we couldn't
> take ownership then we simply can't progress.

The existing code allows the user to switch the default domain from
strict to lazy invalidation mode. The default domain is not changed,
hence it should be seamless and transparent to the device driver.

> But there is more to it than that, a device that is owned should not
> be release and to achieve this the general logic around the owner
> scheme assumes that a driver is attached.

Yes. Current ownership scheme was built on this assumption.

> So if you call it from this non-driver context you have to hold the
> group_mutex as previously discussed,

Yes.

> which also means this needs to be
> an externally version of iommu_group_claim_dma_owner()

Sorry! What does "an externally version of
iommu_group_claim_dma_owner()" mean?

My understanding is that we should limit iommu_group_claim_dma_owner()
use in the driver context. For this non-driver context, we should not
use iommu_group_claim_dma_owner() directly, but hold the group->mutex
and check the group->owner_cnt directly:

        mutex_lock(&group->mutex);
        if (group->owner_cnt) {
                ret = -EPERM;
                goto unlock_out;
        }

the group->mutex should be held until everything is done.

Best regards,
baolu