diff mbox series

[04/10,WORKAROUND] xen/arm: Update hwdom's p2m to trap ECAM space

Message ID 20201109125031.26409-5-andr2000@gmail.com (mailing list archive)
State New, archived
Headers show
Series ARM PCI passthrough configuration and vPCI | expand

Commit Message

Oleksandr Andrushchenko Nov. 9, 2020, 12:50 p.m. UTC 
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Host bridge controller's ECAM space is mapped into Domain-0's p2m,
thus it is not possible to trap the same for vPCI via MMIO handlers.
For this to work we need to unmap those mappings in p2m.

TODO (Julien): It would be best if we avoid the map/unmap operation.
So, maybe we want to introduce another way to avoid the mapping.
Maybe by changing the type of the controller to "PCI_HOSTCONTROLLER"
and checking if this is a PCI hostcontroller avoid the mapping.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 xen/arch/arm/domain_build.c         | 10 +++++++++-
 xen/arch/arm/pci/pci-host-common.c  | 15 +++++++++++++++
 xen/arch/arm/pci/pci-host-generic.c | 28 ++++++++++++++++++++++++++++
 xen/include/asm-arm/pci.h           |  2 ++
 4 files changed, 54 insertions(+), 1 deletion(-)

Comments

Roger Pau Monné Nov. 11, 2020, 2:44 p.m. UTC | #1 
On Mon, Nov 09, 2020 at 02:50:25PM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Host bridge controller's ECAM space is mapped into Domain-0's p2m,
> thus it is not possible to trap the same for vPCI via MMIO handlers.
> For this to work we need to unmap those mappings in p2m.
> 
> TODO (Julien): It would be best if we avoid the map/unmap operation.
> So, maybe we want to introduce another way to avoid the mapping.
> Maybe by changing the type of the controller to "PCI_HOSTCONTROLLER"
> and checking if this is a PCI hostcontroller avoid the mapping.

I know very little about Arm to be able to provide meaningful comments
here. I agree that creating the maps just to remove them afterwards is
not the right approach, we should instead avoid those mappings from
being created in the first place.

Roger.
Oleksandr Andrushchenko Nov. 12, 2020, 12:54 p.m. UTC | #2 
On 11/11/20 4:44 PM, Roger Pau Monné wrote:
> On Mon, Nov 09, 2020 at 02:50:25PM +0200, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> Host bridge controller's ECAM space is mapped into Domain-0's p2m,
>> thus it is not possible to trap the same for vPCI via MMIO handlers.
>> For this to work we need to unmap those mappings in p2m.
>>
>> TODO (Julien): It would be best if we avoid the map/unmap operation.
>> So, maybe we want to introduce another way to avoid the mapping.
>> Maybe by changing the type of the controller to "PCI_HOSTCONTROLLER"
>> and checking if this is a PCI hostcontroller avoid the mapping.
> I know very little about Arm to be able to provide meaningful comments
> here. I agree that creating the maps just to remove them afterwards is
> not the right approach, we should instead avoid those mappings from
> being created in the first place.
Agreed, we'll need to find an acceptable way of doing so
> Roger.

Thank you,

Oleksandr
diff mbox series

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1f83f9048146..3f696d2a6672 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2566,7 +2566,15 @@  int __init construct_dom0(struct domain *d)
     if ( rc < 0 )
         return rc;
 
-    return construct_domain(d, &kinfo);
+    rc = construct_domain(d, &kinfo);
+    if ( rc < 0 )
+        return rc;
+
+#ifdef CONFIG_HAS_PCI
+    if ( has_vpci(d) )
+        rc = pci_host_bridge_update_mappings(d);
+#endif
+    return rc;
 }
 
 /*
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index b81184d34980..b6c4d7b636b1 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -235,6 +235,21 @@  int pci_host_iterate_bridges(struct domain *d,
     }
     return 0;
 }
+
+static int pci_host_bridge_update_mapping(struct domain *d,
+                                          struct pci_host_bridge *bridge)
+{
+    if ( !bridge->ops->update_mappings )
+        return 0;
+
+    return bridge->ops->update_mappings(d, bridge);
+}
+
+int pci_host_bridge_update_mappings(struct domain *d)
+{
+    return pci_host_iterate_bridges(d, pci_host_bridge_update_mapping);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
index 469df3da0116..772c53c881bc 100644
--- a/xen/arch/arm/pci/pci-host-generic.c
+++ b/xen/arch/arm/pci/pci-host-generic.c
@@ -21,6 +21,8 @@ 
 #include <asm/device.h>
 #include <asm/io.h>
 #include <xen/pci.h>
+#include <xen/sched.h>
+#include <asm/p2m.h>
 #include <asm/pci.h>
 
 /*
@@ -85,6 +87,31 @@  int pci_ecam_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
     return 0;
 }
 
+/*
+ * TODO: This is called late on domain creation to mangle p2m if needed:
+ * for ECAM host controller for mmio region traps to work for Domain-0
+ * we need to unmap those mappings in p2m.
+ * This is WIP:
+ * julieng: I think it would be best if we avoid the map/unmap operation.
+ * So maybe we want to introduce another way to avoid the mapping.
+ * Maybe by changing the type of the controller to "PCI_HOSTCONTROLLER"
+ * and check if this is a PCI hostcontroller avoid the mapping.
+ */
+static int pci_ecam_update_mappings(struct domain *d,
+                                    struct pci_host_bridge *bridge)
+{
+    struct pci_config_window *cfg = bridge->sysdata;
+    int ret;
+
+    /* Only for control domain which owns this PCI host bridge. */
+    if ( !is_control_domain(d) )
+        return 0;
+
+    ret = unmap_regions_p2mt(d, gaddr_to_gfn(cfg->phys_addr),
+                             cfg->size >> PAGE_SHIFT, INVALID_MFN);
+    return ret;
+}
+
 static int pci_ecam_register_mmio_handler(struct domain *d,
                                           struct pci_host_bridge *bridge,
                                           const struct mmio_handler_ops *ops)
@@ -101,6 +128,7 @@  struct pci_ecam_ops pci_generic_ecam_ops = {
     .pci_ops    = {
         .read                  = pci_ecam_config_read,
         .write                 = pci_ecam_config_write,
+        .update_mappings       = pci_ecam_update_mappings,
         .register_mmio_handler = pci_ecam_register_mmio_handler,
     }
 };
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index e3a02429b8d4..d94e8a6628de 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -65,6 +65,7 @@  struct pci_ops {
                     uint32_t sbdf, int where, int size, u32 *val);
     int (*write)(struct pci_host_bridge *bridge,
                     uint32_t sbdf, int where, int size, u32 val);
+    int (*update_mappings)(struct domain *d, struct pci_host_bridge *bridge);
     int (*register_mmio_handler)(struct domain *d,
                                  struct pci_host_bridge *bridge,
                                  const struct mmio_handler_ops *ops);
@@ -108,6 +109,7 @@  bool dt_pci_parse_bus_range(struct dt_device_node *dev,
 int pci_host_iterate_bridges(struct domain *d,
                              int (*clb)(struct domain *d,
                                         struct pci_host_bridge *bridge));
+int pci_host_bridge_update_mappings(struct domain *d);
 #else   /*!CONFIG_ARM_PCI*/
 struct arch_pci_dev { };
 static inline void  pci_init(void) { }