Re: [PATCH v5 1/9] xen/arm: don't pass iommu properties to hwdom for iommu-map

[Michal Orzel <michal.orzel@xxxxxxx>]

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@xxxxxxx>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
>> ---
>> 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