iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
From: Robin Murphy <robin.murphy@arm.com>
To: Joerg Roedel <jroedel@suse.de>
Cc: iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
	Tom Murphy <murphyt7@tcd.ie>
Subject: Re: [PATCH] iommu: Implement deferred domain attachment
Date: Fri, 15 May 2020 20:23:13 +0100	[thread overview]
Message-ID: <f5c6ec5b-06c6-42e6-b74d-71cf29b44b8d@arm.com> (raw)
In-Reply-To: <20200515182600.GJ8135@suse.de>

On 2020-05-15 19:26, Joerg Roedel wrote:
> On Fri, May 15, 2020 at 05:28:53PM +0100, Robin Murphy wrote:
>> On 2020-05-15 17:14, Joerg Roedel wrote:
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index ba128d1cdaee..403fda04ea98 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -362,8 +362,8 @@ static int iommu_dma_deferred_attach(struct device *dev,
>>>    		return 0;
>>>    	if (unlikely(ops->is_attach_deferred &&
>>> -			ops->is_attach_deferred(domain, dev)))
>>> -		return iommu_attach_device(domain, dev);
>>> +		     ops->is_attach_deferred(domain, dev)))
>>> +		return iommu_attach_device_no_defer(domain, dev);
>>
>> Wouldn't it be simpler to just invoke ops->attach_dev directly and avoid
>> having to formalise a public interface that nobody else should ever use
>> anyway?
> 
> That would omit the ops->attach_dev != NULL check and the trace-point on
> device attach. Besides that, it would be a layering violation. But the
> function is of course entirely internal to the iommu subsytem and is a
> good canditate to be moved to a header file in drivers/iommu.

Sure, checking the pointer before calling was implied, but the 
tracepoint is a good argument, I'd forgotten about that :)

>> @@ -746,8 +747,11 @@ int iommu_group_add_device(struct iommu_group *group,
>> struct device *dev)
>>
>>          mutex_lock(&group->mutex);
>>          list_add_tail(&device->list, &group->devices);
>> -       if (group->domain)
>> -               ret = __iommu_attach_device(group->domain, dev);
>> +       domain = group->domain;
>> +       if (domain && (!domain->ops->is_attach_deferred ||
>> +                      !domain->ops->is_attach_deferred(domain, dev)))
>> +               ret = __iommu_attach_device(domain, dev);
>> +       }
>>          mutex_unlock(&group->mutex);
>>          if (ret)
>>                  goto err_put_group;
> 
> No, doing this in iommu_group_add_device() doesn't solve the problem.
> The attach must not happen before a device driver took control of the
> device and silenced any DMA initiated by the old kernel. At probe time
> this isn't guaranteed.

But that's not what this is; this is (supposed to be) the exact same 
"don't actually perform the attach yet" logic as before, just 
restricting it to default domains in the one place that it actually 
needs to be, so as not to fundamentally bugger up iommu_attach_device() 
in a way that prevents it from working as expected at the correct point 
later.

Thinking a bit more, consider if the driver resets the device then 
attaches it straight to its own unmanaged domain rather than calling any 
DMA ops (e.g. VFIO?) - it looks like that would also be totally broken 
right now, and no amount of bodges in iommu-dma is going to help there.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply	other threads:[~2020-05-15 19:23 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-15  9:45 [PATCH] iommu: Implement deferred domain attachment Joerg Roedel
2020-05-15 13:51 ` Lu Baolu
2020-05-15 14:20   ` Joerg Roedel
2020-05-15 15:42 ` Robin Murphy
2020-05-15 16:14   ` Joerg Roedel
2020-05-15 16:28     ` Robin Murphy
2020-05-15 18:26       ` Joerg Roedel
2020-05-15 19:23         ` Robin Murphy [this message]
2020-05-18 13:26           ` Joerg Roedel
2020-05-18 22:15             ` Jerry Snitselaar
2020-05-19  7:09             ` Jerry Snitselaar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f5c6ec5b-06c6-42e6-b74d-71cf29b44b8d@arm.com \
    --to=robin.murphy@arm.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jroedel@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=murphyt7@tcd.ie \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).