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?
Would rather a flag 'auto_no_kernel_dma_api_compat' or something
> +/**
> + * 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?
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.
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);
}
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?
> + 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?
Jason
