1. Patchew
  2. Xen
  3. [PATCH v4 0/7] SMMU handling for PCIe Passthrough on ARM
  4. View patch

[PATCH v4 1/7] xen/arm: Improve readability of check for registered devices

Stewart Hildebrand posted 7 patches 2 years, 8 months ago
There is a newer version of this series
[PATCH v4 1/7] xen/arm: Improve readability of check for registered devices
Posted by Stewart Hildebrand 2 years, 8 months ago 
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Improve readability of check for devices already registered with the SMMU with
legacy mmu-masters DT bindings by using is_protected.

There are 2 device tree bindings for registering a device with the SMMU:
* mmu-masters (legacy, SMMUv1/2 only)
* iommus

A device tree may include both mmu-masters and iommus properties (although it is
unnecessary to do so). When a device appears in the mmu-masters list,
np->is_protected and dev->iommu_fwspec both get set by the SMMUv1/2 driver. The
function iommu_add_dt_device() is subsequently invoked for devices that have an
iommus specification.

The check as it was before this patch:

  if ( dev_iommu_fwspec_get(dev) )
      return 0;

and the new check:

  if ( dt_device_is_protected(np) )
      return 0;

are guarding against the same corner case: when a device has both mmu-masters
and iommus specifications in the device tree. The is_protected naming is more
descriptive.

If np->is_protected is not set (i.e. false), but dev->iommu_fwspec is set, it is
an error condition, so return an error in this case.

Expand the comment to further clarify the corner case.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v3->v4:
* new patch: this change was split from ("xen/arm: Move is_protected flag to struct device")
---
 xen/drivers/passthrough/device_tree.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 1c32d7b50cce..d9b63da7260a 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -141,12 +141,17 @@ int iommu_add_dt_device(struct dt_device_node *np)
         return -EINVAL;
 
     /*
-     * The device may already have been registered. As there is no harm in
-     * it just return success early.
+     * Devices that appear in the legacy mmu-masters list may have already been
+     * registered with the SMMU. In case a device has both a mmu-masters entry
+     * and iommus property, there is no need to register it again. In this case
+     * simply return success early.
      */
-    if ( dev_iommu_fwspec_get(dev) )
+    if ( dt_device_is_protected(np) )
         return 0;
 
+    if ( dev_iommu_fwspec_get(dev) )
+        return -EEXIST;
+
     /*
      * According to the Documentation/devicetree/bindings/iommu/iommu.txt
      * from Linux.
-- 
2.40.1
Re: [PATCH v4 1/7] xen/arm: Improve readability of check for registered devices
Posted by Julien Grall 2 years, 8 months ago 
Hi Stewart,

On 07/06/2023 04:02, Stewart Hildebrand wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Improve readability of check for devices already registered with the SMMU with
> legacy mmu-masters DT bindings by using is_protected.

I am unconvinced with this change because...

> 
> There are 2 device tree bindings for registering a device with the SMMU:
> * mmu-masters (legacy, SMMUv1/2 only)
> * iommus
> 
> A device tree may include both mmu-masters and iommus properties (although it is
> unnecessary to do so). When a device appears in the mmu-masters list,
> np->is_protected and dev->iommu_fwspec both get set by the SMMUv1/2 driver. The
> function iommu_add_dt_device() is subsequently invoked for devices that have an
> iommus specification.
> 
> The check as it was before this patch:
> 
>    if ( dev_iommu_fwspec_get(dev) )
>        return 0;
> 
> and the new check:
> 
>    if ( dt_device_is_protected(np) )
>        return 0;
> 
> are guarding against the same corner case: when a device has both mmu-masters
> and iommus specifications in the device tree. The is_protected naming is more
> descriptive.
> 
> If np->is_protected is not set (i.e. false), but dev->iommu_fwspec is set, it is
> an error condition, so return an error in this case.
> 
> Expand the comment to further clarify the corner case.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> v3->v4:
> * new patch: this change was split from ("xen/arm: Move is_protected flag to struct device")
> ---
>   xen/drivers/passthrough/device_tree.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 1c32d7b50cce..d9b63da7260a 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -141,12 +141,17 @@ int iommu_add_dt_device(struct dt_device_node *np)
>           return -EINVAL;
>   
>       /*
> -     * The device may already have been registered. As there is no harm in
> -     * it just return success early.
> +     * Devices that appear in the legacy mmu-masters list may have already been
> +     * registered with the SMMU. In case a device has both a mmu-masters entry
> +     * and iommus property, there is no need to register it again. In this case
> +     * simply return success early.
>        */
> -    if ( dev_iommu_fwspec_get(dev) )
> +    if ( dt_device_is_protected(np) )
... we now have two checks and it implies that it would be normal for 
dt_device_is_protected() to be false and ...

>           return 0;
>   
> +    if ( dev_iommu_fwspec_get(dev) )

... this returning a non-zero value. Is there any other benefits with 
adding the two checks?

If the others agree with the double check, then I think this should gain 
an ASSERT_UNREACHABLE() because AFAIU this is a programming error.

> +        return -EEXIST;
> +
>       /*
>        * According to the Documentation/devicetree/bindings/iommu/iommu.txt
>        * from Linux.

Cheers,

-- 
Julien Grall
Re: [PATCH v4 1/7] xen/arm: Improve readability of check for registered devices
Posted by Stewart Hildebrand 2 years, 8 months ago 
On 6/7/23 03:27, Julien Grall wrote:
> Hi Stewart,
> 
> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Improve readability of check for devices already registered with the SMMU with
>> legacy mmu-masters DT bindings by using is_protected.
> 
> I am unconvinced with this change because...
> 
>>
>> There are 2 device tree bindings for registering a device with the SMMU:
>> * mmu-masters (legacy, SMMUv1/2 only)
>> * iommus
>>
>> A device tree may include both mmu-masters and iommus properties (although it is
>> unnecessary to do so). When a device appears in the mmu-masters list,
>> np->is_protected and dev->iommu_fwspec both get set by the SMMUv1/2 driver. The
>> function iommu_add_dt_device() is subsequently invoked for devices that have an
>> iommus specification.
>>
>> The check as it was before this patch:
>>
>>    if ( dev_iommu_fwspec_get(dev) )
>>        return 0;
>>
>> and the new check:
>>
>>    if ( dt_device_is_protected(np) )
>>        return 0;
>>
>> are guarding against the same corner case: when a device has both mmu-masters
>> and iommus specifications in the device tree. The is_protected naming is more
>> descriptive.
>>
>> If np->is_protected is not set (i.e. false), but dev->iommu_fwspec is set, it is
>> an error condition, so return an error in this case.
>>
>> Expand the comment to further clarify the corner case.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>> v3->v4:
>> * new patch: this change was split from ("xen/arm: Move is_protected flag to struct device")
>> ---
>>   xen/drivers/passthrough/device_tree.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>> index 1c32d7b50cce..d9b63da7260a 100644
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -141,12 +141,17 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>           return -EINVAL;
>>
>>       /*
>> -     * The device may already have been registered. As there is no harm in
>> -     * it just return success early.
>> +     * Devices that appear in the legacy mmu-masters list may have already been
>> +     * registered with the SMMU. In case a device has both a mmu-masters entry
>> +     * and iommus property, there is no need to register it again. In this case
>> +     * simply return success early.
>>        */
>> -    if ( dev_iommu_fwspec_get(dev) )
>> +    if ( dt_device_is_protected(np) )
> ... we now have two checks and it implies that it would be normal for
> dt_device_is_protected() to be false and ...
> 
>>           return 0;
>>
>> +    if ( dev_iommu_fwspec_get(dev) )
> 
> ... this returning a non-zero value. Is there any other benefits with
> adding the two checks?

No, I can't think of any good rationale for the 2nd check. After splitting this change from the other patch ("xen/arm: Move is_protected flag to struct device"), I simply wanted to evaluate it on its own.

> If the others agree with the double check, then I think this should gain
> an ASSERT_UNREACHABLE() because AFAIU this is a programming error.

Right, if the 2nd check was justified, there should be an ASSERT_UNREACHABLE(), good point. But I don't think the 2nd check is justified.

If the 2nd check is dropped (keeping only the is_protected check), then do you think the change is justified?

>> +        return -EEXIST;
>> +
>>       /*
>>        * According to the Documentation/devicetree/bindings/iommu/iommu.txt
>>        * from Linux.
> 
> Cheers,
> 
> -- 
> Julien Grall
Re: [PATCH v4 1/7] xen/arm: Improve readability of check for registered devices
Posted by Julien Grall 2 years, 7 months ago 
Hi,

Sorry for the late answer.

On 07/06/2023 14:41, Stewart Hildebrand wrote:
> On 6/7/23 03:27, Julien Grall wrote:
>> Hi Stewart,
>>
>> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> Improve readability of check for devices already registered with the SMMU with
>>> legacy mmu-masters DT bindings by using is_protected.
>>
>> I am unconvinced with this change because...
>>
>>>
>>> There are 2 device tree bindings for registering a device with the SMMU:
>>> * mmu-masters (legacy, SMMUv1/2 only)
>>> * iommus
>>>
>>> A device tree may include both mmu-masters and iommus properties (although it is
>>> unnecessary to do so). When a device appears in the mmu-masters list,
>>> np->is_protected and dev->iommu_fwspec both get set by the SMMUv1/2 driver. The
>>> function iommu_add_dt_device() is subsequently invoked for devices that have an
>>> iommus specification.
>>>
>>> The check as it was before this patch:
>>>
>>>     if ( dev_iommu_fwspec_get(dev) )
>>>         return 0;
>>>
>>> and the new check:
>>>
>>>     if ( dt_device_is_protected(np) )
>>>         return 0;
>>>
>>> are guarding against the same corner case: when a device has both mmu-masters
>>> and iommus specifications in the device tree. The is_protected naming is more
>>> descriptive.
>>>
>>> If np->is_protected is not set (i.e. false), but dev->iommu_fwspec is set, it is
>>> an error condition, so return an error in this case.
>>>
>>> Expand the comment to further clarify the corner case.
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>> ---
>>> v3->v4:
>>> * new patch: this change was split from ("xen/arm: Move is_protected flag to struct device")
>>> ---
>>>    xen/drivers/passthrough/device_tree.c | 11 ++++++++---
>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>>> index 1c32d7b50cce..d9b63da7260a 100644
>>> --- a/xen/drivers/passthrough/device_tree.c
>>> +++ b/xen/drivers/passthrough/device_tree.c
>>> @@ -141,12 +141,17 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>            return -EINVAL;
>>>
>>>        /*
>>> -     * The device may already have been registered. As there is no harm in
>>> -     * it just return success early.
>>> +     * Devices that appear in the legacy mmu-masters list may have already been
>>> +     * registered with the SMMU. In case a device has both a mmu-masters entry
>>> +     * and iommus property, there is no need to register it again. In this case
>>> +     * simply return success early.
>>>         */
>>> -    if ( dev_iommu_fwspec_get(dev) )
>>> +    if ( dt_device_is_protected(np) )
>> ... we now have two checks and it implies that it would be normal for
>> dt_device_is_protected() to be false and ...
>>
>>>            return 0;
>>>
>>> +    if ( dev_iommu_fwspec_get(dev) )
>>
>> ... this returning a non-zero value. Is there any other benefits with
>> adding the two checks?
> 
> No, I can't think of any good rationale for the 2nd check. After splitting this change from the other patch ("xen/arm: Move is_protected flag to struct device"), I simply wanted to evaluate it on its own.
> 
>> If the others agree with the double check, then I think this should gain
>> an ASSERT_UNREACHABLE() because AFAIU this is a programming error.
> 
> Right, if the 2nd check was justified, there should be an ASSERT_UNREACHABLE(), good point. But I don't think the 2nd check is justified.
> 
> If the 2nd check is dropped (keeping only the is_protected check), then do you think the change is justified?

To be honest not with the current justification. I don't view the new 
check better than the other in term of readability.

Is this the only reason you want to switch to dt_device_is_protected()?

Cheers,

-- 
Julien Grall
Re: [PATCH v4 1/7] xen/arm: Improve readability of check for registered devices
Posted by Stewart Hildebrand 2 years, 7 months ago 
On 6/29/23 17:47, Julien Grall wrote:
> Hi,
> 
> Sorry for the late answer.
> 
> On 07/06/2023 14:41, Stewart Hildebrand wrote:
>> On 6/7/23 03:27, Julien Grall wrote:
>>> Hi Stewart,
>>>
>>> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> Improve readability of check for devices already registered with the SMMU with
>>>> legacy mmu-masters DT bindings by using is_protected.
>>>
>>> I am unconvinced with this change because...
>>>
>>>>
>>>> There are 2 device tree bindings for registering a device with the SMMU:
>>>> * mmu-masters (legacy, SMMUv1/2 only)
>>>> * iommus
>>>>
>>>> A device tree may include both mmu-masters and iommus properties (although it is
>>>> unnecessary to do so). When a device appears in the mmu-masters list,
>>>> np->is_protected and dev->iommu_fwspec both get set by the SMMUv1/2 driver. The
>>>> function iommu_add_dt_device() is subsequently invoked for devices that have an
>>>> iommus specification.
>>>>
>>>> The check as it was before this patch:
>>>>
>>>>     if ( dev_iommu_fwspec_get(dev) )
>>>>         return 0;
>>>>
>>>> and the new check:
>>>>
>>>>     if ( dt_device_is_protected(np) )
>>>>         return 0;
>>>>
>>>> are guarding against the same corner case: when a device has both mmu-masters
>>>> and iommus specifications in the device tree. The is_protected naming is more
>>>> descriptive.
>>>>
>>>> If np->is_protected is not set (i.e. false), but dev->iommu_fwspec is set, it is
>>>> an error condition, so return an error in this case.
>>>>
>>>> Expand the comment to further clarify the corner case.
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>> ---
>>>> v3->v4:
>>>> * new patch: this change was split from ("xen/arm: Move is_protected flag to struct device")
>>>> ---
>>>>    xen/drivers/passthrough/device_tree.c | 11 ++++++++---
>>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>>>> index 1c32d7b50cce..d9b63da7260a 100644
>>>> --- a/xen/drivers/passthrough/device_tree.c
>>>> +++ b/xen/drivers/passthrough/device_tree.c
>>>> @@ -141,12 +141,17 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>>            return -EINVAL;
>>>>
>>>>        /*
>>>> -     * The device may already have been registered. As there is no harm in
>>>> -     * it just return success early.
>>>> +     * Devices that appear in the legacy mmu-masters list may have already been
>>>> +     * registered with the SMMU. In case a device has both a mmu-masters entry
>>>> +     * and iommus property, there is no need to register it again. In this case
>>>> +     * simply return success early.
>>>>         */
>>>> -    if ( dev_iommu_fwspec_get(dev) )
>>>> +    if ( dt_device_is_protected(np) )
>>> ... we now have two checks and it implies that it would be normal for
>>> dt_device_is_protected() to be false and ...
>>>
>>>>            return 0;
>>>>
>>>> +    if ( dev_iommu_fwspec_get(dev) )
>>>
>>> ... this returning a non-zero value. Is there any other benefits with
>>> adding the two checks?
>>
>> No, I can't think of any good rationale for the 2nd check. After splitting this change from the other patch ("xen/arm: Move is_protected flag to struct device"), I simply wanted to evaluate it on its own.
>>
>>> If the others agree with the double check, then I think this should gain
>>> an ASSERT_UNREACHABLE() because AFAIU this is a programming error.
>>
>> Right, if the 2nd check was justified, there should be an ASSERT_UNREACHABLE(), good point. But I don't think the 2nd check is justified.
>>
>> If the 2nd check is dropped (keeping only the is_protected check), then do you think the change is justified?
> 
> To be honest not with the current justification. I don't view the new
> check better than the other in term of readability.
> 
> Is this the only reason you want to switch to dt_device_is_protected()?

It was switched originally in the downstream I cherry-picked the patch ("xen/arm: Move is_protected flag to struct device") [1] from, where the rationale for the change wasn't explicitly written. Improving readability was the only rationale I could think of.

Anyway, I will drop this change for the next revision of this series. Hmm. The comment may still want to be expanded though... But I feel improving the comment alone should not be part of this series.

[1] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/59753aac77528a584d3950936b853ebf264b68e7#9e9e9f5f577b2b9fc19d92dcefe7580c7c3af744

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.