lkml.org 
[lkml]   [2022]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups
From
Date
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

\
 
 \ /
  Last update: 2022-01-07 02:16    [W:0.250 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site