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.

v8->v9:
* replace DT_NO_IOMMU with 1
* guard iommu_add_pci_sideband_ids with CONFIG_ARM

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       | 15 +++++
 xen/include/xen/device_tree.h         | 23 +++++++
 xen/include/xen/iommu.h               | 40 +++++++++++-
 5 files changed, 210 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 4a1971c3fc..37e1437b65 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 1;
+
+    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) ? 1 : 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..dfaca67302 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,20 @@ int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
     return 0;
 }
 
+#ifdef CONFIG_ARM
+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
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 b928c67e19..82319016a1 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);
 
@@ -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 */
 
+/*
+ * This function is not strictly ARM-specific, but it is only used by ARM
+ * as of now. So wrap it with ifdef to avoid creating dead code on other
+ * architectures. When usage is extended to other architectures, this ifdef
+ * should be removed.
+ */
+#ifdef CONFIG_ARM
+/*
+ * 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);
+#endif
+
 struct page_info;
 
 /*
-- 
2.34.1

On 14.03.2025 14:34, Mykyta Poturai wrote:
> 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.
> 
> 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

On 17.03.25 16:56, Jan Beulich wrote:
> On 14.03.2025 14:34, Mykyta Poturai wrote:
>> 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.
>>
>> 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.

The original reason for moving this function was the conflicting ACPI
and EFI headers, I described it in V8 comments here[1].

>
>> --- 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.
>
> Jan

Here is a short summary:

The main problem is that we need this function somewhere, but there is
no good place for it. It is only called on ARM for now but it's not
ARM-specific by nature and can be eventually used on other platforms as
well. It can't be just dropped because of the effort to support the
co-existence of DT and ACPI. It also can't be declared as a static
function because it requires the inclusion of <xen/acpi.h> for
acpi_disabled define, which leads to build errors[1]. And without ifdef
guards it would be a MISRA violation.


[1]
https://patchew.org/Xen/cover.1739182214.git.mykyta._5Fpoturai@epam.com/67b9bd138c12c0df35e5c4b3137c30ad345097d5.1739182214.git.mykyta._5Fpoturai@epam.com/#e6ea52a3-c7ce-411f-8186-cf725aa608f3@epam.com
--
Mykyta

On 19.03.2025 16:21, Mykyta Poturai wrote:
> On 17.03.25 16:56, Jan Beulich wrote:
>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>> 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.
>>>
>>> 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.
> 
> The original reason for moving this function was the conflicting ACPI
> and EFI headers, I described it in V8 comments here[1].
> 
>>
>>> --- 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.
> 
> Here is a short summary:
> 
> The main problem is that we need this function somewhere, but there is
> no good place for it. It is only called on ARM for now but it's not
> ARM-specific by nature and can be eventually used on other platforms as
> well. It can't be just dropped because of the effort to support the
> co-existence of DT and ACPI. It also can't be declared as a static
> function because it requires the inclusion of <xen/acpi.h> for
> acpi_disabled define, which leads to build errors[1]. And without ifdef
> guards it would be a MISRA violation.

An abridged version of this ought to go in the patch description, I think.
This is special, so it needs calling out.

As to the placement - would making an entirely new .c file possibly help?
(Then, instead of in the patch description, maybe the special aspect could
be put in a code comment at the top of the file.)

Jan

On 19.03.25 17:28, Jan Beulich wrote:
> On 19.03.2025 16:21, Mykyta Poturai wrote:
>> On 17.03.25 16:56, Jan Beulich wrote:
>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>>> 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.
>>>>
>>>> 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.
>>
>> The original reason for moving this function was the conflicting ACPI
>> and EFI headers, I described it in V8 comments here[1].
>>
>>>
>>>> --- 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.
>>
>> Here is a short summary:
>>
>> The main problem is that we need this function somewhere, but there is
>> no good place for it. It is only called on ARM for now but it's not
>> ARM-specific by nature and can be eventually used on other platforms as
>> well. It can't be just dropped because of the effort to support the
>> co-existence of DT and ACPI. It also can't be declared as a static
>> function because it requires the inclusion of <xen/acpi.h> for
>> acpi_disabled define, which leads to build errors[1]. And without ifdef
>> guards it would be a MISRA violation.
> 
> An abridged version of this ought to go in the patch description, I think.
> This is special, so it needs calling out.
> 
> As to the placement - would making an entirely new .c file possibly help?
> (Then, instead of in the patch description, maybe the special aspect could
> be put in a code comment at the top of the file.)
> 
> Jan

It seems to me creating a new file would be overkill for one small 
function. I considered moving it to xen/drivers/passthrough/arm/iommu.c 
to reduce ifdefs but I feared it would suggest that it is arch-specific 
a bit too strongly. So maybe move it there after all if you think it 
would be better?

-- 
Mykyta

On 20.03.2025 11:47, Mykyta Poturai wrote:
> On 19.03.25 17:28, Jan Beulich wrote:
>> On 19.03.2025 16:21, Mykyta Poturai wrote:
>>> On 17.03.25 16:56, Jan Beulich wrote:
>>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>>>> 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.
>>>>>
>>>>> 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.
>>>
>>> The original reason for moving this function was the conflicting ACPI
>>> and EFI headers, I described it in V8 comments here[1].
>>>
>>>>
>>>>> --- 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.
>>>
>>> Here is a short summary:
>>>
>>> The main problem is that we need this function somewhere, but there is
>>> no good place for it. It is only called on ARM for now but it's not
>>> ARM-specific by nature and can be eventually used on other platforms as
>>> well. It can't be just dropped because of the effort to support the
>>> co-existence of DT and ACPI. It also can't be declared as a static
>>> function because it requires the inclusion of <xen/acpi.h> for
>>> acpi_disabled define, which leads to build errors[1]. And without ifdef
>>> guards it would be a MISRA violation.
>>
>> An abridged version of this ought to go in the patch description, I think.
>> This is special, so it needs calling out.
>>
>> As to the placement - would making an entirely new .c file possibly help?
>> (Then, instead of in the patch description, maybe the special aspect could
>> be put in a code comment at the top of the file.)
> 
> It seems to me creating a new file would be overkill for one small 
> function. I considered moving it to xen/drivers/passthrough/arm/iommu.c 
> to reduce ifdefs but I feared it would suggest that it is arch-specific 
> a bit too strongly. So maybe move it there after all if you think it 
> would be better?

Well - with "#ifdef CONFIG_ARM" around it's Arm-specific too, isn't it?
IOW - either have a proper (even if simple) abstraction, or perhaps indeed
put it there (if that's also okay with the Arm maintainers).

Jan

On Thu, 20 Mar 2025, Jan Beulich wrote:
> On 20.03.2025 11:47, Mykyta Poturai wrote:
> > On 19.03.25 17:28, Jan Beulich wrote:
> >> On 19.03.2025 16:21, Mykyta Poturai wrote:
> >>> On 17.03.25 16:56, Jan Beulich wrote:
> >>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
> >>>>> 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.
> >>>>>
> >>>>> 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.
> >>>
> >>> The original reason for moving this function was the conflicting ACPI
> >>> and EFI headers, I described it in V8 comments here[1].
> >>>
> >>>>
> >>>>> --- 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.
> >>>
> >>> Here is a short summary:
> >>>
> >>> The main problem is that we need this function somewhere, but there is
> >>> no good place for it. It is only called on ARM for now but it's not
> >>> ARM-specific by nature and can be eventually used on other platforms as
> >>> well. It can't be just dropped because of the effort to support the
> >>> co-existence of DT and ACPI. It also can't be declared as a static
> >>> function because it requires the inclusion of <xen/acpi.h> for
> >>> acpi_disabled define, which leads to build errors[1]. And without ifdef
> >>> guards it would be a MISRA violation.
> >>
> >> An abridged version of this ought to go in the patch description, I think.
> >> This is special, so it needs calling out.
> >>
> >> As to the placement - would making an entirely new .c file possibly help?
> >> (Then, instead of in the patch description, maybe the special aspect could
> >> be put in a code comment at the top of the file.)
> > 
> > It seems to me creating a new file would be overkill for one small 
> > function. I considered moving it to xen/drivers/passthrough/arm/iommu.c 
> > to reduce ifdefs but I feared it would suggest that it is arch-specific 
> > a bit too strongly. So maybe move it there after all if you think it 
> > would be better?
> 
> Well - with "#ifdef CONFIG_ARM" around it's Arm-specific too, isn't it?
> IOW - either have a proper (even if simple) abstraction, or perhaps indeed
> put it there (if that's also okay with the Arm maintainers).

I am OK with that