[PATCH v7 0/7] PCI devices passthrough on Arm, part 2

[PATCH v7 0/7] PCI devices passthrough on Arm, part 2

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Hi, all!

This is an assorted series of patches which aim is to make some further
basis for PCI passthrough on Arm support. The series continues the work
published earlier by Arm [1] and adds new helpers and clears the way for
vPCI changes which will follow.

RFC is at [2], [3]. Design presentation can be found at [4].

I have removed patch
[PATCH v6 5/7] xen/arm: do not map IRQs and memory for disabled devices
as it seems that this needs more time for decision on how to achive
that.

I have also added a new patch
[PATCH v7 4/7] xen/arm: account IO handler for emulated PCI host bridge
with a tiny latent bug fix.

Thank you,
Oleksandr

[1] https://patchwork.kernel.org/project/xen-devel/list/?series=558681
[2] https://lists.xenproject.org/archives/html/xen-devel/2020-07/msg01184.html
[3] https://lists.xenproject.org/archives/html/xen-devel/2020-07/threads.html#01184
[4] https://static.sched.com/hosted_files/xen2021/e4/PCI_Device_Passthrough_On_Arm.pdf

Oleksandr Andrushchenko (7):
  xen/arm: rename DEVICE_PCI to DEVICE_PCI_HOSTBRIDGE
  xen/arm: add pci-domain for disabled devices
  xen/arm: setup MMIO range trap handlers for hardware domain
  xen/arm: account IO handler for emulated PCI host bridge
  xen/arm: do not map PCI ECAM and MMIO space to Domain-0's p2m
  xen/arm: process pending vPCI map/unmap operations
  xen/arm: do not use void pointer in pci_host_common_probe

 xen/arch/arm/domain.c               |   2 +
 xen/arch/arm/domain_build.c         | 132 +++++++++++++++++++---------
 xen/arch/arm/pci/ecam.c             |  18 +++-
 xen/arch/arm/pci/pci-host-common.c  |  81 +++++++++++++++--
 xen/arch/arm/pci/pci-host-generic.c |   2 +-
 xen/arch/arm/pci/pci-host-zynqmp.c  |   3 +-
 xen/arch/arm/pci/pci.c              |   2 +-
 xen/arch/arm/traps.c                |  13 +++
 xen/arch/arm/vpci.c                 |  80 +++++++++++++++--
 xen/arch/arm/vpci.h                 |   6 ++
 xen/arch/x86/hvm/hvm.c              |   6 ++
 xen/common/ioreq.c                  |   9 --
 xen/include/asm-arm/device.h        |   2 +-
 xen/include/asm-arm/pci.h           |  27 +++++-
 xen/include/asm-arm/setup.h         |  13 +++
 15 files changed, 323 insertions(+), 73 deletions(-)

-- 
2.25.1

[PATCH v7 1/7] xen/arm: rename DEVICE_PCI to DEVICE_PCI_HOSTBRIDGE

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

To better reflect the nature of the device type and not to make any
confusion rename DEVICE_PCI to DEVICE_PCI_HOSTBRIDGE which it
really is.

Suggested-by: Julien Grall <julien@xen.org>
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
New in v6
---
 xen/arch/arm/pci/pci-host-generic.c | 2 +-
 xen/arch/arm/pci/pci-host-zynqmp.c  | 2 +-
 xen/arch/arm/pci/pci.c              | 2 +-
 xen/include/asm-arm/device.h        | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
index 33457fbe9615..46de6e43cc72 100644
--- a/xen/arch/arm/pci/pci-host-generic.c
+++ b/xen/arch/arm/pci/pci-host-generic.c
@@ -32,7 +32,7 @@ static int __init pci_host_generic_probe(struct dt_device_node *dev,
     return pci_host_common_probe(dev, &pci_generic_ecam_ops);
 }
 
-DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
+DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI_HOSTBRIDGE)
 .dt_match = gen_pci_dt_match,
 .init = pci_host_generic_probe,
 DT_DEVICE_END
diff --git a/xen/arch/arm/pci/pci-host-zynqmp.c b/xen/arch/arm/pci/pci-host-zynqmp.c
index 61a9807d3d58..516982bca833 100644
--- a/xen/arch/arm/pci/pci-host-zynqmp.c
+++ b/xen/arch/arm/pci/pci-host-zynqmp.c
@@ -49,7 +49,7 @@ static int __init pci_host_generic_probe(struct dt_device_node *dev,
     return pci_host_common_probe(dev, &nwl_pcie_ops);
 }
 
-DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI)
+DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI_HOSTBRIDGE)
 .dt_match = nwl_pcie_dt_match,
 .init = pci_host_generic_probe,
 DT_DEVICE_END
diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
index 082c14e127a8..78b97beaef12 100644
--- a/xen/arch/arm/pci/pci.c
+++ b/xen/arch/arm/pci/pci.c
@@ -46,7 +46,7 @@ static int __init dt_pci_init(void)
 
     dt_for_each_device_node(dt_host, np)
     {
-        rc = device_init(np, DEVICE_PCI, NULL);
+        rc = device_init(np, DEVICE_PCI_HOSTBRIDGE, NULL);
         /*
          * Ignore the following error codes:
          *   - EBADF: Indicate the current device is not a pci device.
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 3782660751b6..086dde13eb6b 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -37,7 +37,7 @@ enum device_class
     DEVICE_SERIAL,
     DEVICE_IOMMU,
     DEVICE_GIC,
-    DEVICE_PCI,
+    DEVICE_PCI_HOSTBRIDGE,
     /* Use for error */
     DEVICE_UNKNOWN,
 };
-- 
2.25.1

[PATCH v7 2/7] xen/arm: add pci-domain for disabled devices

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

If a PCI host bridge device is present in the device tree, but is
disabled, then its PCI host bridge driver was not instantiated.
This results in the failure of the pci_get_host_bridge_segment()
and the following panic during Xen start:

(XEN) Device tree generation failed (-22).
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Could not set up DOM0 guest OS
(XEN) ****************************************

Fix this by adding "linux,pci-domain" property for all device tree nodes
which have "pci" device type, so we know which segments will be used by
the guest for which bridges.

Fixes: 4cfab4425d39 ("xen/arm: Add linux,pci-domain property for hwdom if not available.")

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

---
Since v6:
- use use_dt_domains in pci_get_new_domain_nr and return -1 if set
- do not set "linux,pci-domain" if parent device is "pci"
- move the code to a new helper handle_linux_pci_domain (Julien)
New in v6
---
 xen/arch/arm/domain_build.c        | 66 +++++++++++++++++++++++-------
 xen/arch/arm/pci/pci-host-common.c |  8 +++-
 xen/include/asm-arm/pci.h          |  8 ++++
 3 files changed, 66 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e76cee3caccf..c83c02ab8ac6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -654,6 +654,55 @@ static void __init allocate_static_memory(struct domain *d,
 }
 #endif
 
+/*
+ * When PCI passthrough is available we want to keep the
+ * "linux,pci-domain" in sync for every host bridge.
+ *
+ * Xen may not have a driver for all the host bridges. So we have
+ * to write an heuristic to detect whether a device node describes
+ * a host bridge.
+ *
+ * The current heuristic assumes that a device is a host bridge
+ * if the type is "pci" and then parent type is not "pci".
+ */
+static int handle_linux_pci_domain(struct kernel_info *kinfo,
+                                   const struct dt_device_node *node)
+{
+    uint16_t segment;
+    int res;
+
+    if ( !is_pci_passthrough_enabled() )
+        return 0;
+
+    if ( !dt_device_type_is_equal(node, "pci") )
+        return 0;
+
+    if ( node->parent && dt_device_type_is_equal(node->parent, "pci") )
+        return 0;
+
+    if ( dt_find_property(node, "linux,pci-domain", NULL) )
+        return 0;
+
+    /* Allocate and create the linux,pci-domain */
+    res = pci_get_host_bridge_segment(node, &segment);
+    if ( res < 0 )
+    {
+        res = pci_get_new_domain_nr();
+        if ( res < 0 )
+        {
+            printk(XENLOG_DEBUG "Can't assign PCI segment to %s\n",
+                   node->full_name);
+            return -FDT_ERR_NOTFOUND;
+        }
+
+        segment = res;
+        printk(XENLOG_DEBUG "Assigned segment %d to %s\n",
+               segment, node->full_name);
+    }
+
+    return fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment);
+}
+
 static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
                                    const struct dt_device_node *node)
 {
@@ -755,21 +804,10 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
             return res;
     }
 
-    if ( is_pci_passthrough_enabled() && dt_device_type_is_equal(node, "pci") )
-    {
-        if ( !dt_find_property(node, "linux,pci-domain", NULL) )
-        {
-            uint16_t segment;
-
-            res = pci_get_host_bridge_segment(node, &segment);
-            if ( res < 0 )
-                return res;
+    res = handle_linux_pci_domain(kinfo, node);
 
-            res = fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment);
-            if ( res )
-                return res;
-        }
-    }
+    if ( res )
+        return res;
 
     /*
      * Override the property "status" to disable the device when it's
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index cdeb3a283a77..9653b92b5b2e 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -30,6 +30,8 @@ static LIST_HEAD(pci_host_bridges);
 
 static atomic_t domain_nr = ATOMIC_INIT(-1);
 
+static int use_dt_domains = -1;
+
 static inline void __iomem *pci_remap_cfgspace(paddr_t start, size_t len)
 {
     return ioremap_nocache(start, len);
@@ -137,14 +139,16 @@ void pci_add_host_bridge(struct pci_host_bridge *bridge)
     list_add_tail(&bridge->node, &pci_host_bridges);
 }
 
-static int pci_get_new_domain_nr(void)
+int pci_get_new_domain_nr(void)
 {
+    if ( use_dt_domains )
+        return -1;
+
     return atomic_inc_return(&domain_nr);
 }
 
 static int pci_bus_find_domain_nr(struct dt_device_node *dev)
 {
-    static int use_dt_domains = -1;
     int domain;
 
     domain = dt_get_pci_domain_nr(dev);
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 81273e0d87ac..c20eba643d86 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -108,6 +108,8 @@ static always_inline bool is_pci_passthrough_enabled(void)
 
 void arch_pci_init_pdev(struct pci_dev *pdev);
 
+int pci_get_new_domain_nr(void);
+
 #else   /*!CONFIG_HAS_PCI*/
 
 struct arch_pci_dev { };
@@ -128,5 +130,11 @@ static inline int pci_get_host_bridge_segment(const struct dt_device_node *node,
     return -EINVAL;
 }
 
+static inline int pci_get_new_domain_nr(void)
+{
+    ASSERT_UNREACHABLE();
+    return -1;
+}
+
 #endif  /*!CONFIG_HAS_PCI*/
 #endif /* __ARM_PCI_H__ */
-- 
2.25.1

[PATCH v7 3/7] xen/arm: setup MMIO range trap handlers for hardware domain

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

In order for vPCI to work it needs to maintain guest and hardware
domain's views of the configuration space. For example, BARs and
COMMAND registers require emulation for guests and the guest view
of the registers needs to be in sync with the real contents of the
relevant registers. For that ECAM address space needs to also be
trapped for the hardware domain, so we need to implement PCI host
bridge specific callbacks to properly setup MMIO handlers for those
ranges depending on particular host bridge implementation.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
Since v6:
- eliminate pci_host_get_num_bridges and make pci_host_iterate_bridges
  return the count
- extend comment in domain_vpci_init
- remove not yet relevant code for num MMIOs and virtual bus topology
- add extra check for has_vpci in domain_vpci_get_num_mmio_handlers
- remove code that fixes num MMIOs for guest domain as it doesn't belong
  to this patch
Since v5:
- add vpci_sbdf_from_gpa helper for gpa to SBDF translation
- take bridge's bus start into account while calculating SBDF
Since v4:
- unsigned int for functions working with count
- gate number of MMIO handlers needed for CONFIG_HAS_PCI_MSI
  and fix their number, e.g. single handler for PBA and
  MSI-X tables (Roger)
- re-work code for assigning MMIO handlers to be simpler
  and account on the fact that there could multiple host bridges
  exist for the hwdom
Since v3:
- fixed comment formatting
Since v2:
- removed unneeded assignment (count = 0)
- removed unneeded header inclusion
- update commit message
Since v1:
 - Dynamically calculate the number of MMIO handlers required for vPCI
   and update the total number accordingly
 - s/clb/cb
 - Do not introduce a new callback for MMIO handler setup
---
 xen/arch/arm/domain.c              |  2 +
 xen/arch/arm/pci/pci-host-common.c | 17 +++++++
 xen/arch/arm/vpci.c                | 79 +++++++++++++++++++++++++++---
 xen/arch/arm/vpci.h                |  6 +++
 xen/include/asm-arm/pci.h          |  4 ++
 5 files changed, 100 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 96e1b235501d..92a6c509e5c5 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -739,6 +739,8 @@ int arch_domain_create(struct domain *d,
     if ( (rc = domain_vgic_register(d, &count)) != 0 )
         goto fail;
 
+    count += domain_vpci_get_num_mmio_handlers(d);
+
     if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 )
         goto fail;
 
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 9653b92b5b2e..18b09d5e6f10 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -296,6 +296,23 @@ int pci_get_host_bridge_segment(const struct dt_device_node *node,
     return -EINVAL;
 }
 
+int pci_host_iterate_bridges_and_count(struct domain *d,
+                                       int (*cb)(struct domain *d,
+                                                 struct pci_host_bridge *bridge))
+{
+    struct pci_host_bridge *bridge;
+    int err, count = 0;
+
+    list_for_each_entry( bridge, &pci_host_bridges, node )
+    {
+        err = cb(d, bridge);
+        if ( err )
+            return err;
+        count += err;
+    }
+    return count;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
index 23f45386f4b3..ccd998d8dba2 100644
--- a/xen/arch/arm/vpci.c
+++ b/xen/arch/arm/vpci.c
@@ -16,16 +16,31 @@
 
 #include <asm/mmio.h>
 
+static pci_sbdf_t vpci_sbdf_from_gpa(const struct pci_host_bridge *bridge,
+                                     paddr_t gpa)
+{
+    pci_sbdf_t sbdf;
+
+    if ( bridge )
+    {
+        sbdf.sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr);
+        sbdf.seg = bridge->segment;
+        sbdf.bus += bridge->cfg->busn_start;
+    }
+    else
+        sbdf.sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE);
+
+    return sbdf;
+}
+
 static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
                           register_t *r, void *p)
 {
-    pci_sbdf_t sbdf;
+    struct pci_host_bridge *bridge = p;
+    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
     /* data is needed to prevent a pointer cast on 32bit */
     unsigned long data;
 
-    /* We ignore segment part and always handle segment 0 */
-    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE);
-
     if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
                         1U << info->dabt.size, &data) )
     {
@@ -41,10 +56,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
 static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
                            register_t r, void *p)
 {
-    pci_sbdf_t sbdf;
-
-    /* We ignore segment part and always handle segment 0 */
-    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE);
+    struct pci_host_bridge *bridge = p;
+    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
 
     return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa),
                            1U << info->dabt.size, r);
@@ -55,17 +68,67 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
     .write = vpci_mmio_write,
 };
 
+static int vpci_setup_mmio_handler_cb(struct domain *d,
+                                      struct pci_host_bridge *bridge)
+{
+    struct pci_config_window *cfg = bridge->cfg;
+
+    register_mmio_handler(d, &vpci_mmio_handler,
+                          cfg->phys_addr, cfg->size, bridge);
+
+    /* We have registered a single MMIO handler. */
+    return 1;
+}
+
 int domain_vpci_init(struct domain *d)
 {
     if ( !has_vpci(d) )
         return 0;
 
+    /*
+     * The hardware domain gets as many MMIOs as required by the
+     * physical host bridge.
+     * Guests get the virtual platform layout: one virtual host bridge for now.
+     */
+    if ( is_hardware_domain(d) )
+    {
+        int count;
+
+        count = pci_host_iterate_bridges_and_count(d, vpci_setup_mmio_handler_cb);
+        if ( count < 0 )
+            return count;
+
+        return 0;
+    }
+
     register_mmio_handler(d, &vpci_mmio_handler,
                           GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
 
     return 0;
 }
 
+static int vpci_get_num_handlers_cb(struct domain *d,
+                                    struct pci_host_bridge *bridge)
+{
+    /* Each bridge has a single MMIO handler for the configuration space. */
+    return 1;
+}
+
+unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
+{
+    if ( !has_vpci(d) )
+        return 0;
+
+    if ( is_hardware_domain(d) )
+    {
+        int ret = pci_host_iterate_bridges_and_count(d, vpci_get_num_handlers_cb);
+
+        return ret < 0 ? 0 : ret;
+    }
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h
index d8a7b0e3e802..3c713f3fcdb5 100644
--- a/xen/arch/arm/vpci.h
+++ b/xen/arch/arm/vpci.h
@@ -17,11 +17,17 @@
 
 #ifdef CONFIG_HAS_VPCI
 int domain_vpci_init(struct domain *d);
+unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d);
 #else
 static inline int domain_vpci_init(struct domain *d)
 {
     return 0;
 }
+
+static inline unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
+{
+    return 0;
+}
 #endif
 
 #endif /* __ARCH_ARM_VPCI_H__ */
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index c20eba643d86..4278d66e5eb9 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -110,6 +110,10 @@ void arch_pci_init_pdev(struct pci_dev *pdev);
 
 int pci_get_new_domain_nr(void);
 
+int pci_host_iterate_bridges_and_count(struct domain *d,
+                                       int (*cb)(struct domain *d,
+                                                 struct pci_host_bridge *bridge));
+
 #else   /*!CONFIG_HAS_PCI*/
 
 struct arch_pci_dev { };
-- 
2.25.1

[PATCH v7 4/7] xen/arm: account IO handler for emulated PCI host bridge

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

At the moment, we always allocate an extra 16 slots for IO handlers
(see MAX_IO_HANDLER). So while adding an IO trap handler for the emulated
PCI host bridge we are not breaking anything, but we have a latent bug
as the maximum number of IOs may be exceeded.
Fix this by explicitly telling that we have an additional IO handler, so it is
accounted.

Fixes: d59168dc05a5 ("xen/arm: Enable the existing x86 virtual PCI support for ARM")

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

---
New in v7
---
 xen/arch/arm/vpci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
index ccd998d8dba2..8e801f275879 100644
--- a/xen/arch/arm/vpci.c
+++ b/xen/arch/arm/vpci.c
@@ -126,7 +126,8 @@ unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
         return ret < 0 ? 0 : ret;
     }
 
-    return 0;
+    /* For a single emulated host bridge's configuration space. */
+    return 1;
 }
 
 /*
-- 
2.25.1

[PATCH v7 5/7] xen/arm: do not map PCI ECAM and MMIO space to Domain-0's p2m

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

PCI host bridges are special devices in terms of implementing PCI
passthrough. According to [1] the current implementation depends on
Domain-0 to perform the initialization of the relevant PCI host
bridge hardware and perform PCI device enumeration. In order to
achieve that one of the required changes is to not map all the memory
ranges in map_range_to_domain as we traverse the device tree on startup
and perform some additional checks if the range needs to be mapped to
Domain-0.

The generic PCI host controller device tree binding says [2]:
- ranges: As described in IEEE Std 1275-1994, but must provide
          at least a definition of non-prefetchable memory. One
          or both of prefetchable Memory and IO Space may also
          be provided.

- reg   : The Configuration Space base address and size, as accessed
          from the parent bus.  The base address corresponds to
          the first bus in the "bus-range" property.  If no
          "bus-range" is specified, this will be bus 0 (the default).

From the above none of the memory ranges from the "ranges" property
needs to be mapped to Domain-0 at startup as MMIO mapping is going to
be handled dynamically by vPCI as we assign PCI devices, e.g. each
device assigned to Domain-0/guest will have its MMIOs mapped/unmapped
as needed by Xen.

The "reg" property covers not only ECAM space, but may also have other
then the configuration memory ranges described, for example [3]:
- reg: Should contain rc_dbi, config registers location and length.
- reg-names: Must include the following entries:
   "rc_dbi": controller configuration registers;
   "config": PCIe configuration space registers.

This patch makes it possible to not map all the ranges from the
"ranges" property and also ECAM from the "reg". All the rest from the
"reg" property still needs to be mapped to Domain-0, so the PCI
host bridge remains functional in Domain-0. This is done by first
skipping the mappings while traversing the device tree as it is done for
usual devices and then by calling a dedicated pci_host_bridge_mappings
function which only maps MMIOs required by the host bridges leaving the
regions, needed for vPCI traps, unmapped.

[1] https://lists.xenproject.org/archives/html/xen-devel/2020-07/msg00777.html
[2] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/host-generic-pci.txt
[3] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
Since v7:
- updates in comments and commit message
Since v5:
- remove some need_mapping local variables
- use own_device in handle_device
- add __init for pci_ecam_need_p2m_hwdom_mapping
- make pci_host_bridge_mappings use p2m_mmio_direct_dev directly
Since v4:
- update skip_mapping comment
- add comment why we need to map interrupts to Dom0
Since v3:
 - pass struct map_range_data to map_dt_irq_to_domain
 - remove redundant check from map_range_to_domain
 - fix handle_device's .skip_mapping
Since v2:
 - removed check in map_range_to_domain for PCI_DEV
   and moved it to handle_device, so the code is
   simpler
 - s/map_pci_bridge/skip_mapping
 - extended comment in pci_host_bridge_mappings
 - minor code restructure in construct_dom0
 - s/.need_p2m_mapping/.need_p2m_hwdom_mapping and related
   callbacks
 - unsigned int i; in pci_host_bridge_mappings
Since v1:
 - Added better description of why and what needs to be mapped into
   Domain-0's p2m and what doesn't
 - Do not do any mappings for PCI devices while traversing the DT
 - Walk all the bridges and make required mappings in one go
---
 xen/arch/arm/domain_build.c        | 66 +++++++++++++++++-------------
 xen/arch/arm/pci/ecam.c            | 14 +++++++
 xen/arch/arm/pci/pci-host-common.c | 50 ++++++++++++++++++++++
 xen/arch/arm/pci/pci-host-zynqmp.c |  1 +
 xen/include/asm-arm/pci.h          | 10 +++++
 xen/include/asm-arm/setup.h        | 13 ++++++
 6 files changed, 126 insertions(+), 28 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c83c02ab8ac6..47c884cca2c3 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -51,12 +51,6 @@ static int __init parse_dom0_mem(const char *s)
 }
 custom_param("dom0_mem", parse_dom0_mem);
 
-struct map_range_data
-{
-    struct domain *d;
-    p2m_type_t p2mt;
-};
-
 /* Override macros from asm/page.h to make them work with mfn_t */
 #undef virt_to_mfn
 #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
@@ -1720,10 +1714,10 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
                                        const struct dt_irq *dt_irq,
                                        void *data)
 {
-    struct domain *d = data;
+    struct map_range_data *mr_data = data;
+    struct domain *d = mr_data->d;
     unsigned int irq = dt_irq->irq;
     int res;
-    bool need_mapping = !dt_device_for_passthrough(dev);
 
     if ( irq < NR_LOCAL_IRQS )
     {
@@ -1742,18 +1736,16 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
         return res;
     }
 
-    res = map_irq_to_domain(d, irq, need_mapping, dt_node_name(dev));
+    res = map_irq_to_domain(d, irq, !mr_data->skip_mapping, dt_node_name(dev));
 
     return 0;
 }
 
-static int __init map_range_to_domain(const struct dt_device_node *dev,
-                                      u64 addr, u64 len,
-                                      void *data)
+int __init map_range_to_domain(const struct dt_device_node *dev,
+                               u64 addr, u64 len, void *data)
 {
     struct map_range_data *mr_data = data;
     struct domain *d = mr_data->d;
-    bool need_mapping = !dt_device_for_passthrough(dev);
     int res;
 
     res = iomem_permit_access(d, paddr_to_pfn(addr),
@@ -1767,7 +1759,7 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
         return res;
     }
 
-    if ( need_mapping )
+    if ( !mr_data->skip_mapping )
     {
         res = map_regions_p2mt(d,
                                gaddr_to_gfn(addr),
@@ -1796,23 +1788,21 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
  * then we may need to perform additional mappings in order to make
  * the child resources available to domain 0.
  */
-static int __init map_device_children(struct domain *d,
-                                      const struct dt_device_node *dev,
-                                      p2m_type_t p2mt)
+static int __init map_device_children(const struct dt_device_node *dev,
+                                      struct map_range_data *mr_data)
 {
-    struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
-    int ret;
-
     if ( dt_device_type_is_equal(dev, "pci") )
     {
+        int ret;
+
         dt_dprintk("Mapping children of %s to guest\n",
                    dt_node_full_name(dev));
 
-        ret = dt_for_each_irq_map(dev, &map_dt_irq_to_domain, d);
+        ret = dt_for_each_irq_map(dev, &map_dt_irq_to_domain, mr_data);
         if ( ret < 0 )
             return ret;
 
-        ret = dt_for_each_range(dev, &map_range_to_domain, &mr_data);
+        ret = dt_for_each_range(dev, &map_range_to_domain, mr_data);
         if ( ret < 0 )
             return ret;
     }
@@ -1892,14 +1882,28 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
     unsigned int i;
     int res;
     u64 addr, size;
-    bool need_mapping = !dt_device_for_passthrough(dev);
+    bool own_device = !dt_device_for_passthrough(dev);
+    /*
+     * We want to avoid mapping the MMIO in dom0 for the following cases:
+     *   - The device is owned by dom0 (i.e. it has been flagged for
+     *     passthrough).
+     *   - PCI host bridges with driver in Xen. They will later be mapped by
+     *     pci_host_bridge_mappings().
+     */
+    struct map_range_data mr_data = {
+        .d = d,
+        .p2mt = p2mt,
+        .skip_mapping = !own_device ||
+                        (is_pci_passthrough_enabled() &&
+                        (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE))
+    };
 
     naddr = dt_number_of_address(dev);
 
     dt_dprintk("%s passthrough = %d naddr = %u\n",
-               dt_node_full_name(dev), need_mapping, naddr);
+               dt_node_full_name(dev), own_device, naddr);
 
-    if ( need_mapping )
+    if ( own_device )
     {
         dt_dprintk("Check if %s is behind the IOMMU and add it\n",
                    dt_node_full_name(dev));
@@ -1925,14 +1929,13 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
         }
     }
 
-    res = handle_device_interrupts(d, dev, need_mapping);
+    res = handle_device_interrupts(d, dev, own_device);
     if ( res < 0 )
         return res;
 
     /* Give permission and map MMIOs */
     for ( i = 0; i < naddr; i++ )
     {
-        struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
         res = dt_device_get_address(dev, i, &addr, &size);
         if ( res )
         {
@@ -1946,7 +1949,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
             return res;
     }
 
-    res = map_device_children(d, dev, p2mt);
+    res = map_device_children(dev, &mr_data);
     if ( res )
         return res;
 
@@ -3105,7 +3108,14 @@ static int __init construct_dom0(struct domain *d)
         return rc;
 
     if ( acpi_disabled )
+    {
         rc = prepare_dtb_hwdom(d, &kinfo);
+        if ( rc < 0 )
+            return rc;
+#ifdef CONFIG_HAS_PCI
+        rc = pci_host_bridge_mappings(d);
+#endif
+    }
     else
         rc = prepare_acpi(d, &kinfo);
 
diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
index 602d00799c8d..4f71b11c3057 100644
--- a/xen/arch/arm/pci/ecam.c
+++ b/xen/arch/arm/pci/ecam.c
@@ -40,6 +40,19 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
     return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where;
 }
 
+bool __init pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
+                                            struct pci_host_bridge *bridge,
+                                            uint64_t addr)
+{
+    struct pci_config_window *cfg = bridge->cfg;
+
+    /*
+     * We do not want ECAM address space to be mapped in Domain-0's p2m,
+     * so we can trap access to it.
+     */
+    return cfg->phys_addr != addr;
+}
+
 /* ECAM ops */
 const struct pci_ecam_ops pci_generic_ecam_ops = {
     .bus_shift  = 20,
@@ -47,6 +60,7 @@ const struct pci_ecam_ops pci_generic_ecam_ops = {
         .map_bus                = pci_ecam_map_bus,
         .read                   = pci_generic_config_read,
         .write                  = pci_generic_config_write,
+        .need_p2m_hwdom_mapping = pci_ecam_need_p2m_hwdom_mapping,
     }
 };
 
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 18b09d5e6f10..1b18480adf02 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -22,6 +22,8 @@
 #include <xen/sched.h>
 #include <xen/vmap.h>
 
+#include <asm/setup.h>
+
 /*
  * List for all the pci host bridges.
  */
@@ -313,6 +315,54 @@ int pci_host_iterate_bridges_and_count(struct domain *d,
     return count;
 }
 
+/*
+ * For each PCI host bridge we need to only map those ranges
+ * which are used by Domain-0 to properly initialize the bridge,
+ * e.g. we do not want to map ECAM configuration space which lives in
+ * "reg" device tree property, but we want to map other regions of
+ * the host bridge. The PCI aperture defined by the "ranges" device
+ * tree property should also be skipped.
+ */
+int __init pci_host_bridge_mappings(struct domain *d)
+{
+    struct pci_host_bridge *bridge;
+    struct map_range_data mr_data = {
+        .d = d,
+        .p2mt = p2m_mmio_direct_dev,
+        .skip_mapping = false
+    };
+
+    list_for_each_entry( bridge, &pci_host_bridges, node )
+    {
+        const struct dt_device_node *dev = bridge->dt_node;
+        unsigned int i;
+
+        for ( i = 0; i < dt_number_of_address(dev); i++ )
+        {
+            uint64_t addr, size;
+            int err;
+
+            err = dt_device_get_address(dev, i, &addr, &size);
+            if ( err )
+            {
+                printk(XENLOG_ERR
+                       "Unable to retrieve address range index=%u for %s\n",
+                       i, dt_node_full_name(dev));
+                return err;
+            }
+
+            if ( bridge->ops->need_p2m_hwdom_mapping(d, bridge, addr) )
+            {
+                err = map_range_to_domain(dev, addr, size, &mr_data);
+                if ( err )
+                    return err;
+            }
+        }
+    }
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/pci/pci-host-zynqmp.c b/xen/arch/arm/pci/pci-host-zynqmp.c
index 516982bca833..101edb8593c1 100644
--- a/xen/arch/arm/pci/pci-host-zynqmp.c
+++ b/xen/arch/arm/pci/pci-host-zynqmp.c
@@ -34,6 +34,7 @@ const struct pci_ecam_ops nwl_pcie_ops = {
         .map_bus                = pci_ecam_map_bus,
         .read                   = pci_generic_config_read,
         .write                  = pci_generic_config_write,
+        .need_p2m_hwdom_mapping = pci_ecam_need_p2m_hwdom_mapping,
     }
 };
 
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 4278d66e5eb9..679fc83f713b 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -17,6 +17,8 @@
 
 #ifdef CONFIG_HAS_PCI
 
+#include <asm/p2m.h>
+
 #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
 
 extern bool pci_passthrough_enabled;
@@ -73,6 +75,9 @@ struct pci_ops {
                 uint32_t reg, uint32_t len, uint32_t *value);
     int (*write)(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,
                  uint32_t reg, uint32_t len, uint32_t value);
+    bool (*need_p2m_hwdom_mapping)(struct domain *d,
+                                   struct pci_host_bridge *bridge,
+                                   uint64_t addr);
 };
 
 /*
@@ -96,6 +101,9 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,
                              uint32_t reg, uint32_t len, uint32_t value);
 void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
                                pci_sbdf_t sbdf, uint32_t where);
+bool pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
+                                     struct pci_host_bridge *bridge,
+                                     uint64_t addr);
 struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus);
 struct dt_device_node *pci_find_host_bridge_node(struct device *dev);
 int pci_get_host_bridge_segment(const struct dt_device_node *node,
@@ -114,6 +122,8 @@ int pci_host_iterate_bridges_and_count(struct domain *d,
                                        int (*cb)(struct domain *d,
                                                  struct pci_host_bridge *bridge));
 
+int pci_host_bridge_mappings(struct domain *d);
+
 #else   /*!CONFIG_HAS_PCI*/
 
 struct arch_pci_dev { };
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 95da0b7ab9cd..88d9673db817 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -2,6 +2,8 @@
 #define __ARM_SETUP_H_
 
 #include <public/version.h>
+#include <asm/p2m.h>
+#include <xen/device_tree.h>
 
 #define MIN_FDT_ALIGN 8
 #define MAX_FDT_SIZE SZ_2M
@@ -77,6 +79,14 @@ struct bootinfo {
 #endif
 };
 
+struct map_range_data
+{
+    struct domain *d;
+    p2m_type_t p2mt;
+    /* Set if mapping of the memory ranges must be skipped. */
+    bool skip_mapping;
+};
+
 extern struct bootinfo bootinfo;
 
 extern domid_t max_init_domid;
@@ -124,6 +134,9 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells,
 u32 device_tree_get_u32(const void *fdt, int node,
                         const char *prop_name, u32 dflt);
 
+int map_range_to_domain(const struct dt_device_node *dev,
+                        u64 addr, u64 len, void *data);
+
 #endif
 /*
  * Local variables:
-- 
2.25.1

[PATCH v7 6/7] xen/arm: process pending vPCI map/unmap operations

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

vPCI may map and unmap PCI device memory (BARs) being passed through which
may take a lot of time. For this those operations may be deferred to be
performed later, so that they can be safely preempted.

Currently this deferred processing is happening in common IOREQ code
which doesn't seem to be the right place for x86 and is even more
doubtful because IOREQ may not be enabled for Arm at all.
So, for Arm the pending vPCI work may have no chance to be executed
if the processing is left as is in the common IOREQ code only.
For that reason make vPCI processing happen in arch specific code.

Please be aware that there are a few outstanding TODOs affecting this
code path, see xen/drivers/vpci/header.c:map_range and
xen/drivers/vpci/header.c:vpci_process_pending.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
[x86 part]
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Paul Durrant <paul@xen.org>

Since v5:
 - check_for_vcpu_work: vPCI addition is moved before the
   vcpu_ioreq__handle_completion(v). This is to avoid differences
   with the x86 version. (Julien)
Since v2:
 - update commit message with more insight on x86, IOREQ and Arm
 - restored order of invocation for IOREQ and vPCI processing (Jan)
Since v1:
 - Moved the check for pending vpci work from the common IOREQ code
   to hvm_do_resume on x86
 - Re-worked the code for Arm to ensure we don't miss pending vPCI work
---
 xen/arch/arm/traps.c   | 13 +++++++++++++
 xen/arch/x86/hvm/hvm.c |  6 ++++++
 xen/common/ioreq.c     |  9 ---------
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 219ab3c3fbde..8757210a798b 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -34,6 +34,7 @@
 #include <xen/symbols.h>
 #include <xen/version.h>
 #include <xen/virtual_region.h>
+#include <xen/vpci.h>
 
 #include <public/sched.h>
 #include <public/xen.h>
@@ -2290,6 +2291,18 @@ static bool check_for_vcpu_work(void)
 {
     struct vcpu *v = current;
 
+    if ( has_vpci(v->domain) )
+    {
+        bool pending;
+
+        local_irq_enable();
+        pending = vpci_process_pending(v);
+        local_irq_disable();
+
+        if ( pending )
+            return true;
+    }
+
 #ifdef CONFIG_IOREQ_SERVER
     if ( domain_has_ioreq_server(v->domain) )
     {
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index eee365711d63..096a61b7ea02 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -546,6 +546,12 @@ void hvm_do_resume(struct vcpu *v)
 
     pt_restore_timer(v);
 
+    if ( has_vpci(v->domain) && vpci_process_pending(v) )
+    {
+        raise_softirq(SCHEDULE_SOFTIRQ);
+        return;
+    }
+
     if ( !vcpu_ioreq_handle_completion(v) )
         return;
 
diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index d732dc045df9..689d256544c8 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -25,9 +25,7 @@
 #include <xen/lib.h>
 #include <xen/paging.h>
 #include <xen/sched.h>
-#include <xen/softirq.h>
 #include <xen/trace.h>
-#include <xen/vpci.h>
 
 #include <asm/guest_atomics.h>
 #include <asm/ioreq.h>
@@ -212,19 +210,12 @@ static bool wait_for_io(struct ioreq_vcpu *sv, ioreq_t *p)
 
 bool vcpu_ioreq_handle_completion(struct vcpu *v)
 {
-    struct domain *d = v->domain;
     struct vcpu_io *vio = &v->io;
     struct ioreq_server *s;
     struct ioreq_vcpu *sv;
     enum vio_completion completion;
     bool res = true;
 
-    if ( has_vpci(d) && vpci_process_pending(v) )
-    {
-        raise_softirq(SCHEDULE_SOFTIRQ);
-        return false;
-    }
-
     while ( (sv = get_pending_vcpu(v, &s)) != NULL )
         if ( !wait_for_io(sv, get_ioreq(s, v)) )
             return false;
-- 
2.25.1

[PATCH v7 7/7] xen/arm: do not use void pointer in pci_host_common_probe

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

There is no reason to use void pointer while passing ECAM ops to the
pci_host_common_probe function as it is anyway casted to struct pci_ecam_ops
inside. For that reason remove the void pointer and pass struct pci_ecam_ops
pointer as is.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
New in v4
---
 xen/arch/arm/pci/ecam.c            | 4 ++--
 xen/arch/arm/pci/pci-host-common.c | 6 ++----
 xen/include/asm-arm/pci.h          | 5 +++--
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
index 4f71b11c3057..6aeea12a68bf 100644
--- a/xen/arch/arm/pci/ecam.c
+++ b/xen/arch/arm/pci/ecam.c
@@ -24,8 +24,8 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
                                pci_sbdf_t sbdf, uint32_t where)
 {
     const struct pci_config_window *cfg = bridge->cfg;
-    struct pci_ecam_ops *ops =
-        container_of(bridge->ops, struct pci_ecam_ops, pci_ops);
+    const struct pci_ecam_ops *ops =
+        container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
     unsigned int devfn_shift = ops->bus_shift - 8;
     void __iomem *base;
 
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 1b18480adf02..b1a26eb840ab 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -198,18 +198,16 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
     return domain;
 }
 
-int pci_host_common_probe(struct dt_device_node *dev, const void *data)
+int pci_host_common_probe(struct dt_device_node *dev,
+                          const struct pci_ecam_ops *ops)
 {
     struct pci_host_bridge *bridge;
     struct pci_config_window *cfg;
-    struct pci_ecam_ops *ops;
     int err;
 
     if ( dt_device_for_passthrough(dev) )
         return 0;
 
-    ops = (struct pci_ecam_ops *) data;
-
     bridge = pci_alloc_host_bridge();
     if ( !bridge )
         return -ENOMEM;
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 679fc83f713b..7c7449d64fca 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -65,7 +65,7 @@ struct pci_host_bridge {
     struct list_head node;           /* Node in list of host bridges */
     uint16_t segment;                /* Segment number */
     struct pci_config_window* cfg;   /* Pointer to the bridge config window */
-    struct pci_ops *ops;
+    const struct pci_ops *ops;
 };
 
 struct pci_ops {
@@ -94,7 +94,8 @@ struct pci_ecam_ops {
 /* Default ECAM ops */
 extern const struct pci_ecam_ops pci_generic_ecam_ops;
 
-int pci_host_common_probe(struct dt_device_node *dev, const void *data);
+int pci_host_common_probe(struct dt_device_node *dev,
+                          const struct pci_ecam_ops *ops);
 int pci_generic_config_read(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,
                             uint32_t reg, uint32_t len, uint32_t *value);
 int pci_generic_config_write(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,
-- 
2.25.1

RE: [PATCH v7 1/7] xen/arm: rename DEVICE_PCI to DEVICE_PCI_HOSTBRIDGE

[Jiamei Xie]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Oleksandr Andrushchenko
> Sent: 2021年11月24日 16:00
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; sstabellini@kernel.org;
> oleksandr_tyshchenko@epam.com; volodymyr_babchuk@epam.com;
> Artem_Mygaiev@epam.com; roger.pau@citrix.com; jbeulich@suse.com;
> andrew.cooper3@citrix.com; george.dunlap@citrix.com; paul@xen.org;
> Bertrand Marquis <Bertrand.Marquis@arm.com>; Rahul Singh
> <Rahul.Singh@arm.com>; Oleksandr Andrushchenko
> <oleksandr_andrushchenko@epam.com>; Julien Grall <jgrall@amazon.com>
> Subject: [PATCH v7 1/7] xen/arm: rename DEVICE_PCI to
> DEVICE_PCI_HOSTBRIDGE
> 
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> To better reflect the nature of the device type and not to make any
> confusion rename DEVICE_PCI to DEVICE_PCI_HOSTBRIDGE which it
> really is.
> 
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Oleksandr Andrushchenko
> <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> ---
> New in v6
> ---
>  xen/arch/arm/pci/pci-host-generic.c | 2 +-
>  xen/arch/arm/pci/pci-host-zynqmp.c  | 2 +-
>  xen/arch/arm/pci/pci.c              | 2 +-
>  xen/include/asm-arm/device.h        | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-
> host-generic.c
> index 33457fbe9615..46de6e43cc72 100644
> --- a/xen/arch/arm/pci/pci-host-generic.c
> +++ b/xen/arch/arm/pci/pci-host-generic.c
> @@ -32,7 +32,7 @@ static int __init pci_host_generic_probe(struct
> dt_device_node *dev,
>      return pci_host_common_probe(dev, &pci_generic_ecam_ops);
>  }
> 
> -DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
> +DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI_HOSTBRIDGE)
>  .dt_match = gen_pci_dt_match,
>  .init = pci_host_generic_probe,
>  DT_DEVICE_END
> diff --git a/xen/arch/arm/pci/pci-host-zynqmp.c b/xen/arch/arm/pci/pci-
> host-zynqmp.c
> index 61a9807d3d58..516982bca833 100644
> --- a/xen/arch/arm/pci/pci-host-zynqmp.c
> +++ b/xen/arch/arm/pci/pci-host-zynqmp.c
> @@ -49,7 +49,7 @@ static int __init pci_host_generic_probe(struct
> dt_device_node *dev,
>      return pci_host_common_probe(dev, &nwl_pcie_ops);
>  }
> 
> -DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI)
> +DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI_HOSTBRIDGE)
>  .dt_match = nwl_pcie_dt_match,
>  .init = pci_host_generic_probe,
>  DT_DEVICE_END
> diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
> index 082c14e127a8..78b97beaef12 100644
> --- a/xen/arch/arm/pci/pci.c
> +++ b/xen/arch/arm/pci/pci.c
> @@ -46,7 +46,7 @@ static int __init dt_pci_init(void)
> 
>      dt_for_each_device_node(dt_host, np)
>      {
> -        rc = device_init(np, DEVICE_PCI, NULL);
> +        rc = device_init(np, DEVICE_PCI_HOSTBRIDGE, NULL);
>          /*
>           * Ignore the following error codes:
>           *   - EBADF: Indicate the current device is not a pci device.
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 3782660751b6..086dde13eb6b 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -37,7 +37,7 @@ enum device_class
>      DEVICE_SERIAL,
>      DEVICE_IOMMU,
>      DEVICE_GIC,
> -    DEVICE_PCI,
> +    DEVICE_PCI_HOSTBRIDGE,
>      /* Use for error */
>      DEVICE_UNKNOWN,
>  };
> --
> 2.25.1
> 

[Jiamei Xie] 
LGTM.
Reviewed-by: Jiamei xie <jiamei.xie@arm.com> 

Re: [PATCH v7 1/7] xen/arm: rename DEVICE_PCI to DEVICE_PCI_HOSTBRIDGE

[Rahul Singh]

Hi Oleksandr,

> On 24 Nov 2021, at 7:59 am, Oleksandr Andrushchenko <andr2000@gmail.com> wrote:
> 
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> To better reflect the nature of the device type and not to make any
> confusion rename DEVICE_PCI to DEVICE_PCI_HOSTBRIDGE which it
> really is.
> 
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>

Regards,
Rahul
> ---
> New in v6
> ---
> xen/arch/arm/pci/pci-host-generic.c | 2 +-
> xen/arch/arm/pci/pci-host-zynqmp.c  | 2 +-
> xen/arch/arm/pci/pci.c              | 2 +-
> xen/include/asm-arm/device.h        | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
> index 33457fbe9615..46de6e43cc72 100644
> --- a/xen/arch/arm/pci/pci-host-generic.c
> +++ b/xen/arch/arm/pci/pci-host-generic.c
> @@ -32,7 +32,7 @@ static int __init pci_host_generic_probe(struct dt_device_node *dev,
>     return pci_host_common_probe(dev, &pci_generic_ecam_ops);
> }
> 
> -DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
> +DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI_HOSTBRIDGE)
> .dt_match = gen_pci_dt_match,
> .init = pci_host_generic_probe,
> DT_DEVICE_END
> diff --git a/xen/arch/arm/pci/pci-host-zynqmp.c b/xen/arch/arm/pci/pci-host-zynqmp.c
> index 61a9807d3d58..516982bca833 100644
> --- a/xen/arch/arm/pci/pci-host-zynqmp.c
> +++ b/xen/arch/arm/pci/pci-host-zynqmp.c
> @@ -49,7 +49,7 @@ static int __init pci_host_generic_probe(struct dt_device_node *dev,
>     return pci_host_common_probe(dev, &nwl_pcie_ops);
> }
> 
> -DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI)
> +DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI_HOSTBRIDGE)
> .dt_match = nwl_pcie_dt_match,
> .init = pci_host_generic_probe,
> DT_DEVICE_END
> diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
> index 082c14e127a8..78b97beaef12 100644
> --- a/xen/arch/arm/pci/pci.c
> +++ b/xen/arch/arm/pci/pci.c
> @@ -46,7 +46,7 @@ static int __init dt_pci_init(void)
> 
>     dt_for_each_device_node(dt_host, np)
>     {
> -        rc = device_init(np, DEVICE_PCI, NULL);
> +        rc = device_init(np, DEVICE_PCI_HOSTBRIDGE, NULL);
>         /*
>          * Ignore the following error codes:
>          *   - EBADF: Indicate the current device is not a pci device.
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 3782660751b6..086dde13eb6b 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -37,7 +37,7 @@ enum device_class
>     DEVICE_SERIAL,
>     DEVICE_IOMMU,
>     DEVICE_GIC,
> -    DEVICE_PCI,
> +    DEVICE_PCI_HOSTBRIDGE,
>     /* Use for error */
>     DEVICE_UNKNOWN,
> };
> -- 
> 2.25.1
> 
> 

Re: [PATCH v7 7/7] xen/arm: do not use void pointer in pci_host_common_probe

[Rahul Singh]

Hi Oleksandr,

> On 24 Nov 2021, at 7:59 am, Oleksandr Andrushchenko <andr2000@gmail.com> wrote:
> 
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> There is no reason to use void pointer while passing ECAM ops to the
> pci_host_common_probe function as it is anyway casted to struct pci_ecam_ops
> inside. For that reason remove the void pointer and pass struct pci_ecam_ops
> pointer as is.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Acked-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>

Regards,
Rahul
> ---
> New in v4
> ---
> xen/arch/arm/pci/ecam.c            | 4 ++--
> xen/arch/arm/pci/pci-host-common.c | 6 ++----
> xen/include/asm-arm/pci.h          | 5 +++--
> 3 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
> index 4f71b11c3057..6aeea12a68bf 100644
> --- a/xen/arch/arm/pci/ecam.c
> +++ b/xen/arch/arm/pci/ecam.c
> @@ -24,8 +24,8 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
>                                pci_sbdf_t sbdf, uint32_t where)
> {
>     const struct pci_config_window *cfg = bridge->cfg;
> -    struct pci_ecam_ops *ops =
> -        container_of(bridge->ops, struct pci_ecam_ops, pci_ops);
> +    const struct pci_ecam_ops *ops =
> +        container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
>     unsigned int devfn_shift = ops->bus_shift - 8;
>     void __iomem *base;
> 
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 1b18480adf02..b1a26eb840ab 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -198,18 +198,16 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>     return domain;
> }
> 
> -int pci_host_common_probe(struct dt_device_node *dev, const void *data)
> +int pci_host_common_probe(struct dt_device_node *dev,
> +                          const struct pci_ecam_ops *ops)
> {
>     struct pci_host_bridge *bridge;
>     struct pci_config_window *cfg;
> -    struct pci_ecam_ops *ops;
>     int err;
> 
>     if ( dt_device_for_passthrough(dev) )
>         return 0;
> 
> -    ops = (struct pci_ecam_ops *) data;
> -
>     bridge = pci_alloc_host_bridge();
>     if ( !bridge )
>         return -ENOMEM;
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 679fc83f713b..7c7449d64fca 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -65,7 +65,7 @@ struct pci_host_bridge {
>     struct list_head node;           /* Node in list of host bridges */
>     uint16_t segment;                /* Segment number */
>     struct pci_config_window* cfg;   /* Pointer to the bridge config window */
> -    struct pci_ops *ops;
> +    const struct pci_ops *ops;
> };
> 
> struct pci_ops {
> @@ -94,7 +94,8 @@ struct pci_ecam_ops {
> /* Default ECAM ops */
> extern const struct pci_ecam_ops pci_generic_ecam_ops;
> 
> -int pci_host_common_probe(struct dt_device_node *dev, const void *data);
> +int pci_host_common_probe(struct dt_device_node *dev,
> +                          const struct pci_ecam_ops *ops);
> int pci_generic_config_read(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,
>                             uint32_t reg, uint32_t len, uint32_t *value);
> int pci_generic_config_write(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,
> -- 
> 2.25.1
> 

Re: [PATCH v7 2/7] xen/arm: add pci-domain for disabled devices

[Rahul Singh]

Hi 

> On 24 Nov 2021, at 7:59 am, Oleksandr Andrushchenko <andr2000@gmail.com> wrote:
> 
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> If a PCI host bridge device is present in the device tree, but is
> disabled, then its PCI host bridge driver was not instantiated.
> This results in the failure of the pci_get_host_bridge_segment()
> and the following panic during Xen start:
> 
> (XEN) Device tree generation failed (-22).
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Could not set up DOM0 guest OS
> (XEN) ****************************************
> 
> Fix this by adding "linux,pci-domain" property for all device tree nodes
> which have "pci" device type, so we know which segments will be used by
> the guest for which bridges.
> 
> Fixes: 4cfab4425d39 ("xen/arm: Add linux,pci-domain property for hwdom if not available.")
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>

Regards,
Rahul
> 
> ---
> Since v6:
> - use use_dt_domains in pci_get_new_domain_nr and return -1 if set
> - do not set "linux,pci-domain" if parent device is "pci"
> - move the code to a new helper handle_linux_pci_domain (Julien)
> New in v6
> ---
> xen/arch/arm/domain_build.c        | 66 +++++++++++++++++++++++-------
> xen/arch/arm/pci/pci-host-common.c |  8 +++-
> xen/include/asm-arm/pci.h          |  8 ++++
> 3 files changed, 66 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index e76cee3caccf..c83c02ab8ac6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -654,6 +654,55 @@ static void __init allocate_static_memory(struct domain *d,
> }
> #endif
> 
> +/*
> + * When PCI passthrough is available we want to keep the
> + * "linux,pci-domain" in sync for every host bridge.
> + *
> + * Xen may not have a driver for all the host bridges. So we have
> + * to write an heuristic to detect whether a device node describes
> + * a host bridge.
> + *
> + * The current heuristic assumes that a device is a host bridge
> + * if the type is "pci" and then parent type is not "pci".
> + */
> +static int handle_linux_pci_domain(struct kernel_info *kinfo,
> +                                   const struct dt_device_node *node)
> +{
> +    uint16_t segment;
> +    int res;
> +
> +    if ( !is_pci_passthrough_enabled() )
> +        return 0;
> +
> +    if ( !dt_device_type_is_equal(node, "pci") )
> +        return 0;
> +
> +    if ( node->parent && dt_device_type_is_equal(node->parent, "pci") )
> +        return 0;
> +
> +    if ( dt_find_property(node, "linux,pci-domain", NULL) )
> +        return 0;
> +
> +    /* Allocate and create the linux,pci-domain */
> +    res = pci_get_host_bridge_segment(node, &segment);
> +    if ( res < 0 )
> +    {
> +        res = pci_get_new_domain_nr();
> +        if ( res < 0 )
> +        {
> +            printk(XENLOG_DEBUG "Can't assign PCI segment to %s\n",
> +                   node->full_name);
> +            return -FDT_ERR_NOTFOUND;
> +        }
> +
> +        segment = res;
> +        printk(XENLOG_DEBUG "Assigned segment %d to %s\n",
> +               segment, node->full_name);
> +    }
> +
> +    return fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment);
> +}
> +
> static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>                                    const struct dt_device_node *node)
> {
> @@ -755,21 +804,10 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>             return res;
>     }
> 
> -    if ( is_pci_passthrough_enabled() && dt_device_type_is_equal(node, "pci") )
> -    {
> -        if ( !dt_find_property(node, "linux,pci-domain", NULL) )
> -        {
> -            uint16_t segment;
> -
> -            res = pci_get_host_bridge_segment(node, &segment);
> -            if ( res < 0 )
> -                return res;
> +    res = handle_linux_pci_domain(kinfo, node);
> 
> -            res = fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment);
> -            if ( res )
> -                return res;
> -        }
> -    }
> +    if ( res )
> +        return res;
> 
>     /*
>      * Override the property "status" to disable the device when it's
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index cdeb3a283a77..9653b92b5b2e 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -30,6 +30,8 @@ static LIST_HEAD(pci_host_bridges);
> 
> static atomic_t domain_nr = ATOMIC_INIT(-1);
> 
> +static int use_dt_domains = -1;
> +
> static inline void __iomem *pci_remap_cfgspace(paddr_t start, size_t len)
> {
>     return ioremap_nocache(start, len);
> @@ -137,14 +139,16 @@ void pci_add_host_bridge(struct pci_host_bridge *bridge)
>     list_add_tail(&bridge->node, &pci_host_bridges);
> }
> 
> -static int pci_get_new_domain_nr(void)
> +int pci_get_new_domain_nr(void)
> {
> +    if ( use_dt_domains )
> +        return -1;
> +
>     return atomic_inc_return(&domain_nr);
> }
> 
> static int pci_bus_find_domain_nr(struct dt_device_node *dev)
> {
> -    static int use_dt_domains = -1;
>     int domain;
> 
>     domain = dt_get_pci_domain_nr(dev);
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 81273e0d87ac..c20eba643d86 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -108,6 +108,8 @@ static always_inline bool is_pci_passthrough_enabled(void)
> 
> void arch_pci_init_pdev(struct pci_dev *pdev);
> 
> +int pci_get_new_domain_nr(void);
> +
> #else   /*!CONFIG_HAS_PCI*/
> 
> struct arch_pci_dev { };
> @@ -128,5 +130,11 @@ static inline int pci_get_host_bridge_segment(const struct dt_device_node *node,
>     return -EINVAL;
> }
> 
> +static inline int pci_get_new_domain_nr(void)
> +{
> +    ASSERT_UNREACHABLE();
> +    return -1;
> +}
> +
> #endif  /*!CONFIG_HAS_PCI*/
> #endif /* __ARM_PCI_H__ */
> -- 
> 2.25.1
> 

Re: [PATCH v7 6/7] xen/arm: process pending vPCI map/unmap operations

[Rahul Singh]

Hi Oleksandr,

> On 24 Nov 2021, at 7:59 am, Oleksandr Andrushchenko <andr2000@gmail.com> wrote:
> 
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> vPCI may map and unmap PCI device memory (BARs) being passed through which
> may take a lot of time. For this those operations may be deferred to be
> performed later, so that they can be safely preempted.
> 
> Currently this deferred processing is happening in common IOREQ code
> which doesn't seem to be the right place for x86 and is even more
> doubtful because IOREQ may not be enabled for Arm at all.
> So, for Arm the pending vPCI work may have no chance to be executed
> if the processing is left as is in the common IOREQ code only.
> For that reason make vPCI processing happen in arch specific code.
> 
> Please be aware that there are a few outstanding TODOs affecting this
> code path, see xen/drivers/vpci/header.c:map_range and
> xen/drivers/vpci/header.c:vpci_process_pending.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> [x86 part]
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>

Regards,
Rahul
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Paul Durrant <paul@xen.org>
> 
> Since v5:
> - check_for_vcpu_work: vPCI addition is moved before the
>   vcpu_ioreq__handle_completion(v). This is to avoid differences
>   with the x86 version. (Julien)
> Since v2:
> - update commit message with more insight on x86, IOREQ and Arm
> - restored order of invocation for IOREQ and vPCI processing (Jan)
> Since v1:
> - Moved the check for pending vpci work from the common IOREQ code
>   to hvm_do_resume on x86
> - Re-worked the code for Arm to ensure we don't miss pending vPCI work
> ---
> xen/arch/arm/traps.c   | 13 +++++++++++++
> xen/arch/x86/hvm/hvm.c |  6 ++++++
> xen/common/ioreq.c     |  9 ---------
> 3 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 219ab3c3fbde..8757210a798b 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -34,6 +34,7 @@
> #include <xen/symbols.h>
> #include <xen/version.h>
> #include <xen/virtual_region.h>
> +#include <xen/vpci.h>
> 
> #include <public/sched.h>
> #include <public/xen.h>
> @@ -2290,6 +2291,18 @@ static bool check_for_vcpu_work(void)
> {
>     struct vcpu *v = current;
> 
> +    if ( has_vpci(v->domain) )
> +    {
> +        bool pending;
> +
> +        local_irq_enable();
> +        pending = vpci_process_pending(v);
> +        local_irq_disable();
> +
> +        if ( pending )
> +            return true;
> +    }
> +
> #ifdef CONFIG_IOREQ_SERVER
>     if ( domain_has_ioreq_server(v->domain) )
>     {
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index eee365711d63..096a61b7ea02 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -546,6 +546,12 @@ void hvm_do_resume(struct vcpu *v)
> 
>     pt_restore_timer(v);
> 
> +    if ( has_vpci(v->domain) && vpci_process_pending(v) )
> +    {
> +        raise_softirq(SCHEDULE_SOFTIRQ);
> +        return;
> +    }
> +
>     if ( !vcpu_ioreq_handle_completion(v) )
>         return;
> 
> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> index d732dc045df9..689d256544c8 100644
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -25,9 +25,7 @@
> #include <xen/lib.h>
> #include <xen/paging.h>
> #include <xen/sched.h>
> -#include <xen/softirq.h>
> #include <xen/trace.h>
> -#include <xen/vpci.h>
> 
> #include <asm/guest_atomics.h>
> #include <asm/ioreq.h>
> @@ -212,19 +210,12 @@ static bool wait_for_io(struct ioreq_vcpu *sv, ioreq_t *p)
> 
> bool vcpu_ioreq_handle_completion(struct vcpu *v)
> {
> -    struct domain *d = v->domain;
>     struct vcpu_io *vio = &v->io;
>     struct ioreq_server *s;
>     struct ioreq_vcpu *sv;
>     enum vio_completion completion;
>     bool res = true;
> 
> -    if ( has_vpci(d) && vpci_process_pending(v) )
> -    {
> -        raise_softirq(SCHEDULE_SOFTIRQ);
> -        return false;
> -    }
> -
>     while ( (sv = get_pending_vcpu(v, &s)) != NULL )
>         if ( !wait_for_io(sv, get_ioreq(s, v)) )
>             return false;
> -- 
> 2.25.1
> 

RE: [PATCH v7 1/7] xen/arm: rename DEVICE_PCI to DEVICE_PCI_HOSTBRIDGE

[Henry Wang]

Hi Oleksandr,

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Oleksandr Andrushchenko
> Sent: Wednesday, November 24, 2021 4:00 PM
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; sstabellini@kernel.org;
> oleksandr_tyshchenko@epam.com; volodymyr_babchuk@epam.com;
> Artem_Mygaiev@epam.com; roger.pau@citrix.com; jbeulich@suse.com;
> andrew.cooper3@citrix.com; george.dunlap@citrix.com; paul@xen.org;
> Bertrand Marquis <Bertrand.Marquis@arm.com>; Rahul Singh
> <Rahul.Singh@arm.com>; Oleksandr Andrushchenko
> <oleksandr_andrushchenko@epam.com>; Julien Grall <jgrall@amazon.com>
> Subject: [PATCH v7 1/7] xen/arm: rename DEVICE_PCI to
> DEVICE_PCI_HOSTBRIDGE
> 
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> To better reflect the nature of the device type and not to make any
> confusion rename DEVICE_PCI to DEVICE_PCI_HOSTBRIDGE which it
> really is.
> 
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Oleksandr Andrushchenko
> <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> ---
> New in v6
> ---
>  xen/arch/arm/pci/pci-host-generic.c | 2 +-
>  xen/arch/arm/pci/pci-host-zynqmp.c  | 2 +-
>  xen/arch/arm/pci/pci.c              | 2 +-
>  xen/include/asm-arm/device.h        | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-
> host-generic.c
> index 33457fbe9615..46de6e43cc72 100644
> --- a/xen/arch/arm/pci/pci-host-generic.c
> +++ b/xen/arch/arm/pci/pci-host-generic.c
> @@ -32,7 +32,7 @@ static int __init pci_host_generic_probe(struct
> dt_device_node *dev,
>      return pci_host_common_probe(dev, &pci_generic_ecam_ops);
>  }
> 
> -DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
> +DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI_HOSTBRIDGE)
>  .dt_match = gen_pci_dt_match,
>  .init = pci_host_generic_probe,
>  DT_DEVICE_END
> diff --git a/xen/arch/arm/pci/pci-host-zynqmp.c b/xen/arch/arm/pci/pci-
> host-zynqmp.c
> index 61a9807d3d58..516982bca833 100644
> --- a/xen/arch/arm/pci/pci-host-zynqmp.c
> +++ b/xen/arch/arm/pci/pci-host-zynqmp.c
> @@ -49,7 +49,7 @@ static int __init pci_host_generic_probe(struct
> dt_device_node *dev,
>      return pci_host_common_probe(dev, &nwl_pcie_ops);
>  }
> 
> -DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI)
> +DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI_HOSTBRIDGE)
>  .dt_match = nwl_pcie_dt_match,
>  .init = pci_host_generic_probe,
>  DT_DEVICE_END
> diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
> index 082c14e127a8..78b97beaef12 100644
> --- a/xen/arch/arm/pci/pci.c
> +++ b/xen/arch/arm/pci/pci.c
> @@ -46,7 +46,7 @@ static int __init dt_pci_init(void)
> 
>      dt_for_each_device_node(dt_host, np)
>      {
> -        rc = device_init(np, DEVICE_PCI, NULL);
> +        rc = device_init(np, DEVICE_PCI_HOSTBRIDGE, NULL);
>          /*
>           * Ignore the following error codes:
>           *   - EBADF: Indicate the current device is not a pci device.
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 3782660751b6..086dde13eb6b 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -37,7 +37,7 @@ enum device_class
>      DEVICE_SERIAL,
>      DEVICE_IOMMU,
>      DEVICE_GIC,
> -    DEVICE_PCI,
> +    DEVICE_PCI_HOSTBRIDGE,
>      /* Use for error */
>      DEVICE_UNKNOWN,
>  };
> --
> 2.25.1
> 

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,

Henry

Re: [PATCH v7 6/7] xen/arm: process pending vPCI map/unmap operations

[Julien Grall]

Hi,

On 24/11/2021 07:59, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> vPCI may map and unmap PCI device memory (BARs) being passed through which
> may take a lot of time. For this those operations may be deferred to be
> performed later, so that they can be safely preempted.
> 
> Currently this deferred processing is happening in common IOREQ code
> which doesn't seem to be the right place for x86 and is even more
> doubtful because IOREQ may not be enabled for Arm at all.
> So, for Arm the pending vPCI work may have no chance to be executed
> if the processing is left as is in the common IOREQ code only.
> For that reason make vPCI processing happen in arch specific code.
> 
> Please be aware that there are a few outstanding TODOs affecting this
> code path, see xen/drivers/vpci/header.c:map_range and
> xen/drivers/vpci/header.c:vpci_process_pending.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> [x86 part]
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>

This patch technically needs an acked-by from Paul for the IOREQ part. Paul?

Cheers,

-- 
Julien Grall

Re: [PATCH v7 0/7] PCI devices passthrough on Arm, part 2

[Julien Grall]

Hi Oleksandr,

On 24/11/2021 07:59, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> This is an assorted series of patches which aim is to make some further
> basis for PCI passthrough on Arm support. The series continues the work
> published earlier by Arm [1] and adds new helpers and clears the way for
> vPCI changes which will follow.
> 
> RFC is at [2], [3]. Design presentation can be found at [4].
> 
> I have removed patch
> [PATCH v6 5/7] xen/arm: do not map IRQs and memory for disabled devices
> as it seems that this needs more time for decision on how to achive
> that.
> 
> I have also added a new patch
> [PATCH v7 4/7] xen/arm: account IO handler for emulated PCI host bridge
> with a tiny latent bug fix.
> 
> Thank you,
> Oleksandr
> 
> [1] https://patchwork.kernel.org/project/xen-devel/list/?series=558681
> [2] https://lists.xenproject.org/archives/html/xen-devel/2020-07/msg01184.html
> [3] https://lists.xenproject.org/archives/html/xen-devel/2020-07/threads.html#01184
> [4] https://static.sched.com/hosted_files/xen2021/e4/PCI_Device_Passthrough_On_Arm.pdf
> 
> Oleksandr Andrushchenko (7):
>    xen/arm: rename DEVICE_PCI to DEVICE_PCI_HOSTBRIDGE

[...]

>    xen/arm: do not use void pointer in pci_host_common_probe

I have committed these two commits. I was going to also commit patch #6, 
but it looks like an ack is missing from Paul.

I still haven't reviewed the rest of the patches.

Cheers,

-- 
Julien Grall

Re: [PATCH v7 6/7] xen/arm: process pending vPCI map/unmap operations

[Durrant, Paul]

On 23/11/2021 23:59, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> vPCI may map and unmap PCI device memory (BARs) being passed through which
> may take a lot of time. For this those operations may be deferred to be
> performed later, so that they can be safely preempted.
> 
> Currently this deferred processing is happening in common IOREQ code
> which doesn't seem to be the right place for x86 and is even more
> doubtful because IOREQ may not be enabled for Arm at all.
> So, for Arm the pending vPCI work may have no chance to be executed
> if the processing is left as is in the common IOREQ code only.
> For that reason make vPCI processing happen in arch specific code.
> 
> Please be aware that there are a few outstanding TODOs affecting this
> code path, see xen/drivers/vpci/header.c:map_range and
> xen/drivers/vpci/header.c:vpci_process_pending.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> [x86 part]
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH v7 6/7] xen/arm: process pending vPCI map/unmap operations

[Oleksandr Andrushchenko]

Hi, Julien!

On 03.12.21 18:10, Durrant, Paul wrote:
> On 23/11/2021 23:59, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> vPCI may map and unmap PCI device memory (BARs) being passed through which
>> may take a lot of time. For this those operations may be deferred to be
>> performed later, so that they can be safely preempted.
>>
>> Currently this deferred processing is happening in common IOREQ code
>> which doesn't seem to be the right place for x86 and is even more
>> doubtful because IOREQ may not be enabled for Arm at all.
>> So, for Arm the pending vPCI work may have no chance to be executed
>> if the processing is left as is in the common IOREQ code only.
>> For that reason make vPCI processing happen in arch specific code.
>>
>> Please be aware that there are a few outstanding TODOs affecting this
>> code path, see xen/drivers/vpci/header.c:map_range and
>> xen/drivers/vpci/header.c:vpci_process_pending.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> [x86 part]
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Julien Grall <jgrall@amazon.com>
>
> Reviewed-by: Paul Durrant <paul@xen.org>
>
Do we need anything else for this patch?

Thank you,
Oleksandr

Re: [PATCH v7 6/7] xen/arm: process pending vPCI map/unmap operations

[Julien Grall]

On 07/12/2021 11:57, Oleksandr Andrushchenko wrote:
> Hi, Julien!

Hi,

> On 03.12.21 18:10, Durrant, Paul wrote:
>> On 23/11/2021 23:59, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> vPCI may map and unmap PCI device memory (BARs) being passed through which
>>> may take a lot of time. For this those operations may be deferred to be
>>> performed later, so that they can be safely preempted.
>>>
>>> Currently this deferred processing is happening in common IOREQ code
>>> which doesn't seem to be the right place for x86 and is even more
>>> doubtful because IOREQ may not be enabled for Arm at all.
>>> So, for Arm the pending vPCI work may have no chance to be executed
>>> if the processing is left as is in the common IOREQ code only.
>>> For that reason make vPCI processing happen in arch specific code.
>>>
>>> Please be aware that there are a few outstanding TODOs affecting this
>>> code path, see xen/drivers/vpci/header.c:map_range and
>>> xen/drivers/vpci/header.c:vpci_process_pending.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>> [x86 part]
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>> Reviewed-by: Julien Grall <jgrall@amazon.com>
>>
>> Reviewed-by: Paul Durrant <paul@xen.org>
>>
> Do we need anything else for this patch?

No. I have committed the patch.

Cheers,

-- 
Julien Grall

Re: [PATCH v7 3/7] xen/arm: setup MMIO range trap handlers for hardware domain

[Julien Grall]

Hi Oleksandr,

On 24/11/2021 07:59, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> In order for vPCI to work it needs to maintain guest and hardware
> domain's views of the configuration space. For example, BARs and
> COMMAND registers require emulation for guests and the guest view
> of the registers needs to be in sync with the real contents of the
> relevant registers. For that ECAM address space needs to also be
> trapped for the hardware domain, so we need to implement PCI host
> bridge specific callbacks to properly setup MMIO handlers for those
> ranges depending on particular host bridge implementation.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
> Since v6:
> - eliminate pci_host_get_num_bridges and make pci_host_iterate_bridges
>    return the count
> - extend comment in domain_vpci_init
> - remove not yet relevant code for num MMIOs and virtual bus topology
> - add extra check for has_vpci in domain_vpci_get_num_mmio_handlers
> - remove code that fixes num MMIOs for guest domain as it doesn't belong
>    to this patch
> Since v5:
> - add vpci_sbdf_from_gpa helper for gpa to SBDF translation
> - take bridge's bus start into account while calculating SBDF
> Since v4:
> - unsigned int for functions working with count
> - gate number of MMIO handlers needed for CONFIG_HAS_PCI_MSI
>    and fix their number, e.g. single handler for PBA and
>    MSI-X tables (Roger)
> - re-work code for assigning MMIO handlers to be simpler
>    and account on the fact that there could multiple host bridges
>    exist for the hwdom
> Since v3:
> - fixed comment formatting
> Since v2:
> - removed unneeded assignment (count = 0)
> - removed unneeded header inclusion
> - update commit message
> Since v1:
>   - Dynamically calculate the number of MMIO handlers required for vPCI
>     and update the total number accordingly
>   - s/clb/cb
>   - Do not introduce a new callback for MMIO handler setup
> ---
>   xen/arch/arm/domain.c              |  2 +
>   xen/arch/arm/pci/pci-host-common.c | 17 +++++++
>   xen/arch/arm/vpci.c                | 79 +++++++++++++++++++++++++++---
>   xen/arch/arm/vpci.h                |  6 +++
>   xen/include/asm-arm/pci.h          |  4 ++
>   5 files changed, 100 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 96e1b235501d..92a6c509e5c5 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -739,6 +739,8 @@ int arch_domain_create(struct domain *d,
>       if ( (rc = domain_vgic_register(d, &count)) != 0 )
>           goto fail;
>   
> +    count += domain_vpci_get_num_mmio_handlers(d);
> +
>       if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 )
>           goto fail;
>   
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 9653b92b5b2e..18b09d5e6f10 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -296,6 +296,23 @@ int pci_get_host_bridge_segment(const struct dt_device_node *node,
>       return -EINVAL;
>   }
>   
> +int pci_host_iterate_bridges_and_count(struct domain *d,
> +                                       int (*cb)(struct domain *d,
> +                                                 struct pci_host_bridge *bridge))
> +{
> +    struct pci_host_bridge *bridge;
> +    int err, count = 0;

NIT: err is only used within the loop, so I would suggest to move the 
declartion variable inside the loop.

> +
> +    list_for_each_entry( bridge, &pci_host_bridges, node )
> +    {
> +        err = cb(d, bridge);
> +        if ( err )
> +            return err;
> +        count += err;

Something looks wrong here. You check that err is non-zero and bail out. 
So err would be 0 and by extend you would always return 0 in success?

> +    }
> +    return count;
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index 23f45386f4b3..ccd998d8dba2 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -16,16 +16,31 @@
>   
>   #include <asm/mmio.h>
>   
> +static pci_sbdf_t vpci_sbdf_from_gpa(const struct pci_host_bridge *bridge,
> +                                     paddr_t gpa)
> +{
> +    pci_sbdf_t sbdf;
> +
> +    if ( bridge )
> +    {
> +        sbdf.sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr);
> +        sbdf.seg = bridge->segment;
> +        sbdf.bus += bridge->cfg->busn_start;
> +    }
> +    else
> +        sbdf.sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE);
> +
> +    return sbdf;
> +}
> +
>   static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>                             register_t *r, void *p)
>   {
> -    pci_sbdf_t sbdf;
> +    struct pci_host_bridge *bridge = p;
> +    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>       /* data is needed to prevent a pointer cast on 32bit */
>       unsigned long data;
>   
> -    /* We ignore segment part and always handle segment 0 */
> -    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE);
> -
>       if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>                           1U << info->dabt.size, &data) )
>       {
> @@ -41,10 +56,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>   static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
>                              register_t r, void *p)
>   {
> -    pci_sbdf_t sbdf;
> -
> -    /* We ignore segment part and always handle segment 0 */
> -    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE);
> +    struct pci_host_bridge *bridge = p;
> +    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>   
>       return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa),
>                              1U << info->dabt.size, r);
> @@ -55,17 +68,67 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
>       .write = vpci_mmio_write,
>   };
>   
> +static int vpci_setup_mmio_handler_cb(struct domain *d,
> +                                      struct pci_host_bridge *bridge)
> +{
> +    struct pci_config_window *cfg = bridge->cfg;
> +
> +    register_mmio_handler(d, &vpci_mmio_handler,
> +                          cfg->phys_addr, cfg->size, bridge);
> +
> +    /* We have registered a single MMIO handler. */
> +    return 1;
> +}
> +
>   int domain_vpci_init(struct domain *d)
>   {
>       if ( !has_vpci(d) )
>           return 0;
>   
> +    /*
> +     * The hardware domain gets as many MMIOs as required by the
> +     * physical host bridge.
> +     * Guests get the virtual platform layout: one virtual host bridge for now.
> +     */
> +    if ( is_hardware_domain(d) )
> +    {
> +        int count;
> +
> +        count = pci_host_iterate_bridges_and_count(d, vpci_setup_mmio_handler_cb);
> +        if ( count < 0 )
> +            return count;
> +
> +        return 0;

This could be simplified to "return (count < 0) ? count : 0;". Although 
my preference would be to move the guest part in an else.

> +    }
> +
>       register_mmio_handler(d, &vpci_mmio_handler,
>                             GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
>   
>       return 0;
>   }
>   
> +static int vpci_get_num_handlers_cb(struct domain *d,
> +                                    struct pci_host_bridge *bridge)
> +{
> +    /* Each bridge has a single MMIO handler for the configuration space. */
> +    return 1;
> +}
> +
> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
> +{
> +    if ( !has_vpci(d) )
> +        return 0;
> +
> +    if ( is_hardware_domain(d) )
> +    {
> +        int ret = pci_host_iterate_bridges_and_count(d, vpci_get_num_handlers_cb);
> +
> +        return ret < 0 ? 0 : ret;
> +    }
> +
> +    return 0;
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h
> index d8a7b0e3e802..3c713f3fcdb5 100644
> --- a/xen/arch/arm/vpci.h
> +++ b/xen/arch/arm/vpci.h
> @@ -17,11 +17,17 @@
>   
>   #ifdef CONFIG_HAS_VPCI
>   int domain_vpci_init(struct domain *d);
> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d);
>   #else
>   static inline int domain_vpci_init(struct domain *d)
>   {
>       return 0;
>   }
> +
> +static inline unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
> +{
> +    return 0;
> +}
>   #endif
>   
>   #endif /* __ARCH_ARM_VPCI_H__ */
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index c20eba643d86..4278d66e5eb9 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -110,6 +110,10 @@ void arch_pci_init_pdev(struct pci_dev *pdev);
>   
>   int pci_get_new_domain_nr(void);
>   
> +int pci_host_iterate_bridges_and_count(struct domain *d,
> +                                       int (*cb)(struct domain *d,
> +                                                 struct pci_host_bridge *bridge));
> +
>   #else   /*!CONFIG_HAS_PCI*/
>   
>   struct arch_pci_dev { };
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v7 4/7] xen/arm: account IO handler for emulated PCI host bridge

[Julien Grall]

Hi Oleksandr,

On 24/11/2021 07:59, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> At the moment, we always allocate an extra 16 slots for IO handlers
> (see MAX_IO_HANDLER). So while adding an IO trap handler for the emulated
> PCI host bridge we are not breaking anything, but we have a latent bug
> as the maximum number of IOs may be exceeded.
> Fix this by explicitly telling that we have an additional IO handler, so it is
> accounted.
> 
> Fixes: d59168dc05a5 ("xen/arm: Enable the existing x86 virtual PCI support for ARM")

In general, it is better to have the fixes at the beginning of a series. 
So they are relying on less rework and easier to backport (if needed).

In this case, PCI passthrough is still a technical preview so it doesn't 
matter too much.

> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
 >
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index ccd998d8dba2..8e801f275879 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -126,7 +126,8 @@ unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
>           return ret < 0 ? 0 : ret;
>       }
>   
> -    return 0;
> +    /* For a single emulated host bridge's configuration space. */

This comment is lacking some context. I would suggest to reword to 
something like:

"For the guests, each host bridge requires one region to cover the 
configuration space. At the moment, we only expose a single host bridge.
"

With that (or a similar comment):

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v7 5/7] xen/arm: do not map PCI ECAM and MMIO space to Domain-0's p2m

[Julien Grall]

Hi Oleksandr,

On 24/11/2021 07:59, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> PCI host bridges are special devices in terms of implementing PCI
> passthrough. According to [1] the current implementation depends on
> Domain-0 to perform the initialization of the relevant PCI host
> bridge hardware and perform PCI device enumeration. In order to
> achieve that one of the required changes is to not map all the memory
> ranges in map_range_to_domain as we traverse the device tree on startup
> and perform some additional checks if the range needs to be mapped to
> Domain-0.
> 
> The generic PCI host controller device tree binding says [2]:
> - ranges: As described in IEEE Std 1275-1994, but must provide
>            at least a definition of non-prefetchable memory. One
>            or both of prefetchable Memory and IO Space may also
>            be provided.
> 
> - reg   : The Configuration Space base address and size, as accessed
>            from the parent bus.  The base address corresponds to
>            the first bus in the "bus-range" property.  If no
>            "bus-range" is specified, this will be bus 0 (the default).
> 
>  From the above none of the memory ranges from the "ranges" property
> needs to be mapped to Domain-0 at startup as MMIO mapping is going to
> be handled dynamically by vPCI as we assign PCI devices, e.g. each
> device assigned to Domain-0/guest will have its MMIOs mapped/unmapped
> as needed by Xen.
> 
> The "reg" property covers not only ECAM space, but may also have other
> then the configuration memory ranges described, for example [3]:
> - reg: Should contain rc_dbi, config registers location and length.
> - reg-names: Must include the following entries:
>     "rc_dbi": controller configuration registers;
>     "config": PCIe configuration space registers.
> 
> This patch makes it possible to not map all the ranges from the
> "ranges" property and also ECAM from the "reg". All the rest from the
> "reg" property still needs to be mapped to Domain-0, so the PCI
> host bridge remains functional in Domain-0. This is done by first
> skipping the mappings while traversing the device tree as it is done for
> usual devices and then by calling a dedicated pci_host_bridge_mappings
> function which only maps MMIOs required by the host bridges leaving the
> regions, needed for vPCI traps, unmapped.
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2020-07/msg00777.html
> [2] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> [3] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

I haven't committed because it is not clear whether this patch depends 
on earlier patches that are still under review. Can you advise?

Cheers,

-- 
Julien Grall

Re: [PATCH v7 2/7] xen/arm: add pci-domain for disabled devices

[Julien Grall]

Hi Oleksandr,

On 24/11/2021 07:59, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> If a PCI host bridge device is present in the device tree, but is
> disabled, then its PCI host bridge driver was not instantiated.
> This results in the failure of the pci_get_host_bridge_segment()
> and the following panic during Xen start:
> 
> (XEN) Device tree generation failed (-22).
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Could not set up DOM0 guest OS
> (XEN) ****************************************
> 
> Fix this by adding "linux,pci-domain" property for all device tree nodes
> which have "pci" device type, so we know which segments will be used by
> the guest for which bridges.
> 
> Fixes: 4cfab4425d39 ("xen/arm: Add linux,pci-domain property for hwdom if not available.")
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v7 3/7] xen/arm: setup MMIO range trap handlers for hardware domain

[Oleksandr Andrushchenko]

Hi, Julien!

On 08.12.21 18:48, Julien Grall wrote:
> Hi Oleksandr,
>
> On 24/11/2021 07:59, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> In order for vPCI to work it needs to maintain guest and hardware
>> domain's views of the configuration space. For example, BARs and
>> COMMAND registers require emulation for guests and the guest view
>> of the registers needs to be in sync with the real contents of the
>> relevant registers. For that ECAM address space needs to also be
>> trapped for the hardware domain, so we need to implement PCI host
>> bridge specific callbacks to properly setup MMIO handlers for those
>> ranges depending on particular host bridge implementation.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> ---
>> Since v6:
>> - eliminate pci_host_get_num_bridges and make pci_host_iterate_bridges
>>    return the count
>> - extend comment in domain_vpci_init
>> - remove not yet relevant code for num MMIOs and virtual bus topology
>> - add extra check for has_vpci in domain_vpci_get_num_mmio_handlers
>> - remove code that fixes num MMIOs for guest domain as it doesn't belong
>>    to this patch
>> Since v5:
>> - add vpci_sbdf_from_gpa helper for gpa to SBDF translation
>> - take bridge's bus start into account while calculating SBDF
>> Since v4:
>> - unsigned int for functions working with count
>> - gate number of MMIO handlers needed for CONFIG_HAS_PCI_MSI
>>    and fix their number, e.g. single handler for PBA and
>>    MSI-X tables (Roger)
>> - re-work code for assigning MMIO handlers to be simpler
>>    and account on the fact that there could multiple host bridges
>>    exist for the hwdom
>> Since v3:
>> - fixed comment formatting
>> Since v2:
>> - removed unneeded assignment (count = 0)
>> - removed unneeded header inclusion
>> - update commit message
>> Since v1:
>>   - Dynamically calculate the number of MMIO handlers required for vPCI
>>     and update the total number accordingly
>>   - s/clb/cb
>>   - Do not introduce a new callback for MMIO handler setup
>> ---
>>   xen/arch/arm/domain.c              |  2 +
>>   xen/arch/arm/pci/pci-host-common.c | 17 +++++++
>>   xen/arch/arm/vpci.c                | 79 +++++++++++++++++++++++++++---
>>   xen/arch/arm/vpci.h                |  6 +++
>>   xen/include/asm-arm/pci.h          |  4 ++
>>   5 files changed, 100 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 96e1b235501d..92a6c509e5c5 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -739,6 +739,8 @@ int arch_domain_create(struct domain *d,
>>       if ( (rc = domain_vgic_register(d, &count)) != 0 )
>>           goto fail;
>>   +    count += domain_vpci_get_num_mmio_handlers(d);
>> +
>>       if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 )
>>           goto fail;
>>   diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
>> index 9653b92b5b2e..18b09d5e6f10 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -296,6 +296,23 @@ int pci_get_host_bridge_segment(const struct dt_device_node *node,
>>       return -EINVAL;
>>   }
>>   +int pci_host_iterate_bridges_and_count(struct domain *d,
>> +                                       int (*cb)(struct domain *d,
>> +                                                 struct pci_host_bridge *bridge))
>> +{
>> +    struct pci_host_bridge *bridge;
>> +    int err, count = 0;
>
> NIT: err is only used within the loop, so I would suggest to move the declartion variable inside the loop.
Yes, and it better sounds if s/err/ret
>
>> +
>> +    list_for_each_entry( bridge, &pci_host_bridges, node )
>> +    {
>> +        err = cb(d, bridge);
>> +        if ( err )
>> +            return err;
>> +        count += err;
>
> Something looks wrong here. You check that err is non-zero and bail out. So err would be 0 and by extend you would always return 0 in success?
Indeed, I need "if ( err < 0 )" here

>
>> +    }
>> +    return count;
>> +}
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> index 23f45386f4b3..ccd998d8dba2 100644
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -16,16 +16,31 @@
>>     #include <asm/mmio.h>
>>   +static pci_sbdf_t vpci_sbdf_from_gpa(const struct pci_host_bridge *bridge,
>> +                                     paddr_t gpa)
>> +{
>> +    pci_sbdf_t sbdf;
>> +
>> +    if ( bridge )
>> +    {
>> +        sbdf.sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr);
>> +        sbdf.seg = bridge->segment;
>> +        sbdf.bus += bridge->cfg->busn_start;
>> +    }
>> +    else
>> +        sbdf.sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE);
>> +
>> +    return sbdf;
>> +}
>> +
>>   static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>>                             register_t *r, void *p)
>>   {
>> -    pci_sbdf_t sbdf;
>> +    struct pci_host_bridge *bridge = p;
>> +    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>>       /* data is needed to prevent a pointer cast on 32bit */
>>       unsigned long data;
>>   -    /* We ignore segment part and always handle segment 0 */
>> -    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE);
>> -
>>       if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>>                           1U << info->dabt.size, &data) )
>>       {
>> @@ -41,10 +56,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>>   static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
>>                              register_t r, void *p)
>>   {
>> -    pci_sbdf_t sbdf;
>> -
>> -    /* We ignore segment part and always handle segment 0 */
>> -    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE);
>> +    struct pci_host_bridge *bridge = p;
>> +    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>>         return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa),
>>                              1U << info->dabt.size, r);
>> @@ -55,17 +68,67 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
>>       .write = vpci_mmio_write,
>>   };
>>   +static int vpci_setup_mmio_handler_cb(struct domain *d,
>> +                                      struct pci_host_bridge *bridge)
>> +{
>> +    struct pci_config_window *cfg = bridge->cfg;
>> +
>> +    register_mmio_handler(d, &vpci_mmio_handler,
>> +                          cfg->phys_addr, cfg->size, bridge);
>> +
>> +    /* We have registered a single MMIO handler. */
>> +    return 1;
>> +}
>> +
>>   int domain_vpci_init(struct domain *d)
>>   {
>>       if ( !has_vpci(d) )
>>           return 0;
>>   +    /*
>> +     * The hardware domain gets as many MMIOs as required by the
>> +     * physical host bridge.
>> +     * Guests get the virtual platform layout: one virtual host bridge for now.
>> +     */
>> +    if ( is_hardware_domain(d) )
>> +    {
>> +        int count;
>> +
>> +        count = pci_host_iterate_bridges_and_count(d, vpci_setup_mmio_handler_cb);
>> +        if ( count < 0 )
>> +            return count;
>> +
>> +        return 0;
>
> This could be simplified to "return (count < 0) ? count : 0;". Although my preference would be to move the guest part in an else.
Ok

Thank you,
Oleksandr
>
>> +    }
>> +
>>       register_mmio_handler(d, &vpci_mmio_handler,
>>                             GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
>>         return 0;
>>   }
>>   +static int vpci_get_num_handlers_cb(struct domain *d,
>> +                                    struct pci_host_bridge *bridge)
>> +{
>> +    /* Each bridge has a single MMIO handler for the configuration space. */
>> +    return 1;
>> +}
>> +
>> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
>> +{
>> +    if ( !has_vpci(d) )
>> +        return 0;
>> +
>> +    if ( is_hardware_domain(d) )
>> +    {
>> +        int ret = pci_host_iterate_bridges_and_count(d, vpci_get_num_handlers_cb);
>> +
>> +        return ret < 0 ? 0 : ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h
>> index d8a7b0e3e802..3c713f3fcdb5 100644
>> --- a/xen/arch/arm/vpci.h
>> +++ b/xen/arch/arm/vpci.h
>> @@ -17,11 +17,17 @@
>>     #ifdef CONFIG_HAS_VPCI
>>   int domain_vpci_init(struct domain *d);
>> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d);
>>   #else
>>   static inline int domain_vpci_init(struct domain *d)
>>   {
>>       return 0;
>>   }
>> +
>> +static inline unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
>> +{
>> +    return 0;
>> +}
>>   #endif
>>     #endif /* __ARCH_ARM_VPCI_H__ */
>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>> index c20eba643d86..4278d66e5eb9 100644
>> --- a/xen/include/asm-arm/pci.h
>> +++ b/xen/include/asm-arm/pci.h
>> @@ -110,6 +110,10 @@ void arch_pci_init_pdev(struct pci_dev *pdev);
>>     int pci_get_new_domain_nr(void);
>>   +int pci_host_iterate_bridges_and_count(struct domain *d,
>> +                                       int (*cb)(struct domain *d,
>> +                                                 struct pci_host_bridge *bridge));
>> +
>>   #else   /*!CONFIG_HAS_PCI*/
>>     struct arch_pci_dev { };
>>
>
> Cheers,
>

Re: [PATCH v7 4/7] xen/arm: account IO handler for emulated PCI host bridge

[Oleksandr Andrushchenko]

Hi, Julien!

On 08.12.21 18:53, Julien Grall wrote:
> Hi Oleksandr,
>
> On 24/11/2021 07:59, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> At the moment, we always allocate an extra 16 slots for IO handlers
>> (see MAX_IO_HANDLER). So while adding an IO trap handler for the emulated
>> PCI host bridge we are not breaking anything, but we have a latent bug
>> as the maximum number of IOs may be exceeded.
>> Fix this by explicitly telling that we have an additional IO handler, so it is
>> accounted.
>>
>> Fixes: d59168dc05a5 ("xen/arm: Enable the existing x86 virtual PCI support for ARM")
>
> In general, it is better to have the fixes at the beginning of a series. So they are relying on less rework and easier to backport (if needed).
>
> In this case, PCI passthrough is still a technical preview so it doesn't matter too much.
I am planning to resend the whole series, so I can move this to the bottom,
but it is indeed doesn't matter at the moment
>
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> >
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> index ccd998d8dba2..8e801f275879 100644
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -126,7 +126,8 @@ unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
>>           return ret < 0 ? 0 : ret;
>>       }
>>   -    return 0;
>> +    /* For a single emulated host bridge's configuration space. */
>
> This comment is lacking some context. I would suggest to reword to something like:
>
> "For the guests, each host bridge requires one region to cover the configuration space. At the moment, we only expose a single host bridge.
> "
>
Ok, will change
> With that (or a similar comment):
>
> Acked-by: Julien Grall <jgrall@amazon.com>
Thank you,
Oleksandr
>
> Cheers,
>

Re: [PATCH v7 5/7] xen/arm: do not map PCI ECAM and MMIO space to Domain-0's p2m

[Oleksandr Andrushchenko]

Hi, Julien!

On 08.12.21 19:20, Julien Grall wrote:
> Hi Oleksandr,
>
> On 24/11/2021 07:59, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> PCI host bridges are special devices in terms of implementing PCI
>> passthrough. According to [1] the current implementation depends on
>> Domain-0 to perform the initialization of the relevant PCI host
>> bridge hardware and perform PCI device enumeration. In order to
>> achieve that one of the required changes is to not map all the memory
>> ranges in map_range_to_domain as we traverse the device tree on startup
>> and perform some additional checks if the range needs to be mapped to
>> Domain-0.
>>
>> The generic PCI host controller device tree binding says [2]:
>> - ranges: As described in IEEE Std 1275-1994, but must provide
>>            at least a definition of non-prefetchable memory. One
>>            or both of prefetchable Memory and IO Space may also
>>            be provided.
>>
>> - reg   : The Configuration Space base address and size, as accessed
>>            from the parent bus.  The base address corresponds to
>>            the first bus in the "bus-range" property.  If no
>>            "bus-range" is specified, this will be bus 0 (the default).
>>
>>  From the above none of the memory ranges from the "ranges" property
>> needs to be mapped to Domain-0 at startup as MMIO mapping is going to
>> be handled dynamically by vPCI as we assign PCI devices, e.g. each
>> device assigned to Domain-0/guest will have its MMIOs mapped/unmapped
>> as needed by Xen.
>>
>> The "reg" property covers not only ECAM space, but may also have other
>> then the configuration memory ranges described, for example [3]:
>> - reg: Should contain rc_dbi, config registers location and length.
>> - reg-names: Must include the following entries:
>>     "rc_dbi": controller configuration registers;
>>     "config": PCIe configuration space registers.
>>
>> This patch makes it possible to not map all the ranges from the
>> "ranges" property and also ECAM from the "reg". All the rest from the
>> "reg" property still needs to be mapped to Domain-0, so the PCI
>> host bridge remains functional in Domain-0. This is done by first
>> skipping the mappings while traversing the device tree as it is done for
>> usual devices and then by calling a dedicated pci_host_bridge_mappings
>> function which only maps MMIOs required by the host bridges leaving the
>> regions, needed for vPCI traps, unmapped.
>>
>> [1] https://lists.xenproject.org/archives/html/xen-devel/2020-07/msg00777.html
>> [2] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>> [3] https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Reviewed-by: Julien Grall <jgrall@amazon.com>
>
> I haven't committed because it is not clear whether this patch depends on earlier patches that are still under review. Can you advise?
I will resend the whole series (leftovers), so no need to commit now

Thank you,
Oleksandr
>
> Cheers,
>

Re: [PATCH v7 4/7] xen/arm: account IO handler for emulated PCI host bridge

[Oleksandr Andrushchenko]

In general, it is better to have the fixes at the beginning of a series. So they are relying on less rework and easier to backport (if needed).
>> In this case, PCI passthrough is still a technical preview so it doesn't matter too much.
> I am planning to resend the whole series, so I can move this to the bottom,
> but it is indeed doesn't matter at the moment
I tried to re-order, but this patch already depends on the previous in the series.
It needs to be fixed in a different way then which will change the patches above,
so I leave this where it is.

Thank you,
Oleksandr