[PATCH v1 0/8] Implement PCI device enumeration on Arm part 1

[PATCH v1 0/8] Implement PCI device enumeration on Arm part 1

[Mykyta Poturai]

This series adds basic PCI device enumeration in Xen on Arm. This will allow us
to not rely on Dom0 enumeration for supported controllers, which will enable PCI
passthrough for dom0less setups.

Enumeration is disabled by default and can be enabled with "pci-scan" cmdline
option.

For now the discovered devices are only assigned to HW domain. To achieve this,
several things need to be done:
1. A VPCI node is created for HW domain device tree, and the real PCI nodes are
hidden from it.
2. Discovered devices BARs are initialized.
3. Register handles for VPCI are updated to change behaviour depending on
whether or not the calling domain uses VPCI or HW PCI, instead of relying on
is_hardware_domain()
4. PCI physdev ops are disabled for HW domain using VPCI.

Edward Pickup (1):
  arm/pci: Add pci-scan boot argument

Luca Fancellu (3):
  xen/pci: update DT for hwdom when it uses vpci
  xen/pci: disable pci_device_{add,remove} when hwdom uses vpci on arm
  xen/pci: assign discovered devices to hwdom

Mykyta Poturai (1):
  arm/pci: enable vpci for hwdom when pci-scan is enabled

Stefano Stabellini (1):
  xen/pci: introduce has_vpci_bridge

Stewart Hildebrand (2):
  xen/dt: pass flags to callback in dt_for_each_range()
  xen/pci: initialize BARs

 docs/misc/xen-command-line.pandoc       |   9 ++
 xen/arch/arm/device.c                   |   4 +-
 xen/arch/arm/domain_build.c             | 154 +++++++++++++++++++++++-
 xen/arch/arm/include/asm/domain.h       |   3 +-
 xen/arch/arm/include/asm/domain_build.h |   1 +
 xen/arch/arm/include/asm/pci.h          |  25 ++++
 xen/arch/arm/include/asm/setup.h        |   2 +-
 xen/arch/arm/pci/pci-host-common.c      |  83 ++++++++++++-
 xen/arch/arm/pci/pci.c                  |  24 +++-
 xen/arch/x86/include/asm/pci.h          |  20 +++
 xen/common/device-tree/device-tree.c    |   5 +-
 xen/common/rangeset.c                   |  35 ++++++
 xen/drivers/passthrough/arm/iommu.c     |   9 ++
 xen/drivers/passthrough/pci.c           | 119 +++++++++++++++++-
 xen/drivers/pci/physdev.c               |   6 +
 xen/drivers/vpci/header.c               |  14 +--
 xen/drivers/vpci/vpci.c                 |   4 +-
 xen/include/xen/device_tree.h           |   2 +-
 xen/include/xen/rangeset.h              |   6 +-
 xen/include/xen/vpci.h                  |   9 ++
 20 files changed, 509 insertions(+), 25 deletions(-)

-- 
2.34.1

[PATCH v1 1/8] arm/pci: Add pci-scan boot argument

[Mykyta Poturai]

From: Edward Pickup <Edward.Pickup@arm.com>

This patch adds a Xen boot arguments that, if enabled, causes a call to
existing code to scan pci devices enumerated by the firmware.

This will be needed ahead of dom0less support for pci passthrough on
arm.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
(cherry picked from commit bce463e1588a45e1bfdf59fc0d5f88b16604e439 from
 the downstream branch poc/pci-passthrough from
 https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)

v1->v2:
* remove is_pci_scan_enabled wrapper
* make pci_scan_enabled ro_after_init
* drop debug prints
* drop Edward's SOB

changes since cherry-pick:
* s/always_inline/inline/
* replace additional kconfig option with config DEBUG
---
 docs/misc/xen-command-line.pandoc  |  7 +++++++
 xen/arch/arm/include/asm/pci.h     |  3 +++
 xen/arch/arm/pci/pci-host-common.c |  1 +
 xen/arch/arm/pci/pci.c             | 24 ++++++++++++++++++++++--
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 518e42d965..4a66c5a8f9 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2072,6 +2072,13 @@ This option can be specified more than once (up to 8 times at present).
 
 Flag to enable or disable support for PCI passthrough
 
+### pci-scan (arm)
+> `= <boolean>`
+
+> Default: `false`
+
+Flag to enable or disable Xen PCI scan at boot.
+
 ### pcid (x86)
 > `= <boolean> | xpti=<bool>`
 
diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 08ffcd4438..7289f7688b 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -23,6 +23,7 @@
 #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
 
 extern bool pci_passthrough_enabled;
+extern bool pci_scan_enabled;
 
 struct rangeset;
 
@@ -155,6 +156,8 @@ bool arch_pci_device_physdevop(void);
 
 #else   /*!CONFIG_HAS_PCI*/
 
+#define pci_scan_enabled false
+
 struct pci_dev;
 
 static inline void arch_pci_init_pdev(struct pci_dev *pdev) {}
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 487c545f3a..d3481b05eb 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -284,6 +284,7 @@ pci_host_common_probe(struct dt_device_node *dev,
     }
 
     pci_add_host_bridge(bridge);
+    pci_add_segment(bridge->segment);
 
     return bridge;
 
diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
index beb1f971fa..1b34e17517 100644
--- a/xen/arch/arm/pci/pci.c
+++ b/xen/arch/arm/pci/pci.c
@@ -91,8 +91,14 @@ bool arch_pci_device_physdevop(void)
 bool __read_mostly pci_passthrough_enabled;
 boolean_param("pci-passthrough", pci_passthrough_enabled);
 
+/* By default pci scan is disabled. */
+bool __ro_after_init pci_scan_enabled;
+boolean_param("pci-scan", pci_scan_enabled);
+
 static int __init pci_init(void)
 {
+    int ret;
+
     /*
      * Enable PCI passthrough when has been enabled explicitly
      * (pci-passthrough=on).
@@ -104,9 +110,23 @@ static int __init pci_init(void)
         panic("Could not initialize PCI segment 0\n");
 
     if ( acpi_disabled )
-        return dt_pci_init();
+        ret = dt_pci_init();
     else
-        return acpi_pci_init();
+        ret = acpi_pci_init();
+
+    if ( ret < 0 )
+        return ret;
+
+    if ( pci_scan_enabled )
+    {
+        ret = scan_pci_devices();
+
+        if ( ret < 0 )
+            return ret;
+
+    }
+
+    return 0;
 }
 __initcall(pci_init);
 
-- 
2.34.1

[PATCH v1 2/8] xen/dt: pass flags to callback in dt_for_each_range()

[Mykyta Poturai]

From: Stewart Hildebrand <stewart.hildebrand@amd.com>

PCI ranges have prefetchable / non-prefetchable flags that will be
needed in the callback. Pass the flags to the callback.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 xen/arch/arm/device.c                | 4 ++--
 xen/arch/arm/domain_build.c          | 3 ++-
 xen/arch/arm/include/asm/setup.h     | 2 +-
 xen/arch/arm/pci/pci-host-common.c   | 4 ++--
 xen/common/device-tree/device-tree.c | 5 +++--
 xen/include/xen/device_tree.h        | 2 +-
 6 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 74b54cad34..cc0759023e 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -50,7 +50,7 @@ int map_irq_to_domain(struct domain *d, unsigned int irq,
 }
 
 int map_range_to_domain(const struct dt_device_node *dev,
-                        uint64_t addr, uint64_t len, void *data)
+                        uint32_t flags, uint64_t addr, uint64_t len, void *data)
 {
     struct map_range_data *mr_data = data;
     struct domain *d = mr_data->d;
@@ -325,7 +325,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
             return res;
         }
 
-        res = map_range_to_domain(dev, addr, size, &mr_data);
+        res = map_range_to_domain(dev, 0, addr, size, &mr_data);
         if ( res )
             return res;
     }
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index fb8fbb1650..4bbffdf535 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -762,7 +762,8 @@ int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
 }
 
 static int __init handle_pci_range(const struct dt_device_node *dev,
-                                   uint64_t addr, uint64_t len, void *data)
+                                   uint32_t flags, uint64_t addr, uint64_t len,
+                                   void *data)
 {
     struct rangeset *mem_holes = data;
     paddr_t start, end;
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 1eaf13bd66..97bc5f90a1 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -62,7 +62,7 @@ int map_device_irqs_to_domain(struct domain *d, struct dt_device_node *dev,
 int map_irq_to_domain(struct domain *d, unsigned int irq,
                       bool need_mapping, const char *devname);
 
-int map_range_to_domain(const struct dt_device_node *dev,
+int map_range_to_domain(const struct dt_device_node *dev, uint32_t flags,
                         uint64_t addr, uint64_t len, void *data);
 
 struct init_info
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index d3481b05eb..67447d8696 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -418,7 +418,7 @@ int __init pci_host_bridge_mappings(struct domain *d)
                     bridge->child_ops->need_p2m_hwdom_mapping(d, bridge, addr);
             if ( need_mapping )
             {
-                err = map_range_to_domain(dev, addr, size, &mr_data);
+                err = map_range_to_domain(dev, 0, addr, size, &mr_data);
                 if ( err )
                     return err;
             }
@@ -434,7 +434,7 @@ int __init pci_host_bridge_mappings(struct domain *d)
  * right place for alignment check.
  */
 static int is_bar_valid(const struct dt_device_node *dev,
-                        uint64_t addr, uint64_t len, void *data)
+                        uint32_t flags, uint64_t addr, uint64_t len, void *data)
 {
     struct pdev_bar_check *bar_data = data;
     paddr_t s = bar_data->start;
diff --git a/xen/common/device-tree/device-tree.c b/xen/common/device-tree/device-tree.c
index 0b5375f151..5ee1fa4eb1 100644
--- a/xen/common/device-tree/device-tree.c
+++ b/xen/common/device-tree/device-tree.c
@@ -976,7 +976,7 @@ int dt_device_get_paddr(const struct dt_device_node *dev, unsigned int index,
 
 int dt_for_each_range(const struct dt_device_node *dev,
                       int (*cb)(const struct dt_device_node *dev,
-                                uint64_t addr, uint64_t length,
+                                uint32_t flags, uint64_t addr, uint64_t length,
                                 void *data),
                       void *data)
 {
@@ -1041,13 +1041,14 @@ int dt_for_each_range(const struct dt_device_node *dev,
     {
         uint64_t a, s;
         int ret;
+        uint32_t flags = bus->get_flags(ranges);
 
         memcpy(addr, ranges + na, 4 * pna);
 
         a = __dt_translate_address(dev, addr, "ranges");
         s = dt_read_number(ranges + na + pna, ns);
 
-        ret = cb(dev, a, s, data);
+        ret = cb(dev, flags, a, s, data);
         if ( ret )
         {
             dt_dprintk(" -> callback failed=%d\n", ret);
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 06d7643622..1091e34a10 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -667,7 +667,7 @@ int dt_for_each_irq_map(const struct dt_device_node *dev,
  */
 int dt_for_each_range(const struct dt_device_node *dev,
                       int (*cb)(const struct dt_device_node *dev,
-                                uint64_t addr, uint64_t length,
+                                uint32_t flags, uint64_t addr, uint64_t length,
                                 void *data),
                       void *data);
 
-- 
2.34.1

[PATCH v1 3/8] xen/pci: update DT for hwdom when it uses vpci

[Mykyta Poturai]

From: Luca Fancellu <luca.fancellu@arm.com>

When pci-scan is enabled and Xen supports vpci for guests, Xen will
scan the pci bus to find devices and emulate the pci bus, so the hw
domain must see the emulated bus instead of the real one.

A new helper function, hwdom_uses_vpci, is implemented and returns true
when pci-scan is enabled and Xen is built with
CONFIG_HAS_VPCI_GUEST_SUPPORT=y. When hwdom_uses_vpci() is true, a vpci
node is created for the hwdom device tree.

Depending on whether the guest is using vPCI or not, and whether the
domain is using host layout or not, generate the appropriate device tree
nodes for the guest and handle the right MMIO regions traps.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 docs/misc/xen-command-line.pandoc       |   4 +-
 xen/arch/arm/domain_build.c             | 151 +++++++++++++++++++++++-
 xen/arch/arm/include/asm/domain_build.h |   1 +
 xen/arch/arm/include/asm/pci.h          |  15 +++
 xen/arch/x86/include/asm/pci.h          |   6 +
 5 files changed, 175 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 4a66c5a8f9..ac8e7bfb7a 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2077,7 +2077,9 @@ Flag to enable or disable support for PCI passthrough
 
 > Default: `false`
 
-Flag to enable or disable Xen PCI scan at boot.
+Flag to enable or disable Xen PCI scan at boot. When the flag is enabled, the
+hardware domain cannot have access to the real PCI bus, it will see the bus
+emulated by Xen.
 
 ### pcid (x86)
 > `= <boolean> | xpti=<bool>`
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4bbffdf535..f49e0e6486 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -41,6 +41,7 @@
 #include <xen/grant_table.h>
 #include <asm/grant_table.h>
 #include <xen/serial.h>
+#include <xen/resource.h>
 
 static unsigned int __initdata opt_dom0_max_vcpus;
 integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
@@ -1557,6 +1558,142 @@ int __init make_chosen_node(const struct kernel_info *kinfo)
     return res;
 }
 
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+struct vpci_param {
+   uint64_t vpci_ecam_base;
+   uint64_t vpci_ecam_size;
+   uint64_t vpci_mem_base;
+   uint64_t vpci_mem_size;
+   uint64_t vpci_mem_prefetch_base;
+   uint64_t vpci_mem_prefetch_size;
+};
+
+static int __init handle_vpci_range(const struct dt_device_node *dev,
+                                    uint32_t flags, uint64_t addr, uint64_t len,
+                                    void *data)
+{
+    struct vpci_param *vpci = (struct vpci_param *)data;
+
+    if ( !(flags & IORESOURCE_MEM) )
+        return 0;
+
+    if ( !(flags & IORESOURCE_PREFETCH) && addr < GB(4) )
+    {
+        vpci->vpci_mem_base = addr;
+        vpci->vpci_mem_size = len;
+    }
+    else if ( flags & IORESOURCE_PREFETCH )
+    {
+        vpci->vpci_mem_prefetch_base = addr;
+        vpci->vpci_mem_prefetch_size = len;
+    }
+    return 0;
+}
+
+int __init make_vpci_node(struct domain *d, void *fdt)
+{
+    /* reg is sized to be used for all the needed properties below */
+    __be32 reg[((GUEST_ROOT_ADDRESS_CELLS * 2) + GUEST_ROOT_SIZE_CELLS + 1)
+               * 2];
+    __be32 *cells;
+    char buf[22]; /* pcie@ + max 16 char address + '\0' */
+    int res;
+    struct vpci_param vpci = {
+        .vpci_ecam_base = GUEST_VPCI_ECAM_BASE,
+        .vpci_ecam_size = GUEST_VPCI_ECAM_SIZE,
+        .vpci_mem_base = GUEST_VPCI_MEM_ADDR,
+        .vpci_mem_size = GUEST_VPCI_MEM_SIZE,
+        .vpci_mem_prefetch_base = GUEST_VPCI_PREFETCH_MEM_ADDR,
+        .vpci_mem_prefetch_size = GUEST_VPCI_PREFETCH_MEM_SIZE
+    };
+
+    if ( domain_use_host_layout(d) )
+    {
+        struct pci_host_bridge *bridge;
+
+        bridge = pci_find_host_bridge(0, 0);
+
+        vpci.vpci_ecam_base = bridge->cfg->phys_addr;
+        vpci.vpci_ecam_size = bridge->cfg->size;
+
+        res = dt_for_each_range(bridge->dt_node, handle_vpci_range, &vpci);
+        if ( res < 0 )
+            return -EINVAL;
+    }
+
+    snprintf(buf, sizeof(buf), "pcie@%"PRIx64, vpci.vpci_ecam_base);
+    dt_dprintk("Create vpci node\n");
+    res = fdt_begin_node(fdt, buf);
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "compatible", "pci-host-ecam-generic");
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "device_type", "pci");
+    if ( res )
+        return res;
+
+    /* Create reg property */
+    cells = &reg[0];
+    dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+                       vpci.vpci_ecam_base, vpci.vpci_ecam_size);
+
+    res = fdt_property(fdt, "reg", reg,
+                       (GUEST_ROOT_ADDRESS_CELLS +
+                       GUEST_ROOT_SIZE_CELLS) * sizeof(*reg));
+    if ( res )
+        return res;
+
+    /* Create bus-range property */
+    cells = &reg[0];
+    dt_set_cell(&cells, 1, 0);
+    dt_set_cell(&cells, 1, 255);
+    res = fdt_property(fdt, "bus-range", reg, 2 * sizeof(*reg));
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "#address-cells", 3);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "#size-cells", 2);
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "status", "okay");
+    if ( res )
+        return res;
+
+    /*
+     * Create ranges property as:
+     * <(PCI bitfield) (PCI address) (CPU address) (Size)>
+     */
+    cells = &reg[0];
+    dt_set_cell(&cells, 1, GUEST_VPCI_ADDR_TYPE_MEM);
+    dt_set_cell(&cells, GUEST_ROOT_ADDRESS_CELLS, vpci.vpci_mem_base);
+    dt_set_cell(&cells, GUEST_ROOT_ADDRESS_CELLS, vpci.vpci_mem_base);
+    dt_set_cell(&cells, GUEST_ROOT_SIZE_CELLS, vpci.vpci_mem_size);
+    dt_set_cell(&cells, 1, GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM);
+    dt_set_cell(&cells, GUEST_ROOT_ADDRESS_CELLS, vpci.vpci_mem_prefetch_base);
+    dt_set_cell(&cells, GUEST_ROOT_ADDRESS_CELLS, vpci.vpci_mem_prefetch_base);
+    dt_set_cell(&cells, GUEST_ROOT_SIZE_CELLS, vpci.vpci_mem_prefetch_size);
+    res = fdt_property(fdt, "ranges", reg, sizeof(reg));
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+#else
+static inline int __init make_vpci_node(struct domain *d, void *fdt)
+{
+    return 0;
+}
+#endif
+
 static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
                               struct dt_device_node *node,
                               p2m_type_t p2mt)
@@ -1615,7 +1752,12 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
         dt_dprintk("  Skip it (blacklisted)\n");
         return 0;
     }
-
+    /* If Xen is scanning the PCI devices, don't expose real bus to hwdom */
+    if ( hwdom_uses_vpci() && dt_device_type_is_equal(node, "pci") )
+    {
+        dt_dprintk("  Skip it (pci-scan is enabled)\n");
+        return 0;
+    }
     /*
      * Replace these nodes with our own. Note that the original may be
      * used_by DOMID_XEN so this check comes first.
@@ -1766,6 +1908,13 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
             if ( res )
                 return res;
         }
+
+        if ( hwdom_uses_vpci() )
+        {
+            res = make_vpci_node(d, kinfo->fdt);
+            if ( res )
+                return res;
+        }
     }
 
     res = fdt_end_node(kinfo->fdt);
diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
index c6fec3168c..bb36e0150b 100644
--- a/xen/arch/arm/include/asm/domain_build.h
+++ b/xen/arch/arm/include/asm/domain_build.h
@@ -6,6 +6,7 @@
 
 typedef __be32 gic_interrupt_t[3];
 int make_psci_node(void *fdt);
+int make_vpci_node(struct domain *d, void *fdt);
 void evtchn_allocate(struct domain *d);
 
 /*
diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 7289f7688b..4fc46da315 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -176,4 +176,19 @@ static inline int pci_get_new_domain_nr(void)
 }
 
 #endif  /*!CONFIG_HAS_PCI*/
+
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+static inline bool hwdom_uses_vpci(void)
+{
+    return pci_scan_enabled;
+}
+
+#else  /*!CONFIG_HAS_VPCI_GUEST_SUPPORT*/
+
+static inline bool hwdom_uses_vpci(void)
+{
+    return false;
+}
+#endif  /*!CONFIG_HAS_VPCI_GUEST_SUPPORT*/
+
 #endif /* __ARM_PCI_H__ */
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index 0b98081aea..08e8959d0d 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -76,4 +76,10 @@ int pci_sanitize_bar_memory(struct rangeset *r);
 
 void pci_setup(void);
 
+/* Unlike ARM, HW domain does not ever use vpci for x86 */
+static inline bool hwdom_uses_vpci(void)
+{
+    return false;
+}
+
 #endif /* __X86_PCI_H__ */
-- 
2.34.1

[PATCH v1 4/8] xen/pci: disable pci_device_{add,remove} when hwdom uses vpci on arm

[Mykyta Poturai]

From: Luca Fancellu <luca.fancellu@arm.com>

On ARM, if the hardware domain is using the emulated bus, it should not
be allowed to add/remove pci devices, so return EOPNOTSUPP in that case.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 xen/drivers/pci/physdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
index d46501b884..5b3f8dde14 100644
--- a/xen/drivers/pci/physdev.c
+++ b/xen/drivers/pci/physdev.c
@@ -19,6 +19,9 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct pci_dev_info pdev_info;
         nodeid_t node = NUMA_NO_NODE;
 
+        if ( hwdom_uses_vpci() )
+            return -EOPNOTSUPP;
+
         if ( !is_pci_passthrough_enabled() && !arch_pci_device_physdevop() )
             return -EOPNOTSUPP;
 
@@ -57,6 +60,9 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case PHYSDEVOP_pci_device_remove: {
         struct physdev_pci_device dev;
 
+        if ( hwdom_uses_vpci() )
+            return -EOPNOTSUPP;
+
         if ( !is_pci_passthrough_enabled() && !arch_pci_device_physdevop() )
             return -EOPNOTSUPP;
 
-- 
2.34.1

[PATCH v1 5/8] xen/pci: introduce has_vpci_bridge

[Mykyta Poturai]

From: Stefano Stabellini <stefano.stabellini@amd.com>

has_vpci_bridge is a macro to check if the domain is a domU or is dom0
with vPCI (pci-scan=yes) enabled.

Use the macro in drivers/vpci.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 xen/drivers/vpci/header.c | 14 +++++++-------
 xen/drivers/vpci/vpci.c   |  4 ++--
 xen/include/xen/vpci.h    |  9 +++++++++
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 469f497744..903168ff96 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -230,7 +230,7 @@ bool vpci_process_pending(struct vcpu *v)
 
             read_unlock(&v->domain->pci_lock);
 
-            if ( !is_hardware_domain(v->domain) )
+            if ( has_vpci_bridge(v->domain) )
                 domain_crash(v->domain);
 
             return false;
@@ -492,7 +492,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             }
         }
 
-        if ( !is_hardware_domain(d) )
+        if ( has_vpci_bridge(d) )
             break;
 
         d = dom_xen;
@@ -522,7 +522,7 @@ static void cf_check cmd_write(
 {
     struct vpci_header *header = data;
 
-    if ( !is_hardware_domain(pdev->domain) )
+    if ( has_vpci_bridge(pdev->domain) )
     {
         const struct vpci *vpci = pdev->vpci;
 
@@ -564,7 +564,7 @@ static void cf_check bar_write(
     struct vpci_bar *bar = data;
     bool hi = false;
 
-    ASSERT(is_hardware_domain(pdev->domain));
+    ASSERT(!has_vpci_bridge(pdev->domain));
 
     if ( bar->type == VPCI_BAR_MEM64_HI )
     {
@@ -747,7 +747,7 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
 {
     int rc;
     bool mask_cap_list = false;
-    bool is_hwdom = is_hardware_domain(pdev->domain);
+    bool is_hwdom = !has_vpci_bridge(pdev->domain);
 
     if ( pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST )
     {
@@ -829,7 +829,7 @@ static int vpci_init_ext_capability_list(const struct pci_dev *pdev)
 {
     unsigned int pos = PCI_CFG_SPACE_SIZE;
 
-    if ( !is_hardware_domain(pdev->domain) )
+    if ( has_vpci_bridge(pdev->domain) )
         /* Extended capabilities read as zero, write ignore for DomU */
         return vpci_add_register(pdev->vpci, vpci_read_val, NULL,
                                  pos, 4, (void *)0);
@@ -866,7 +866,7 @@ int vpci_init_header(struct pci_dev *pdev)
     struct vpci_header *header = &pdev->vpci->header;
     struct vpci_bar *bars = header->bars;
     int rc;
-    bool is_hwdom = is_hardware_domain(pdev->domain);
+    bool is_hwdom = !has_vpci_bridge(pdev->domain);
 
     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
 
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 07c7071d0a..8ea89b9805 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -48,7 +48,7 @@ static int assign_virtual_sbdf(struct pci_dev *pdev)
 
     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
 
-    if ( is_hardware_domain(d) )
+    if ( !has_vpci_bridge(d) )
         return 0;
 
     /*
@@ -429,7 +429,7 @@ static const struct pci_dev *translate_virtual_device(const struct domain *d,
 #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
     const struct pci_dev *pdev;
 
-    ASSERT(!is_hardware_domain(d));
+    ASSERT(has_vpci_bridge(d));
     ASSERT(rw_is_locked(&d->pci_lock));
 
     for_each_pdev ( d, pdev )
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 9ae75d946a..e0aecfac72 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -339,6 +339,15 @@ static inline int __must_check vpci_reset_device(struct pci_dev *pdev)
     return vpci_assign_device(pdev);
 }
 
+#ifdef CONFIG_ARM
+#include <asm/pci.h>
+
+#define has_vpci_bridge(d) (!is_hardware_domain(d) || \
+                           (is_hardware_domain(d) && pci_scan_enabled))
+#else
+#define has_vpci_bridge(d) (!is_hardware_domain(d))
+#endif
+
 #endif
 
 /*
-- 
2.34.1

[PATCH v1 6/8] xen/pci: initialize BARs

[Mykyta Poturai]

From: Stewart Hildebrand <stewart.hildebrand@amd.com>

A PCI device must have valid BARs in order to assign it to a domain.  On
ARM, firmware is unlikely to have initialized the BARs, so we must do
this in Xen. During setup_hwdom_pci_devices(), check if each BAR is
valid. If the BAR happens to already be valid, remove the BAR range from
a rangeset of valid PCI ranges so as to avoid overlap when reserving a
new BAR. If not valid, reserve a new BAR address from the rangeset and
write it to the device.

Avaliable ranges are read from DT during init and stored in distinct
rangesets.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 xen/arch/arm/include/asm/pci.h     |  7 +++
 xen/arch/arm/pci/pci-host-common.c | 78 +++++++++++++++++++++++++++++
 xen/arch/x86/include/asm/pci.h     | 14 ++++++
 xen/common/rangeset.c              | 35 +++++++++++++
 xen/drivers/passthrough/pci.c      | 79 ++++++++++++++++++++++++++++++
 xen/include/xen/rangeset.h         |  6 ++-
 6 files changed, 218 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 4fc46da315..ba4caa56ae 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -74,6 +74,8 @@ struct pci_host_bridge {
     struct pci_config_window *child_cfg;
     const struct pci_ops *child_ops;
     void *priv;                      /* Private data of the bridge. */
+    struct rangeset *bar_ranges;
+    struct rangeset *bar_ranges_prefetch;
 };
 
 struct pci_ops {
@@ -154,6 +156,11 @@ void pci_generic_init_bus_range_child(struct dt_device_node *dev,
 
 bool arch_pci_device_physdevop(void);
 
+uint64_t pci_get_new_bar_addr(const struct pci_dev *pdev, uint64_t size,
+                              bool is_64bit, bool prefetch);
+int pci_reserve_bar_range(const struct pci_dev *pdev, uint64_t addr,
+                          uint64_t size, bool prefetch);
+
 #else   /*!CONFIG_HAS_PCI*/
 
 #define pci_scan_enabled false
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 67447d8696..bfe610a403 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -21,6 +21,7 @@
 #include <xen/rwlock.h>
 #include <xen/sched.h>
 #include <xen/vmap.h>
+#include <xen/resource.h>
 
 #include <asm/setup.h>
 
@@ -232,6 +233,21 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
     return domain;
 }
 
+static int add_bar_range(const struct dt_device_node *dev, uint32_t flags,
+                         uint64_t addr, uint64_t len, void *data)
+{
+    struct pci_host_bridge *bridge = data;
+
+    if ( !(flags & IORESOURCE_MEM) )
+        return 0;
+
+    if ( flags & IORESOURCE_PREFETCH )
+        return rangeset_add_range(bridge->bar_ranges_prefetch, addr,
+                                  addr + len - 1);
+    else
+        return rangeset_add_range(bridge->bar_ranges, addr, addr + len - 1);
+}
+
 struct pci_host_bridge *
 pci_host_common_probe(struct dt_device_node *dev,
                       const struct pci_ecam_ops *ops,
@@ -286,6 +302,14 @@ pci_host_common_probe(struct dt_device_node *dev,
     pci_add_host_bridge(bridge);
     pci_add_segment(bridge->segment);
 
+    bridge->bar_ranges = rangeset_new(NULL, "BAR ranges",
+                                      RANGESETF_prettyprint_hex);
+    bridge->bar_ranges_prefetch = rangeset_new(NULL,
+                                               "BAR ranges (prefetchable)",
+                                               RANGESETF_prettyprint_hex);
+    if ( bridge->bar_ranges && bridge->bar_ranges_prefetch )
+        dt_for_each_range(bridge->dt_node, add_bar_range, bridge);
+
     return bridge;
 
 err_child:
@@ -476,6 +500,60 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
 
     return bar_data.is_valid;
 }
+
+uint64_t pci_get_new_bar_addr(const struct pci_dev *pdev, uint64_t size,
+                              bool is_64bit, bool prefetch)
+{
+    struct pci_host_bridge *bridge;
+    struct rangeset *range;
+    uint64_t addr;
+
+    bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
+    if ( !bridge )
+        return 0;
+
+    range = prefetch ? bridge->bar_ranges_prefetch : bridge->bar_ranges;
+
+    if ( size < PAGE_SIZE )
+        size = PAGE_SIZE;
+
+    if ( is_64bit && !rangeset_find_aligned_range(range, size, GB(4), &addr) )
+    {
+        if ( rangeset_remove_range(range, addr, addr + size - 1) )
+        {
+            printk("%s:%d:%s error\n", __FILE__, __LINE__, __func__);
+        }
+        return addr;
+    }
+    if ( !rangeset_find_aligned_range(range, size, 0, &addr) )
+    {
+        if ( !is_64bit && addr >= GB(4) )
+            return 0;
+        if ( rangeset_remove_range(range, addr, addr + size - 1) )
+        {
+            printk("%s:%d:%s error\n", __FILE__, __LINE__, __func__);
+        }
+        return addr;
+    }
+
+    return 0;
+}
+
+int pci_reserve_bar_range(const struct pci_dev *pdev, uint64_t addr,
+                          uint64_t size, bool prefetch)
+{
+    struct pci_host_bridge *bridge;
+    struct rangeset *range;
+
+    bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
+    if ( !bridge )
+        return 0;
+
+    range = prefetch ? bridge->bar_ranges_prefetch : bridge->bar_ranges;
+
+    return rangeset_remove_range(range, addr, addr + size - 1);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index 08e8959d0d..2cabbbb5d0 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -82,4 +82,18 @@ static inline bool hwdom_uses_vpci(void)
     return false;
 }
 
+static inline uint64_t pci_get_new_bar_addr(const struct pci_dev *pdev,
+                                            uint64_t size, bool is_64bit,
+                                            bool prefetch)
+{
+    return 0;
+}
+
+static inline int pci_reserve_bar_range(const struct pci_dev *pdev,
+                                        uint64_t addr, uint64_t size,
+                                        bool prefetch)
+{
+    return 0;
+}
+
 #endif /* __X86_PCI_H__ */
diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index 0e3b9acd35..c1c6f8610c 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -357,6 +357,41 @@ int rangeset_claim_range(struct rangeset *r, unsigned long size,
     return 0;
 }
 
+int rangeset_find_aligned_range(struct rangeset *r, unsigned long size,
+                                unsigned long min, unsigned long *s)
+{
+    struct range *x;
+
+    /* Power of 2 check */
+    if ( (size & (size - 1)) != 0 )
+    {
+        *s = 0;
+        return -EINVAL;
+    }
+
+    read_lock(&r->lock);
+
+    for ( x = first_range(r); x; x = next_range(r, x) )
+    {
+        /* Assumes size is a power of 2 */
+        unsigned long start_aligned = (x->s + size - 1) & ~(size - 1);
+
+        if ( x->e > start_aligned &&
+             (x->e - start_aligned) >= size &&
+             start_aligned >= min )
+        {
+            read_unlock(&r->lock);
+            *s = start_aligned;
+            return 0;
+        }
+    }
+
+    read_unlock(&r->lock);
+    *s = 0;
+
+    return -ENOSPC;
+}
+
 int rangeset_consume_ranges(struct rangeset *r,
                             int (*cb)(unsigned long s, unsigned long e,
                                       void *ctxt, unsigned long *c),
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 3edcfa8a04..4f5de9a542 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1172,6 +1172,80 @@ int __init scan_pci_devices(void)
     return ret;
 }
 
+static void __init cf_check reserve_bar_range(struct pci_dev *pdev, uint8_t reg,
+                                              uint64_t addr, uint64_t size,
+                                              bool is_64bit, bool prefetch)
+{
+    if ( pci_check_bar(pdev, maddr_to_mfn(addr),
+                       maddr_to_mfn(addr + size - 1)) )
+        pci_reserve_bar_range(pdev, addr, size, prefetch);
+}
+
+static void __init cf_check get_new_bar_addr(struct pci_dev *pdev, uint8_t reg,
+                                             uint64_t addr, uint64_t size,
+                                             bool is_64bit, bool prefetch)
+{
+    if ( !pci_check_bar(pdev, maddr_to_mfn(addr),
+                        maddr_to_mfn(addr + size - 1)) )
+    {
+        uint16_t cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
+
+        addr = pci_get_new_bar_addr(pdev, size, is_64bit, prefetch);
+
+        pci_conf_write16(pdev->sbdf, PCI_COMMAND,
+                         cmd & ~(PCI_COMMAND_MEMORY | PCI_COMMAND_IO));
+
+        pci_conf_write32(pdev->sbdf, reg,
+                         (addr & GENMASK(31, 0)) |
+                         (is_64bit ? PCI_BASE_ADDRESS_MEM_TYPE_64 : 0));
+
+        if ( is_64bit )
+            pci_conf_write32(pdev->sbdf, reg + 4, addr >> 32);
+
+        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
+    }
+}
+
+static int __init cf_check bars_iterate(struct pci_seg *pseg, void *arg)
+{
+    struct pci_dev *pdev;
+    unsigned int i, ret, num_bars = PCI_HEADER_NORMAL_NR_BARS;
+    uint64_t addr, size;
+    void (*cb)(struct pci_dev *, uint8_t, uint64_t, uint64_t, bool, bool) = arg;
+
+    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
+    {
+        if ( (pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f) ==
+             PCI_HEADER_TYPE_NORMAL )
+        {
+            for ( i = 0; i < num_bars; i += ret )
+            {
+                uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
+                bool prefetch;
+
+                if ( (pci_conf_read32(pdev->sbdf, reg) & PCI_BASE_ADDRESS_SPACE)
+                     == PCI_BASE_ADDRESS_SPACE_IO )
+                {
+                    ret = 1;
+                    continue;
+                }
+
+                ret = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
+                                      (i == num_bars - 1) ? PCI_BAR_LAST : 0);
+
+                if ( !size )
+                    continue;
+                prefetch = !!(pci_conf_read32(pdev->sbdf, reg) &
+                              PCI_BASE_ADDRESS_MEM_PREFETCH);
+
+                cb(pdev, reg, addr, size, ret == 2, prefetch);
+            }
+        }
+    }
+
+    return 0;
+}
+
 struct setup_hwdom {
     struct domain *d;
     int (*handler)(uint8_t devfn, struct pci_dev *pdev);
@@ -1263,6 +1337,11 @@ void __hwdom_init setup_hwdom_pci_devices(
     struct setup_hwdom ctxt = { .d = d, .handler = handler };
 
     pcidevs_lock();
+    if ( hwdom_uses_vpci() )
+    {
+        pci_segments_iterate(bars_iterate, reserve_bar_range);
+        pci_segments_iterate(bars_iterate, get_new_bar_addr);
+    }
     pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt);
     pcidevs_unlock();
 }
diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 817505badf..e71e810f82 100644
--- a/xen/include/xen/rangeset.h
+++ b/xen/include/xen/rangeset.h
@@ -56,11 +56,15 @@ void rangeset_limit(
 bool __must_check rangeset_is_empty(
     const struct rangeset *r);
 
-/* Add/claim/remove/query/purge a numeric range. */
+/* Add/claim/find/remove/query/purge a numeric range. */
 int __must_check rangeset_add_range(
     struct rangeset *r, unsigned long s, unsigned long e);
 int __must_check rangeset_claim_range(struct rangeset *r, unsigned long size,
                                       unsigned long *s);
+int __must_check rangeset_find_aligned_range(struct rangeset *r,
+                                             unsigned long size,
+                                             unsigned long min,
+                                             unsigned long *s);
 int __must_check rangeset_remove_range(
     struct rangeset *r, unsigned long s, unsigned long e);
 bool __must_check rangeset_contains_range(
-- 
2.34.1

[PATCH v1 7/8] xen/pci: assign discovered devices to hwdom

[Mykyta Poturai]

From: Luca Fancellu <luca.fancellu@arm.com>

Hook up existing PCI setup routines for hwdom into Arm iommu
initialization sequence, only assign endpoint devices.

During scanned PCI device assignment, also permit access to the BAR
ranges if hwdom is using vpci.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 xen/drivers/passthrough/arm/iommu.c |  9 +++++++
 xen/drivers/passthrough/pci.c       | 40 ++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 100545e23f..d110520e0d 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -19,6 +19,7 @@
 #include <xen/device_tree.h>
 #include <xen/iommu.h>
 #include <xen/lib.h>
+#include <xen/sched.h>
 
 #include <asm/device.h>
 
@@ -133,6 +134,12 @@ void arch_iommu_domain_destroy(struct domain *d)
 {
 }
 
+static int iommu_add_hwdom_pci_device(u8 devfn, struct pci_dev *pdev)
+{
+    const struct domain_iommu *hd = dom_iommu(hardware_domain);
+    return iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
+}
+
 void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
 {
     /* Set to false options not supported on ARM. */
@@ -142,6 +149,8 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
     if ( iommu_hwdom_reserved == 1 )
         printk(XENLOG_WARNING "map-reserved dom0-iommu option is not supported on ARM\n");
     iommu_hwdom_reserved = 0;
+
+    setup_hwdom_pci_devices(d, iommu_add_hwdom_pci_device);
 }
 
 /*
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 4f5de9a542..534faaaa84 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -20,6 +20,7 @@
 #include <xen/pci_ids.h>
 #include <xen/list.h>
 #include <xen/prefetch.h>
+#include <xen/iocap.h>
 #include <xen/iommu.h>
 #include <xen/irq.h>
 #include <xen/param.h>
@@ -1040,6 +1041,12 @@ enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
     return pos ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
 }
 
+static bool pdev_is_endpoint(struct pci_dev *pdev)
+{
+    enum pdev_type type = pdev_type(pdev->seg, pdev->bus, pdev->devfn);
+    return type == DEV_TYPE_PCIe_ENDPOINT || type == DEV_TYPE_PCI;
+}
+
 /*
  * find the upstream PCIe-to-PCI/PCIX bridge or PCI legacy bridge
  * return 0: the device is integrated PCI device or PCIe
@@ -1255,7 +1262,7 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom *ctxt,
                                                 struct pci_dev *pdev)
 {
     u8 devfn = pdev->devfn;
-    int err;
+    int err, i, rc;
 
     do {
         err = ctxt->handler(devfn, pdev);
@@ -1276,6 +1283,34 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom *ctxt,
     if ( err )
         printk(XENLOG_ERR "setup of vPCI for d%d failed: %d\n",
                ctxt->d->domain_id, err);
+
+    if ( !hwdom_uses_vpci() )
+        return;
+
+    for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS; i += rc )
+    {
+        uint64_t addr, size;
+        uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
+
+        if ( (pci_conf_read32(pdev->sbdf, reg) & PCI_BASE_ADDRESS_SPACE)
+             == PCI_BASE_ADDRESS_SPACE_IO )
+        {
+            rc = 1;
+            continue;
+        }
+
+        rc = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
+                              (i == PCI_HEADER_NORMAL_NR_BARS - 1)
+                                  ? PCI_BAR_LAST : 0);
+
+        if ( !size )
+            continue;
+
+        err = iomem_permit_access(hardware_domain, paddr_to_pfn(addr),
+                             paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
+        if ( err )
+            break;
+    }
 }
 
 static int __hwdom_init cf_check _setup_hwdom_pci_devices(
@@ -1294,6 +1329,9 @@ static int __hwdom_init cf_check _setup_hwdom_pci_devices(
             if ( !pdev )
                 continue;
 
+            if ( hwdom_uses_vpci() && !pdev_is_endpoint(pdev) )
+                continue;
+
             if ( !pdev->domain )
             {
                 pdev->domain = ctxt->d;
-- 
2.34.1

[PATCH v1 8/8] arm/pci: enable vpci for hwdom when pci-scan is enabled

[Mykyta Poturai]

With pci-scan implemented it is now possible to use vpci for hardware
domains. Update has_vpci to reflrect this change.

Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 xen/arch/arm/include/asm/domain.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index af3e168374..dbe3347cea 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -305,8 +305,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
 
 #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag)
 
-/* vPCI is not available on Arm */
-#define has_vpci(d)    ({ (void)(d); false; })
+#define has_vpci(d)    (is_hardware_domain(d) && hwdom_uses_vpci())
 
 struct arch_vcpu_io {
     struct instr_details dabt_instr; /* when the instruction is decoded */
-- 
2.34.1

Re: [PATCH v1 8/8] arm/pci: enable vpci for hwdom when pci-scan is enabled

[Yann Sionneau]

On 9/24/25 10:01, Mykyta Poturai wrote:
> With pci-scan implemented it is now possible to use vpci for hardware
> domains. Update has_vpci to reflrect this change.

Small typo in commit message "reflect".

> 
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
>   xen/arch/arm/include/asm/domain.h | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index af3e168374..dbe3347cea 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -305,8 +305,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
>   
>   #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag)
>   
> -/* vPCI is not available on Arm */
> -#define has_vpci(d)    ({ (void)(d); false; })
> +#define has_vpci(d)    (is_hardware_domain(d) && hwdom_uses_vpci())
>   
>   struct arch_vcpu_io {
>       struct instr_details dabt_instr; /* when the instruction is decoded */


-- 


--
Yann Sionneau | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v1 1/8] arm/pci: Add pci-scan boot argument

[Jan Beulich]

On 24.09.2025 09:59, Mykyta Poturai wrote:
> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -23,6 +23,7 @@
>  #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
>  
>  extern bool pci_passthrough_enabled;
> +extern bool pci_scan_enabled;

At this point of the series this isn't needed, ...

> @@ -155,6 +156,8 @@ bool arch_pci_device_physdevop(void);
>  
>  #else   /*!CONFIG_HAS_PCI*/
>  
> +#define pci_scan_enabled false

... this is entirely unused, and ...

> --- a/xen/arch/arm/pci/pci.c
> +++ b/xen/arch/arm/pci/pci.c
> @@ -91,8 +91,14 @@ bool arch_pci_device_physdevop(void)
>  bool __read_mostly pci_passthrough_enabled;
>  boolean_param("pci-passthrough", pci_passthrough_enabled);
>  
> +/* By default pci scan is disabled. */
> +bool __ro_after_init pci_scan_enabled;

... and hence this wants to be static __initdata. Otherwise, strictly
speaking, this is "unreachable data" post-init as per Misra's unreachable
code criteria (which surely ought to extend to data as well).

> @@ -104,9 +110,23 @@ static int __init pci_init(void)
>          panic("Could not initialize PCI segment 0\n");
>  
>      if ( acpi_disabled )
> -        return dt_pci_init();
> +        ret = dt_pci_init();
>      else
> -        return acpi_pci_init();
> +        ret = acpi_pci_init();
> +
> +    if ( ret < 0 )
> +        return ret;

I understand this is merely transforming existing code, but I don't see
what actual use the returning of an error here and hence also ...

> +    if ( pci_scan_enabled )
> +    {
> +        ret = scan_pci_devices();
> +
> +        if ( ret < 0 )
> +            return ret;

... here has. The system will continue booting nevertheless, iirc without
even a diagnostic from caller.

Jan

Re: [PATCH v1 3/8] xen/pci: update DT for hwdom when it uses vpci

[Jan Beulich]

On 24.09.2025 09:59, Mykyta Poturai wrote:
> --- a/xen/arch/x86/include/asm/pci.h
> +++ b/xen/arch/x86/include/asm/pci.h
> @@ -76,4 +76,10 @@ int pci_sanitize_bar_memory(struct rangeset *r);
>  
>  void pci_setup(void);
>  
> +/* Unlike ARM, HW domain does not ever use vpci for x86 */

What did you derive this from? PVH Dom0 very well uses vPCI. In fact that's
what it was first introduced for, iirc.

Jan

> +static inline bool hwdom_uses_vpci(void)
> +{
> +    return false;
> +}
> +
>  #endif /* __X86_PCI_H__ */

Re: [PATCH v1 4/8] xen/pci: disable pci_device_{add,remove} when hwdom uses vpci on arm

[Jan Beulich]

On 24.09.2025 09:59, Mykyta Poturai wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> On ARM, if the hardware domain is using the emulated bus, it should not
> be allowed to add/remove pci devices, so return EOPNOTSUPP in that case.

And how exactly would PCI hotplug be made work then?

> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -19,6 +19,9 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          struct pci_dev_info pdev_info;
>          nodeid_t node = NUMA_NO_NODE;
>  
> +        if ( hwdom_uses_vpci() )
> +            return -EOPNOTSUPP;
> +
>          if ( !is_pci_passthrough_enabled() && !arch_pci_device_physdevop() )
>              return -EOPNOTSUPP;
>  
> @@ -57,6 +60,9 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      case PHYSDEVOP_pci_device_remove: {
>          struct physdev_pci_device dev;
>  
> +        if ( hwdom_uses_vpci() )
> +            return -EOPNOTSUPP;
> +
>          if ( !is_pci_passthrough_enabled() && !arch_pci_device_physdevop() )
>              return -EOPNOTSUPP;
>  

Along the lines of my comment on patch 3, these uses are clearly _visually_
wrong for x86; they're correct from a functional perspective simply because
the predicate returns the (apparently) wrong value there right now.

Jan

Re: [PATCH v1 5/8] xen/pci: introduce has_vpci_bridge

[Jan Beulich]

On 24.09.2025 09:59, Mykyta Poturai wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
> 
> has_vpci_bridge is a macro to check if the domain is a domU or is dom0
> with vPCI (pci-scan=yes) enabled.

Hmm. Why would DomU-s, now and forever, not have (virtual) bridges? Wasn't them
gaining (virtual) bridges actually the longer-term plan?

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -230,7 +230,7 @@ bool vpci_process_pending(struct vcpu *v)
>  
>              read_unlock(&v->domain->pci_lock);
>  
> -            if ( !is_hardware_domain(v->domain) )
> +            if ( has_vpci_bridge(v->domain) )
>                  domain_crash(v->domain);
>  
>              return false;
> @@ -492,7 +492,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>              }
>          }
>  
> -        if ( !is_hardware_domain(d) )
> +        if ( has_vpci_bridge(d) )
>              break;
>  
>          d = dom_xen;
> @@ -522,7 +522,7 @@ static void cf_check cmd_write(
>  {
>      struct vpci_header *header = data;
>  
> -    if ( !is_hardware_domain(pdev->domain) )
> +    if ( has_vpci_bridge(pdev->domain) )
>      {
>          const struct vpci *vpci = pdev->vpci;
>  
> @@ -564,7 +564,7 @@ static void cf_check bar_write(
>      struct vpci_bar *bar = data;
>      bool hi = false;
>  
> -    ASSERT(is_hardware_domain(pdev->domain));
> +    ASSERT(!has_vpci_bridge(pdev->domain));
>  
>      if ( bar->type == VPCI_BAR_MEM64_HI )
>      {
> @@ -747,7 +747,7 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
>  {
>      int rc;
>      bool mask_cap_list = false;
> -    bool is_hwdom = is_hardware_domain(pdev->domain);
> +    bool is_hwdom = !has_vpci_bridge(pdev->domain);
>  
>      if ( pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST )
>      {
> @@ -829,7 +829,7 @@ static int vpci_init_ext_capability_list(const struct pci_dev *pdev)
>  {
>      unsigned int pos = PCI_CFG_SPACE_SIZE;
>  
> -    if ( !is_hardware_domain(pdev->domain) )
> +    if ( has_vpci_bridge(pdev->domain) )
>          /* Extended capabilities read as zero, write ignore for DomU */
>          return vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>                                   pos, 4, (void *)0);
> @@ -866,7 +866,7 @@ int vpci_init_header(struct pci_dev *pdev)
>      struct vpci_header *header = &pdev->vpci->header;
>      struct vpci_bar *bars = header->bars;
>      int rc;
> -    bool is_hwdom = is_hardware_domain(pdev->domain);
> +    bool is_hwdom = !has_vpci_bridge(pdev->domain);
>  
>      ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));

For none of the changes (also further ones) it is clear (to me) why the
substitution is (logically) correct. For this last instance the variable
name also ends up wrong after the replacement.

> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -339,6 +339,15 @@ static inline int __must_check vpci_reset_device(struct pci_dev *pdev)
>      return vpci_assign_device(pdev);
>  }
>  
> +#ifdef CONFIG_ARM
> +#include <asm/pci.h>
> +
> +#define has_vpci_bridge(d) (!is_hardware_domain(d) || \
> +                           (is_hardware_domain(d) && pci_scan_enabled))

Nit: Off-by-1 indentation.

Jan

Re: [PATCH v1 6/8] xen/pci: initialize BARs

[Jan Beulich]

On 24.09.2025 09:59, Mykyta Poturai wrote:
> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
> 
> A PCI device must have valid BARs in order to assign it to a domain.  On
> ARM, firmware is unlikely to have initialized the BARs, so we must do
> this in Xen. During setup_hwdom_pci_devices(), check if each BAR is
> valid. If the BAR happens to already be valid, remove the BAR range from
> a rangeset of valid PCI ranges so as to avoid overlap when reserving a
> new BAR. If not valid, reserve a new BAR address from the rangeset and
> write it to the device.
> 
> Avaliable ranges are read from DT during init and stored in distinct
> rangesets.

The "distinct" lacks context here, imo. Maybe add "non-prefetchable and
prefetchable" earlier in the sentence?

> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -21,6 +21,7 @@
>  #include <xen/rwlock.h>
>  #include <xen/sched.h>
>  #include <xen/vmap.h>
> +#include <xen/resource.h>
>  
>  #include <asm/setup.h>
>  
> @@ -232,6 +233,21 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>      return domain;
>  }
>  
> +static int add_bar_range(const struct dt_device_node *dev, uint32_t flags,
> +                         uint64_t addr, uint64_t len, void *data)

Simply by its purpose, this function looks like it ought to be __init. And indeed,
while its only caller - pci_host_common_probe() - isn't, all callers of that
function are. Hence another prereq patch is wanted to add the missing __init there.

> @@ -286,6 +302,14 @@ pci_host_common_probe(struct dt_device_node *dev,
>      pci_add_host_bridge(bridge);
>      pci_add_segment(bridge->segment);
>  
> +    bridge->bar_ranges = rangeset_new(NULL, "BAR ranges",
> +                                      RANGESETF_prettyprint_hex);
> +    bridge->bar_ranges_prefetch = rangeset_new(NULL,
> +                                               "BAR ranges (prefetchable)",
> +                                               RANGESETF_prettyprint_hex);
> +    if ( bridge->bar_ranges && bridge->bar_ranges_prefetch )
> +        dt_for_each_range(bridge->dt_node, add_bar_range, bridge);
> +
>      return bridge;

This is odd: Why would you silently ignore the -ENOMEM condition here? You can't
blindly use the NULL pointer(s) ...

> @@ -476,6 +500,60 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>  
>      return bar_data.is_valid;
>  }
> +
> +uint64_t pci_get_new_bar_addr(const struct pci_dev *pdev, uint64_t size,
> +                              bool is_64bit, bool prefetch)

(__init as well here)

> +{
> +    struct pci_host_bridge *bridge;
> +    struct rangeset *range;
> +    uint64_t addr;
> +
> +    bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
> +    if ( !bridge )
> +        return 0;
> +
> +    range = prefetch ? bridge->bar_ranges_prefetch : bridge->bar_ranges;

... here, for example.

> +    if ( size < PAGE_SIZE )
> +        size = PAGE_SIZE;
> +
> +    if ( is_64bit && !rangeset_find_aligned_range(range, size, GB(4), &addr) )
> +    {
> +        if ( rangeset_remove_range(range, addr, addr + size - 1) )
> +        {
> +            printk("%s:%d:%s error\n", __FILE__, __LINE__, __func__);
> +        }
> +        return addr;

No need for the inner figure braces, but more importantly: printk()-style
error handling is, well, insufficient? I'm pretty sure I indicated before that
you can't assume the return value of rangeset_find_aligned_range() isn't stale
the moment you look at (or use) it, unless you make further (written down)
assumptions. For example, rangeset_find_aligned_range() becoming __init would
already be a partial indication that it's not fit for general use.

__FILE__ / __LINE__ based diagnostics also aren't all this useful (and get in
the way elsewhere, e.g. for livepatching).

> +    }
> +    if ( !rangeset_find_aligned_range(range, size, 0, &addr) )
> +    {
> +        if ( !is_64bit && addr >= GB(4) )
> +            return 0;

What guarantees that no lower range is available? It's not said anywhere that
the function would search addresses in increasing order.

> +int pci_reserve_bar_range(const struct pci_dev *pdev, uint64_t addr,
> +                          uint64_t size, bool prefetch)

__init again (I won't further repeat this)

> --- a/xen/common/rangeset.c
> +++ b/xen/common/rangeset.c
> @@ -357,6 +357,41 @@ int rangeset_claim_range(struct rangeset *r, unsigned long size,
>      return 0;
>  }
>  
> +int rangeset_find_aligned_range(struct rangeset *r, unsigned long size,
> +                                unsigned long min, unsigned long *s)

What is "min" when there ought to be some alignment specified instead?
(Reading the function body I understand that "size" also specifies the
alignment, but that's not clear at all when looking at e.g. just the
function declaration.)

How does the use of unsigned long here fit with the use of uint64_t in the
callers?

> +{
> +    struct range *x;
> +
> +    /* Power of 2 check */
> +    if ( (size & (size - 1)) != 0 )
> +    {
> +        *s = 0;
> +        return -EINVAL;
> +    }
> +
> +    read_lock(&r->lock);
> +
> +    for ( x = first_range(r); x; x = next_range(r, x) )
> +    {
> +        /* Assumes size is a power of 2 */
> +        unsigned long start_aligned = (x->s + size - 1) & ~(size - 1);

The comment says what somehow you should actually assure: It _needs_ to be
a power of 2 for this to work, yet size being 0 passes the check at the top
of the function.

Also please don't open-code ROUNDUP().

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1172,6 +1172,80 @@ int __init scan_pci_devices(void)
>      return ret;
>  }
>  
> +static void __init cf_check reserve_bar_range(struct pci_dev *pdev, uint8_t reg,
> +                                              uint64_t addr, uint64_t size,
> +                                              bool is_64bit, bool prefetch)
> +{
> +    if ( pci_check_bar(pdev, maddr_to_mfn(addr),
> +                       maddr_to_mfn(addr + size - 1)) )
> +        pci_reserve_bar_range(pdev, addr, size, prefetch);
> +}
> +
> +static void __init cf_check get_new_bar_addr(struct pci_dev *pdev, uint8_t reg,
> +                                             uint64_t addr, uint64_t size,
> +                                             bool is_64bit, bool prefetch)
> +{
> +    if ( !pci_check_bar(pdev, maddr_to_mfn(addr),
> +                        maddr_to_mfn(addr + size - 1)) )
> +    {

With how pci_check_bar() works on x86 right now, ...

> +        uint16_t cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> +
> +        addr = pci_get_new_bar_addr(pdev, size, is_64bit, prefetch);
> +
> +        pci_conf_write16(pdev->sbdf, PCI_COMMAND,
> +                         cmd & ~(PCI_COMMAND_MEMORY | PCI_COMMAND_IO));
> +
> +        pci_conf_write32(pdev->sbdf, reg,
> +                         (addr & GENMASK(31, 0)) |
> +                         (is_64bit ? PCI_BASE_ADDRESS_MEM_TYPE_64 : 0));
> +
> +        if ( is_64bit )
> +            pci_conf_write32(pdev->sbdf, reg + 4, addr >> 32);
> +
> +        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);

... all of this is unreachable code there. Misra doesn't like such.

Furthermore x86'es variant emits diagnostics, and aiui we'd now see them more
frequently (possibly: than wanted).

> +static int __init cf_check bars_iterate(struct pci_seg *pseg, void *arg)
> +{
> +    struct pci_dev *pdev;
> +    unsigned int i, ret, num_bars = PCI_HEADER_NORMAL_NR_BARS;
> +    uint64_t addr, size;
> +    void (*cb)(struct pci_dev *, uint8_t, uint64_t, uint64_t, bool, bool) = arg;

There is still no connection at all between this function pointer type variable
and the two functions tobe called? Why is "cb" not directly the function
parameter? (For Misra's sake it may also be necessary to name the six parameters;
I'm not entirely sure, though. There was some back and forth at the time.)

Also, why is the first parameter not pointer-to-const?

> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> +    {
> +        if ( (pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f) ==
> +             PCI_HEADER_TYPE_NORMAL )
> +        {
> +            for ( i = 0; i < num_bars; i += ret )
> +            {
> +                uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
> +                bool prefetch;
> +
> +                if ( (pci_conf_read32(pdev->sbdf, reg) & PCI_BASE_ADDRESS_SPACE)
> +                     == PCI_BASE_ADDRESS_SPACE_IO )

Nit (style): Operator placement.

> +                {
> +                    ret = 1;
> +                    continue;
> +                }
> +
> +                ret = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
> +                                      (i == num_bars - 1) ? PCI_BAR_LAST : 0);

Nit (style): Indentation.

> +                if ( !size )
> +                    continue;
> +                prefetch = !!(pci_conf_read32(pdev->sbdf, reg) &
> +                              PCI_BASE_ADDRESS_MEM_PREFETCH);

No need for !!.

> @@ -1263,6 +1337,11 @@ void __hwdom_init setup_hwdom_pci_devices(
>      struct setup_hwdom ctxt = { .d = d, .handler = handler };
>  
>      pcidevs_lock();
> +    if ( hwdom_uses_vpci() )
> +    {
> +        pci_segments_iterate(bars_iterate, reserve_bar_range);
> +        pci_segments_iterate(bars_iterate, get_new_bar_addr);
> +    }
>      pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt);
>      pcidevs_unlock();

Does this really need splitting into 3 steps? Can't _setup_hwdom_pci_devices()
take care of the uninitialized BARs?

Jan

Re: [PATCH v1 7/8] xen/pci: assign discovered devices to hwdom

[Jan Beulich]

On 24.09.2025 09:59, Mykyta Poturai wrote:
> @@ -133,6 +134,12 @@ void arch_iommu_domain_destroy(struct domain *d)
>  {
>  }
>  
> +static int iommu_add_hwdom_pci_device(u8 devfn, struct pci_dev *pdev)
> +{
> +    const struct domain_iommu *hd = dom_iommu(hardware_domain);
> +    return iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));

Nit (style): Blank line please between declaration(s) and statement(s). And
blank line please also ahead of the main return statement of a function.

> @@ -142,6 +149,8 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>      if ( iommu_hwdom_reserved == 1 )
>          printk(XENLOG_WARNING "map-reserved dom0-iommu option is not supported on ARM\n");
>      iommu_hwdom_reserved = 0;
> +
> +    setup_hwdom_pci_devices(d, iommu_add_hwdom_pci_device);
With this function being __hwdom_init, why is iommu_add_hwdom_pci_device()
not also given that attribute?

As to cf_check I don't know what the Arm policy is: My suggestion would be
to put that attribute wherever it is (potentially) needed.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -20,6 +20,7 @@
>  #include <xen/pci_ids.h>
>  #include <xen/list.h>
>  #include <xen/prefetch.h>
> +#include <xen/iocap.h>
>  #include <xen/iommu.h>
>  #include <xen/irq.h>
>  #include <xen/param.h>
> @@ -1040,6 +1041,12 @@ enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
>      return pos ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
>  }
>  
> +static bool pdev_is_endpoint(struct pci_dev *pdev)

__hwdom_init? And parameter pointer-to-const?

> +{
> +    enum pdev_type type = pdev_type(pdev->seg, pdev->bus, pdev->devfn);
> +    return type == DEV_TYPE_PCIe_ENDPOINT || type == DEV_TYPE_PCI;
> +}
> +
>  /*
>   * find the upstream PCIe-to-PCI/PCIX bridge or PCI legacy bridge
>   * return 0: the device is integrated PCI device or PCIe
> @@ -1255,7 +1262,7 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom *ctxt,
>                                                  struct pci_dev *pdev)
>  {
>      u8 devfn = pdev->devfn;
> -    int err;
> +    int err, i, rc;

i clearly wants to be of an unsigned type. And rc, afaics, can have its scope
limited to the loop body.

> @@ -1276,6 +1283,34 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom *ctxt,
>      if ( err )
>          printk(XENLOG_ERR "setup of vPCI for d%d failed: %d\n",
>                 ctxt->d->domain_id, err);
> +
> +    if ( !hwdom_uses_vpci() )
> +        return;
> +
> +    for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS; i += rc )
> +    {
> +        uint64_t addr, size;
> +        uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
> +
> +        if ( (pci_conf_read32(pdev->sbdf, reg) & PCI_BASE_ADDRESS_SPACE)
> +             == PCI_BASE_ADDRESS_SPACE_IO )

Nit (style): Operator placement again.

> +        {
> +            rc = 1;
> +            continue;
> +        }
> +
> +        rc = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
> +                              (i == PCI_HEADER_NORMAL_NR_BARS - 1)
> +                                  ? PCI_BAR_LAST : 0);

Nit (style): Indentation again.

> +
> +        if ( !size )
> +            continue;
> +
> +        err = iomem_permit_access(hardware_domain, paddr_to_pfn(addr),
> +                             paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));

And again.

> +        if ( err )
> +            break;
> +    }
>  }

This change supports my comment on the earlier patch, regarding the option
of doing here some of what needs doing, rather than by another round of
iterating all devices.

> @@ -1294,6 +1329,9 @@ static int __hwdom_init cf_check _setup_hwdom_pci_devices(
>              if ( !pdev )
>                  continue;
>  
> +            if ( hwdom_uses_vpci() && !pdev_is_endpoint(pdev) )
> +                continue;
> +
>              if ( !pdev->domain )
>              {
>                  pdev->domain = ctxt->d;

This is (logically) wrong: On x86 PVH Dom0 uses vPCI but wants all devices
to be handed to it. _This_ may be a place where has_vpci_bridge() might
make sense to use (simply by its name, that is).

Jan

Re: [PATCH v1 5/8] xen/pci: introduce has_vpci_bridge

[Mykyta Poturai]

On 06.11.25 14:15, Jan Beulich wrote:> On 24.09.2025 09:59, Mykyta 
Poturai wrote:
 >> From: Stefano Stabellini <stefano.stabellini@amd.com>
 >>
 >> has_vpci_bridge is a macro to check if the domain is a domU or is dom0
 >> with vPCI (pci-scan=yes) enabled.
 >
 > Hmm. Why would DomU-s, now and forever, not have (virtual) bridges? 
Wasn't them
 > gaining (virtual) bridges actually the longer-term plan?
 >

Seems like there is a misunderstanding here. Indeed it is the plan for 
DomUs to have virtual bridges and this check is designed to 
differentiate two categories of domains.
1. All DomUs + Dom0 with the virtual bridge.
2. Dom0 with HW bridge.

I will try to rephrase the commit message to be more clear.

 >> @@ -866,7 +866,7 @@ int vpci_init_header(struct pci_dev *pdev)
 >>       struct vpci_header *header = &pdev->vpci->header;
 >>       struct vpci_bar *bars = header->bars;
 >>       int rc;
 >> -    bool is_hwdom = is_hardware_domain(pdev->domain);
 >> +    bool is_hwdom = !has_vpci_bridge(pdev->domain);
 >>
 >>       ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
 >
 > For none of the changes (also further ones) it is clear (to me) why the
 > substitution is (logically) correct. For this last instance the variable
 > name also ends up wrong after the replacement.
 >> Jan

The general logic for this change is that before it we assumed that 
hwdom always uses HW bridge. Now it is not always true and hwdom can 
also use a virtual bridge, so it needs to be treated the same way as 
DomUs are.

You also mentioned on a previous series

 > Here and perhaps everywhere else I wonder: Is this really an 
appropriately
 > named predicate for the purpose / context?

Maybe you have some ideas of a better name? From what I came up with 
this seems like the best one.

-- 
Mykyta

Re: [PATCH v1 5/8] xen/pci: introduce has_vpci_bridge

[Jan Beulich]

On 13.11.2025 10:49, Mykyta Poturai wrote:
> On 06.11.25 14:15, Jan Beulich wrote:> On 24.09.2025 09:59, Mykyta 
> Poturai wrote:
>  >> From: Stefano Stabellini <stefano.stabellini@amd.com>
>  >>
>  >> has_vpci_bridge is a macro to check if the domain is a domU or is dom0
>  >> with vPCI (pci-scan=yes) enabled.
>  >
>  > Hmm. Why would DomU-s, now and forever, not have (virtual) bridges? 
> Wasn't them
>  > gaining (virtual) bridges actually the longer-term plan?
>  >
> 
> Seems like there is a misunderstanding here. Indeed it is the plan for 
> DomUs to have virtual bridges and this check is designed to 
> differentiate two categories of domains.
> 1. All DomUs + Dom0 with the virtual bridge.
> 2. Dom0 with HW bridge.
> 
> I will try to rephrase the commit message to be more clear.
> 
>  >> @@ -866,7 +866,7 @@ int vpci_init_header(struct pci_dev *pdev)
>  >>       struct vpci_header *header = &pdev->vpci->header;
>  >>       struct vpci_bar *bars = header->bars;
>  >>       int rc;
>  >> -    bool is_hwdom = is_hardware_domain(pdev->domain);
>  >> +    bool is_hwdom = !has_vpci_bridge(pdev->domain);
>  >>
>  >>       ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
>  >
>  > For none of the changes (also further ones) it is clear (to me) why the
>  > substitution is (logically) correct. For this last instance the variable
>  > name also ends up wrong after the replacement.
>  >> Jan
> 
> The general logic for this change is that before it we assumed that 
> hwdom always uses HW bridge. Now it is not always true and hwdom can 
> also use a virtual bridge, so it needs to be treated the same way as 
> DomUs are.
> 
> You also mentioned on a previous series
> 
>  > Here and perhaps everywhere else I wonder: Is this really an 
> appropriately
>  > named predicate for the purpose / context?
> 
> Maybe you have some ideas of a better name? From what I came up with 
> this seems like the best one.

How to name it depends on the (ultimate) purpose. If, as you say, it's intended
to cover Dom0 and DomU uniformly when they have a virtual bridge, the name might
be quite okay. Then, as you said you would do, better wording may be needed,
variable names like the one mentioned above may need adjustment, and any code
paths suddenly becoming usable for DomU-s need to be audited for this being a
safe/secure thing to do (quite a few of the Dom0-only paths have been taking
some liberties so far).

Jan