Patchwork [2/5] iommu/of: Use device_iommu_mapped()

login
register
mail settings
Submitter Joerg Roedel
Date Dec. 4, 2018, 5:25 p.m.
Message ID <20181204172504.19708-3-joro@8bytes.org>
Download mbox | patch
Permalink /patch/672231/
State New
Headers show

Comments

Joerg Roedel - Dec. 4, 2018, 5:25 p.m.
From: Joerg Roedel <jroedel@suse.de>

Use Use device_iommu_mapped() to check if the device is
already mapped by an IOMMU.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/of_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Robin Murphy - Dec. 5, 2018, 5:17 p.m.
On 04/12/2018 17:25, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Use Use device_iommu_mapped() to check if the device is
> already mapped by an IOMMU.

FWIW, this check (and its ACPI equivalent in patch #3) is specifically 
asking "has .add_device() already been called?", rather than the more 
general "is this device managed by an IOMMU?" (to which the exact answer 
at this point is "yes, provided we return successfully from here").

I have no objection to the change as-is - especially if that usage is 
within the intended scope of this API - I just wanted to call it out in 
case you're also planning to introduce something else which would be 
even more appropriate for that.

Robin.

> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   drivers/iommu/of_iommu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index c5dd63072529..bfcf139503f0 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -220,7 +220,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>   	 * If we have reason to believe the IOMMU driver missed the initial
>   	 * add_device callback for dev, replay it to get things in order.
>   	 */
> -	if (ops && ops->add_device && dev->bus && !dev->iommu_group)
> +	if (ops && ops->add_device && dev->bus && !device_iommu_mapped(dev))
>   		err = ops->add_device(dev);
>   
>   	/* Ignore all other errors apart from EPROBE_DEFER */
>
Joerg Roedel - Dec. 6, 2018, 3:35 p.m.
Hi Robin,

On Wed, Dec 05, 2018 at 05:17:54PM +0000, Robin Murphy wrote:
> FWIW, this check (and its ACPI equivalent in patch #3) is specifically
> asking "has .add_device() already been called?", rather than the more
> general "is this device managed by an IOMMU?" (to which the exact answer at
> this point is "yes, provided we return successfully from here").
> 
> I have no objection to the change as-is - especially if that usage is within
> the intended scope of this API - I just wanted to call it out in case you're
> also planning to introduce something else which would be even more
> appropriate for that.

Yes, the purpose of the device_iommu_mapped() functions is to check
whether the device has been initialized by the IOMMU driver that handles
it, if any.

So it answers the question: Can I use the device in an IOMMU-API call?
And it is more readable than the dev->iommu_group checks everywhere :)

Regards,

	Joerg
Robin Murphy - Dec. 6, 2018, 5:42 p.m.
On 06/12/2018 15:35, Joerg Roedel wrote:
> Hi Robin,
> 
> On Wed, Dec 05, 2018 at 05:17:54PM +0000, Robin Murphy wrote:
>> FWIW, this check (and its ACPI equivalent in patch #3) is specifically
>> asking "has .add_device() already been called?", rather than the more
>> general "is this device managed by an IOMMU?" (to which the exact answer at
>> this point is "yes, provided we return successfully from here").
>>
>> I have no objection to the change as-is - especially if that usage is within
>> the intended scope of this API - I just wanted to call it out in case you're
>> also planning to introduce something else which would be even more
>> appropriate for that.
> 
> Yes, the purpose of the device_iommu_mapped() functions is to check
> whether the device has been initialized by the IOMMU driver that handles
> it, if any.
> 
> So it answers the question: Can I use the device in an IOMMU-API call?

OK, another way to consider the usage here would be "is the device ready 
to use in IOMMU API calls (or do I need to call add_device to finish 
setting it up)?", so it does in fact seem like a perfect fit, great!

> And it is more readable than the dev->iommu_group checks everywhere :)

For sure - although I am now wondering whether "mapped" is perhaps a 
little ambiguous in the naming, since the answer to "can I use the API" 
is yes even when the device may currently be attached to an 
identity/passthrough domain or blocked completely, neither of which 
involve any "mapping". Maybe simply "device_has_iommu()" would convey 
the intent better?

Robin.
Joerg Roedel - Dec. 7, 2018, 9:31 a.m.
On Thu, Dec 06, 2018 at 05:42:16PM +0000, Robin Murphy wrote:
> For sure - although I am now wondering whether "mapped" is perhaps a little
> ambiguous in the naming, since the answer to "can I use the API" is yes even
> when the device may currently be attached to an identity/passthrough domain
> or blocked completely, neither of which involve any "mapping". Maybe simply
> "device_has_iommu()" would convey the intent better?

The name is shorter version of:

	device_is_behind_an_iommu_remapping_its_dma_transactions()
	:)

The name is not perfect, but device_has_iommu() is not better because it
might be considered as a check whether the device itself has an IOMMU
built-in.

In the end an identity-mapping is also still a mapping (if the iommu
needs a page-table for that is an implementation detail), as is a
mapping with no page-table entries at all (blocking). So I think
device_iommu_mapped() is a reasonable choice.

Regards,

	Joerg

Patch

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index c5dd63072529..bfcf139503f0 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -220,7 +220,7 @@  const struct iommu_ops *of_iommu_configure(struct device *dev,
 	 * If we have reason to believe the IOMMU driver missed the initial
 	 * add_device callback for dev, replay it to get things in order.
 	 */
-	if (ops && ops->add_device && dev->bus && !dev->iommu_group)
+	if (ops && ops->add_device && dev->bus && !device_iommu_mapped(dev))
 		err = ops->add_device(dev);
 
 	/* Ignore all other errors apart from EPROBE_DEFER */