A device tree node for a PCIe root controller may have an iommu-map property [1]
with a phandle reference to the SMMU node, but not necessarily an iommus
property. In this case, we want to treat it the same as we currently handle
devices with an iommus property: don't pass the iommu related properties to
hwdom.

[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt

Reported-by: Michal Orzel <michal.orzel@amd.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v4->v5:
* new patch
---
 xen/arch/arm/domain_build.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 24c9019cc43c..7da254709d17 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1135,6 +1135,8 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
      * should be skipped.
      */
     iommu_node = dt_parse_phandle(node, "iommus", 0);
+    if ( !iommu_node )
+        iommu_node = dt_parse_phandle(node, "iommu-map", 1);
     if ( iommu_node && device_get_class(iommu_node) != DEVICE_IOMMU )
         iommu_node = NULL;
 
-- 
2.42.0

Hi,

On 04/10/2023 15:55, Stewart Hildebrand wrote:
> A device tree node for a PCIe root controller may have an iommu-map property [1]
> with a phandle reference to the SMMU node, but not necessarily an iommus
> property. In this case, we want to treat it the same as we currently handle
> devices with an iommus property: don't pass the iommu related properties to
> hwdom.
> 
> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
> 
> Reported-by: Michal Orzel <michal.orzel@amd.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Acked-by: Julien Grall <jgrall@amazon.com>

> ---
> v4->v5:
> * new patch
> ---
>   xen/arch/arm/domain_build.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 24c9019cc43c..7da254709d17 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1135,6 +1135,8 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>        * should be skipped.
>        */
>       iommu_node = dt_parse_phandle(node, "iommus", 0);
> +    if ( !iommu_node )
> +        iommu_node = dt_parse_phandle(node, "iommu-map", 1);
>       if ( iommu_node && device_get_class(iommu_node) != DEVICE_IOMMU )
>           iommu_node = NULL;

I was looking at the history to understand why we decided to not always 
hide the properties.

AFAICT, this was mainly to avoid removing the master properties but 
keeping the IOMMU nodes in the DT. However, in reality, I don't expect 
the IOMMU to function properly in dom0 (in particular when grants are 
used for DMA).

Looking at the bindings, it turns out that all the IOMMU using the 
generic bindigns would contain the property #iommu-cells. So we could 
remove any device with such property.

This would not work for IOMMU bindings not using the generic one. AFAIK, 
this is only legacy SMMUv{1,2} binding so we could filter them by 
compatible.

With that we would unconditionally remove the properties iommu-* and 
avoid increasing the complexity.

Any thoughts?

Cheers,

-- 
Julien Grall

Hi,

On 20/10/2023 20:48, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 04/10/2023 15:55, Stewart Hildebrand wrote:
>> A device tree node for a PCIe root controller may have an iommu-map property [1]
>> with a phandle reference to the SMMU node, but not necessarily an iommus
>> property. In this case, we want to treat it the same as we currently handle
>> devices with an iommus property: don't pass the iommu related properties to
>> hwdom.
>>
>> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
>>
>> Reported-by: Michal Orzel <michal.orzel@amd.com>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
>> ---
>> v4->v5:
>> * new patch
>> ---
>>   xen/arch/arm/domain_build.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 24c9019cc43c..7da254709d17 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1135,6 +1135,8 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>>        * should be skipped.
>>        */
>>       iommu_node = dt_parse_phandle(node, "iommus", 0);
>> +    if ( !iommu_node )
>> +        iommu_node = dt_parse_phandle(node, "iommu-map", 1);
>>       if ( iommu_node && device_get_class(iommu_node) != DEVICE_IOMMU )
>>           iommu_node = NULL;
> 
> I was looking at the history to understand why we decided to not always
> hide the properties.
> 
> AFAICT, this was mainly to avoid removing the master properties but
> keeping the IOMMU nodes in the DT. However, in reality, I don't expect
> the IOMMU to function properly in dom0 (in particular when grants are
> used for DMA).
> 
> Looking at the bindings, it turns out that all the IOMMU using the
> generic bindigns would contain the property #iommu-cells. So we could
> remove any device with such property.
> 
> This would not work for IOMMU bindings not using the generic one. AFAIK,
> this is only legacy SMMUv{1,2} binding so we could filter them by
> compatible.
> 
> With that we would unconditionally remove the properties iommu-* and
> avoid increasing the complexity.
> 
> Any thoughts?
I think it is a good idea to skip all IOMMU nodes and properties and not only the ones
recognized by Xen. I realized that it may have an impact on Rahul's RFC vSMMU series
but I checked the patches and impact is negligible. As for supporting legacy bindings, I think
we would be good if we search for #iommu-cells || mmu-masters.

~Michal

Hi,

On 23/10/2023 10:00, Michal Orzel wrote:
> As for supporting legacy bindings, I think
> we would be good if we search for #iommu-cells || mmu-masters

For me, it is clear in the device-tree bindings documentation that 
#iommu-cells will belong to a IOMMU node. It is less clear for 
mmu-masters, therefore I would rather prefer if we use the compatible to 
detect legacy bindings.

I am not that concerned to properly remove any nodes using legacy 
bindings because new platforms should not use it. So we could also keep 
the existing behavior (i.e. exposing the IOMMU unless it is used by Xen).

Cheers,

-- 
Julien Grall