| Message ID | 20231004145604.1085358-2-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SMMU handling for PCIe Passthrough on ARM | expand |
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,
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,
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;
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(+)