Re: [PATCH v1 3/6] iommu/arm: Introduce iommu_add_dt_pci_device API

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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(). 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?


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.

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.

Please correct me if I've misunderstood anything.

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