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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.02.2025 14:54, Mykyta Poturai wrote:
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -27,6 +27,7 @@
>  #include <xen/errno.h>
>  #include <public/domctl.h>
>  #include <public/hvm/ioreq.h>
> +#include <xen/acpi.h>
>  #include <asm/device.h>

Please insert where the other xen/ #include-s are. (Those aren't sorted
yet, so there is more than one place where you could reasonably put it.
I'd recommend ahead of xen/errno.h.)

> @@ -241,8 +244,31 @@ int iommu_remove_dt_device(struct dt_device_node *np);
>  /* Error code for reporting no IOMMU is present */
>  #define NO_IOMMU    1
>  
> +#else /* !HAS_DEVICE_TREE */
> +static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
> +{
> +    return -ENOSYS;

No abuse of ENOSYS please. EOPNOTSUPP if nothing better can be found.

> +}
> +
>  #endif /* HAS_DEVICE_TREE */
>  
> +#ifdef CONFIG_HAS_PCI
> +static inline int iommu_add_pci_sideband_ids(struct pci_dev *pdev)

>From its name it's unclear whether the function actually means to alter
the passed in pdev (my initial guess was that it wouldn't, but the call
tree from iommu_add_dt_pci_sideband_ids() is getting deep-ish). If not,
the parameter should be pointer-to-const.

> +{
> +    int ret = -ENOSYS;
> +
> +    if ( acpi_disabled )
> +        ret = iommu_add_dt_pci_sideband_ids(pdev);
> +
> +    return ret;
> +}
> +#else /* !HAS_PCI */
> +static inline int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
> +{
> +    return -ENOSYS;
> +}
> +#endif

Why the redundancy?

static inline 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;
}

Jan