On 14.03.2025 14:34, Mykyta Poturai wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
>
> The main purpose of this patch is to add a way to register PCI device
> (which is behind the IOMMU) using the generic PCI-IOMMU DT bindings [1]
> before assigning that device to a domain.
>
> This behaves similarly to the existing iommu_add_dt_device API, except it
> handles PCI devices, and it is to be invoked from the add_device hook in the
> SMMU driver.
>
> The function dt_map_id to translate an ID through a downstream mapping
> (which is also suitable for mapping Requester ID) was borrowed from Linux
> (v5.10-rc6) and updated according to the Xen code base.
>
> [1]
> https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebr...@amd.com>
> Signed-off-by: Mykyta Poturai <mykyta_potu...@epam.com>
> ---
> Regarding pci_for_each_dma_alias question: getting host bridge node
> directly seems like a simpler solution with the same result. AFAIU
> with pci_for_each_dma_alias in linux we would arrive to the host brige
> node anyway, but also try to call dt_map_id for each device along the
> way. I am not sure why exactly it is done this way in linux, as
> according to the pci-iommu.txt, iommu-map node can only be present in
> the PCI root.
>
> v8->v9:
> * replace DT_NO_IOMMU with 1
> * guard iommu_add_pci_sideband_ids with CONFIG_ARM
I fear I'm confused: Isn't this contradicting ...
> v7->v8:
> * ENOSYS->EOPNOTSUPP
> * move iommu_add_pci_sideband_ids to iommu.c to fix x86 build
... this earlier change? Really, with there being no caller, I can't see
why there could be any build issue here affecting only x86. Except for
Misra complaining about unreachable code being introduced, which I'm sure
I said before should be avoided.
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -20,6 +20,7 @@
> #include <xen/param.h>
> #include <xen/softirq.h>
> #include <xen/keyhandler.h>
> +#include <xen/acpi.h>
> #include <xsm/xsm.h>
>
> #ifdef CONFIG_X86
> @@ -744,6 +745,20 @@ int __init
> iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
> return 0;
> }
>
> +#ifdef CONFIG_ARM
I realize we have CONFIG_X86 here as well (visible even in context of the
earlier hunk. Yet then the goal ought to be to reduce these anomalies, not
add new ones. Since I don't have a clear picture of what's wanted, I'm also
in trouble suggesting any alternative, I'm afraid.
> +int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
> +{
> + int ret = -EOPNOTSUPP;
> +
> +#ifdef CONFIG_HAS_PCI
> + if ( acpi_disabled )
> + ret = iommu_add_dt_pci_sideband_ids(pdev);
> +#endif
> +
> + return ret;
> +}
> +#endif
> +
> /*
> * Local variables:
> * mode: C
Having reached the end of the changes to this file, it's also not quite clear
why xen/acpi.h would need including uniformly. Can't this live inside the
outer #ifdef?
Further, since #ifdef-ary is used here already anyway, why ...
> @@ -238,8 +252,32 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct
> domain *d,
> */
> int iommu_remove_dt_device(struct dt_device_node *np);
>
> +#else /* !HAS_DEVICE_TREE */
> +static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> #endif /* HAS_DEVICE_TREE */
... resort to a stub when the inner #ifdef could simple be extended?
Jan
