On 11.05.2023 21:16, Stewart Hildebrand wrote:
> @@ -762,9 +767,20 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> pdev->domain = NULL;
> goto out;
> }
> +#ifdef CONFIG_HAS_DEVICE_TREE
> + ret = iommu_add_dt_pci_device(pdev);
> + if ( ret < 0 )
> + {
> + printk(XENLOG_ERR "pci-iommu translation failed: %d\n", ret);
> + goto out;
> + }
> +#endif
> ret = iommu_add_device(pdev);
Hmm, am I misremembering that in the earlier patch you had #else to
invoke the alternative behavior? Now you end up calling both functions;
if that's indeed intended, this may still want doing differently.
Looking at the earlier patch introducing the function, I can't infer
though whether that's intended: iommu_add_dt_pci_device() checks that
the add_device hook is present, but then I didn't find any use of this
hook. The revlog there suggests the check might be stale.
If indeed the function does only preparatory work, I don't see why it
would need naming "iommu_..."; I'd rather consider pci_add_dt_device()
then. Plus in such a case #ifdef-ary here probably wants avoiding by
introducing a suitable no-op stub for the !HAS_DEVICE_TREE case. Then
...
> if ( ret )
> {
> +#ifdef CONFIG_HAS_DEVICE_TREE
> + iommu_fwspec_free(pci_to_dev(pdev));
> +#endif
... this (which I understand is doing the corresponding cleanup) then
also wants wrapping in a suitably named tiny helper function.
But yet further I'm then no longer convinced this is the right place
for the addition. pci_add_device() is backing physdev hypercalls. It
would seem to me that the function may want invoking yet one layer
further up, or it may even want invoking from a brand new DT-specific
physdev-op. This would then leave at least the x86-only paths (invoking
pci_add_device() from outside of pci_physdev_op()) entirely alone.
Jan
