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
