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

[Jan Beulich <jbeulich@xxxxxxxx>]

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