Messages in this thread | | | Subject | Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups | From | Lu Baolu <> | Date | Fri, 7 Jan 2022 09:14:38 +0800 |
| |
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@nvidia.com> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> 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
| |