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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/5/23 02:20, Jan Beulich wrote:
> 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.

Okay, I'm following now. Right, so, the first parameter is not needed.

>> 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.

I spoke too soon, the SMMU drivers do appear to support multiple IDs.

>>
>> 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.

After doing a little more investigation, I should be able to add in the 
plumbing to iterate over the phantom functions in iommu_add_dt_pci_device(). 
Stay tuned for v2 of this 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.
> 
> 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.

Thanks for clarifying!

Stewart