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
