Re: [PATCH 10/11] xen/arm: Do not map PCI ECAM space to Domain-0's p2m

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

>
>>
>>     If so, it would be better if the MMIO region in question was never
>>     mapped into Dom0 at all rather having to unmap it.
>>
> Ok, I'll do that

Sorry for pasting here quite a big patch, but I feel I need clarification if 
this is

the way we want it. Please note I had to modify setup.h.


 From 6eee96bc046efb41ec25f87362b1f6e973a4e6c2 Mon Sep 17 00:00:00 2001
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
Date: Tue, 14 Sep 2021 12:14:43 +0300
Subject: [PATCH] Fixes: a57dc84da5fd ("xen/arm: Do not map PCI ECAM space to
  Domain-0's p2m")

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
---
  xen/arch/arm/domain_build.c        | 37 +++++++++++++--------
  xen/arch/arm/pci/ecam.c            | 11 +++----
  xen/arch/arm/pci/pci-host-common.c | 53 ++++++++++++++++++++++--------
  xen/include/asm-arm/pci.h          | 18 ++++------
  xen/include/asm-arm/setup.h        | 13 ++++++++
  5 files changed, 86 insertions(+), 46 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 76f5b513280c..b4bfda9d5b5a 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -10,7 +10,6 @@
  #include <asm/regs.h>
  #include <xen/errno.h>
  #include <xen/err.h>
-#include <xen/device_tree.h>
  #include <xen/libfdt/libfdt.h>
  #include <xen/guest_access.h>
  #include <xen/iocap.h>
@@ -47,12 +46,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))
@@ -1228,9 +1221,8 @@ static int __init map_dt_irq_to_domain(const struct 
dt_device_node *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;
@@ -1257,8 +1249,10 @@ static int __init map_range_to_domain(const struct 
dt_device_node *dev,
          }
      }

-    if ( need_mapping && (device_get_class(dev) == DEVICE_PCI) )
-        need_mapping = pci_host_bridge_need_p2m_mapping(d, dev, addr, len);
+#ifdef CONFIG_HAS_PCI
+    if ( (device_get_class(dev) == DEVICE_PCI) && !mr_data->map_pci_bridge )
+        need_mapping = false;
+#endif

      if ( need_mapping )
      {
@@ -1293,7 +1287,11 @@ static int __init map_device_children(struct domain *d,
                                        const struct dt_device_node *dev,
                                        p2m_type_t p2mt)
  {
-    struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
+    struct map_range_data mr_data = {
+        .d = d,
+        .p2mt = p2mt,
+        .map_pci_bridge = false
+    };
      int ret;

      if ( dt_device_type_is_equal(dev, "pci") )
@@ -1425,7 +1423,11 @@ static int __init handle_device(struct domain *d, struct 
dt_device_node *dev,
      /* Give permission and map MMIOs */
      for ( i = 0; i < naddr; i++ )
      {
-        struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
+        struct map_range_data mr_data = {
+            .d = d,
+            .p2mt = p2mt,
+            .map_pci_bridge = false
+        };
          res = dt_device_get_address(dev, i, &addr, &size);
          if ( res )
          {
@@ -2594,7 +2596,14 @@ static int __init construct_dom0(struct domain *d)
          return rc;

      if ( acpi_disabled )
+    {
          rc = prepare_dtb_hwdom(d, &kinfo);
+#ifdef CONFIG_HAS_PCI
+        if ( rc < 0 )
+            return rc;
+        rc = pci_host_bridge_mappings(d, p2m_mmio_direct_c);
+#endif
+    }
      else
          rc = prepare_acpi(d, &kinfo);

diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
index d32efb7fcbd0..e08b9c6909b6 100644
--- a/xen/arch/arm/pci/ecam.c
+++ b/xen/arch/arm/pci/ecam.c
@@ -52,17 +52,14 @@ static int pci_ecam_register_mmio_handler(struct domain *d,
      return 0;
  }

-static int pci_ecam_need_p2m_mapping(struct domain *d,
-                                     struct pci_host_bridge *bridge,
-                                     uint64_t addr, uint64_t len)
+static bool pci_ecam_need_p2m_mapping(struct domain *d,
+                                      struct pci_host_bridge *bridge,
+                                      uint64_t addr)
  {
      struct pci_config_window *cfg = bridge->sysdata;

-    if ( !is_hardware_domain(d) )
-        return true;
-
      /*
-     * We do not want ECAM address space to be mapped in domain's p2m,
+     * 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;
diff --git a/xen/arch/arm/pci/pci-host-common.c 
b/xen/arch/arm/pci/pci-host-common.c
index c04be636452d..74077dec8c72 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -25,6 +25,7 @@
  #include <xen/init.h>
  #include <xen/pci.h>
  #include <asm/pci.h>
+#include <asm/setup.h>
  #include <xen/rwlock.h>
  #include <xen/sched.h>
  #include <xen/vmap.h>
@@ -335,25 +336,51 @@ int pci_host_iterate_bridges(struct domain *d,
      return 0;
  }

-bool pci_host_bridge_need_p2m_mapping(struct domain *d,
-                                      const struct dt_device_node *node,
-                                      uint64_t addr, uint64_t len)
+int __init pci_host_bridge_mappings(struct domain *d, p2m_type_t p2mt)
  {
      struct pci_host_bridge *bridge;
+    struct map_range_data mr_data = {
+        .d = d,
+        .p2mt = p2mt,
+        .map_pci_bridge = true
+    };

+    /*
+     * 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" or "assigned-addresses" device tree property.
+     * Neither we want to map any of the MMIO ranges found in the  "ranges"
+     * device tree property.
+     */
      list_for_each_entry( bridge, &pci_host_bridges, node )
      {
-        if ( bridge->dt_node != node )
-            continue;
-
-        if ( !bridge->ops->need_p2m_mapping )
-            return true;
-
-        return bridge->ops->need_p2m_mapping(d, bridge, addr, len);
+        const struct dt_device_node *dev = bridge->dt_node;
+        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 %u for %s\n",
+                       i, dt_node_full_name(dev));
+                return err;
+            }
+
+            if ( bridge->ops->need_p2m_mapping(d, bridge, addr) )
+            {
+                err = map_range_to_domain(dev, addr, size, &mr_data);
+                if ( err )
+                    return err;
+            }
+        }
      }
-    printk(XENLOG_ERR "Unable to find PCI bridge for %s segment %d, addr 
%lx\n",
-           node->full_name, bridge->segment, addr);
-    return true;
+
+    return 0;
  }

  /*
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 9c28a4bdc4b7..97fbaac01370 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -21,6 +21,8 @@

  #ifdef CONFIG_HAS_PCI

+#include <asm/p2m.h>
+
  #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
  #define PRI_pci "%04x:%02x:%02x.%u"

@@ -82,8 +84,9 @@ struct pci_ops {
      int (*register_mmio_handler)(struct domain *d,
                                   struct pci_host_bridge *bridge,
                                   const struct mmio_handler_ops *ops);
-    int (*need_p2m_mapping)(struct domain *d, struct pci_host_bridge *bridge,
-                            uint64_t addr, uint64_t len);
+    bool (*need_p2m_mapping)(struct domain *d,
+                             struct pci_host_bridge *bridge,
+                             uint64_t addr);
  };

  /*
@@ -117,19 +120,10 @@ struct dt_device_node *pci_find_host_bridge_node(struct 
device *dev);
  int pci_host_iterate_bridges(struct domain *d,
                               int (*clb)(struct domain *d,
                                          struct pci_host_bridge *bridge));
-bool pci_host_bridge_need_p2m_mapping(struct domain *d,
-                                      const struct dt_device_node *node,
-                                      uint64_t addr, uint64_t len);
+int pci_host_bridge_mappings(struct domain *d, p2m_type_t p2mt);
  #else   /*!CONFIG_HAS_PCI*/

  struct arch_pci_dev { };

-static inline bool
-pci_host_bridge_need_p2m_mapping(struct domain *d,
-                                 const struct dt_device_node *node,
-                                 uint64_t addr, uint64_t len)
-{
-    return true;
-}
  #endif  /*!CONFIG_HAS_PCI*/
  #endif /* __ARM_PCI_H__ */
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index c4b6af602995..a1c31c0bb024 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
@@ -76,6 +78,14 @@ struct bootinfo {
  #endif
  };

+struct map_range_data
+{
+    struct domain *d;
+    p2m_type_t p2mt;
+    /* Set if mappings for PCI host bridges must not be skipped. */
+    bool map_pci_bridge;
+};
+
  extern struct bootinfo bootinfo;

  extern domid_t max_init_domid;
@@ -123,6 +133,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


With the patch above I have the following log in Domain-0:

generic-armv8-xt-dom0 login: root
root@generic-armv8-xt-dom0:~# (XEN) *** Serial input to Xen (type 'CTRL-a' 
three times to switch input)
(XEN) ==== PCI devices ====
(XEN) ==== segment 0000 ====
(XEN) 0000:03:00.0 - d0 - node -1
(XEN) 0000:02:02.0 - d0 - node -1
(XEN) 0000:02:01.0 - d0 - node -1
(XEN) 0000:02:00.0 - d0 - node -1
(XEN) 0000:01:00.0 - d0 - node -1
(XEN) 0000:00:00.0 - d0 - node -1
(XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input)

root@generic-armv8-xt-dom0:~# modprobe e1000e
[   46.104729] e1000e: Intel(R) PRO/1000 Network Driver
[   46.105479] e1000e: Copyright(c) 1999 - 2015 Intel Corporation.
[   46.107297] e1000e 0000:03:00.0: enabling device (0000 -> 0002)
(XEN) map [e0000, e001f] -> 0xe0000 for d0
(XEN) map [e0020, e003f] -> 0xe0020 for d0
[   46.178513] e1000e 0000:03:00.0: Interrupt Throttling Rate (ints/sec) set to 
dynamic conservative mode
[   46.189668] pci_msi_setup_msi_irqs
[   46.191016] nwl_compose_msi_msg msg at fe440000
(XEN) traps.c:2014:d0v0 HSR=0x00000093810047 pc=0xffff8000104b4b00 
gva=0xffff800010fa5000 gpa=0x000000e0040000
[   46.200455] Unhandled fault at 0xffff800010fa5000

[snip]

[   46.233079] Call trace:
[   46.233559]  __pci_write_msi_msg+0x70/0x180
[   46.234149]  pci_msi_domain_write_msg+0x28/0x30
[   46.234869]  msi_domain_activate+0x5c/0x88

 From the above you can see that BARs are mapped for Domain-0 now

only when an assigned PCI device gets enabled in Domain-0.

Another thing to note is that we crash on MSI-X access as BARs are mapped

to the domain while enabling memory decoding in the COMMAND register,

but MSI-X are handled differently, e.g. we have MSI-X holes in the mappings.

This is because before this change the whole PCI aperture was mapped into

Domain-0 and it is not. Thus, MSI-X holes are left unmapped now and there

was no need to do so, e.g. they were always mapped into Domain-0 and

emulated for guests.

Please note that one cannot use xl pci-attach in this case to attach the PCI 
device

in question to Domain-0 as (please see the log) that device is already attached.

Neither it can be detached and re-attached. So, without mapping MSI-X holes for

Domain-0 the device becomes unusable in Domain-0. At the same time the device

can be successfully passed to DomU.


Julien, Stefano! Please let me know how can we proceed with this.

Thank you,

Oleksandr