[PATCH v4 0/7] SMMU handling for PCIe Passthrough on ARM

[PATCH v4 0/7] SMMU handling for PCIe Passthrough on ARM

[Stewart Hildebrand]

This series introduces SMMU handling for PCIe passthrough on ARM. These patches
are independent from (and don't depend on) the vPCI reference counting/locking
work in progress, and should be able to be upstreamed independently.

v3->v4:
* split a change from ("xen/arm: Move is_protected flag to struct device") into
  a new separate patch
* see individual patches for further details

v2->v3:
* drop "pci/arm: Use iommu_add_dt_pci_device()"
* drop "RFC: pci/arm: don't do iommu call for phantom functions"
* move invocation of sideband ID mapping function to add_device()
  platform_ops/iommu_ops hook

v1->v2:
* phantom device handling
* shuffle around iommu_add_dt_pci_device()

Oleksandr Andrushchenko (1):
  xen/arm: smmuv2: Add PCI devices support for SMMUv2

Oleksandr Tyshchenko (4):
  xen/arm: Improve readability of check for registered devices
  xen/arm: Move is_protected flag to struct device
  iommu/arm: Add iommu_dt_xlate()
  iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

Rahul Singh (1):
  xen/arm: smmuv3: Add PCI devices support for SMMUv3

Stewart Hildebrand (1):
  iommu/arm: iommu_add_dt_pci_sideband_ids phantom handling

 xen/arch/arm/domain_build.c              |   4 +-
 xen/arch/arm/include/asm/device.h        |  14 ++
 xen/common/device_tree.c                 |   2 +-
 xen/drivers/passthrough/arm/ipmmu-vmsa.c |   4 +-
 xen/drivers/passthrough/arm/smmu-v3.c    |  81 ++++++++-
 xen/drivers/passthrough/arm/smmu.c       | 122 +++++++++++---
 xen/drivers/passthrough/device_tree.c    | 205 ++++++++++++++++++++---
 xen/include/xen/device_tree.h            |  38 +++--
 xen/include/xen/iommu.h                  |  22 ++-
 9 files changed, 423 insertions(+), 69 deletions(-)

-- 
2.40.1

[PATCH v4 1/7] xen/arm: Improve readability of check for registered devices

[Stewart Hildebrand]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Improve readability of check for devices already registered with the SMMU with
legacy mmu-masters DT bindings by using is_protected.

There are 2 device tree bindings for registering a device with the SMMU:
* mmu-masters (legacy, SMMUv1/2 only)
* iommus

A device tree may include both mmu-masters and iommus properties (although it is
unnecessary to do so). When a device appears in the mmu-masters list,
np->is_protected and dev->iommu_fwspec both get set by the SMMUv1/2 driver. The
function iommu_add_dt_device() is subsequently invoked for devices that have an
iommus specification.

The check as it was before this patch:

  if ( dev_iommu_fwspec_get(dev) )
      return 0;

and the new check:

  if ( dt_device_is_protected(np) )
      return 0;

are guarding against the same corner case: when a device has both mmu-masters
and iommus specifications in the device tree. The is_protected naming is more
descriptive.

If np->is_protected is not set (i.e. false), but dev->iommu_fwspec is set, it is
an error condition, so return an error in this case.

Expand the comment to further clarify the corner case.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v3->v4:
* new patch: this change was split from ("xen/arm: Move is_protected flag to struct device")
---
 xen/drivers/passthrough/device_tree.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 1c32d7b50cce..d9b63da7260a 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -141,12 +141,17 @@ int iommu_add_dt_device(struct dt_device_node *np)
         return -EINVAL;
 
     /*
-     * The device may already have been registered. As there is no harm in
-     * it just return success early.
+     * Devices that appear in the legacy mmu-masters list may have already been
+     * registered with the SMMU. In case a device has both a mmu-masters entry
+     * and iommus property, there is no need to register it again. In this case
+     * simply return success early.
      */
-    if ( dev_iommu_fwspec_get(dev) )
+    if ( dt_device_is_protected(np) )
         return 0;
 
+    if ( dev_iommu_fwspec_get(dev) )
+        return -EEXIST;
+
     /*
      * According to the Documentation/devicetree/bindings/iommu/iommu.txt
      * from Linux.
-- 
2.40.1

[PATCH v4 2/7] xen/arm: Move is_protected flag to struct device

[Stewart Hildebrand]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

This flag will be re-used for PCI devices by the subsequent
patches.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v3->v4:
* move is_protected flag within struct device to reduce padding
* re-add device_is_protected checks in add_device hooks in smmu-v3.c/ipmmu-vmsa.c
* split mmu-masters check into separate patch

v2->v3:
* no change

v1->v2:
* no change

downstream->v1:
* rebase
* s/dev_node->is_protected/dev_node->dev.is_protected/ in smmu.c
* s/dt_device_set_protected(dev_to_dt(dev))/device_set_protected(dev)/ in smmu-v3.c
* remove redundant device_is_protected checks in smmu-v3.c/ipmmu-vmsa.c

(cherry picked from commit 59753aac77528a584d3950936b853ebf264b68e7 from
 the downstream branch poc/pci-passthrough from
 https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
---
 xen/arch/arm/domain_build.c              |  4 ++--
 xen/arch/arm/include/asm/device.h        | 14 ++++++++++++++
 xen/common/device_tree.c                 |  2 +-
 xen/drivers/passthrough/arm/ipmmu-vmsa.c |  4 ++--
 xen/drivers/passthrough/arm/smmu-v3.c    |  5 +++--
 xen/drivers/passthrough/arm/smmu.c       |  2 +-
 xen/drivers/passthrough/device_tree.c    |  8 ++++----
 xen/include/xen/device_tree.h            | 13 -------------
 8 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3f4558ade67f..b229bfaae712 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2524,7 +2524,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
             return res;
         }
 
-        if ( dt_device_is_protected(dev) )
+        if ( device_is_protected(dt_to_dev(dev)) )
         {
             dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
             res = iommu_assign_dt_device(d, dev);
@@ -3024,7 +3024,7 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
         return res;
 
     /* If xen_force, we allow assignment of devices without IOMMU protection. */
-    if ( xen_force && !dt_device_is_protected(node) )
+    if ( xen_force && !device_is_protected(dt_to_dev(node)) )
         return 0;
 
     return iommu_assign_dt_device(kinfo->d, node);
diff --git a/xen/arch/arm/include/asm/device.h b/xen/arch/arm/include/asm/device.h
index b5d451e08776..8ac807482737 100644
--- a/xen/arch/arm/include/asm/device.h
+++ b/xen/arch/arm/include/asm/device.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_ARM_DEVICE_H
 #define __ASM_ARM_DEVICE_H
 
+#include <xen/types.h>
+
 enum device_type
 {
     DEV_DT,
@@ -15,6 +17,8 @@ struct dev_archdata {
 struct device
 {
     enum device_type type;
+    bool is_protected; /* Shows that device is protected by IOMMU */
+    uint8_t _pad[3];
 #ifdef CONFIG_HAS_DEVICE_TREE
     struct dt_device_node *of_node; /* Used by drivers imported from Linux */
 #endif
@@ -94,6 +98,16 @@ int device_init(struct dt_device_node *dev, enum device_class class,
  */
 enum device_class device_get_class(const struct dt_device_node *dev);
 
+static inline void device_set_protected(struct device *device)
+{
+    device->is_protected = true;
+}
+
+static inline bool device_is_protected(const struct device *device)
+{
+    return device->is_protected;
+}
+
 #define DT_DEVICE_START(_name, _namestr, _class)                    \
 static const struct device_desc __dev_desc_##_name __used           \
 __section(".dev.info") = {                                          \
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 8da105291184..7444da3e0aa5 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -1910,7 +1910,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
         /* By default dom0 owns the device */
         np->used_by = 0;
         /* By default the device is not protected */
-        np->is_protected = false;
+        np->dev.is_protected = false;
         INIT_LIST_HEAD(&np->domain_list);
 
         if ( new_format )
diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
index 611d9eeba5c3..a71fd76d89a3 100644
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -1288,14 +1288,14 @@ static int ipmmu_add_device(u8 devfn, struct device *dev)
     if ( !to_ipmmu(dev) )
         return -ENODEV;
 
-    if ( dt_device_is_protected(dev_to_dt(dev)) )
+    if ( device_is_protected(dev) )
     {
         dev_err(dev, "Already added to IPMMU\n");
         return -EEXIST;
     }
 
     /* Let Xen know that the master device is protected by an IOMMU. */
-    dt_device_set_protected(dev_to_dt(dev));
+    device_set_protected(dev);
 
     dev_info(dev, "Added master device (IPMMU %s micro-TLBs %u)\n",
              dev_name(fwspec->iommu_dev), fwspec->num_ids);
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index 720aa69ff23e..8842db1ec07e 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -1521,13 +1521,14 @@ static int arm_smmu_add_device(u8 devfn, struct device *dev)
 	 */
 	arm_smmu_enable_pasid(master);
 
-	if (dt_device_is_protected(dev_to_dt(dev))) {
+	if ( device_is_protected(dev) )
+	{
 		dev_err(dev, "Already added to SMMUv3\n");
 		return -EEXIST;
 	}
 
 	/* Let Xen know that the master device is protected by an IOMMU. */
-	dt_device_set_protected(dev_to_dt(dev));
+	device_set_protected(dev);
 
 	dev_info(dev, "Added master device (SMMUv3 %s StreamIds %u)\n",
 			dev_name(fwspec->iommu_dev), fwspec->num_ids);
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index c37fa9af1366..d874417958b5 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -837,7 +837,7 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
 	master->of_node = dev_node;
 
 	/* Xen: Let Xen know that the device is protected by an SMMU */
-	dt_device_set_protected(dev_node);
+	device_set_protected(dev);
 
 	for (i = 0; i < fwspec->num_ids; ++i) {
 		if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index d9b63da7260a..c60e78eaf556 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -34,7 +34,7 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
     if ( !is_iommu_enabled(d) )
         return -EINVAL;
 
-    if ( !dt_device_is_protected(dev) )
+    if ( !device_is_protected(dt_to_dev(dev)) )
         return -EINVAL;
 
     spin_lock(&dtdevs_lock);
@@ -65,7 +65,7 @@ int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev)
     if ( !is_iommu_enabled(d) )
         return -EINVAL;
 
-    if ( !dt_device_is_protected(dev) )
+    if ( !device_is_protected(dt_to_dev(dev)) )
         return -EINVAL;
 
     spin_lock(&dtdevs_lock);
@@ -87,7 +87,7 @@ static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
 {
     bool_t assigned = 0;
 
-    if ( !dt_device_is_protected(dev) )
+    if ( !device_is_protected(dt_to_dev(dev)) )
         return 0;
 
     spin_lock(&dtdevs_lock);
@@ -146,7 +146,7 @@ int iommu_add_dt_device(struct dt_device_node *np)
      * and iommus property, there is no need to register it again. In this case
      * simply return success early.
      */
-    if ( dt_device_is_protected(np) )
+    if ( device_is_protected(dev) )
         return 0;
 
     if ( dev_iommu_fwspec_get(dev) )
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index c2eada748915..c2f315140560 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -90,9 +90,6 @@ struct dt_device_node {
     struct dt_device_node *next; /* TODO: Remove it. Only use to know the last children */
     struct dt_device_node *allnext;
 
-    /* IOMMU specific fields */
-    bool is_protected;
-
     /* HACK: Remove this if there is a need of space */
     bool_t static_evtchn_created;
 
@@ -329,16 +326,6 @@ static inline domid_t dt_device_used_by(const struct dt_device_node *device)
     return device->used_by;
 }
 
-static inline void dt_device_set_protected(struct dt_device_node *device)
-{
-    device->is_protected = true;
-}
-
-static inline bool dt_device_is_protected(const struct dt_device_node *device)
-{
-    return device->is_protected;
-}
-
 static inline bool_t dt_property_name_is_equal(const struct dt_property *pp,
                                                const char *name)
 {
-- 
2.40.1

[PATCH v4 3/7] iommu/arm: Add iommu_dt_xlate()

[Stewart Hildebrand]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Move code for processing DT IOMMU specifier to a separate helper.
This helper will be re-used for adding PCI devices by the subsequent
patches as we will need exact the same actions for processing
DT PCI-IOMMU specifier.

While at it introduce NO_IOMMU to avoid magic "1".

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v3->v4:
* make dt_phandle_args *iommu_spec const
* move !ops->add_device check to helper

v2->v3:
* no change

v1->v2:
* no change

downstream->v1:
* trivial rebase
* s/dt_iommu_xlate/iommu_dt_xlate/

(cherry picked from commit c26bab0415ca303df86aba1d06ef8edc713734d3 from
 the downstream branch poc/pci-passthrough from
 https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
---
 xen/drivers/passthrough/device_tree.c | 47 ++++++++++++++++++---------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index c60e78eaf556..ff9e66ebf92a 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -127,15 +127,42 @@ int iommu_release_dt_devices(struct domain *d)
     return 0;
 }
 
+/* This correlation must not be altered */
+#define NO_IOMMU    1
+
+static int iommu_dt_xlate(struct device *dev,
+                          const struct dt_phandle_args *iommu_spec)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+    int rc;
+
+    if ( !ops->dt_xlate )
+        return -EINVAL;
+
+    if ( !dt_device_is_available(iommu_spec->np) )
+        return NO_IOMMU;
+
+    rc = iommu_fwspec_init(dev, &iommu_spec->np->dev);
+    if ( rc )
+        return rc;
+
+    /*
+     * Provide DT IOMMU specifier which describes the IOMMU master
+     * interfaces of that device (device IDs, etc) to the driver.
+     * The driver is responsible to decide how to interpret them.
+     */
+    return ops->dt_xlate(dev, iommu_spec);
+}
+
 int iommu_add_dt_device(struct dt_device_node *np)
 {
     const struct iommu_ops *ops = iommu_get_ops();
     struct dt_phandle_args iommu_spec;
     struct device *dev = dt_to_dev(np);
-    int rc = 1, index = 0;
+    int rc = NO_IOMMU, index = 0;
 
     if ( !iommu_enabled )
-        return 1;
+        return NO_IOMMU;
 
     if ( !ops )
         return -EINVAL;
@@ -163,22 +190,10 @@ int iommu_add_dt_device(struct dt_device_node *np)
          * The driver which supports generic IOMMU DT bindings must have
          * these callback implemented.
          */
-        if ( !ops->add_device || !ops->dt_xlate )
+        if ( !ops->add_device )
             return -EINVAL;
 
-        if ( !dt_device_is_available(iommu_spec.np) )
-            break;
-
-        rc = iommu_fwspec_init(dev, &iommu_spec.np->dev);
-        if ( rc )
-            break;
-
-        /*
-         * Provide DT IOMMU specifier which describes the IOMMU master
-         * interfaces of that device (device IDs, etc) to the driver.
-         * The driver is responsible to decide how to interpret them.
-         */
-        rc = ops->dt_xlate(dev, &iommu_spec);
+        rc = iommu_dt_xlate(dev, &iommu_spec);
         if ( rc )
             break;
 
-- 
2.40.1

[PATCH v4 4/7] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

[Stewart Hildebrand]

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 of_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>
---
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/drivers/passthrough/device_tree.c | 134 ++++++++++++++++++++++++++
 xen/include/xen/device_tree.h         |  25 +++++
 xen/include/xen/iommu.h               |  22 ++++-
 3 files changed, 180 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index ff9e66ebf92a..bd0aed5df651 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -154,6 +154,140 @@ static int iommu_dt_xlate(struct device *dev,
     return ops->dt_xlate(dev, iommu_spec);
 }
 
+#ifdef CONFIG_HAS_PCI
+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: Error: Bad %s length: %u\n", np->full_name,
+               map_name, map_len);
+        return -EINVAL;
+    }
+
+    /* The default is to select all bits. */
+    map_mask = 0xffffffff;
+
+    /*
+     * Can be overridden by "{iommu,msi}-map-mask" property.
+     * If df_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 ( ; (int)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: Invalid %s translation - %s-mask (0x%"PRIx32") ignores id-base (0x%"PRIx32")\n",
+                   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;
+
+        printk(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;
+    }
+
+    printk(XENLOG_ERR "%s: no %s translation for id 0x%"PRIx32" on %s\n",
+           np->full_name, map_name, id, (target && *target) ? (*target)->full_name : NULL);
+
+    /*
+     * NOTE: Linux bypasses translation without returning an error here,
+     * but should we behave in the same way on Xen? Restrict for now.
+     */
+    return -EFAULT;
+}
+
+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 = NO_IOMMU;
+
+    if ( !iommu_enabled )
+        return NO_IOMMU;
+
+    if ( !ops )
+        return -EINVAL;
+
+    if ( device_is_protected(dev) )
+        return 0;
+
+    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) ? NO_IOMMU : rc;
+
+    rc = iommu_dt_xlate(dev, &iommu_spec);
+    if ( rc < 0 )
+    {
+        iommu_fwspec_free(dev);
+        return -EINVAL;
+    }
+
+    return rc;
+}
+#endif /* CONFIG_HAS_PCI */
+
 int iommu_add_dt_device(struct dt_device_node *np)
 {
     const struct iommu_ops *ops = iommu_get_ops();
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index c2f315140560..8385cd538a58 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -892,6 +892,31 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
  */
 int dt_get_pci_domain_nr(struct dt_device_node *node);
 
+#ifdef CONFIG_HAS_PCI
+/**
+ * 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);
+#endif /* CONFIG_HAS_PCI */
+
 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 405db59971c5..3cac177840f7 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -26,6 +26,9 @@
 #include <xen/spinlock.h>
 #include <public/domctl.h>
 #include <public/hvm/ioreq.h>
+#ifdef CONFIG_ACPI
+#include <asm/acpi.h>
+#endif
 #include <asm/device.h>
 
 TYPE_SAFE(uint64_t, dfn);
@@ -219,7 +222,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
@@ -228,12 +232,28 @@ int iommu_release_dt_devices(struct domain *d);
  *      (IOMMU is not enabled/present or device is not connected to it).
  */
 int iommu_add_dt_device(struct dt_device_node *np);
+int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev);
 
 int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
                        XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
 
+#else /* !HAS_DEVICE_TREE */
+static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
+{
+    return 0;
+}
 #endif /* HAS_DEVICE_TREE */
 
+static inline int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
+{
+    int ret = 0;
+#ifdef CONFIG_ACPI
+    if ( acpi_disabled )
+#endif
+        ret = iommu_add_dt_pci_sideband_ids(pdev);
+    return ret;
+}
+
 struct page_info;
 
 /*
-- 
2.40.1

[PATCH v4 5/7] iommu/arm: iommu_add_dt_pci_sideband_ids phantom handling

[Stewart Hildebrand]

Handle phantom functions in iommu_add_dt_pci_sideband_ids(). Each phantom
function will have a unique requestor ID (RID)/BDF. On ARM, we need to
map/translate the RID/BDF to an AXI stream ID for each phantom function
according to the pci-iommu device tree mapping [1]. The RID/BDF -> AXI stream ID
mapping in DT could allow phantom devices (i.e. devices with phantom functions)
to use different AXI stream IDs based on the (phantom) function.

[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v3->v4:
* s/iommu_dt_pci_map_id/dt_map_id/

v2->v3:
* new patch title (was: iommu/arm: iommu_add_dt_pci_device phantom handling)
* rework loop to reduce duplication
* s/iommu_fwspec_free(pci_to_dev(pdev))/iommu_fwspec_free(dev)/

v1->v2:
* new patch
---
 xen/drivers/passthrough/device_tree.c | 33 ++++++++++++++++-----------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index bd0aed5df651..b7de0175ec7e 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -251,6 +251,7 @@ int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
     struct device *dev = pci_to_dev(pdev);
     const struct dt_device_node *np;
     int rc = NO_IOMMU;
+    unsigned int devfn = pdev->devfn;
 
     if ( !iommu_enabled )
         return NO_IOMMU;
@@ -268,21 +269,27 @@ int iommu_add_dt_pci_sideband_ids(struct pci_dev *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) ? NO_IOMMU : rc;
+    do {
+        /*
+         * According to the Documentation/devicetree/bindings/pci/pci-iommu.txt
+         * from Linux.
+         */
+        rc = dt_map_id(np, PCI_BDF(pdev->bus, devfn), "iommu-map",
+                       "iommu-map-mask", &iommu_spec.np, iommu_spec.args);
+        if ( rc )
+            return (rc == -ENODEV) ? NO_IOMMU : rc;
 
-    rc = iommu_dt_xlate(dev, &iommu_spec);
-    if ( rc < 0 )
-    {
-        iommu_fwspec_free(dev);
-        return -EINVAL;
+        rc = iommu_dt_xlate(dev, &iommu_spec);
+        if ( rc < 0 )
+        {
+            iommu_fwspec_free(dev);
+            return -EINVAL;
+        }
+
+        devfn += pdev->phantom_stride;
     }
+    while ( (devfn != pdev->devfn) &&
+            (PCI_SLOT(devfn) == PCI_SLOT(pdev->devfn)) );
 
     return rc;
 }
-- 
2.40.1

[PATCH v4 6/7] xen/arm: smmuv2: Add PCI devices support for SMMUv2

[Stewart Hildebrand]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v3->v4:
* add new device_is_protected check in add_device hook to match SMMUv3 and
  IPMMU-VMSA drivers

v2->v3:
* invoke iommu_add_pci_sideband_ids() from add_device hook

v1->v2:
* ignore add_device/assign_device/reassign_device calls for phantom functions
  (i.e. devfn != pdev->devfn)

downstream->v1:
* wrap unused function in #ifdef 0
* remove the remove_device() stub since it was submitted separately to the list
  [XEN][PATCH v6 12/19] xen/smmu: Add remove_device callback for smmu_iommu ops
  https://lists.xenproject.org/archives/html/xen-devel/2023-05/msg00204.html
* arm_smmu_(de)assign_dev: return error instead of crashing system
* update condition in arm_smmu_reassign_dev
* style fixup
* add && !is_hardware_domain(d) into condition in arm_smmu_assign_dev()

(cherry picked from commit 0c11a7f65f044c26d87d1e27ac6283ef1f9cfb7a from
 the downstream branch spider-master from
 https://github.com/xen-troops/xen.git)
---

This is a file imported from Linux with modifications for Xen. What should be
the coding style for Xen modifications?
---
 xen/drivers/passthrough/arm/smmu.c | 120 ++++++++++++++++++++++++-----
 1 file changed, 99 insertions(+), 21 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index d874417958b5..edb5345fb1cd 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -134,8 +134,20 @@ typedef enum irqreturn irqreturn_t;
 /* Device logger functions
  * TODO: Handle PCI
  */
-#define dev_print(dev, lvl, fmt, ...)						\
-	 printk(lvl "smmu: %s: " fmt, dt_node_full_name(dev_to_dt(dev)), ## __VA_ARGS__)
+#ifndef CONFIG_HAS_PCI
+#define dev_print(dev, lvl, fmt, ...)    \
+    printk(lvl "smmu: %s: " fmt, dev_name(dev), ## __VA_ARGS__)
+#else
+#define dev_print(dev, lvl, fmt, ...) ({                                \
+    if ( !dev_is_pci((dev)) )                                           \
+        printk(lvl "smmu: %s: " fmt, dev_name((dev)), ## __VA_ARGS__);  \
+    else                                                                \
+    {                                                                   \
+        struct pci_dev *pdev = dev_to_pci((dev));                       \
+        printk(lvl "smmu: %pp: " fmt, &pdev->sbdf, ## __VA_ARGS__);     \
+    }                                                                   \
+})
+#endif
 
 #define dev_dbg(dev, fmt, ...) dev_print(dev, XENLOG_DEBUG, fmt, ## __VA_ARGS__)
 #define dev_notice(dev, fmt, ...) dev_print(dev, XENLOG_INFO, fmt, ## __VA_ARGS__)
@@ -187,6 +199,7 @@ static void __iomem *devm_ioremap_resource(struct device *dev,
  * Xen: PCI functions
  * TODO: It should be implemented when PCI will be supported
  */
+#if 0 /* unused */
 #define to_pci_dev(dev)	(NULL)
 static inline int pci_for_each_dma_alias(struct pci_dev *pdev,
 					 int (*fn) (struct pci_dev *pdev,
@@ -196,6 +209,7 @@ static inline int pci_for_each_dma_alias(struct pci_dev *pdev,
 	BUG();
 	return 0;
 }
+#endif
 
 /* Xen: misc */
 #define PHYS_MASK_SHIFT		PADDR_BITS
@@ -631,7 +645,7 @@ struct arm_smmu_master_cfg {
 	for (i = 0; idx = cfg->smendx[i], i < num; ++i)
 
 struct arm_smmu_master {
-	struct device_node		*of_node;
+	struct device			*dev;
 	struct rb_node			node;
 	struct arm_smmu_master_cfg	cfg;
 };
@@ -723,7 +737,7 @@ arm_smmu_get_fwspec(struct arm_smmu_master_cfg *cfg)
 {
 	struct arm_smmu_master *master = container_of(cfg,
 			                                      struct arm_smmu_master, cfg);
-	return dev_iommu_fwspec_get(&master->of_node->dev);
+	return dev_iommu_fwspec_get(master->dev);
 }
 
 static void parse_driver_options(struct arm_smmu_device *smmu)
@@ -756,7 +770,7 @@ static struct device_node *dev_get_dev_node(struct device *dev)
 }
 
 static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
-						struct device_node *dev_node)
+						struct device *dev)
 {
 	struct rb_node *node = smmu->masters.rb_node;
 
@@ -765,9 +779,9 @@ static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
 
 		master = container_of(node, struct arm_smmu_master, node);
 
-		if (dev_node < master->of_node)
+		if (dev < master->dev)
 			node = node->rb_left;
-		else if (dev_node > master->of_node)
+		else if (dev > master->dev)
 			node = node->rb_right;
 		else
 			return master;
@@ -802,9 +816,9 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 			= container_of(*new, struct arm_smmu_master, node);
 
 		parent = *new;
-		if (master->of_node < this->of_node)
+		if (master->dev < this->dev)
 			new = &((*new)->rb_left);
-		else if (master->of_node > this->of_node)
+		else if (master->dev > this->dev)
 			new = &((*new)->rb_right);
 		else
 			return -EEXIST;
@@ -823,18 +837,24 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
 	struct arm_smmu_master *master;
 	struct device_node *dev_node = dev_get_dev_node(dev);
 
-	master = find_smmu_master(smmu, dev_node);
+	master = find_smmu_master(smmu, dev);
 	if (master) {
 		dev_err(dev,
 			"rejecting multiple registrations for master device %s\n",
-			dev_node->name);
+			dev_node ? dev_node->name : "");
 		return -EBUSY;
 	}
 
 	master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
 	if (!master)
 		return -ENOMEM;
-	master->of_node = dev_node;
+	master->dev = dev;
+
+	if ( device_is_protected(dev) )
+	{
+		dev_err(dev, "Already added to SMMU\n");
+		return -EEXIST;
+	}
 
 	/* Xen: Let Xen know that the device is protected by an SMMU */
 	device_set_protected(dev);
@@ -844,7 +864,7 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
 		     (fwspec->ids[i] >= smmu->num_mapping_groups)) {
 			dev_err(dev,
 				"stream ID for master device %s greater than maximum allowed (%d)\n",
-				dev_node->name, smmu->num_mapping_groups);
+				dev_node ? dev_node->name : "", smmu->num_mapping_groups);
 			return -ERANGE;
 		}
 		master->cfg.smendx[i] = INVALID_SMENDX;
@@ -880,6 +900,21 @@ static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
 	struct arm_smmu_device *smmu;
 	struct iommu_fwspec *fwspec;
 
+#ifdef CONFIG_HAS_PCI
+	if ( dev_is_pci(dev) )
+	{
+		struct pci_dev *pdev = dev_to_pci(dev);
+		int ret;
+
+		if ( devfn != pdev->devfn )
+			return 0;
+
+		ret = iommu_add_pci_sideband_ids(pdev);
+		if ( ret < 0 )
+			iommu_fwspec_free(dev);
+	}
+#endif
+
 	fwspec = dev_iommu_fwspec_get(dev);
 	if (fwspec == NULL)
 		return -ENXIO;
@@ -911,11 +946,10 @@ static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
 {
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_master *master = NULL;
-	struct device_node *dev_node = dev_get_dev_node(dev);
 
 	spin_lock(&arm_smmu_devices_lock);
 	list_for_each_entry(smmu, &arm_smmu_devices, list) {
-		master = find_smmu_master(smmu, dev_node);
+		master = find_smmu_master(smmu, dev);
 		if (master)
 			break;
 	}
@@ -2007,6 +2041,7 @@ static bool arm_smmu_capable(enum iommu_cap cap)
 }
 #endif
 
+#if 0 /* Not used */
 static int __arm_smmu_get_pci_sid(struct pci_dev *pdev, u16 alias, void *data)
 {
 	*((u16 *)data) = alias;
@@ -2017,6 +2052,7 @@ static void __arm_smmu_release_pci_iommudata(void *data)
 {
 	kfree(data);
 }
+#endif
 
 static int arm_smmu_add_device(struct device *dev)
 {
@@ -2024,12 +2060,13 @@ static int arm_smmu_add_device(struct device *dev)
 	struct arm_smmu_master_cfg *cfg;
 	struct iommu_group *group;
 	void (*releasefn)(void *) = NULL;
-	int ret;
 
 	smmu = find_smmu_for_device(dev);
 	if (!smmu)
 		return -ENODEV;
 
+	/* There is no need to distinguish here, thanks to PCI-IOMMU DT bindings */
+#if 0
 	if (dev_is_pci(dev)) {
 		struct pci_dev *pdev = to_pci_dev(dev);
 		struct iommu_fwspec *fwspec;
@@ -2054,10 +2091,12 @@ static int arm_smmu_add_device(struct device *dev)
 				       &fwspec->ids[0]);
 		releasefn = __arm_smmu_release_pci_iommudata;
 		cfg->smmu = smmu;
-	} else {
+	} else
+#endif
+	{
 		struct arm_smmu_master *master;
 
-		master = find_smmu_master(smmu, dev->of_node);
+		master = find_smmu_master(smmu, dev);
 		if (!master) {
 			return -ENODEV;
 		}
@@ -2725,6 +2764,27 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
 			return -ENOMEM;
 	}
 
+#ifdef CONFIG_HAS_PCI
+	if ( dev_is_pci(dev) && !is_hardware_domain(d) )
+	{
+		struct pci_dev *pdev = dev_to_pci(dev);
+
+		printk(XENLOG_INFO "Assigning device %04x:%02x:%02x.%u to dom%d\n",
+		       pdev->seg, pdev->bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
+		       d->domain_id);
+
+		if ( devfn != pdev->devfn || pdev->domain == d )
+			return 0;
+
+		list_move(&pdev->domain_list, &d->pdev_list);
+		pdev->domain = d;
+
+		/* dom_io is used as a sentinel for quarantined devices */
+		if ( d == dom_io )
+			return 0;
+	}
+#endif
+
 	if (!dev_iommu_group(dev)) {
 		ret = arm_smmu_add_device(dev);
 		if (ret)
@@ -2774,11 +2834,29 @@ out:
 	return ret;
 }
 
-static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
+static int arm_smmu_deassign_dev(struct domain *d, u8 devfn, struct device *dev)
 {
 	struct iommu_domain *domain = dev_iommu_domain(dev);
 	struct arm_smmu_xen_domain *xen_domain;
 
+#ifdef CONFIG_HAS_PCI
+	if ( dev_is_pci(dev) )
+	{
+		struct pci_dev *pdev = dev_to_pci(dev);
+
+		printk(XENLOG_INFO "Deassigning device %04x:%02x:%02x.%u from dom%d\n",
+		       pdev->seg, pdev->bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
+		       d->domain_id);
+
+		if ( devfn != pdev->devfn )
+			return 0;
+
+		/* dom_io is used as a sentinel for quarantined devices */
+		if ( d == dom_io )
+			return 0;
+	}
+#endif
+
 	xen_domain = dom_iommu(d)->arch.priv;
 
 	if (!domain || domain->priv->cfg.domain != d) {
@@ -2806,13 +2884,13 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t,
 	int ret = 0;
 
 	/* Don't allow remapping on other domain than hwdom */
-	if ( t && !is_hardware_domain(t) )
+	if ( t && !is_hardware_domain(t) && t != dom_io )
 		return -EPERM;
 
 	if (t == s)
 		return 0;
 
-	ret = arm_smmu_deassign_dev(s, dev);
+	ret = arm_smmu_deassign_dev(s, devfn, dev);
 	if (ret)
 		return ret;
 
-- 
2.40.1

[PATCH v4 7/7] xen/arm: smmuv3: Add PCI devices support for SMMUv3

[Stewart Hildebrand]

From: Rahul Singh <rahul.singh@arm.com>

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v3->v4:
* no change

v2->v3:
* rebase
* invoke iommu_add_pci_sideband_ids() from add_device hook

v1->v2:
* ignore add_device/assign_device/reassign_device calls for phantom functions
  (i.e. devfn != pdev->devfn)

downstream->v1:
* rebase
* move 2 replacements of s/dt_device_set_protected(dev_to_dt(dev))/device_set_protected(dev)/
  from this commit to ("xen/arm: Move is_protected flag to struct device")
  so as to not break ability to bisect
* adjust patch title (remove stray space)
* arm_smmu_(de)assign_dev: return error instead of crashing system
* remove arm_smmu_remove_device() stub
* update condition in arm_smmu_reassign_dev
* style fixup

(cherry picked from commit 7ed6c3ab250d899fe6e893a514278e406a2893e8 from
 the downstream branch poc/pci-passthrough from
 https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
---

This is a file imported from Linux with modifications for Xen. What should be
the coding style used for Xen modifications?
---
 xen/drivers/passthrough/arm/smmu-v3.c | 76 +++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 4 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index 8842db1ec07e..427946e68f9d 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -1469,14 +1469,32 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
 }
 /* Forward declaration */
 static struct arm_smmu_device *arm_smmu_get_by_dev(const struct device *dev);
+static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
+			struct device *dev, u32 flag);
 
 static int arm_smmu_add_device(u8 devfn, struct device *dev)
 {
 	int i, ret;
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_master *master;
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct iommu_fwspec *fwspec;
+
+#ifdef CONFIG_HAS_PCI
+	if ( dev_is_pci(dev) )
+	{
+		struct pci_dev *pdev = dev_to_pci(dev);
+		int ret;
+
+		if ( devfn != pdev->devfn )
+			return 0;
+
+		ret = iommu_add_pci_sideband_ids(pdev);
+		if ( ret < 0 )
+			iommu_fwspec_free(dev);
+	}
+#endif
 
+	fwspec = dev_iommu_fwspec_get(dev);
 	if (!fwspec)
 		return -ENODEV;
 
@@ -1533,6 +1551,17 @@ static int arm_smmu_add_device(u8 devfn, struct device *dev)
 	dev_info(dev, "Added master device (SMMUv3 %s StreamIds %u)\n",
 			dev_name(fwspec->iommu_dev), fwspec->num_ids);
 
+#ifdef CONFIG_HAS_PCI
+	if ( dev_is_pci(dev) )
+	{
+		struct pci_dev *pdev = dev_to_pci(dev);
+
+		ret = arm_smmu_assign_dev(pdev->domain, devfn, dev, 0);
+		if (ret)
+			goto err_free_master;
+	}
+#endif
+
 	return 0;
 
 err_free_master:
@@ -2622,6 +2651,27 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
 	struct arm_smmu_domain *smmu_domain;
 	struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
 
+#ifdef CONFIG_HAS_PCI
+	if ( dev_is_pci(dev) && !is_hardware_domain(d) )
+	{
+		struct pci_dev *pdev = dev_to_pci(dev);
+
+		printk(XENLOG_INFO "Assigning device %04x:%02x:%02x.%u to dom%d\n",
+			pdev->seg, pdev->bus, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), d->domain_id);
+
+		if ( devfn != pdev->devfn || pdev->domain == d )
+			return 0;
+
+		list_move(&pdev->domain_list, &d->pdev_list);
+		pdev->domain = d;
+
+		/* dom_io is used as a sentinel for quarantined devices */
+		if ( d == dom_io )
+			return 0;
+	}
+#endif
+
 	spin_lock(&xen_domain->lock);
 
 	/*
@@ -2655,7 +2705,7 @@ out:
 	return ret;
 }
 
-static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
+static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn, struct device *dev)
 {
 	struct iommu_domain *io_domain = arm_smmu_get_domain(d, dev);
 	struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
@@ -2667,6 +2717,24 @@ static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
 		return -ESRCH;
 	}
 
+#ifdef CONFIG_HAS_PCI
+	if ( dev_is_pci(dev) )
+	{
+		struct pci_dev *pdev = dev_to_pci(dev);
+
+		printk(XENLOG_INFO "Deassigning device %04x:%02x:%02x.%u from dom%d\n",
+			pdev->seg, pdev->bus, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), d->domain_id);
+
+		if ( devfn != pdev->devfn )
+			return 0;
+
+		/* dom_io is used as a sentinel for quarantined devices */
+		if ( d == dom_io )
+			return 0;
+	}
+#endif
+
 	spin_lock(&xen_domain->lock);
 
 	arm_smmu_detach_dev(master);
@@ -2686,13 +2754,13 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t,
 	int ret = 0;
 
 	/* Don't allow remapping on other domain than hwdom */
-	if ( t && !is_hardware_domain(t) )
+	if ( t && !is_hardware_domain(t) && (t != dom_io) )
 		return -EPERM;
 
 	if (t == s)
 		return 0;
 
-	ret = arm_smmu_deassign_dev(s, dev);
+	ret = arm_smmu_deassign_dev(s, devfn, dev);
 	if (ret)
 		return ret;
 
-- 
2.40.1

Re: [PATCH v4 0/7] SMMU handling for PCIe Passthrough on ARM

[Julien Grall]

Hi Stewart,

On 07/06/2023 04:02, Stewart Hildebrand wrote:
> This series introduces SMMU handling for PCIe passthrough on ARM. These patches
> are independent from (and don't depend on) the vPCI reference counting/locking
> work in progress, and should be able to be upstreamed independently.

Can you clarify how this code was tested? Does this require code not yet 
upstreamed?

Cheers,

-- 
Julien Grall

Re: [PATCH v4 1/7] xen/arm: Improve readability of check for registered devices

[Julien Grall]

Hi Stewart,

On 07/06/2023 04:02, Stewart Hildebrand wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Improve readability of check for devices already registered with the SMMU with
> legacy mmu-masters DT bindings by using is_protected.

I am unconvinced with this change because...

> 
> There are 2 device tree bindings for registering a device with the SMMU:
> * mmu-masters (legacy, SMMUv1/2 only)
> * iommus
> 
> A device tree may include both mmu-masters and iommus properties (although it is
> unnecessary to do so). When a device appears in the mmu-masters list,
> np->is_protected and dev->iommu_fwspec both get set by the SMMUv1/2 driver. The
> function iommu_add_dt_device() is subsequently invoked for devices that have an
> iommus specification.
> 
> The check as it was before this patch:
> 
>    if ( dev_iommu_fwspec_get(dev) )
>        return 0;
> 
> and the new check:
> 
>    if ( dt_device_is_protected(np) )
>        return 0;
> 
> are guarding against the same corner case: when a device has both mmu-masters
> and iommus specifications in the device tree. The is_protected naming is more
> descriptive.
> 
> If np->is_protected is not set (i.e. false), but dev->iommu_fwspec is set, it is
> an error condition, so return an error in this case.
> 
> Expand the comment to further clarify the corner case.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> v3->v4:
> * new patch: this change was split from ("xen/arm: Move is_protected flag to struct device")
> ---
>   xen/drivers/passthrough/device_tree.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 1c32d7b50cce..d9b63da7260a 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -141,12 +141,17 @@ int iommu_add_dt_device(struct dt_device_node *np)
>           return -EINVAL;
>   
>       /*
> -     * The device may already have been registered. As there is no harm in
> -     * it just return success early.
> +     * Devices that appear in the legacy mmu-masters list may have already been
> +     * registered with the SMMU. In case a device has both a mmu-masters entry
> +     * and iommus property, there is no need to register it again. In this case
> +     * simply return success early.
>        */
> -    if ( dev_iommu_fwspec_get(dev) )
> +    if ( dt_device_is_protected(np) )
... we now have two checks and it implies that it would be normal for 
dt_device_is_protected() to be false and ...

>           return 0;
>   
> +    if ( dev_iommu_fwspec_get(dev) )

... this returning a non-zero value. Is there any other benefits with 
adding the two checks?

If the others agree with the double check, then I think this should gain 
an ASSERT_UNREACHABLE() because AFAIU this is a programming error.

> +        return -EEXIST;
> +
>       /*
>        * According to the Documentation/devicetree/bindings/iommu/iommu.txt
>        * from Linux.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 4/7] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

[Jan Beulich]

On 07.06.2023 05:02, Stewart Hildebrand wrote:
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -26,6 +26,9 @@
>  #include <xen/spinlock.h>
>  #include <public/domctl.h>
>  #include <public/hvm/ioreq.h>
> +#ifdef CONFIG_ACPI
> +#include <asm/acpi.h>
> +#endif

This header is supposed to be usable without #ifdef, and then ...

> @@ -228,12 +232,28 @@ int iommu_release_dt_devices(struct domain *d);
>   *      (IOMMU is not enabled/present or device is not connected to it).
>   */
>  int iommu_add_dt_device(struct dt_device_node *np);
> +int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev);
>  
>  int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
>                         XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>  
> +#else /* !HAS_DEVICE_TREE */
> +static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
> +{
> +    return 0;
> +}
>  #endif /* HAS_DEVICE_TREE */
>  
> +static inline int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
> +{
> +    int ret = 0;
> +#ifdef CONFIG_ACPI
> +    if ( acpi_disabled )
> +#endif

... you don't need #ifdef here either.

> +        ret = iommu_add_dt_pci_sideband_ids(pdev);
> +    return ret;
> +}

Also (nit) please follow (partly unwritten, I admit) style guidelines:
A blank line between declaration(s) and statement(s), and another one
ahead of a function's main "return".

Jan

Re: [PATCH v4 4/7] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

[Stewart Hildebrand]

On 6/7/23 03:59, Jan Beulich wrote:
> On 07.06.2023 05:02, Stewart Hildebrand wrote:
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -26,6 +26,9 @@
>>  #include <xen/spinlock.h>
>>  #include <public/domctl.h>
>>  #include <public/hvm/ioreq.h>
>> +#ifdef CONFIG_ACPI
>> +#include <asm/acpi.h>
>> +#endif
> 
> This header is supposed to be usable without #ifdef, and then ...

You suggested adding the #ifdef

https://lists.xenproject.org/archives/html/xen-devel/2023-05/msg01409.html

Please clarify

>> @@ -228,12 +232,28 @@ int iommu_release_dt_devices(struct domain *d);
>>   *      (IOMMU is not enabled/present or device is not connected to it).
>>   */
>>  int iommu_add_dt_device(struct dt_device_node *np);
>> +int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev);
>>
>>  int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
>>                         XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>>
>> +#else /* !HAS_DEVICE_TREE */
>> +static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
>> +{
>> +    return 0;
>> +}
>>  #endif /* HAS_DEVICE_TREE */
>>
>> +static inline int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
>> +{
>> +    int ret = 0;
>> +#ifdef CONFIG_ACPI
>> +    if ( acpi_disabled )
>> +#endif
> 
> ... you don't need #ifdef here either.
> 
>> +        ret = iommu_add_dt_pci_sideband_ids(pdev);
>> +    return ret;
>> +}
> 
> Also (nit) please follow (partly unwritten, I admit) style guidelines:
> A blank line between declaration(s) and statement(s), and another one
> ahead of a function's main "return".

OK

Re: [PATCH v4 1/7] xen/arm: Improve readability of check for registered devices

[Stewart Hildebrand]

On 6/7/23 03:27, Julien Grall wrote:
> Hi Stewart,
> 
> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Improve readability of check for devices already registered with the SMMU with
>> legacy mmu-masters DT bindings by using is_protected.
> 
> I am unconvinced with this change because...
> 
>>
>> There are 2 device tree bindings for registering a device with the SMMU:
>> * mmu-masters (legacy, SMMUv1/2 only)
>> * iommus
>>
>> A device tree may include both mmu-masters and iommus properties (although it is
>> unnecessary to do so). When a device appears in the mmu-masters list,
>> np->is_protected and dev->iommu_fwspec both get set by the SMMUv1/2 driver. The
>> function iommu_add_dt_device() is subsequently invoked for devices that have an
>> iommus specification.
>>
>> The check as it was before this patch:
>>
>>    if ( dev_iommu_fwspec_get(dev) )
>>        return 0;
>>
>> and the new check:
>>
>>    if ( dt_device_is_protected(np) )
>>        return 0;
>>
>> are guarding against the same corner case: when a device has both mmu-masters
>> and iommus specifications in the device tree. The is_protected naming is more
>> descriptive.
>>
>> If np->is_protected is not set (i.e. false), but dev->iommu_fwspec is set, it is
>> an error condition, so return an error in this case.
>>
>> Expand the comment to further clarify the corner case.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>> v3->v4:
>> * new patch: this change was split from ("xen/arm: Move is_protected flag to struct device")
>> ---
>>   xen/drivers/passthrough/device_tree.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>> index 1c32d7b50cce..d9b63da7260a 100644
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -141,12 +141,17 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>           return -EINVAL;
>>
>>       /*
>> -     * The device may already have been registered. As there is no harm in
>> -     * it just return success early.
>> +     * Devices that appear in the legacy mmu-masters list may have already been
>> +     * registered with the SMMU. In case a device has both a mmu-masters entry
>> +     * and iommus property, there is no need to register it again. In this case
>> +     * simply return success early.
>>        */
>> -    if ( dev_iommu_fwspec_get(dev) )
>> +    if ( dt_device_is_protected(np) )
> ... we now have two checks and it implies that it would be normal for
> dt_device_is_protected() to be false and ...
> 
>>           return 0;
>>
>> +    if ( dev_iommu_fwspec_get(dev) )
> 
> ... this returning a non-zero value. Is there any other benefits with
> adding the two checks?

No, I can't think of any good rationale for the 2nd check. After splitting this change from the other patch ("xen/arm: Move is_protected flag to struct device"), I simply wanted to evaluate it on its own.

> If the others agree with the double check, then I think this should gain
> an ASSERT_UNREACHABLE() because AFAIU this is a programming error.

Right, if the 2nd check was justified, there should be an ASSERT_UNREACHABLE(), good point. But I don't think the 2nd check is justified.

If the 2nd check is dropped (keeping only the is_protected check), then do you think the change is justified?

>> +        return -EEXIST;
>> +
>>       /*
>>        * According to the Documentation/devicetree/bindings/iommu/iommu.txt
>>        * from Linux.
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v4 4/7] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

[Jan Beulich]

On 07.06.2023 14:45, Stewart Hildebrand wrote:
> On 6/7/23 03:59, Jan Beulich wrote:
>> On 07.06.2023 05:02, Stewart Hildebrand wrote:
>>> --- a/xen/include/xen/iommu.h
>>> +++ b/xen/include/xen/iommu.h
>>> @@ -26,6 +26,9 @@
>>>  #include <xen/spinlock.h>
>>>  #include <public/domctl.h>
>>>  #include <public/hvm/ioreq.h>
>>> +#ifdef CONFIG_ACPI
>>> +#include <asm/acpi.h>
>>> +#endif
>>
>> This header is supposed to be usable without #ifdef, and then ...
> 
> You suggested adding the #ifdef
> 
> https://lists.xenproject.org/archives/html/xen-devel/2023-05/msg01409.html
> 
> Please clarify

Hmm, indeed. Which is a sign that neither solution is really nice. I
guess the one with #ifdef is slightly preferable because it'll avoid
(as said in the earlier reply you refer to above) the need for every
arch, even if ignorant of ACPI altogether, needing to have an
asm/acpi.h. I guess we really need to deal with all of this better
in / by having suitable common code.

Jan

Re: [PATCH v4 0/7] SMMU handling for PCIe Passthrough on ARM

[Stewart Hildebrand]

On 6/7/23 03:19, Julien Grall wrote:
> Hi Stewart,
> 
> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>> This series introduces SMMU handling for PCIe passthrough on ARM. These patches
>> are independent from (and don't depend on) the vPCI reference counting/locking
>> work in progress, and should be able to be upstreamed independently.
> 
> Can you clarify how this code was tested? Does this require code not yet
> upstreamed?

I'm testing the series standalone (+ config changes) by using a PCI device in dom0, and also in combination with the vPCI series [3] [4] for passthrough to a domU.


Here are some more details on the test cases I'm using.


1. Using the PCI device in dom0 with the pci-passthrough=yes arg. In this case a couple of additional config changes [1] [2] are needed to select CONFIG_HAS_PCI=y, CONFIG_HAS_VPCI=y, and make has_vpci() return true. Aside from this series itself and the config changes, nothing else not-yet-upstreamed is required for this test case. It is on my TODO list to upstream these config changes, which I think will be useful on their own, not necessarily as part of any other series.

Actually, your question prompted me to look at my test cases a bit more carefully, and I discovered a case that v4 of this series doesn't handle right. In order for the PCI device to be usable in dom0, it should be assigned by default to dom0/hardware domain during PHYSDEVOP_pci_device_add. But v4 of this series doesn't assign it by default to dom0/hardware domain. I initially missed this because I happened to assign the stream ID of the PCI device to dom0 by the iommus property.

In other words, I initially tested with this:

&pcie {
    iommus = <&smmu 0x4d0>;
    iommu-map = <0x0 &smmu 0x4d0 0x10000>;
    iommu-map-mask = <0x0>;
};

But I should be testing with this (i.e. omitting the iommus property):

&pcie {
    iommu-map = <0x0 &smmu 0x4d0 0x10000>;
    iommu-map-mask = <0x0>;
};

Omitting iommus currently breaks using a PCI device in dom0 in v4. I'll fix this in v5.


2. To test the phantom bits, since I don't have a phantom device readily available, I use the pci-phantom arg and a carefully constructed iommu-map property. The PCI device's stream ID is actually 0x4d0, but I put 0x4ce in the iommu-map to make it appear as if it's one of the phantom functions generating the DMA traffic.

pci-phantom=01:00,1

&pcie {
    /* phantom test */
    iommu-map = <0x0 &smmu 0x4ce 0x10000>;
    iommu-map-mask = <0x7>;
};


3. Passthrough to a domU. In this case the vPCI series is needed [3], and an MSI series not yet submitted to the list [4] (or another way to hack/assign the IRQ to the domU), in addition to the 2 config changes [1] [2]. The procedure is described at [5].

[1] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/9a08f1f7ce28ec619640ba9ce11018bf443e9a0e
[2] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/27be1729ce8128dbe37275ce7946b2fbd2e5a382
[3] https://lists.xenproject.org/archives/html/xen-devel/2023-06/msg00863.html
[4] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commits/poc/pci-passthrough
[5] https://lists.xenproject.org/archives/html/xen-devel/2022-12/msg00682.html

Re: [PATCH v4 0/7] SMMU handling for PCIe Passthrough on ARM

[Julien Grall]

Hi,

On 15/06/2023 22:05, Stewart Hildebrand wrote:
> On 6/7/23 03:19, Julien Grall wrote:
>> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>>> This series introduces SMMU handling for PCIe passthrough on ARM. These patches
>>> are independent from (and don't depend on) the vPCI reference counting/locking
>>> work in progress, and should be able to be upstreamed independently.
>>
>> Can you clarify how this code was tested? Does this require code not yet
>> upstreamed?
> 
> I'm testing the series standalone (+ config changes) by using a PCI device in dom0, and also in combination with the vPCI series [3] [4] for passthrough to a domU.
> 
> 
> Here are some more details on the test cases I'm using.

Thanks that's helpful! One comment about the first test case.

> 
> 
> 1. Using the PCI device in dom0 with the pci-passthrough=yes arg. In this case a couple of additional config changes [1] [2] are needed to select CONFIG_HAS_PCI=y, CONFIG_HAS_VPCI=y, and make has_vpci() return true. Aside from this series itself and the config changes, nothing else not-yet-upstreamed is required for this test case. It is on my TODO list to upstream these config changes, which I think will be useful on their own, not necessarily as part of any other series.

I find a bit confusing that the IOMMU support for dom0 is gated behind 
'pci-passthrough'. I was expecting that the iommu would also be properly 
configured for PCI if we using 'iommu=yes'.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 0/7] SMMU handling for PCIe Passthrough on ARM

[Rahul Singh]

Hi Julien,

> On 25 Jun 2023, at 1:56 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 15/06/2023 22:05, Stewart Hildebrand wrote:
>> On 6/7/23 03:19, Julien Grall wrote:
>>> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>>>> This series introduces SMMU handling for PCIe passthrough on ARM. These patches
>>>> are independent from (and don't depend on) the vPCI reference counting/locking
>>>> work in progress, and should be able to be upstreamed independently.
>>> 
>>> Can you clarify how this code was tested? Does this require code not yet
>>> upstreamed?
>> I'm testing the series standalone (+ config changes) by using a PCI device in dom0, and also in combination with the vPCI series [3] [4] for passthrough to a domU.
>> Here are some more details on the test cases I'm using.
> 
> Thanks that's helpful! One comment about the first test case.
> 
>> 1. Using the PCI device in dom0 with the pci-passthrough=yes arg. In this case a couple of additional config changes [1] [2] are needed to select CONFIG_HAS_PCI=y, CONFIG_HAS_VPCI=y, and make has_vpci() return true. Aside from this series itself and the config changes, nothing else not-yet-upstreamed is required for this test case. It is on my TODO list to upstream these config changes, which I think will be useful on their own, not necessarily as part of any other series.
> 
> I find a bit confusing that the IOMMU support for dom0 is gated behind 'pci-passthrough'. I was expecting that the iommu would also be properly configured for PCI if we using 'iommu=yes'.

As per my understanding Xen can configure the iommus for PCI device without "pci-passthrough” enabled
if we follow below path:

   1) PCI host bridge is already enumerated and powered on in firmware before Xen boot
   2) PCI devices are scanned in Xen.
       (https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/bce463e1588a45e1bfdf59fc0d5f88b16604e439)
   3) After scanning the PCI devices add PCI devices to iommu ( iommu_add_device() )

If PCI host bridge is not enumerated then we need "pci-passthrough” enabled to allow Linux to do
enumeration and to inform Xen via PHYSDEVOP_pci_device_add hyper call to add PCI devices in Xen
This is implemented as part of PCI passthrough feature.

Regards,
Rahul

Re: [PATCH v4 0/7] SMMU handling for PCIe Passthrough on ARM

[Julien Grall]

Hi Rahul,

On 25/06/2023 15:28, Rahul Singh wrote:
>> On 25 Jun 2023, at 1:56 pm, Julien Grall <julien@xen.org> wrote:
>> On 15/06/2023 22:05, Stewart Hildebrand wrote:
>>> On 6/7/23 03:19, Julien Grall wrote:
>>>> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>>>>> This series introduces SMMU handling for PCIe passthrough on ARM. These patches
>>>>> are independent from (and don't depend on) the vPCI reference counting/locking
>>>>> work in progress, and should be able to be upstreamed independently.
>>>>
>>>> Can you clarify how this code was tested? Does this require code not yet
>>>> upstreamed?
>>> I'm testing the series standalone (+ config changes) by using a PCI device in dom0, and also in combination with the vPCI series [3] [4] for passthrough to a domU.
>>> Here are some more details on the test cases I'm using.
>>
>> Thanks that's helpful! One comment about the first test case.
>>
>>> 1. Using the PCI device in dom0 with the pci-passthrough=yes arg. In this case a couple of additional config changes [1] [2] are needed to select CONFIG_HAS_PCI=y, CONFIG_HAS_VPCI=y, and make has_vpci() return true. Aside from this series itself and the config changes, nothing else not-yet-upstreamed is required for this test case. It is on my TODO list to upstream these config changes, which I think will be useful on their own, not necessarily as part of any other series.
>>
>> I find a bit confusing that the IOMMU support for dom0 is gated behind 'pci-passthrough'. I was expecting that the iommu would also be properly configured for PCI if we using 'iommu=yes'.
> 
> As per my understanding Xen can configure the iommus for PCI device without "pci-passthrough” enabled
> if we follow below path:
> 
>     1) PCI host bridge is already enumerated and powered on in firmware before Xen boot
>     2) PCI devices are scanned in Xen.
>         (https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/bce463e1588a45e1bfdf59fc0d5f88b16604e439)
>     3) After scanning the PCI devices add PCI devices to iommu ( iommu_add_device() )
> 
> If PCI host bridge is not enumerated then we need "pci-passthrough” enabled to allow Linux to do
> enumeration and to inform Xen via PHYSDEVOP_pci_device_add hyper call to add PCI devices in Xen
> This is implemented as part of PCI passthrough feature.
Right, but selecting "pci-passthrough" to be able to use the IOMMU in 
dom0 is confusing. We already support PCI in dom0 and adding the support 
(IOMMU + PCI) has no relation with assigning a device to the guest. IOW 
one may want to use IOMMU in dom0 without assigning devices to the guest.

I think part of the code gated by "pci-passthrough" should also be 
available when the IOMMU is enabled. This would allow users to use IOMMU 
+ PCI in dom0 without any extra patches and/or command line option.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 1/7] xen/arm: Improve readability of check for registered devices

[Julien Grall]

Hi,

Sorry for the late answer.

On 07/06/2023 14:41, Stewart Hildebrand wrote:
> On 6/7/23 03:27, Julien Grall wrote:
>> Hi Stewart,
>>
>> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> Improve readability of check for devices already registered with the SMMU with
>>> legacy mmu-masters DT bindings by using is_protected.
>>
>> I am unconvinced with this change because...
>>
>>>
>>> There are 2 device tree bindings for registering a device with the SMMU:
>>> * mmu-masters (legacy, SMMUv1/2 only)
>>> * iommus
>>>
>>> A device tree may include both mmu-masters and iommus properties (although it is
>>> unnecessary to do so). When a device appears in the mmu-masters list,
>>> np->is_protected and dev->iommu_fwspec both get set by the SMMUv1/2 driver. The
>>> function iommu_add_dt_device() is subsequently invoked for devices that have an
>>> iommus specification.
>>>
>>> The check as it was before this patch:
>>>
>>>     if ( dev_iommu_fwspec_get(dev) )
>>>         return 0;
>>>
>>> and the new check:
>>>
>>>     if ( dt_device_is_protected(np) )
>>>         return 0;
>>>
>>> are guarding against the same corner case: when a device has both mmu-masters
>>> and iommus specifications in the device tree. The is_protected naming is more
>>> descriptive.
>>>
>>> If np->is_protected is not set (i.e. false), but dev->iommu_fwspec is set, it is
>>> an error condition, so return an error in this case.
>>>
>>> Expand the comment to further clarify the corner case.
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>> ---
>>> v3->v4:
>>> * new patch: this change was split from ("xen/arm: Move is_protected flag to struct device")
>>> ---
>>>    xen/drivers/passthrough/device_tree.c | 11 ++++++++---
>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>>> index 1c32d7b50cce..d9b63da7260a 100644
>>> --- a/xen/drivers/passthrough/device_tree.c
>>> +++ b/xen/drivers/passthrough/device_tree.c
>>> @@ -141,12 +141,17 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>            return -EINVAL;
>>>
>>>        /*
>>> -     * The device may already have been registered. As there is no harm in
>>> -     * it just return success early.
>>> +     * Devices that appear in the legacy mmu-masters list may have already been
>>> +     * registered with the SMMU. In case a device has both a mmu-masters entry
>>> +     * and iommus property, there is no need to register it again. In this case
>>> +     * simply return success early.
>>>         */
>>> -    if ( dev_iommu_fwspec_get(dev) )
>>> +    if ( dt_device_is_protected(np) )
>> ... we now have two checks and it implies that it would be normal for
>> dt_device_is_protected() to be false and ...
>>
>>>            return 0;
>>>
>>> +    if ( dev_iommu_fwspec_get(dev) )
>>
>> ... this returning a non-zero value. Is there any other benefits with
>> adding the two checks?
> 
> No, I can't think of any good rationale for the 2nd check. After splitting this change from the other patch ("xen/arm: Move is_protected flag to struct device"), I simply wanted to evaluate it on its own.
> 
>> If the others agree with the double check, then I think this should gain
>> an ASSERT_UNREACHABLE() because AFAIU this is a programming error.
> 
> Right, if the 2nd check was justified, there should be an ASSERT_UNREACHABLE(), good point. But I don't think the 2nd check is justified.
> 
> If the 2nd check is dropped (keeping only the is_protected check), then do you think the change is justified?

To be honest not with the current justification. I don't view the new 
check better than the other in term of readability.

Is this the only reason you want to switch to dt_device_is_protected()?

Cheers,

-- 
Julien Grall

Re: [PATCH v4 2/7] xen/arm: Move is_protected flag to struct device

[Julien Grall]

Hi Stewart,

On 07/06/2023 04:02, Stewart Hildebrand wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> This flag will be re-used for PCI devices by the subsequent
> patches.

I was expecting that we would only do PCI passthrough on boards where 
all the PCI devices are behind an IOMMU. So it would be a all or nothing 
and therefore is_protected would not be used.

Do you have an example where this wouldn't be the case?

Regardless that, it would be good to spell out that you also rename 
helpers. But see below.

> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> v3->v4:
> * move is_protected flag within struct device to reduce padding
> * re-add device_is_protected checks in add_device hooks in smmu-v3.c/ipmmu-vmsa.c
> * split mmu-masters check into separate patch
> 
> v2->v3:
> * no change
> 
> v1->v2:
> * no change
> 
> downstream->v1:
> * rebase
> * s/dev_node->is_protected/dev_node->dev.is_protected/ in smmu.c
> * s/dt_device_set_protected(dev_to_dt(dev))/device_set_protected(dev)/ in smmu-v3.c
> * remove redundant device_is_protected checks in smmu-v3.c/ipmmu-vmsa.c
> 
> (cherry picked from commit 59753aac77528a584d3950936b853ebf264b68e7 from
>   the downstream branch poc/pci-passthrough from
>   https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
> ---
>   xen/arch/arm/domain_build.c              |  4 ++--
>   xen/arch/arm/include/asm/device.h        | 14 ++++++++++++++
>   xen/common/device_tree.c                 |  2 +-
>   xen/drivers/passthrough/arm/ipmmu-vmsa.c |  4 ++--
>   xen/drivers/passthrough/arm/smmu-v3.c    |  5 +++--
>   xen/drivers/passthrough/arm/smmu.c       |  2 +-
>   xen/drivers/passthrough/device_tree.c    |  8 ++++----
>   xen/include/xen/device_tree.h            | 13 -------------
>   8 files changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 3f4558ade67f..b229bfaae712 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2524,7 +2524,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
>               return res;
>           }
>   
> -        if ( dt_device_is_protected(dev) )
> +        if ( device_is_protected(dt_to_dev(dev)) )

I would actually prefer if we keep dt_device_is_protected() and call 
device_is_protected(dt_to_dev(...)) within it.

>           {
>               dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
>               res = iommu_assign_dt_device(d, dev);
> @@ -3024,7 +3024,7 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>           return res;
>   
>       /* If xen_force, we allow assignment of devices without IOMMU protection. */
> -    if ( xen_force && !dt_device_is_protected(node) )
> +    if ( xen_force && !device_is_protected(dt_to_dev(node)) )
>           return 0;
>   
>       return iommu_assign_dt_device(kinfo->d, node);
> diff --git a/xen/arch/arm/include/asm/device.h b/xen/arch/arm/include/asm/device.h
> index b5d451e08776..8ac807482737 100644
> --- a/xen/arch/arm/include/asm/device.h
> +++ b/xen/arch/arm/include/asm/device.h
> @@ -1,6 +1,8 @@
>   #ifndef __ASM_ARM_DEVICE_H
>   #define __ASM_ARM_DEVICE_H
>   
> +#include <xen/types.h>
> +
>   enum device_type
>   {
>       DEV_DT,
> @@ -15,6 +17,8 @@ struct dev_archdata {
>   struct device
>   {
>       enum device_type type;
> +    bool is_protected; /* Shows that device is protected by IOMMU */
> +    uint8_t _pad[3];

AFAIK, a compiler would be allowed to use 8-byte for the enum. So the 
padding would increase the size of the structure.

Therefore, I don't think you want to add an explicit padding here.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 3/7] iommu/arm: Add iommu_dt_xlate()

[Julien Grall]

Hi Stewart,

On 07/06/2023 04:02, Stewart Hildebrand wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Move code for processing DT IOMMU specifier to a separate helper.
> This helper will be re-used for adding PCI devices by the subsequent
> patches as we will need exact the same actions for processing
> DT PCI-IOMMU specifier.
> 
> While at it introduce NO_IOMMU to avoid magic "1".
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> v3->v4:
> * make dt_phandle_args *iommu_spec const
> * move !ops->add_device check to helper
> 
> v2->v3:
> * no change
> 
> v1->v2:
> * no change
> 
> downstream->v1:
> * trivial rebase
> * s/dt_iommu_xlate/iommu_dt_xlate/
> 
> (cherry picked from commit c26bab0415ca303df86aba1d06ef8edc713734d3 from
>   the downstream branch poc/pci-passthrough from
>   https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
> ---
>   xen/drivers/passthrough/device_tree.c | 47 ++++++++++++++++++---------
>   1 file changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index c60e78eaf556..ff9e66ebf92a 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -127,15 +127,42 @@ int iommu_release_dt_devices(struct domain *d)
>       return 0;
>   }
>   
> +/* This correlation must not be altered */

Please expand why.

> +#define NO_IOMMU    1

Given that the value is returned, should not this be moved to an header 
and used by the callers?

> +
> +static int iommu_dt_xlate(struct device *dev,
> +                          const struct dt_phandle_args *iommu_spec)
> +{
> +    const struct iommu_ops *ops = iommu_get_ops();
> +    int rc;
> +
> +    if ( !ops->dt_xlate )
> +        return -EINVAL;
> +
> +    if ( !dt_device_is_available(iommu_spec->np) )
> +        return NO_IOMMU;
> +
> +    rc = iommu_fwspec_init(dev, &iommu_spec->np->dev);
> +    if ( rc )
> +        return rc;
> +
> +    /*
> +     * Provide DT IOMMU specifier which describes the IOMMU master
> +     * interfaces of that device (device IDs, etc) to the driver.
> +     * The driver is responsible to decide how to interpret them.
> +     */
> +    return ops->dt_xlate(dev, iommu_spec);
> +}
> +
>   int iommu_add_dt_device(struct dt_device_node *np)
>   {
>       const struct iommu_ops *ops = iommu_get_ops();
>       struct dt_phandle_args iommu_spec;
>       struct device *dev = dt_to_dev(np);
> -    int rc = 1, index = 0;
> +    int rc = NO_IOMMU, index = 0;
>   
>       if ( !iommu_enabled )
> -        return 1;
> +        return NO_IOMMU;
>   
>       if ( !ops )
>           return -EINVAL;
> @@ -163,22 +190,10 @@ int iommu_add_dt_device(struct dt_device_node *np)
>            * The driver which supports generic IOMMU DT bindings must have
>            * these callback implemented.

The 's' was missing to callback. But now, I think you want to replace 
'these' with 'this' as you move the second check.

>            */
> -        if ( !ops->add_device || !ops->dt_xlate )
> +        if ( !ops->add_device )
>               return -EINVAL;
>   
> -        if ( !dt_device_is_available(iommu_spec.np) )
> -            break;
> -
> -        rc = iommu_fwspec_init(dev, &iommu_spec.np->dev);
> -        if ( rc )
> -            break;
> -
> -        /*
> -         * Provide DT IOMMU specifier which describes the IOMMU master
> -         * interfaces of that device (device IDs, etc) to the driver.
> -         * The driver is responsible to decide how to interpret them.
> -         */
> -        rc = ops->dt_xlate(dev, &iommu_spec);
> +        rc = iommu_dt_xlate(dev, &iommu_spec);
>           if ( rc )
>               break;
>   

Cheers,

-- 
Julien Grall

Re: [PATCH v4 4/7] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

[Julien Grall]

Hi Stewart,

I haven't yet fully reviewed the code. However, I have one question...

On 07/06/2023 04:02, Stewart Hildebrand wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

> +static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
> +{
> +    return 0;
> +}
>   #endif /* HAS_DEVICE_TREE */
>   
> +static inline int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
> +{
> +    int ret = 0;

Shouldn't this return NO_IOMMU when booting on ACPI platform?

> +#ifdef CONFIG_ACPI
> +    if ( acpi_disabled )
> +#endif
> +        ret = iommu_add_dt_pci_sideband_ids(pdev);
> +    return ret;
> +}
> +
>   struct page_info;
>   
>   /*

Cheers,

-- 
Julien Grall

Re: [PATCH v4 4/7] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

[Julien Grall]

Hi,

On 07/06/2023 04:02, Stewart Hildebrand 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 of_map_id to translate an ID through a downstream mapping

You called the function dt_map_id in Xen.

> (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>
> ---
> 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/drivers/passthrough/device_tree.c | 134 ++++++++++++++++++++++++++
>   xen/include/xen/device_tree.h         |  25 +++++
>   xen/include/xen/iommu.h               |  22 ++++-
>   3 files changed, 180 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index ff9e66ebf92a..bd0aed5df651 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -154,6 +154,140 @@ static int iommu_dt_xlate(struct device *dev,
>       return ops->dt_xlate(dev, iommu_spec);
>   }
>   
> +#ifdef CONFIG_HAS_PCI

The code below doesn't seem to be specific to PCI.

> +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)

AFAICT, this function can also be used outside of the IOMMU code. So 
shouldn't this be implemented in common/device_tree.c?


> +{
> +    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: Error: Bad %s length: %u\n", np->full_name,
> +               map_name, map_len);

I think it would be helpful if you add the function name in the error 
message.

> +        return -EINVAL;
> +    }
> +
> +    /* The default is to select all bits. */
> +    map_mask = 0xffffffff;

Please add a U. That said, I would switch to GENMASK(31, 0) so it is 
easier to check the value.

> +
> +    /*
> +     * Can be overridden by "{iommu,msi}-map-mask" property.
> +     * If df_property_read_u32() fails, the default is used.

s/df/dt/

> +     */
> +    if ( map_mask_name )
> +        dt_property_read_u32(np, map_mask_name, &map_mask);
> +
> +    masked_id = map_mask & id;
> +    for ( ; (int)map_len > 0; map_len -= 4 * sizeof(*map), map += 4 )

Why do you cast map_len to 'int'?

> +    {
> +        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: Invalid %s translation - %s-mask (0x%"PRIx32") ignores id-base (0x%"PRIx32")\n",
> +                   np->full_name, map_name, map_name, map_mask, id_base);

Same here about adding the function name.

> +            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;
> +
> +        printk(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",

How about using dprintk()?

> +               np->full_name, map_name, map_mask, id_base, out_base, id_len, id,
> +               masked_id - id_base + out_base);
> +        return 0;
> +    }
> +
> +    printk(XENLOG_ERR "%s: no %s translation for id 0x%"PRIx32" on %s\n",
> +           np->full_name, map_name, id, (target && *target) ? (*target)->full_name : NULL);

Same here about adding the function name.

> +
> +    /*
> +     * NOTE: Linux bypasses translation without returning an error here,
> +     * but should we behave in the same way on Xen? Restrict for now.
> +     */

Can you outline why we would want to bypass the translation?

> +    return -EFAULT;
> +}
> +
> +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 = NO_IOMMU;

AFAICT, the initial value will never be read. So is it necessary?

> +
> +    if ( !iommu_enabled )
> +        return NO_IOMMU;
> +
> +    if ( !ops )
> +        return -EINVAL;
> +
> +    if ( device_is_protected(dev) )
> +        return 0;

These two lines are a bit odd to read because you would think that if 
the device is protected, then you want to continue translation. So can 
you add a comment explaining what this check means?

> +
> +    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) ? NO_IOMMU : rc;
> +
> +    rc = iommu_dt_xlate(dev, &iommu_spec);
> +    if ( rc < 0 )
> +    {
> +        iommu_fwspec_free(dev);
> +        return -EINVAL;
> +    }
> +
> +    return rc;
> +}
> +#endif /* CONFIG_HAS_PCI */
> +
>   int iommu_add_dt_device(struct dt_device_node *np)
>   {
>       const struct iommu_ops *ops = iommu_get_ops();
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index c2f315140560..8385cd538a58 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -892,6 +892,31 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
>    */
>   int dt_get_pci_domain_nr(struct dt_device_node *node);
>   
> +#ifdef CONFIG_HAS_PCI

In Xen, we don't usually add #ifdef for prototype only.

> +/**
> + * 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);
> +#endif /* CONFIG_HAS_PCI */
> +
>   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 405db59971c5..3cac177840f7 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -26,6 +26,9 @@
>   #include <xen/spinlock.h>
>   #include <public/domctl.h>
>   #include <public/hvm/ioreq.h>
> +#ifdef CONFIG_ACPI
> +#include <asm/acpi.h>
> +#endif
>   #include <asm/device.h>
>   
>   TYPE_SAFE(uint64_t, dfn);
> @@ -219,7 +222,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
> @@ -228,12 +232,28 @@ int iommu_release_dt_devices(struct domain *d);
>    *      (IOMMU is not enabled/present or device is not connected to it).
>    */
>   int iommu_add_dt_device(struct dt_device_node *np);
> +int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev);
>   
>   int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>   
> +#else /* !HAS_DEVICE_TREE */
> +static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
> +{
> +    return 0;
Shouldn't this return an error because we wouldn't be able to add the 
Device?

> +}
>   #endif /* HAS_DEVICE_TREE */
>   
> +static inline int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
> +{
> +    int ret = 0;

Same here.

> +#ifdef CONFIG_ACPI
> +    if ( acpi_disabled )
> +#endif
> +        ret = iommu_add_dt_pci_sideband_ids(pdev);
> +    return ret;
> +}
> +
>   struct page_info;
>   
>   /*

Cheers,

-- 
Julien Grall

Re: [PATCH v4 1/7] xen/arm: Improve readability of check for registered devices

[Stewart Hildebrand]

On 6/29/23 17:47, Julien Grall wrote:
> Hi,
> 
> Sorry for the late answer.
> 
> On 07/06/2023 14:41, Stewart Hildebrand wrote:
>> On 6/7/23 03:27, Julien Grall wrote:
>>> Hi Stewart,
>>>
>>> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> Improve readability of check for devices already registered with the SMMU with
>>>> legacy mmu-masters DT bindings by using is_protected.
>>>
>>> I am unconvinced with this change because...
>>>
>>>>
>>>> There are 2 device tree bindings for registering a device with the SMMU:
>>>> * mmu-masters (legacy, SMMUv1/2 only)
>>>> * iommus
>>>>
>>>> A device tree may include both mmu-masters and iommus properties (although it is
>>>> unnecessary to do so). When a device appears in the mmu-masters list,
>>>> np->is_protected and dev->iommu_fwspec both get set by the SMMUv1/2 driver. The
>>>> function iommu_add_dt_device() is subsequently invoked for devices that have an
>>>> iommus specification.
>>>>
>>>> The check as it was before this patch:
>>>>
>>>>     if ( dev_iommu_fwspec_get(dev) )
>>>>         return 0;
>>>>
>>>> and the new check:
>>>>
>>>>     if ( dt_device_is_protected(np) )
>>>>         return 0;
>>>>
>>>> are guarding against the same corner case: when a device has both mmu-masters
>>>> and iommus specifications in the device tree. The is_protected naming is more
>>>> descriptive.
>>>>
>>>> If np->is_protected is not set (i.e. false), but dev->iommu_fwspec is set, it is
>>>> an error condition, so return an error in this case.
>>>>
>>>> Expand the comment to further clarify the corner case.
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>>>> ---
>>>> v3->v4:
>>>> * new patch: this change was split from ("xen/arm: Move is_protected flag to struct device")
>>>> ---
>>>>    xen/drivers/passthrough/device_tree.c | 11 ++++++++---
>>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>>>> index 1c32d7b50cce..d9b63da7260a 100644
>>>> --- a/xen/drivers/passthrough/device_tree.c
>>>> +++ b/xen/drivers/passthrough/device_tree.c
>>>> @@ -141,12 +141,17 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>>            return -EINVAL;
>>>>
>>>>        /*
>>>> -     * The device may already have been registered. As there is no harm in
>>>> -     * it just return success early.
>>>> +     * Devices that appear in the legacy mmu-masters list may have already been
>>>> +     * registered with the SMMU. In case a device has both a mmu-masters entry
>>>> +     * and iommus property, there is no need to register it again. In this case
>>>> +     * simply return success early.
>>>>         */
>>>> -    if ( dev_iommu_fwspec_get(dev) )
>>>> +    if ( dt_device_is_protected(np) )
>>> ... we now have two checks and it implies that it would be normal for
>>> dt_device_is_protected() to be false and ...
>>>
>>>>            return 0;
>>>>
>>>> +    if ( dev_iommu_fwspec_get(dev) )
>>>
>>> ... this returning a non-zero value. Is there any other benefits with
>>> adding the two checks?
>>
>> No, I can't think of any good rationale for the 2nd check. After splitting this change from the other patch ("xen/arm: Move is_protected flag to struct device"), I simply wanted to evaluate it on its own.
>>
>>> If the others agree with the double check, then I think this should gain
>>> an ASSERT_UNREACHABLE() because AFAIU this is a programming error.
>>
>> Right, if the 2nd check was justified, there should be an ASSERT_UNREACHABLE(), good point. But I don't think the 2nd check is justified.
>>
>> If the 2nd check is dropped (keeping only the is_protected check), then do you think the change is justified?
> 
> To be honest not with the current justification. I don't view the new
> check better than the other in term of readability.
> 
> Is this the only reason you want to switch to dt_device_is_protected()?

It was switched originally in the downstream I cherry-picked the patch ("xen/arm: Move is_protected flag to struct device") [1] from, where the rationale for the change wasn't explicitly written. Improving readability was the only rationale I could think of.

Anyway, I will drop this change for the next revision of this series. Hmm. The comment may still want to be expanded though... But I feel improving the comment alone should not be part of this series.

[1] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/59753aac77528a584d3950936b853ebf264b68e7#9e9e9f5f577b2b9fc19d92dcefe7580c7c3af744

Re: [PATCH v4 2/7] xen/arm: Move is_protected flag to struct device

[Stewart Hildebrand]

On 6/29/23 18:22, Julien Grall wrote:
> Hi Stewart,
> 
> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> This flag will be re-used for PCI devices by the subsequent
>> patches.
> 
> I was expecting that we would only do PCI passthrough on boards where
> all the PCI devices are behind an IOMMU. So it would be a all or nothing
> and therefore is_protected would not be used.

That makes sense.

> Do you have an example where this wouldn't be the case?

No. I'll drop this patch for v5 of the series.

Re: [PATCH v4 3/7] iommu/arm: Add iommu_dt_xlate()

[Stewart Hildebrand]

On 6/29/23 18:29, Julien Grall wrote:
> Hi Stewart,
> 
> On 07/06/2023 04:02, Stewart Hildebrand wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Move code for processing DT IOMMU specifier to a separate helper.
>> This helper will be re-used for adding PCI devices by the subsequent
>> patches as we will need exact the same actions for processing
>> DT PCI-IOMMU specifier.
>>
>> While at it introduce NO_IOMMU to avoid magic "1".
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>> v3->v4:
>> * make dt_phandle_args *iommu_spec const
>> * move !ops->add_device check to helper
>>
>> v2->v3:
>> * no change
>>
>> v1->v2:
>> * no change
>>
>> downstream->v1:
>> * trivial rebase
>> * s/dt_iommu_xlate/iommu_dt_xlate/
>>
>> (cherry picked from commit c26bab0415ca303df86aba1d06ef8edc713734d3 from
>>   the downstream branch poc/pci-passthrough from
>>   https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
>> ---
>>   xen/drivers/passthrough/device_tree.c | 47 ++++++++++++++++++---------
>>   1 file changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>> index c60e78eaf556..ff9e66ebf92a 100644
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -127,15 +127,42 @@ int iommu_release_dt_devices(struct domain *d)
>>       return 0;
>>   }
>>
>> +/* This correlation must not be altered */
> 
> Please expand why.

I don't have any insight regarding the rationale for why the comment was added. I don't believe the comment is adding any value, so I'll remove it.

>> +#define NO_IOMMU    1
> 
> Given that the value is returned, should not this be moved to an header
> and used by the callers?

Moving it to a header: yes. I'll move it to xen/include/xen/iommu.h, as that is where the iommu_add_dt_device() prototype also lives.

Used by callers: the callers currently are checking for error by comparing ( rc < 0 ). I think that's okay to leave as is since the comment in iommu.h by the prototype describe the possible return value for iommu_add_dt_device() as:
 * Return values:
 *  0 : device is protected by an IOMMU
 * <0 : device is not protected by an IOMMU, but must be (error condition)
 * >0 : device doesn't need to be protected by an IOMMU
 *      (IOMMU is not enabled/present or device is not connected to it).

>> +
>> +static int iommu_dt_xlate(struct device *dev,
>> +                          const struct dt_phandle_args *iommu_spec)
>> +{
>> +    const struct iommu_ops *ops = iommu_get_ops();
>> +    int rc;
>> +
>> +    if ( !ops->dt_xlate )
>> +        return -EINVAL;
>> +
>> +    if ( !dt_device_is_available(iommu_spec->np) )
>> +        return NO_IOMMU;
>> +
>> +    rc = iommu_fwspec_init(dev, &iommu_spec->np->dev);
>> +    if ( rc )
>> +        return rc;
>> +
>> +    /*
>> +     * Provide DT IOMMU specifier which describes the IOMMU master
>> +     * interfaces of that device (device IDs, etc) to the driver.
>> +     * The driver is responsible to decide how to interpret them.
>> +     */
>> +    return ops->dt_xlate(dev, iommu_spec);
>> +}
>> +
>>   int iommu_add_dt_device(struct dt_device_node *np)
>>   {
>>       const struct iommu_ops *ops = iommu_get_ops();
>>       struct dt_phandle_args iommu_spec;
>>       struct device *dev = dt_to_dev(np);
>> -    int rc = 1, index = 0;
>> +    int rc = NO_IOMMU, index = 0;
>>
>>       if ( !iommu_enabled )
>> -        return 1;
>> +        return NO_IOMMU;
>>
>>       if ( !ops )
>>           return -EINVAL;
>> @@ -163,22 +190,10 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>            * The driver which supports generic IOMMU DT bindings must have
>>            * these callback implemented.
> 
> The 's' was missing to callback. But now, I think you want to replace
> 'these' with 'this' as you move the second check.

I will fix it

>>            */
>> -        if ( !ops->add_device || !ops->dt_xlate )
>> +        if ( !ops->add_device )
>>               return -EINVAL;
>>
>> -        if ( !dt_device_is_available(iommu_spec.np) )
>> -            break;
>> -
>> -        rc = iommu_fwspec_init(dev, &iommu_spec.np->dev);
>> -        if ( rc )
>> -            break;
>> -
>> -        /*
>> -         * Provide DT IOMMU specifier which describes the IOMMU master
>> -         * interfaces of that device (device IDs, etc) to the driver.
>> -         * The driver is responsible to decide how to interpret them.
>> -         */
>> -        rc = ops->dt_xlate(dev, &iommu_spec);
>> +        rc = iommu_dt_xlate(dev, &iommu_spec);
>>           if ( rc )
>>               break;
>>
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v4 4/7] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

[Stewart Hildebrand]

On 7/4/23 05:35, Julien Grall wrote:
> Hi,
> 
> On 07/06/2023 04:02, Stewart Hildebrand 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 of_map_id to translate an ID through a downstream mapping
> 
> You called the function dt_map_id in Xen.

I will fix

>> (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>
>> ---
>> 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/drivers/passthrough/device_tree.c | 134 ++++++++++++++++++++++++++
>>   xen/include/xen/device_tree.h         |  25 +++++
>>   xen/include/xen/iommu.h               |  22 ++++-
>>   3 files changed, 180 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>> index ff9e66ebf92a..bd0aed5df651 100644
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -154,6 +154,140 @@ static int iommu_dt_xlate(struct device *dev,
>>       return ops->dt_xlate(dev, iommu_spec);
>>   }
>>
>> +#ifdef CONFIG_HAS_PCI
> 
> The code below doesn't seem to be specific to PCI.

I will move dt_map_id() outside the #ifdef CONFIG_HAS_PCI ...

> 
>> +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)
> 
> AFAICT, this function can also be used outside of the IOMMU code. So
> shouldn't this be implemented in common/device_tree.c?

... to common/device_tree.c.

>> +{
>> +    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: Error: Bad %s length: %u\n", np->full_name,
>> +               map_name, map_len);
> 
> I think it would be helpful if you add the function name in the error
> message.

Will do

>> +        return -EINVAL;
>> +    }
>> +
>> +    /* The default is to select all bits. */
>> +    map_mask = 0xffffffff;
> 
> Please add a U. That said, I would switch to GENMASK(31, 0) so it is
> easier to check the value.

OK, I will use GENMASK and add #include <xen/bitops.h>.

>> +
>> +    /*
>> +     * Can be overridden by "{iommu,msi}-map-mask" property.
>> +     * If df_property_read_u32() fails, the default is used.
> 
> s/df/dt/

Will fix, thanks for spotting this

>> +     */
>> +    if ( map_mask_name )
>> +        dt_property_read_u32(np, map_mask_name, &map_mask);
>> +
>> +    masked_id = map_mask & id;
>> +    for ( ; (int)map_len > 0; map_len -= 4 * sizeof(*map), map += 4 )
> 
> Why do you cast map_len to 'int'?

No good reason, I will remove the cast.

>> +    {
>> +        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: Invalid %s translation - %s-mask (0x%"PRIx32") ignores id-base (0x%"PRIx32")\n",
>> +                   np->full_name, map_name, map_name, map_mask, id_base);
> 
> Same here about adding the function name.

Will do

>> +            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;
>> +
>> +        printk(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",
> 
> How about using dprintk()?

OK

>> +               np->full_name, map_name, map_mask, id_base, out_base, id_len, id,
>> +               masked_id - id_base + out_base);
>> +        return 0;
>> +    }
>> +
>> +    printk(XENLOG_ERR "%s: no %s translation for id 0x%"PRIx32" on %s\n",
>> +           np->full_name, map_name, id, (target && *target) ? (*target)->full_name : NULL);
> 
> Same here about adding the function name.

I'll change this one to dprintk too, since it's not necessarily an error condition to reach here (see below).

>> +
>> +    /*
>> +     * NOTE: Linux bypasses translation without returning an error here,
>> +     * but should we behave in the same way on Xen? Restrict for now.
>> +     */
> 
> Can you outline why we would want to bypass the translation?

We can reach here if the iommu-map property doesn't cover all possible RIDs, or is otherwise malformed in some way. E.g. you might have iommu-map = <0x0 &smmu 0x8000 0x8000>;. RIDs 0x0-0x7fff will get translated, but RIDs 0x8000-0xffff won't. Also, RIDs 0x8000-0xffff won't have the &smmu phandle reference.

Reference the iommu-map DT binding [1] linked in the commit message for more info.

On an ARM platform, it would probably be an error to not apply a RID -> stream ID translation. However, this code is not ARM specific. Also, in case of no translation, the SMMU driver will fail to add the device, because the *target = phandle_node; line above would not get executed. So I think we can safely change it to return 0, which is the same as of_map_id in Linux.

>> +    return -EFAULT;
>> +}
>> +
>> +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 = NO_IOMMU;
> 
> AFAICT, the initial value will never be read. So is it necessary?

You're right, no need to initialize. I will fix.

>> +
>> +    if ( !iommu_enabled )
>> +        return NO_IOMMU;
>> +
>> +    if ( !ops )
>> +        return -EINVAL;
>> +
>> +    if ( device_is_protected(dev) )
>> +        return 0;
> 
> These two lines are a bit odd to read because you would think that if
> the device is protected, then you want to continue translation. So can
> you add a comment explaining what this check means?

Since dropping ("xen/arm: Move is_protected flag to struct device") the is_protected check is no longer relevant. I will remove it.

>> +
>> +    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) ? NO_IOMMU : rc;
>> +
>> +    rc = iommu_dt_xlate(dev, &iommu_spec);
>> +    if ( rc < 0 )
>> +    {
>> +        iommu_fwspec_free(dev);
>> +        return -EINVAL;
>> +    }
>> +
>> +    return rc;
>> +}
>> +#endif /* CONFIG_HAS_PCI */
>> +
>>   int iommu_add_dt_device(struct dt_device_node *np)
>>   {
>>       const struct iommu_ops *ops = iommu_get_ops();
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index c2f315140560..8385cd538a58 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -892,6 +892,31 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
>>    */
>>   int dt_get_pci_domain_nr(struct dt_device_node *node);
>>
>> +#ifdef CONFIG_HAS_PCI
> 
> In Xen, we don't usually add #ifdef for prototype only.

OK, I will remove the #ifdef

>> +/**
>> + * 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);
>> +#endif /* CONFIG_HAS_PCI */
>> +
>>   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 405db59971c5..3cac177840f7 100644
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -26,6 +26,9 @@
>>   #include <xen/spinlock.h>
>>   #include <public/domctl.h>
>>   #include <public/hvm/ioreq.h>
>> +#ifdef CONFIG_ACPI
>> +#include <asm/acpi.h>
>> +#endif
>>   #include <asm/device.h>
>>
>>   TYPE_SAFE(uint64_t, dfn);
>> @@ -219,7 +222,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
>> @@ -228,12 +232,28 @@ int iommu_release_dt_devices(struct domain *d);
>>    *      (IOMMU is not enabled/present or device is not connected to it).
>>    */
>>   int iommu_add_dt_device(struct dt_device_node *np);
>> +int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev);
>>
>>   int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
>>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>>
>> +#else /* !HAS_DEVICE_TREE */
>> +static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
>> +{
>> +    return 0;
> Shouldn't this return an error because we wouldn't be able to add the
> Device?

Yep, I will fix

>> +}
>>   #endif /* HAS_DEVICE_TREE */
>>
>> +static inline int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
>> +{
>> +    int ret = 0;
> 
> Same here.

OK

>> +#ifdef CONFIG_ACPI
>> +    if ( acpi_disabled )
>> +#endif
>> +        ret = iommu_add_dt_pci_sideband_ids(pdev);
>> +    return ret;
>> +}
>> +
>>   struct page_info;
>>
>>   /*
> 
> Cheers,
> 
> -- 
> Julien Grall