From: Oleksandr Tyshchenko <oleksandr_tyshchenko@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_tyshchenko@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@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.

v7->v8:
* ENOSYS->EOPNOTSUPP
* move iommu_add_pci_sideband_ids to iommu.c to fix x86 build
* simplify iommu_add_pci_sideband_ids
* add docstrings to iommu_add_{dt_}pci_sideband_ids

v6->v7:
* put iommu_add_pci_sideband_ids under ifdef
* remove ifdef CONFIG_APCI
* style: add newline for symmetry

v5->v6:
* pass ops to iommu_dt_xlate()

v4->v5:
* style: add newlines after variable declarations and before return in iommu.h
* drop device_is_protected() check in iommu_add_dt_pci_sideband_ids()
* rebase on top of ("dynamic node programming using overlay dtbo") series
* fix typo in commit message
* remove #ifdef around dt_map_id() prototype
* move dt_map_id() to xen/common/device_tree.c
* add function name in error prints
* use dprintk for debug prints
* use GENMASK and #include <xen/bitops.h>
* fix typo in comment
* remove unnecessary (int) cast in loop condition
* assign *id_out and return success in case of no translation in dt_map_id()
* don't initialize local variable unnecessarily
* return error in case of ACPI/no DT in iommu_add_{dt_}pci_sideband_ids()

v3->v4:
* wrap #include <asm/acpi.h> and if ( acpi_disabled ) in #ifdef CONFIG_ACPI
* fix Michal's remarks about style, parenthesis, and print formats
* remove !ops->dt_xlate check since it is already in iommu_dt_xlate helper
* rename s/iommu_dt_pci_map_id/dt_map_id/ because it is generic, not specific
  to iommu
* update commit description

v2->v3:
* new patch title (was: iommu/arm: Introduce iommu_add_dt_pci_device API)
* renamed function
  from: iommu_add_dt_pci_device
  to: iommu_add_dt_pci_sideband_ids
* removed stale ops->add_device check
* iommu.h: add empty stub iommu_add_dt_pci_sideband_ids for !HAS_DEVICE_TREE
* iommu.h: add iommu_add_pci_sideband_ids helper
* iommu.h: don't wrap prototype in #ifdef CONFIG_HAS_PCI
* s/iommu_fwspec_free(pci_to_dev(pdev))/iommu_fwspec_free(dev)/

v1->v2:
* remove extra devfn parameter since pdev fully describes the device
* remove ops->add_device() call from iommu_add_dt_pci_device(). Instead, rely on
  the existing iommu call in iommu_add_device().
* move the ops->add_device and ops->dt_xlate checks earlier

downstream->v1:
* rebase
* add const qualifier to struct dt_device_node *np arg in dt_map_id()
* add const qualifier to struct dt_device_node *np declaration in iommu_add_pci_device()
* use stdint.h types instead of u8/u32/etc...
* rename functions:
  s/dt_iommu_xlate/iommu_dt_xlate/
  s/dt_map_id/iommu_dt_pci_map_id/
  s/iommu_add_pci_device/iommu_add_dt_pci_device/
* add device_is_protected check in iommu_add_dt_pci_device
* wrap prototypes in CONFIG_HAS_PCI

(cherry picked from commit 734e3bf6ee77e7947667ab8fa96c25b349c2e1da from
 the downstream branch poc/pci-passthrough from
 https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
---
 xen/common/device-tree/device-tree.c  | 91 +++++++++++++++++++++++++++
 xen/drivers/passthrough/device_tree.c | 42 +++++++++++++
 xen/drivers/passthrough/iommu.c       | 13 ++++
 xen/include/xen/device_tree.h         | 23 +++++++
 xen/include/xen/iommu.h               | 32 +++++++++-
 5 files changed, 200 insertions(+), 1 deletion(-)

diff --git a/xen/common/device-tree/device-tree.c b/xen/common/device-tree/device-tree.c
index d0528c5825..3de7858df6 100644
--- a/xen/common/device-tree/device-tree.c
+++ b/xen/common/device-tree/device-tree.c
@@ -10,6 +10,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <xen/bitops.h>
 #include <xen/types.h>
 #include <xen/init.h>
 #include <xen/guest_access.h>
@@ -2243,6 +2244,96 @@ int dt_get_pci_domain_nr(struct dt_device_node *node)
     return (u16)domain;
 }
 
+int dt_map_id(const struct dt_device_node *np, uint32_t id,
+              const char *map_name, const char *map_mask_name,
+              struct dt_device_node **target, uint32_t *id_out)
+{
+    uint32_t map_mask, masked_id, map_len;
+    const __be32 *map = NULL;
+
+    if ( !np || !map_name || (!target && !id_out) )
+        return -EINVAL;
+
+    map = dt_get_property(np, map_name, &map_len);
+    if ( !map )
+    {
+        if ( target )
+            return -ENODEV;
+
+        /* Otherwise, no map implies no translation */
+        *id_out = id;
+        return 0;
+    }
+
+    if ( !map_len || (map_len % (4 * sizeof(*map))) )
+    {
+        printk(XENLOG_ERR "%s(): %s: Error: Bad %s length: %u\n", __func__,
+               np->full_name, map_name, map_len);
+        return -EINVAL;
+    }
+
+    /* The default is to select all bits. */
+    map_mask = GENMASK(31, 0);
+
+    /*
+     * Can be overridden by "{iommu,msi}-map-mask" property.
+     * If dt_property_read_u32() fails, the default is used.
+     */
+    if ( map_mask_name )
+        dt_property_read_u32(np, map_mask_name, &map_mask);
+
+    masked_id = map_mask & id;
+    for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4 )
+    {
+        struct dt_device_node *phandle_node;
+        uint32_t id_base = be32_to_cpup(map + 0);
+        uint32_t phandle = be32_to_cpup(map + 1);
+        uint32_t out_base = be32_to_cpup(map + 2);
+        uint32_t id_len = be32_to_cpup(map + 3);
+
+        if ( id_base & ~map_mask )
+        {
+            printk(XENLOG_ERR "%s(): %s: Invalid %s translation - %s-mask (0x%"PRIx32") ignores id-base (0x%"PRIx32")\n",
+                   __func__, np->full_name, map_name, map_name, map_mask,
+                   id_base);
+            return -EFAULT;
+        }
+
+        if ( (masked_id < id_base) || (masked_id >= (id_base + id_len)) )
+            continue;
+
+        phandle_node = dt_find_node_by_phandle(phandle);
+        if ( !phandle_node )
+            return -ENODEV;
+
+        if ( target )
+        {
+            if ( !*target )
+                *target = phandle_node;
+
+            if ( *target != phandle_node )
+                continue;
+        }
+
+        if ( id_out )
+            *id_out = masked_id - id_base + out_base;
+
+        dprintk(XENLOG_DEBUG, "%s: %s, using mask %08"PRIx32", id-base: %08"PRIx32", out-base: %08"PRIx32", length: %08"PRIx32", id: %08"PRIx32" -> %08"PRIx32"\n",
+               np->full_name, map_name, map_mask, id_base, out_base, id_len, id,
+               masked_id - id_base + out_base);
+        return 0;
+    }
+
+    dprintk(XENLOG_DEBUG, "%s: no %s translation for id 0x%"PRIx32" on %s\n",
+           np->full_name, map_name, id,
+           (target && *target) ? (*target)->full_name : NULL);
+
+    if ( id_out )
+        *id_out = id;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index fe2aaef2df..065fbbc0fd 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -161,6 +161,48 @@ static int iommu_dt_xlate(struct device *dev,
     return ops->dt_xlate(dev, iommu_spec);
 }
 
+#ifdef CONFIG_HAS_PCI
+int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+    struct dt_phandle_args iommu_spec = { .args_count = 1 };
+    struct device *dev = pci_to_dev(pdev);
+    const struct dt_device_node *np;
+    int rc;
+
+    if ( !iommu_enabled )
+        return DT_NO_IOMMU;
+
+    if ( !ops )
+        return -EINVAL;
+
+    if ( dev_iommu_fwspec_get(dev) )
+        return -EEXIST;
+
+    np = pci_find_host_bridge_node(pdev);
+    if ( !np )
+        return -ENODEV;
+
+    /*
+     * According to the Documentation/devicetree/bindings/pci/pci-iommu.txt
+     * from Linux.
+     */
+    rc = dt_map_id(np, PCI_BDF(pdev->bus, pdev->devfn), "iommu-map",
+                   "iommu-map-mask", &iommu_spec.np, iommu_spec.args);
+    if ( rc )
+        return (rc == -ENODEV) ? DT_NO_IOMMU : rc;
+
+    rc = iommu_dt_xlate(dev, &iommu_spec, ops);
+    if ( rc < 0 )
+    {
+        iommu_fwspec_free(dev);
+        return -EINVAL;
+    }
+
+    return rc;
+}
+#endif /* CONFIG_HAS_PCI */
+
 int iommu_remove_dt_device(struct dt_device_node *np)
 {
     const struct iommu_ops *ops = iommu_get_ops();
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 9e74a1fc72..40c840a4b3 100644
--- 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,18 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
     return 0;
 }
 
+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;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 5ff763bb80..9254204af6 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -946,6 +946,29 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
  */
 int dt_get_pci_domain_nr(struct dt_device_node *node);
 
+/**
+ * dt_map_id - Translate an ID through a downstream mapping.
+ * @np: root complex device node.
+ * @id: device ID to map.
+ * @map_name: property name of the map to use.
+ * @map_mask_name: optional property name of the mask to use.
+ * @target: optional pointer to a target device node.
+ * @id_out: optional pointer to receive the translated ID.
+ *
+ * Given a device ID, look up the appropriate implementation-defined
+ * platform ID and/or the target device which receives transactions on that
+ * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or
+ * @id_out may be NULL if only the other is required. If @target points to
+ * a non-NULL device node pointer, only entries targeting that node will be
+ * matched; if it points to a NULL value, it will receive the device node of
+ * the first matching target phandle, with a reference held.
+ *
+ * Return: 0 on success or a standard error code on failure.
+ */
+int dt_map_id(const struct dt_device_node *np, uint32_t id,
+              const char *map_name, const char *map_mask_name,
+              struct dt_device_node **target, uint32_t *id_out);
+
 struct dt_device_node *dt_find_node_by_phandle(dt_phandle handle);
 
 #ifdef CONFIG_DEVICE_TREE_DEBUG
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 2b6e6e8a3f..15ec261238 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -215,7 +215,8 @@ int iommu_dt_domain_init(struct domain *d);
 int iommu_release_dt_devices(struct domain *d);
 
 /*
- * Helper to add master device to the IOMMU using generic IOMMU DT bindings.
+ * Helpers to add master device to the IOMMU using generic (PCI-)IOMMU
+ * DT bindings.
  *
  * Return values:
  *  0 : device is protected by an IOMMU
@@ -225,6 +226,19 @@ int iommu_release_dt_devices(struct domain *d);
  */
 int iommu_add_dt_device(struct dt_device_node *np);
 
+/*
+ * Fills out the device's IOMMU fwspec with IOMMU ids from the DT.
+ * Ids are specified in the iommu-map property in the host bridge node.
+ * More information on the iommu-map property format can be found in
+ * Documentation/devicetree/bindings/pci/pci-iommu.txt from Linux.
+ *
+ * Return values:
+ *  0 : iommu_fwspec is filled out successfully.
+ * <0 : error while filling out the iommu_fwspec.
+ * >0 : IOMMU is not enabled/present or device is not connected to it.
+ */
+int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev);
+
 int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
                        XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
 
@@ -246,8 +260,24 @@ int iommu_remove_dt_device(struct dt_device_node *np);
 
 #define DT_NO_IOMMU    1
 
+#else /* !HAS_DEVICE_TREE */
+static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
+{
+    return -EOPNOTSUPP;
+}
+
 #endif /* HAS_DEVICE_TREE */
 
+/*
+ * Fills out the device's IOMMU fwspec with IOMMU ids.
+ *
+ * Return values:
+ *  0 : iommu_fwspec is filled out successfully.
+ * <0 : error while filling out the iommu_fwspec.
+ * >0 : IOMMU is not enabled/present or device is not connected to it.
+ */
+int iommu_add_pci_sideband_ids(struct pci_dev *pdev);
+
 struct page_info;
 
 /*
-- 
2.34.1

On 10.02.2025 11:30, Mykyta Poturai wrote:
> --- 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,18 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>      return 0;
>  }
>  
> +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;
> +}

This function has no caller, which violates a Misra rule iirc. Considering
all information given here it's also unclear why it would gain a caller on
x86 (at least as long as DT isn't used there).

Jan

On 10.02.25 12:52, Jan Beulich wrote:
> On 10.02.2025 11:30, Mykyta Poturai wrote:
>> --- 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,18 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>>       return 0;
>>   }
>>   
>> +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;
>> +}
> 
> This function has no caller, which violates a Misra rule iirc. Considering
> all information given here it's also unclear why it would gain a caller on
> x86 (at least as long as DT isn't used there).
> 
> Jan

Would it be ok to wrap it with CONFIG_ARM? I am not quite sure how 
relevant this mapping functionality is to X86 iommus, although Linux has 
similar implementations for ACPI.

Alternatively, we can remove this abstraction for now, to call 
iommu_add_dt_pci_sideband_ids from Arm directly and only introduce it 
back when at least some ACPI implementation is done.

Also, just want to mention the issue that forced me to move this from 
the header in the first place in case it is not known. There is a 
conflict in fixed width integers definitions between actypes.h and 
efibind.h and it was revealed when including acpi.h into iommu.h.
I initially tried to fix the source of this conflict, but I don't know 
enough about ACPI and EFI quirks to confidently do it.

In file included from ./include/acpi/acpi.h:57,
                  from ./include/xen/acpi.h:57,
                  from ./include/xen/iommu.h:28,
                  from ./include/xen/sched.h:12,
                  from ./arch/x86/include/asm/paging.h:17,
                  from ./arch/x86/include/asm/guest_access.h:11,
                  from ./include/xen/guest_access.h:10,
                  from arch/x86/efi/runtime.c:5:
./include/acpi/actypes.h:130:35: error: conflicting types for ‘UINT64’; 
have ‘long long unsigned int’
   130 | typedef COMPILER_DEPENDENT_UINT64 UINT64;
       |                                   ^~~~~~
In file included from ./arch/x86/include/asm/efibind.h:2,
                  from ./common/efi/efi.h:1,
                  from arch/x86/efi/runtime.c:1:
./arch/x86/include/asm/x86_64/efibind.h:89:20: note: previous 
declaration of ‘UINT64’ with type ‘UINT64’ {aka ‘long unsigned int’}
    89 | typedef uint64_t   UINT64;
-- 
Mykyta

On 26.02.2025 10:58, Mykyta Poturai wrote:
> On 10.02.25 12:52, Jan Beulich wrote:
>> On 10.02.2025 11:30, Mykyta Poturai wrote:
>>> --- 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,18 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>>>       return 0;
>>>   }
>>>   
>>> +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;
>>> +}
>>
>> This function has no caller, which violates a Misra rule iirc. Considering
>> all information given here it's also unclear why it would gain a caller on
>> x86 (at least as long as DT isn't used there).
> 
> Would it be ok to wrap it with CONFIG_ARM? I am not quite sure how 
> relevant this mapping functionality is to X86 iommus, although Linux has 
> similar implementations for ACPI.

Besides it being unclear to me whether the function is really Arm-specific
(what about RISC-V or PPC), I also don't see how that would address the
Misra concern. (If the function was truly Arm-specific, it would better
move into an Arm-specific source file.)

> Alternatively, we can remove this abstraction for now, to call 
> iommu_add_dt_pci_sideband_ids from Arm directly and only introduce it 
> back when at least some ACPI implementation is done.

I'd leave that to Arm folks to judge.

> Also, just want to mention the issue that forced me to move this from 
> the header in the first place in case it is not known. There is a 
> conflict in fixed width integers definitions between actypes.h and 
> efibind.h and it was revealed when including acpi.h into iommu.h.
> I initially tried to fix the source of this conflict, but I don't know 
> enough about ACPI and EFI quirks to confidently do it.
> 
> In file included from ./include/acpi/acpi.h:57,
>                   from ./include/xen/acpi.h:57,
>                   from ./include/xen/iommu.h:28,
>                   from ./include/xen/sched.h:12,
>                   from ./arch/x86/include/asm/paging.h:17,
>                   from ./arch/x86/include/asm/guest_access.h:11,
>                   from ./include/xen/guest_access.h:10,
>                   from arch/x86/efi/runtime.c:5:
> ./include/acpi/actypes.h:130:35: error: conflicting types for ‘UINT64’; 
> have ‘long long unsigned int’
>    130 | typedef COMPILER_DEPENDENT_UINT64 UINT64;
>        |                                   ^~~~~~
> In file included from ./arch/x86/include/asm/efibind.h:2,
>                   from ./common/efi/efi.h:1,
>                   from arch/x86/efi/runtime.c:1:
> ./arch/x86/include/asm/x86_64/efibind.h:89:20: note: previous 
> declaration of ‘UINT64’ with type ‘UINT64’ {aka ‘long unsigned int’}
>     89 | typedef uint64_t   UINT64;

Yeah, sadly ACPI and EFI headers (both imported from different origins)
aren't overly compatible with one another.

Jan

On 26.02.25 12:48, Jan Beulich wrote:
> On 26.02.2025 10:58, Mykyta Poturai wrote:
>> On 10.02.25 12:52, Jan Beulich wrote:
>>> On 10.02.2025 11:30, Mykyta Poturai wrote:
>>>> --- 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,18 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>>>>        return 0;
>>>>    }
>>>>    
>>>> +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;
>>>> +}
>>>
>>> This function has no caller, which violates a Misra rule iirc. Considering
>>> all information given here it's also unclear why it would gain a caller on
>>> x86 (at least as long as DT isn't used there).
>>
>> Would it be ok to wrap it with CONFIG_ARM? I am not quite sure how
>> relevant this mapping functionality is to X86 iommus, although Linux has
>> similar implementations for ACPI.
> 
> Besides it being unclear to me whether the function is really Arm-specific
> (what about RISC-V or PPC), I also don't see how that would address the
> Misra concern. (If the function was truly Arm-specific, it would better
> move into an Arm-specific source file.)
> 
>> Alternatively, we can remove this abstraction for now, to call
>> iommu_add_dt_pci_sideband_ids from Arm directly and only introduce it
>> back when at least some ACPI implementation is done.
> 
> I'd leave that to Arm folks to judge.
> 
>> Also, just want to mention the issue that forced me to move this from
>> the header in the first place in case it is not known. There is a
>> conflict in fixed width integers definitions between actypes.h and
>> efibind.h and it was revealed when including acpi.h into iommu.h.
>> I initially tried to fix the source of this conflict, but I don't know
>> enough about ACPI and EFI quirks to confidently do it.
>>
>> In file included from ./include/acpi/acpi.h:57,
>>                    from ./include/xen/acpi.h:57,
>>                    from ./include/xen/iommu.h:28,
>>                    from ./include/xen/sched.h:12,
>>                    from ./arch/x86/include/asm/paging.h:17,
>>                    from ./arch/x86/include/asm/guest_access.h:11,
>>                    from ./include/xen/guest_access.h:10,
>>                    from arch/x86/efi/runtime.c:5:
>> ./include/acpi/actypes.h:130:35: error: conflicting types for ‘UINT64’;
>> have ‘long long unsigned int’
>>     130 | typedef COMPILER_DEPENDENT_UINT64 UINT64;
>>         |                                   ^~~~~~
>> In file included from ./arch/x86/include/asm/efibind.h:2,
>>                    from ./common/efi/efi.h:1,
>>                    from arch/x86/efi/runtime.c:1:
>> ./arch/x86/include/asm/x86_64/efibind.h:89:20: note: previous
>> declaration of ‘UINT64’ with type ‘UINT64’ {aka ‘long unsigned int’}
>>      89 | typedef uint64_t   UINT64;
> 
> Yeah, sadly ACPI and EFI headers (both imported from different origins)
> aren't overly compatible with one another.
> 
> Jan

Hi everyone
I searched for an appropriate place to put this function but I am a 
little stuck here:
- Can't be put in the header as static inline because of header conflict.
- Putting it in Arm only files or defines feels wrong because it is not 
in fact Arm-specific.
- In iommu.c it can become dead code on some architectures.
- Removing it and calling iommu_add_dt_pci_sideband_ids goes against the 
effort to make DT and ACPI able to co-exist.

Could you please suggest a good way forward from here? I see two 
possible ones:

1. Fix the header conflict and return it to the header as static inline 
- best solution in my opinion
2. Move to arm files it gains callers on other architectures.

If we are going for the first approach maybe you can provide some 
pointers on how to better deal with this header conflict? Adding ifdef 
guards to the definitions? Renaming the types in one of them to 
something like EFI_UINT64 or ACPI_UINT64? Would a successful boot on the 
ACPI/EFI system be enough to confirm that I didn't break anything or 
will it require some more specific tests?

-- 
Mykyta

On 28.02.2025 12:31, Mykyta Poturai wrote:
> 
> 
> On 26.02.25 12:48, Jan Beulich wrote:
>> On 26.02.2025 10:58, Mykyta Poturai wrote:
>>> On 10.02.25 12:52, Jan Beulich wrote:
>>>> On 10.02.2025 11:30, Mykyta Poturai wrote:
>>>>> --- 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,18 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>>>>>        return 0;
>>>>>    }
>>>>>    
>>>>> +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;
>>>>> +}
>>>>
>>>> This function has no caller, which violates a Misra rule iirc. Considering
>>>> all information given here it's also unclear why it would gain a caller on
>>>> x86 (at least as long as DT isn't used there).
>>>
>>> Would it be ok to wrap it with CONFIG_ARM? I am not quite sure how
>>> relevant this mapping functionality is to X86 iommus, although Linux has
>>> similar implementations for ACPI.
>>
>> Besides it being unclear to me whether the function is really Arm-specific
>> (what about RISC-V or PPC), I also don't see how that would address the
>> Misra concern. (If the function was truly Arm-specific, it would better
>> move into an Arm-specific source file.)
>>
>>> Alternatively, we can remove this abstraction for now, to call
>>> iommu_add_dt_pci_sideband_ids from Arm directly and only introduce it
>>> back when at least some ACPI implementation is done.
>>
>> I'd leave that to Arm folks to judge.
>>
>>> Also, just want to mention the issue that forced me to move this from
>>> the header in the first place in case it is not known. There is a
>>> conflict in fixed width integers definitions between actypes.h and
>>> efibind.h and it was revealed when including acpi.h into iommu.h.
>>> I initially tried to fix the source of this conflict, but I don't know
>>> enough about ACPI and EFI quirks to confidently do it.
>>>
>>> In file included from ./include/acpi/acpi.h:57,
>>>                    from ./include/xen/acpi.h:57,
>>>                    from ./include/xen/iommu.h:28,
>>>                    from ./include/xen/sched.h:12,
>>>                    from ./arch/x86/include/asm/paging.h:17,
>>>                    from ./arch/x86/include/asm/guest_access.h:11,
>>>                    from ./include/xen/guest_access.h:10,
>>>                    from arch/x86/efi/runtime.c:5:
>>> ./include/acpi/actypes.h:130:35: error: conflicting types for ‘UINT64’;
>>> have ‘long long unsigned int’
>>>     130 | typedef COMPILER_DEPENDENT_UINT64 UINT64;
>>>         |                                   ^~~~~~
>>> In file included from ./arch/x86/include/asm/efibind.h:2,
>>>                    from ./common/efi/efi.h:1,
>>>                    from arch/x86/efi/runtime.c:1:
>>> ./arch/x86/include/asm/x86_64/efibind.h:89:20: note: previous
>>> declaration of ‘UINT64’ with type ‘UINT64’ {aka ‘long unsigned int’}
>>>      89 | typedef uint64_t   UINT64;
>>
>> Yeah, sadly ACPI and EFI headers (both imported from different origins)
>> aren't overly compatible with one another.
>>
>> Jan
> 
> Hi everyone
> I searched for an appropriate place to put this function but I am a 
> little stuck here:
> - Can't be put in the header as static inline because of header conflict.
> - Putting it in Arm only files or defines feels wrong because it is not 
> in fact Arm-specific.
> - In iommu.c it can become dead code on some architectures.
> - Removing it and calling iommu_add_dt_pci_sideband_ids goes against the 
> effort to make DT and ACPI able to co-exist.
> 
> Could you please suggest a good way forward from here? I see two 
> possible ones:
> 
> 1. Fix the header conflict and return it to the header as static inline 
> - best solution in my opinion

I'm very likely to say "no" to anything trying to go this route. The
headers in question want leaving alone as much as possible, to stay
close to their originals. The only acceptable approach there would be
to propose adjusting said originals.

> 2. Move to arm files it gains callers on other architectures.

I fear I don't understand this one.

Jan

> If we are going for the first approach maybe you can provide some 
> pointers on how to better deal with this header conflict? Adding ifdef 
> guards to the definitions? Renaming the types in one of them to 
> something like EFI_UINT64 or ACPI_UINT64? Would a successful boot on the 
> ACPI/EFI system be enough to confirm that I didn't break anything or 
> will it require some more specific tests?
>