Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Lu Baolu]

Hi Jason,

On 1/7/22 1:22 AM, Jason Gunthorpe wrote:
> On Thu, Jan 06, 2022 at 10:20:48AM +0800, Lu Baolu wrote:
> > The iommu_attach/detach_device() interfaces were exposed for the device
> > drivers to attach/detach their own domains. The commit <426a273834eae>
> > ("iommu: Limit iommu_attach/detach_device to device with their own group")
> > restricted them to singleton groups to avoid different device in a group
> > attaching different domain.
> >
> > As we've introduced device DMA ownership into the iommu core. We can now
> > extend these interfaces for muliple-device groups, and "all devices are in
> > the same address space" is still guaranteed.
> >
> > For multiple devices belonging to a same group, iommu_device_use_dma_api()
> > and iommu_attach_device() are exclusive. Therefore, when drivers decide to
> > use iommu_attach_domain(), they cannot call iommu_device_use_dma_api() at
> > the same time.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >   drivers/iommu/iommu.c | 79 +++++++++++++++++++++++++++++++++----------
> >   1 file changed, 62 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index ab8ab95969f5..2c9efd85e447 100644
> > +++ b/drivers/iommu/iommu.c
> > @@ -47,6 +47,7 @@ struct iommu_group {
> >   	struct iommu_domain *domain;
> >   	struct list_head entry;
> >   	unsigned int owner_cnt;
> > +	unsigned int attach_cnt;
>
> Why did we suddenly need another counter? None of the prior versions
> needed this. I suppose this is being used a some flag to indicate if
> owner_cnt == 1 or owner_cnt == 0 should restore the default domain?

Yes, exactly.

> Would rather a flag 'auto_no_kernel_dma_api_compat' or something

Adding a flag also works.

> > +/**
> > + * iommu_attach_device() - attach external or UNMANAGED domain to device
> > + * @domain: the domain about to attach
> > + * @dev: the device about to be attached
> > + *
> > + * For devices belonging to the same group, iommu_device_use_dma_api() and
> > + * iommu_attach_device() are exclusive. Therefore, when drivers decide to
> > + * use iommu_attach_domain(), they cannot call iommu_device_use_dma_api()
> > + * at the same time.
> > + */
> >   int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
> >   {
> >   	struct iommu_group *group;
> > +	int ret = 0;
> > +
> > +	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
> > +		return -EINVAL;
> >   
> >   	group = iommu_group_get(dev);
> >   	if (!group)
> >   		return -ENODEV;
> >   
> > +	if (group->owner_cnt) {
> > +		/*
> > +		 * Group has been used for kernel-api dma or claimed explicitly
> > +		 * for exclusive occupation. For backward compatibility, device
> > +		 * in a singleton group is allowed to ignore setting the
> > +		 * drv.no_kernel_api_dma field.
>
> BTW why is this call 'no kernel api dma' ? That reads backwards 'no
> kernel dma api' right?

Yes. Need to rephrase this wording.

> Aother appeal of putting no_kernel_api_dma in the struct device_driver
> is that this could could simply do 'dev->driver->no_kernel_api_dma' to
> figure out how it is being called and avoid this messy implicitness.

Yes.

> Once we know our calling context we can always automatic switch from
> DMA API mode to another domain without any trouble or special
> counters:
>
> if (!dev->driver->no_kernel_api_dma) {
>      if (group->owner_cnt > 1 || group->owner)
>          return -EBUSY;
>      return __iommu_attach_group(domain, group);
> }

Is there any lock issue when referencing dev->driver here? I guess this
requires iommu_attach_device() only being called during the driver life
(a.k.a. between driver .probe and .release).

> if (!group->owner_cnt) {
>      ret = __iommu_attach_group(domain, group);
>      if (ret)
>          return ret;
> } else if (group->owner || group->domain != domain)
>      return -EBUSY;
> group->owner_cnt++;
>
> Right?

Yes. It's more straightforward if there's no issue around dev->driver
referencing.

> > +	if (!group->attach_cnt) {
> > +		ret = __iommu_attach_group(domain, group);
>
> How come we don't have to detatch the default domain here? Doesn't
> that mean that the iommu_replace_group could also just call attach
> directly without going through detatch?

__iommu_attach_group() allows replacing the default domain with a
private domain. Corresponding __iommu_detach_group() automatically
replaces private domain with the default domain.

The auto-switch logic should not apply to iommu_group_replace_domain()
which is designed for components with iommu_set_dma_owner() called.

> Jason

Best regards,
baolu