On 04.05.2023 23:54, Stewart Hildebrand wrote: > On 5/2/23 03:44, Jan Beulich wrote: >> On 01.05.2023 22:03, Stewart Hildebrand wrote: >>> @@ -228,6 +229,9 @@ int iommu_release_dt_devices(struct domain *d); >>> * (IOMMU is not enabled/present or device is not connected to it). >>> */ >>> int iommu_add_dt_device(struct dt_device_node *np); >>> +#ifdef CONFIG_HAS_PCI >>> +int iommu_add_dt_pci_device(uint8_t devfn, struct pci_dev *pdev); >>> +#endif >> >> Why the first parameter? Doesn't the 2nd one describe the device in full? > > It's related to phantom device/function handling, although this series > unfortunately does not properly handle phantom devices. > >> If this is about phantom devices, then I'd expect the function to take >> care of those (like iommu_add_device() does), rather than its caller. > > In the next patch ("[PATCH v1 4/6] pci/arm: Use iommu_add_dt_pci_device() > instead of arch hook"), we will invoke iommu_add_dt_pci_device(devfn, pdev) > from iommu_add_device().
Which I think I said there already I consider wrong. > Since iommu_add_device() iterates over the phantom functions, it would be > redundant to also have such a loop inside of iommu_add_dt_pci_device(). > > If we are to properly handle phantom devices on ARM, the SMMU drivers > (smmu.c/smmu-v3.c) would need some more work. In patches 5/6 and 6/6 in this > series, we have: > > if ( devfn != pdev->devfn ) > return -EOPNOTSUPP; > > Also, the ARM SMMU drivers in Xen currently only support a single AXI stream > ID per device, so some development would need to occur in order to support > phantom devices. > > Should phantom device support be part of this series, or would it be > acceptable to introduce phantom device support on ARM as part of a future > series? I wouldn't view this as a strict requirement, so long as it is made clear in respective patch descriptions. > Lastly, I'd like to check my understanding since phantom devices are new to > me. Here's my understanding: > > A phantom device is a device that advertises itself as single function, but > actually has multiple phantom functions. These phantom functions will have > unique requestor IDs (RID). The RID is essentially the BDF. To use a phantom > device with Xen, we specify the pci-phantom command line option, and we > identify phantom devices/functions in code by devfn != pdev->devfn. The command line option is there only to work around errata, i.e. devices behaving as if they had phantom functions without advertising themselves as such. See our use of PCI_EXP_DEVCAP_PHANTOM. As you can see, this being PCIe only, and legacy PCI device bevaing this way would require use of the command line option. > On ARM, we need to map/translate a BDF to an AXI stream ID in order for the > SMMU to identify the device and apply translation. That BDF -> stream ID > mapping is defined by the iommu-map/iommu-map-mask property in the device > tree [1]. The BDF -> AXI stream ID mapping in DT could allow phantom devices > (i.e. devices with phantom functions) to use different stream IDs based on > the (phantom) function. > > So, in theory, on ARM, there is a possibility we may have a device that > advertises itself as single function, but will issue AXI transactions with > multiple different AXI stream IDs due to phantom functions. In this case, we > will want each AXI stream ID to be programmed into the SMMU to avoid SMMU > faults. Right, which of course first requires that you know the mapping between these IDs. Jan > Please correct me if I've misunderstood anything. > > [1] > https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt