lkml.org 
[lkml]   [2021]   [Dec]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 07/13] iommu: Add iommu_at[de]tach_device_shared() for multi-device groups
Hi Jason,

On 2021/12/24 10:50, Jason Gunthorpe wrote:
> On Fri, Dec 24, 2021 at 09:30:17AM +0800, Lu Baolu wrote:
>> Hi Jason,
>>
>> On 12/23/21 10:03 PM, Jason Gunthorpe wrote:
>>>>> I think it would be clear why iommu_group_set_dma_owner(), which
>>>>> actually does detatch, is not the same thing as iommu_attach_device().
>>>> iommu_device_set_dma_owner() will eventually call
>>>> iommu_group_set_dma_owner(). I didn't get why
>>>> iommu_group_set_dma_owner() is special and need to keep.
>>> Not quite, they would not call each other, they have different
>>> implementations:
>>>
>>> int iommu_device_use_dma_api(struct device *device)
>>> {
>>> struct iommu_group *group = device->iommu_group;
>>>
>>> if (!group)
>>> return 0;
>>>
>>> mutex_lock(&group->mutex);
>>> if (group->owner_cnt != 0 ||
>>> group->domain != group->default_domain) {
>>> mutex_unlock(&group->mutex);
>>> return -EBUSY;
>>> }
>>> group->owner_cnt = 1;
>>> group->owner = NULL;
>>> mutex_unlock(&group->mutex);
>>> return 0;
>>> }
>> It seems that this function doesn't work for multi-device groups. When
>> the user unbinds all native drivers from devices in the group and start
>> to bind them with vfio-pci and assign them to user, how could iommu know
>> whether the group is viable for user?
> It is just a mistake, I made this very fast. It should work as your
> patch had it with a ++. More like this:
>
> int iommu_device_use_dma_api(struct device *device)
> {
> struct iommu_group *group = device->iommu_group;
>
> if (!group)
> return 0;
>
> mutex_lock(&group->mutex);
> if (group->owner_cnt != 0) {
> if (group->domain != group->default_domain ||
> group->owner != NULL) {
> mutex_unlock(&group->mutex);
> return -EBUSY;
> }
> }
> group->owner_cnt++;
> mutex_unlock(&group->mutex);
> return 0;
> }
>
>>> See, we get rid of the enum as a multiplexor parameter, each API does
>>> only wnat it needs, they don't call each other.
>> I like the idea of removing enum parameter and make the API name
>> specific. But I didn't get why they can't call each other even the
>> data in group is the same.
> Well, I think when you type them out you'll find they don't work the
> same. Ie the iommu_group_set_dma_owner() does __iommu_detach_group()
> which iommu_device_use_dma_api() definately doesn't want to
> do. iommu_device_use_dma_api() checks the domain while
> iommu_group_set_dma_owner() must not.
>
> This is basically the issue, all the places touching ownercount are
> superficially the same but each use different predicates. Given the
> predicate is more than half the code I wouldn't try to share the rest
> of it. But maybe when it is all typed in something will become
> obvious?
>

Get you and agree with you. For the remaining comments, let me wait and
listen what Robin will comment.

Best regards,
baolu

\
 
 \ /
  Last update: 2021-12-24 07:45    [W:0.149 / U:0.480 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site