Re: [PATCH v9 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.03.2025 14:34, Mykyta Poturai wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> 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_tyshchenko@xxxxxxxx>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> ---
> 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