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
