[PATCH v5 00/10] PCI devices passthrough on Arm, part 2

[PATCH v5 00/10] 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].

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 (9):
  xen/arm: Add new device type for PCI
  xen/arm: Introduce pci_find_host_bridge_node helper
  xen/device-tree: Make dt_find_node_by_phandle global
  xen/arm: Mark device as PCI while creating one
  libxl: Allow removing PCI devices for all types of domains
  libxl: Only map legacy PCI IRQs if they are supported
  xen/arm: Setup MMIO range trap handlers for hardware domain
  xen/arm: Do not map PCI ECAM and MMIO space to Domain-0's p2m
  xen/arm: Process pending vPCI map/unmap operations

Oleksandr Tyshchenko (1):
  xen/domain: Call pci_release_devices() when releasing domain resources

 tools/libs/light/Makefile          |  4 ++
 tools/libs/light/libxl_pci.c       | 15 ++++-
 xen/arch/arm/domain.c              | 11 +++-
 xen/arch/arm/domain_build.c        | 57 ++++++++++--------
 xen/arch/arm/pci/ecam.c            | 14 +++++
 xen/arch/arm/pci/pci-host-common.c | 93 ++++++++++++++++++++++++++++++
 xen/arch/arm/pci/pci-host-zynqmp.c |  1 +
 xen/arch/arm/pci/pci.c             | 12 ++++
 xen/arch/arm/traps.c               | 13 +++++
 xen/arch/arm/vpci.c                | 34 +++++++++++
 xen/arch/arm/vpci.h                |  6 ++
 xen/arch/x86/hvm/hvm.c             |  6 ++
 xen/common/device_tree.c           |  2 +-
 xen/common/ioreq.c                 |  9 ---
 xen/drivers/passthrough/pci.c      |  2 +
 xen/include/asm-arm/device.h       |  4 +-
 xen/include/asm-arm/pci.h          | 30 ++++++++++
 xen/include/asm-arm/setup.h        | 13 +++++
 xen/include/asm-x86/pci.h          |  2 +
 xen/include/xen/device_tree.h      |  2 +
 20 files changed, 292 insertions(+), 38 deletions(-)

-- 
2.25.1

[PATCH v5 01/10] xen/arm: Add new device type for PCI

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Add new device type (DEV_PCI) to distinguish PCI devices from platform
DT devices, so some drivers, like IOMMU, can handle PCI devices
differently.

Also add a helper which is when given a struct device returns the
corresponding struct pci_dev which this device is a part of.

Because of the header cross-dependencies, e.g. we need both
struct pci_dev and struct arch_pci_dev at the same time, this cannot be
done with an inline.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
---
Since v2:
 - !! dropped Stefano's r-b because of the changes
 - simplified dev_to_pci to use a single
   container_of(dev, struct pci_dev, arch.dev) (Jan)
Since v1:
 - Folded new device type (DEV_PCI) into this patch.
---
 xen/arch/arm/pci/pci.c       | 7 +++++++
 xen/include/asm-arm/device.h | 4 ++--
 xen/include/asm-arm/pci.h    | 7 +++++++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
index 73540045d187..138da19284ab 100644
--- a/xen/arch/arm/pci/pci.c
+++ b/xen/arch/arm/pci/pci.c
@@ -27,6 +27,13 @@ int arch_pci_clean_pirqs(struct domain *d)
     return 0;
 }
 
+struct pci_dev *dev_to_pci(struct device *dev)
+{
+    ASSERT(dev->type == DEV_PCI);
+
+    return container_of(dev, struct pci_dev, arch.dev);
+}
+
 static int __init dt_pci_init(void)
 {
     struct dt_device_node *np;
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index ebe84ea853cd..7bf040560363 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -4,6 +4,7 @@
 enum device_type
 {
     DEV_DT,
+    DEV_PCI,
 };
 
 struct dev_archdata {
@@ -25,8 +26,7 @@ typedef struct device device_t;
 
 #include <xen/device_tree.h>
 
-/* TODO: Correctly implement dev_is_pci when PCI is supported on ARM */
-#define dev_is_pci(dev) ((void)(dev), 0)
+#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
 #define dev_is_dt(dev)  ((dev)->type == DEV_DT)
 
 enum device_class
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index f99356af1eba..7a91ebbdaf0c 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -26,6 +26,13 @@ struct arch_pci_dev {
     struct device dev;
 };
 
+/*
+ * Because of the header cross-dependencies, e.g. we need both
+ * struct pci_dev and struct arch_pci_dev at the same time, this cannot be
+ * done with an inline here. Macro can be implemented, but looks scary.
+ */
+struct pci_dev *dev_to_pci(struct device *dev);
+
 /* Arch-specific MSI data for vPCI. */
 struct vpci_arch_msi {
 };
-- 
2.25.1

[PATCH v5 02/10] xen/arm: Introduce pci_find_host_bridge_node helper

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Get host bridge node given a PCI device attached to it.

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

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
---
Since v2:
 - !! dropped Stefano's r-b because of the changes
 - s/PRI_pci/%pp after rebase onto Arm series
---
 xen/arch/arm/pci/pci-host-common.c | 16 ++++++++++++++++
 xen/include/asm-arm/pci.h          |  1 +
 2 files changed, 17 insertions(+)

diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 593beeb48ce4..592c01aae5bb 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -276,6 +276,22 @@ int pci_get_host_bridge_segment(const struct dt_device_node *node,
     return -EINVAL;
 }
 
+/*
+ * Get host bridge node given a device attached to it.
+ */
+struct dt_device_node *pci_find_host_bridge_node(struct device *dev)
+{
+    struct pci_host_bridge *bridge;
+    struct pci_dev *pdev = dev_to_pci(dev);
+
+    bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
+    if ( unlikely(!bridge) )
+    {
+        printk(XENLOG_ERR "Unable to find PCI bridge for %pp\n", &pdev->sbdf);
+        return NULL;
+    }
+    return bridge->dt_node;
+}
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 7a91ebbdaf0c..9aa8eca53175 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -99,6 +99,7 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
 struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus);
 int pci_get_host_bridge_segment(const struct dt_device_node *node,
                                 uint16_t *segment);
+struct dt_device_node *pci_find_host_bridge_node(struct device *dev);
 
 static always_inline bool is_pci_passthrough_enabled(void)
 {
-- 
2.25.1

[PATCH v5 03/10] xen/device-tree: Make dt_find_node_by_phandle global

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Make dt_find_node_by_phandle globally visible, so it can be re-used by
other frameworks.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/common/device_tree.c      | 2 +-
 xen/include/xen/device_tree.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index ea93da1725f6..4aae281e89bf 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -1047,7 +1047,7 @@ int dt_for_each_range(const struct dt_device_node *dev,
  *
  * Returns a node pointer.
  */
-static struct dt_device_node *dt_find_node_by_phandle(dt_phandle handle)
+struct dt_device_node *dt_find_node_by_phandle(dt_phandle handle)
 {
     struct dt_device_node *np;
 
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 2297c59ce66d..fd6cd00b433a 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -849,6 +849,8 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
  */
 int dt_get_pci_domain_nr(struct dt_device_node *node);
 
+struct dt_device_node *dt_find_node_by_phandle(dt_phandle handle);
+
 #ifdef CONFIG_DEVICE_TREE_DEBUG
 #define dt_dprintk(fmt, args...)  \
     printk(XENLOG_DEBUG fmt, ## args)
-- 
2.25.1

[PATCH v5 04/10] xen/arm: Mark device as PCI while creating one

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

While adding a PCI device mark it as such, so other frameworks
can distinguish it from DT devices.
For that introduce an architecture defined helper which may perform
additional initialization of the newly created PCI device.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
[applicable parts]
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Paul Durrant <paul@xen.org>

Since v3:
- fixed Arm build without CONFIG_HAS_PCI
Since v2:
 - !! dropped Stefano's r-b because of the changes
 - introduced arch_pci_init_pdev (Jan)
Since v1:
 - moved the assignment from iommu_add_device to alloc_pdev
---
 xen/arch/arm/pci/pci.c        | 5 +++++
 xen/drivers/passthrough/pci.c | 2 ++
 xen/include/asm-arm/pci.h     | 7 +++++++
 xen/include/asm-x86/pci.h     | 2 ++
 4 files changed, 16 insertions(+)

diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
index 138da19284ab..082c14e127a8 100644
--- a/xen/arch/arm/pci/pci.c
+++ b/xen/arch/arm/pci/pci.c
@@ -34,6 +34,11 @@ struct pci_dev *dev_to_pci(struct device *dev)
     return container_of(dev, struct pci_dev, arch.dev);
 }
 
+void arch_pci_init_pdev(struct pci_dev *pdev)
+{
+    pci_to_dev(pdev)->type = DEV_PCI;
+}
+
 static int __init dt_pci_init(void)
 {
     struct dt_device_node *np;
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index b36d5a481162..9e6246afeef5 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -329,6 +329,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
     *((u8*) &pdev->devfn) = devfn;
     pdev->domain = NULL;
 
+    arch_pci_init_pdev(pdev);
+
     rc = pdev_msi_init(pdev);
     if ( rc )
     {
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 9aa8eca53175..98cb22d39ef1 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -105,6 +105,9 @@ static always_inline bool is_pci_passthrough_enabled(void)
 {
     return pci_passthrough_enabled;
 }
+
+void arch_pci_init_pdev(struct pci_dev *pdev);
+
 #else   /*!CONFIG_HAS_PCI*/
 
 struct arch_pci_dev { };
@@ -121,5 +124,9 @@ static inline int pci_get_host_bridge_segment(const struct dt_device_node *node,
     return -EINVAL;
 }
 
+struct pci_dev;
+
+static inline void arch_pci_init_pdev(struct pci_dev *pdev) {}
+
 #endif  /*!CONFIG_HAS_PCI*/
 #endif /* __ARM_PCI_H__ */
diff --git a/xen/include/asm-x86/pci.h b/xen/include/asm-x86/pci.h
index c4a4fdcbc239..443f25347d08 100644
--- a/xen/include/asm-x86/pci.h
+++ b/xen/include/asm-x86/pci.h
@@ -36,4 +36,6 @@ static always_inline bool is_pci_passthrough_enabled(void)
     return true;
 }
 
+static inline void arch_pci_init_pdev(struct pci_dev *pdev) {}
+
 #endif /* __X86_PCI_H__ */
-- 
2.25.1

[PATCH v5 05/10] xen/domain: Call pci_release_devices() when releasing domain resources

[Oleksandr Andrushchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

This is the very same that we already do for DT devices. Moreover, x86
already calls pci_release_devices().

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
---
Since v1:
 - re-wording in the commit message
---
 xen/arch/arm/domain.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index fbb52f78f1a6..79012bf77757 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -985,7 +985,8 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list)
  * function which may return -ERESTART.
  */
 enum {
-    PROG_tee = 1,
+    PROG_pci = 1,
+    PROG_tee,
     PROG_xen,
     PROG_page,
     PROG_mapping,
@@ -1022,6 +1023,12 @@ int domain_relinquish_resources(struct domain *d)
 #ifdef CONFIG_IOREQ_SERVER
         ioreq_server_destroy_all(d);
 #endif
+#ifdef CONFIG_HAS_PCI
+    PROGRESS(pci):
+        ret = pci_release_devices(d);
+        if ( ret )
+            return ret;
+#endif
 
     PROGRESS(tee):
         ret = tee_relinquish_resources(d);
-- 
2.25.1

[PATCH v5 06/10] libxl: Allow removing PCI devices for all types of domains

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

The PCI device remove path may now be used by PVH on ARM, so the
assert is no longer valid.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Juergen Gross <jgross@suse.com>
---
 tools/libs/light/libxl_pci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 1a1c2630803b..59f3686fc85e 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1947,8 +1947,6 @@ static void do_pci_remove(libxl__egc *egc, pci_remove_state *prs)
             goto out_fail;
         }
     } else {
-        assert(type == LIBXL_DOMAIN_TYPE_PV);
-
         char *sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/resource", pci->domain,
                                      pci->bus, pci->dev, pci->func);
         FILE *f = fopen(sysfs_path, "r");
-- 
2.25.1

[PATCH v5 07/10] libxl: Only map legacy PCI IRQs if they are supported

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Arm's PCI passthrough implementation doesn't support legacy interrupts,
but MSI/MSI-X. This can be the case for other platforms too.
For that reason introduce a new CONFIG_PCI_SUPP_LEGACY_IRQ and add
it to the CFLAGS and compile the relevant code in the toolstack only if
applicable.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Juergen Gross <jgross@suse.com>

Since v1:
 - Minimized #idefery by introducing pci_supp_legacy_irq function
   for relevant checks
---
 tools/libs/light/Makefile    |  4 ++++
 tools/libs/light/libxl_pci.c | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
index 7d8c51d49242..bd3f6be2a183 100644
--- a/tools/libs/light/Makefile
+++ b/tools/libs/light/Makefile
@@ -46,6 +46,10 @@ CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
 	-Wno-declaration-after-statement -Wformat-nonliteral
 CFLAGS += -I.
 
+ifeq ($(CONFIG_X86),y)
+CFLAGS += -DCONFIG_PCI_SUPP_LEGACY_IRQ
+endif
+
 SRCS-$(CONFIG_X86) += libxl_cpuid.c
 SRCS-$(CONFIG_X86) += libxl_x86.c
 SRCS-$(CONFIG_X86) += libxl_psr.c
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 59f3686fc85e..4c2d7aeefbb2 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1364,6 +1364,15 @@ static void pci_add_timeout(libxl__egc *egc, libxl__ev_time *ev,
     pci_add_dm_done(egc, pas, rc);
 }
 
+static bool pci_supp_legacy_irq(void)
+{
+#ifdef CONFIG_PCI_SUPP_LEGACY_IRQ
+    return true;
+#else
+    return false;
+#endif
+}
+
 static void pci_add_dm_done(libxl__egc *egc,
                             pci_add_state *pas,
                             int rc)
@@ -1434,6 +1443,8 @@ static void pci_add_dm_done(libxl__egc *egc,
         }
     }
     fclose(f);
+    if (!pci_supp_legacy_irq())
+        goto out_no_irq;
     sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                                 pci->bus, pci->dev, pci->func);
     f = fopen(sysfs_path, "r");
@@ -1983,6 +1994,8 @@ static void do_pci_remove(libxl__egc *egc, pci_remove_state *prs)
         }
         fclose(f);
 skip1:
+        if (!pci_supp_legacy_irq())
+            goto skip_irq;
         sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                                pci->bus, pci->dev, pci->func);
         f = fopen(sysfs_path, "r");
-- 
2.25.1

[PATCH v5 08/10] 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
---
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 | 28 ++++++++++++++++++++++++
 xen/arch/arm/vpci.c                | 34 ++++++++++++++++++++++++++++++
 xen/arch/arm/vpci.h                |  6 ++++++
 xen/include/asm-arm/pci.h          |  5 +++++
 5 files changed, 75 insertions(+)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 79012bf77757..fa6fcc5e467c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -733,6 +733,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 592c01aae5bb..1eb4daa87365 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -292,6 +292,34 @@ struct dt_device_node *pci_find_host_bridge_node(struct device *dev)
     }
     return bridge->dt_node;
 }
+
+int pci_host_iterate_bridges(struct domain *d,
+                             int (*cb)(struct domain *d,
+                                       struct pci_host_bridge *bridge))
+{
+    struct pci_host_bridge *bridge;
+    int err;
+
+    list_for_each_entry( bridge, &pci_host_bridges, node )
+    {
+        err = cb(d, bridge);
+        if ( err )
+            return err;
+    }
+    return 0;
+}
+
+int pci_host_get_num_bridges(void)
+{
+    struct pci_host_bridge *bridge;
+    int count = 0;
+
+    list_for_each_entry( bridge, &pci_host_bridges, node )
+        count++;
+
+    return count;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
index 76c12b92814f..6e179cd3010b 100644
--- a/xen/arch/arm/vpci.c
+++ b/xen/arch/arm/vpci.c
@@ -80,17 +80,51 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
     .write = vpci_mmio_write,
 };
 
+static int vpci_setup_mmio_handler(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, NULL);
+    return 0;
+}
+
 int domain_vpci_init(struct domain *d)
 {
     if ( !has_vpci(d) )
         return 0;
 
+    if ( is_hardware_domain(d) )
+        return pci_host_iterate_bridges(d, vpci_setup_mmio_handler);
+
+    /* Guest domains use what is programmed in their device tree. */
     register_mmio_handler(d, &vpci_mmio_handler,
                           GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
 
     return 0;
 }
 
+int domain_vpci_get_num_mmio_handlers(struct domain *d)
+{
+    int count;
+
+    if ( is_hardware_domain(d) )
+        /* For each PCI host bridge's configuration space. */
+        count = pci_host_get_num_bridges();
+    else
+        /*
+         * VPCI_MSIX_MEM_NUM handlers for MSI-X tables per each PCI device
+         * being passed through. Maximum number of supported devices
+         * is 32 as virtual bus topology emulates the devices as embedded
+         * endpoints.
+         * +1 for a single emulated host bridge's configuration space.
+         */
+        count = VPCI_MSIX_MEM_NUM * 32 + 1;
+
+    return count;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h
index d8a7b0e3e802..27a2b069abd2 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);
+int domain_vpci_get_num_mmio_handlers(struct domain *d);
 #else
 static inline int domain_vpci_init(struct domain *d)
 {
     return 0;
 }
+
+static inline 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 98cb22d39ef1..a71b0eea8cb9 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -108,6 +108,11 @@ static always_inline bool is_pci_passthrough_enabled(void)
 
 void arch_pci_init_pdev(struct pci_dev *pdev);
 
+int pci_host_iterate_bridges(struct domain *d,
+                             int (*clb)(struct domain *d,
+                                        struct pci_host_bridge *bridge));
+int pci_host_get_num_bridges(void);
+
 #else   /*!CONFIG_HAS_PCI*/
 
 struct arch_pci_dev { };
-- 
2.25.1

[PATCH v5 09/10] 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.

[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 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        | 57 ++++++++++++++++++------------
 xen/arch/arm/pci/ecam.c            | 14 ++++++++
 xen/arch/arm/pci/pci-host-common.c | 49 +++++++++++++++++++++++++
 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, 121 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b51176b31bef..0d673b06a2f3 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>
@@ -51,12 +50,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))
@@ -1663,10 +1656,11 @@ 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);
+    bool need_mapping = !mr_data->skip_mapping;
 
     if ( irq < NR_LOCAL_IRQS )
     {
@@ -1690,13 +1684,12 @@ 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;
-    bool need_mapping = !dt_device_for_passthrough(dev);
+    bool need_mapping = !mr_data->skip_mapping;
     int res;
 
     /*
@@ -1748,23 +1741,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;
     }
@@ -1845,6 +1836,20 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
     int res;
     u64 addr, size;
     bool need_mapping = !dt_device_for_passthrough(dev);
+    /*
+     * For PCI passthrough we only need to remap to Dom0 the interrupts
+     * and memory ranges from "reg" property which cover controller's
+     * configuration registers and such. PCIe configuration space registers
+     * of the PCIe Root Complex and PCIe aperture should not be mapped
+     * automatically to Dom0.
+     */
+    struct map_range_data mr_data = {
+        .d = d,
+        .p2mt = p2mt,
+        .skip_mapping = !need_mapping ||
+                        (is_pci_passthrough_enabled() &&
+                         (device_get_class(dev) == DEVICE_PCI))
+    };
 
     naddr = dt_number_of_address(dev);
 
@@ -1884,7 +1889,6 @@ 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 };
         res = dt_device_get_address(dev, i, &addr, &size);
         if ( res )
         {
@@ -1898,7 +1902,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;
 
@@ -3056,7 +3060,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, 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 602d00799c8d..b81620074a91 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 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 1eb4daa87365..085f08e23e0c 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -18,6 +18,7 @@
 
 #include <xen/init.h>
 #include <xen/pci.h>
+#include <asm/setup.h>
 #include <xen/rwlock.h>
 #include <xen/sched.h>
 #include <xen/vmap.h>
@@ -320,6 +321,54 @@ int pci_host_get_num_bridges(void)
     return count;
 }
 
+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,
+        .skip_mapping = false
+    };
+
+    /*
+     * 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, 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.
+     */
+    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 61a9807d3d58..6ad2b31e810d 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 a71b0eea8cb9..b5b85ccd0937 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);
 int pci_get_host_bridge_segment(const struct dt_device_node *node,
                                 uint16_t *segment);
@@ -113,6 +121,8 @@ int pci_host_iterate_bridges(struct domain *d,
                                         struct pci_host_bridge *bridge));
 int pci_host_get_num_bridges(void);
 
+int pci_host_bridge_mappings(struct domain *d, p2m_type_t p2mt);
+
 #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 v5 10/10] 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 changes]
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Paul Durrant <paul@xen.org>

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..b246f51086e3 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>
@@ -2304,6 +2305,18 @@ static bool check_for_vcpu_work(void)
     }
 #endif
 
+    if ( has_vpci(v->domain) )
+    {
+        bool pending;
+
+        local_irq_enable();
+        pending = vpci_process_pending(v);
+        local_irq_disable();
+
+        if ( pending )
+            return true;
+    }
+
     if ( likely(!v->arch.need_flush_to_ram) )
         return false;
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index aa418a3ca1b7..c491242e4b8b 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 v5 00/10] PCI devices passthrough on Arm, part 2

[Stefano Stabellini]

Hi Oleksandr,

I committed patches #1-#5


On Fri, 8 Oct 2021, Oleksandr Andrushchenko wrote:
> 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].
> 
> 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 (9):
>   xen/arm: Add new device type for PCI
>   xen/arm: Introduce pci_find_host_bridge_node helper
>   xen/device-tree: Make dt_find_node_by_phandle global
>   xen/arm: Mark device as PCI while creating one
>   libxl: Allow removing PCI devices for all types of domains
>   libxl: Only map legacy PCI IRQs if they are supported
>   xen/arm: Setup MMIO range trap handlers for hardware domain
>   xen/arm: Do not map PCI ECAM and MMIO space to Domain-0's p2m
>   xen/arm: Process pending vPCI map/unmap operations
> 
> Oleksandr Tyshchenko (1):
>   xen/domain: Call pci_release_devices() when releasing domain resources
> 
>  tools/libs/light/Makefile          |  4 ++
>  tools/libs/light/libxl_pci.c       | 15 ++++-
>  xen/arch/arm/domain.c              | 11 +++-
>  xen/arch/arm/domain_build.c        | 57 ++++++++++--------
>  xen/arch/arm/pci/ecam.c            | 14 +++++
>  xen/arch/arm/pci/pci-host-common.c | 93 ++++++++++++++++++++++++++++++
>  xen/arch/arm/pci/pci-host-zynqmp.c |  1 +
>  xen/arch/arm/pci/pci.c             | 12 ++++
>  xen/arch/arm/traps.c               | 13 +++++
>  xen/arch/arm/vpci.c                | 34 +++++++++++
>  xen/arch/arm/vpci.h                |  6 ++
>  xen/arch/x86/hvm/hvm.c             |  6 ++
>  xen/common/device_tree.c           |  2 +-
>  xen/common/ioreq.c                 |  9 ---
>  xen/drivers/passthrough/pci.c      |  2 +
>  xen/include/asm-arm/device.h       |  4 +-
>  xen/include/asm-arm/pci.h          | 30 ++++++++++
>  xen/include/asm-arm/setup.h        | 13 +++++
>  xen/include/asm-x86/pci.h          |  2 +
>  xen/include/xen/device_tree.h      |  2 +
>  20 files changed, 292 insertions(+), 38 deletions(-)
> 
> -- 
> 2.25.1
> 

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

[Stefano Stabellini]

On Fri, 8 Oct 2021, 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.
> 
> [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: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 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        | 57 ++++++++++++++++++------------
>  xen/arch/arm/pci/ecam.c            | 14 ++++++++
>  xen/arch/arm/pci/pci-host-common.c | 49 +++++++++++++++++++++++++
>  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, 121 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b51176b31bef..0d673b06a2f3 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>
> @@ -51,12 +50,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))
> @@ -1663,10 +1656,11 @@ 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);
> +    bool need_mapping = !mr_data->skip_mapping;
>  
>      if ( irq < NR_LOCAL_IRQS )
>      {
> @@ -1690,13 +1684,12 @@ 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;
> -    bool need_mapping = !dt_device_for_passthrough(dev);
> +    bool need_mapping = !mr_data->skip_mapping;
>      int res;
>  
>      /*
> @@ -1748,23 +1741,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;
>      }
> @@ -1845,6 +1836,20 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
>      int res;
>      u64 addr, size;
>      bool need_mapping = !dt_device_for_passthrough(dev);
> +    /*
> +     * For PCI passthrough we only need to remap to Dom0 the interrupts
> +     * and memory ranges from "reg" property which cover controller's
> +     * configuration registers and such. PCIe configuration space registers
> +     * of the PCIe Root Complex and PCIe aperture should not be mapped
> +     * automatically to Dom0.
> +     */
> +    struct map_range_data mr_data = {
> +        .d = d,
> +        .p2mt = p2mt,
> +        .skip_mapping = !need_mapping ||
> +                        (is_pci_passthrough_enabled() &&
> +                         (device_get_class(dev) == DEVICE_PCI))
> +    };
>  
>      naddr = dt_number_of_address(dev);
>  
> @@ -1884,7 +1889,6 @@ 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 };
>          res = dt_device_get_address(dev, i, &addr, &size);
>          if ( res )
>          {
> @@ -1898,7 +1902,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;
>  
> @@ -3056,7 +3060,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, 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 602d00799c8d..b81620074a91 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 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 1eb4daa87365..085f08e23e0c 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -18,6 +18,7 @@
>  
>  #include <xen/init.h>
>  #include <xen/pci.h>
> +#include <asm/setup.h>
>  #include <xen/rwlock.h>
>  #include <xen/sched.h>
>  #include <xen/vmap.h>
> @@ -320,6 +321,54 @@ int pci_host_get_num_bridges(void)
>      return count;
>  }
>  
> +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,
> +        .skip_mapping = false
> +    };
> +
> +    /*
> +     * 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, 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.
> +     */
> +    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 61a9807d3d58..6ad2b31e810d 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 a71b0eea8cb9..b5b85ccd0937 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);
>  int pci_get_host_bridge_segment(const struct dt_device_node *node,
>                                  uint16_t *segment);
> @@ -113,6 +121,8 @@ int pci_host_iterate_bridges(struct domain *d,
>                                          struct pci_host_bridge *bridge));
>  int pci_host_get_num_bridges(void);
>  
> +int pci_host_bridge_mappings(struct domain *d, p2m_type_t p2mt);
> +
>  #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
> 

Re: [PATCH v5 06/10] libxl: Allow removing PCI devices for all types of domains

[Anthony PERARD]

On Fri, Oct 08, 2021 at 08:55:31AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> The PCI device remove path may now be used by PVH on ARM, so the
> assert is no longer valid.

Is there maybe a patch or a commit you could point to which say that
there can be PCI passthrough on PVH?

> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> Tested-by: Rahul Singh <rahul.singh@arm.com>

In any case,
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v5 06/10] libxl: Allow removing PCI devices for all types of domains

[Oleksandr Andrushchenko]

Hi, Anthony!

On 11.10.21 19:11, Anthony PERARD wrote:
> On Fri, Oct 08, 2021 at 08:55:31AM +0300, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> The PCI device remove path may now be used by PVH on ARM, so the
>> assert is no longer valid.
> Is there maybe a patch or a commit you could point to which say that
> there can be PCI passthrough on PVH?
Unfortunately no, this is what all the PCI series on Arm are for
>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
>> Tested-by: Rahul Singh <rahul.singh@arm.com>
> In any case,
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Thank you,
Oleksandr
> Thanks,
>

Re: [PATCH v5 07/10] libxl: Only map legacy PCI IRQs if they are supported

[Anthony PERARD]

On Fri, Oct 08, 2021 at 08:55:32AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Arm's PCI passthrough implementation doesn't support legacy interrupts,
> but MSI/MSI-X. This can be the case for other platforms too.
> For that reason introduce a new CONFIG_PCI_SUPP_LEGACY_IRQ and add
> it to the CFLAGS and compile the relevant code in the toolstack only if
> applicable.

I don't think that's true anymore since v2 ;-). The compiler may
choose to avoid compiling code that wouldn't be executed, but I think
that would just be optimisation.

> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> Tested-by: Rahul Singh <rahul.singh@arm.com>
> ---
> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
> index 7d8c51d49242..bd3f6be2a183 100644
> --- a/tools/libs/light/Makefile
> +++ b/tools/libs/light/Makefile
> @@ -46,6 +46,10 @@ CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
>  	-Wno-declaration-after-statement -Wformat-nonliteral
>  CFLAGS += -I.
>  
> +ifeq ($(CONFIG_X86),y)
> +CFLAGS += -DCONFIG_PCI_SUPP_LEGACY_IRQ
> +endif
> +

Could you write this instead:
  CFLAGS-$(CONFIG_X86) += -DCONFIG_PCI_SUPP_LEGACY_IRQ

In any case,
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v5 07/10] libxl: Only map legacy PCI IRQs if they are supported

[Stefano Stabellini]

On Mon, 11 Oct 2021, Anthony PERARD wrote:
> On Fri, Oct 08, 2021 at 08:55:32AM +0300, Oleksandr Andrushchenko wrote:
> > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> > 
> > Arm's PCI passthrough implementation doesn't support legacy interrupts,
> > but MSI/MSI-X. This can be the case for other platforms too.
> > For that reason introduce a new CONFIG_PCI_SUPP_LEGACY_IRQ and add
> > it to the CFLAGS and compile the relevant code in the toolstack only if
> > applicable.
> 
> I don't think that's true anymore since v2 ;-). The compiler may
> choose to avoid compiling code that wouldn't be executed, but I think
> that would just be optimisation.
> 
> > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> > Tested-by: Rahul Singh <rahul.singh@arm.com>
> > ---
> > diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
> > index 7d8c51d49242..bd3f6be2a183 100644
> > --- a/tools/libs/light/Makefile
> > +++ b/tools/libs/light/Makefile
> > @@ -46,6 +46,10 @@ CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
> >  	-Wno-declaration-after-statement -Wformat-nonliteral
> >  CFLAGS += -I.
> >  
> > +ifeq ($(CONFIG_X86),y)
> > +CFLAGS += -DCONFIG_PCI_SUPP_LEGACY_IRQ
> > +endif
> > +
> 
> Could you write this instead:
>   CFLAGS-$(CONFIG_X86) += -DCONFIG_PCI_SUPP_LEGACY_IRQ
> 
> In any case,
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks Anthony! I made this change on commit and committed both this
patch and the previous one.

Re: [PATCH v5 08/10] xen/arm: Setup MMIO range trap handlers for hardware domain

[Roger Pau Monné]

On Fri, Oct 08, 2021 at 08:55:33AM +0300, 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>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> Tested-by: Rahul Singh <rahul.singh@arm.com>
> ---
> 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 | 28 ++++++++++++++++++++++++
>  xen/arch/arm/vpci.c                | 34 ++++++++++++++++++++++++++++++
>  xen/arch/arm/vpci.h                |  6 ++++++
>  xen/include/asm-arm/pci.h          |  5 +++++
>  5 files changed, 75 insertions(+)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 79012bf77757..fa6fcc5e467c 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -733,6 +733,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 )

IMO it might be better to convert the fixed array into a linked list,
I guess this made sense when Arm had a very limited number of mmio
trap handlers, but having to do all this accounting seems quite
tedious every time you want to add new handlers.

>          goto fail;
>  
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 592c01aae5bb..1eb4daa87365 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -292,6 +292,34 @@ struct dt_device_node *pci_find_host_bridge_node(struct device *dev)
>      }
>      return bridge->dt_node;
>  }
> +
> +int pci_host_iterate_bridges(struct domain *d,
> +                             int (*cb)(struct domain *d,
> +                                       struct pci_host_bridge *bridge))
> +{
> +    struct pci_host_bridge *bridge;
> +    int err;
> +
> +    list_for_each_entry( bridge, &pci_host_bridges, node )
> +    {
> +        err = cb(d, bridge);
> +        if ( err )
> +            return err;
> +    }
> +    return 0;
> +}
> +
> +int pci_host_get_num_bridges(void)
> +{
> +    struct pci_host_bridge *bridge;
> +    int count = 0;

unsigned int for both the local variable and the return type.

> +
> +    list_for_each_entry( bridge, &pci_host_bridges, node )
> +        count++;
> +
> +    return count;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index 76c12b92814f..6e179cd3010b 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -80,17 +80,51 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
>      .write = vpci_mmio_write,
>  };
>  
> +static int vpci_setup_mmio_handler(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, NULL);

I'm confused here, don't you need to use a slightly different handler
for dom0 so that you can differentiate between the segments of the
host bridges?

AFAICT the translation done by vpci_mmio_handler using MMCFG_BDF
always assume segment 0.

> +    return 0;
> +}
> +
>  int domain_vpci_init(struct domain *d)
>  {
>      if ( !has_vpci(d) )
>          return 0;
>  
> +    if ( is_hardware_domain(d) )
> +        return pci_host_iterate_bridges(d, vpci_setup_mmio_handler);
> +
> +    /* Guest domains use what is programmed in their device tree. */
>      register_mmio_handler(d, &vpci_mmio_handler,
>                            GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
>  
>      return 0;
>  }
>  
> +int domain_vpci_get_num_mmio_handlers(struct domain *d)
> +{
> +    int count;

unsigned for both types.

> +
> +    if ( is_hardware_domain(d) )
> +        /* For each PCI host bridge's configuration space. */
> +        count = pci_host_get_num_bridges();

There's no need to trap MSI-X Table/PBA accesses for dom0 I assume?

> +    else
> +        /*
> +         * VPCI_MSIX_MEM_NUM handlers for MSI-X tables per each PCI device
> +         * being passed through. Maximum number of supported devices
> +         * is 32 as virtual bus topology emulates the devices as embedded
> +         * endpoints.
> +         * +1 for a single emulated host bridge's configuration space.
> +         */
> +        count = VPCI_MSIX_MEM_NUM * 32 + 1;

There's a single MSI-X mmio handler that deals with both PBA and MSIX
tables, so I don't see the need to * VPCI_MSIX_MEM_NUM.

Thanks, Roger.

Re: [PATCH v5 10/10] xen/arm: Process pending vPCI map/unmap operations

[Julien Grall]

Hi Oleksandr,

On 08/10/2021 06:55, 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 changes]
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> Tested-by: Rahul Singh <rahul.singh@arm.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Paul Durrant <paul@xen.org>
> 
> 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..b246f51086e3 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>
> @@ -2304,6 +2305,18 @@ static bool check_for_vcpu_work(void)
>       }
>   #endif
>   
> +    if ( has_vpci(v->domain) )
> +    {
> +        bool pending;
> +
> +        local_irq_enable();
> +        pending = vpci_process_pending(v);
> +        local_irq_disable();
> +
> +        if ( pending )
> +            return true;
> +    }
> +

I would prefer if this addition is moved before the 
vcpu_ioreq__handle_completion(v). This is to avoid differences with the 
x86 version.

>       if ( likely(!v->arch.need_flush_to_ram) )
>           return false;
>   
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index aa418a3ca1b7..c491242e4b8b 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;
> 

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi Oleksandr,

On 08/10/2021 06:55, 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

NIT: The space before "From" looks odd.

> 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.
> 
> [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 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        | 57 ++++++++++++++++++------------
>   xen/arch/arm/pci/ecam.c            | 14 ++++++++
>   xen/arch/arm/pci/pci-host-common.c | 49 +++++++++++++++++++++++++
>   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, 121 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b51176b31bef..0d673b06a2f3 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>
> @@ -51,12 +50,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))
> @@ -1663,10 +1656,11 @@ 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);
> +    bool need_mapping = !mr_data->skip_mapping;

Before, the variable 'need_mapping' was helpful to understand what 
!dt_device_for_passthrough(). Now it feels odd to read. There is only 
one user, so can the local variable be dropped?

>   
>       if ( irq < NR_LOCAL_IRQS )
>       {
> @@ -1690,13 +1684,12 @@ 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;
> -    bool need_mapping = !dt_device_for_passthrough(dev);
> +    bool need_mapping = !mr_data->skip_mapping;

Same here.

>       int res;
>   
>       /*
> @@ -1748,23 +1741,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;
>       }
> @@ -1845,6 +1836,20 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
>       int res;
>       u64 addr, size;
>       bool need_mapping = !dt_device_for_passthrough(dev);

I find the difference between .skip_mapping and the local variable 
need_mapping quite puzzling. We are likely going to misuse them in the 
future.

I think it would be clearer if the local variable is renamed to 
'own_device' or similar.

> +    /*
> +     * For PCI passthrough we only need to remap to Dom0 the interrupts
> +     * and memory ranges from "reg" property which cover controller's
> +     * configuration registers and such. PCIe configuration space registers
> +     * of the PCIe Root Complex and PCIe aperture should not be mapped
> +     * automatically to Dom0.
> +     */
> +    struct map_range_data mr_data = {
> +        .d = d,
> +        .p2mt = p2mt,
> +        .skip_mapping = !need_mapping ||
> +                        (is_pci_passthrough_enabled() &&
> +                         (device_get_class(dev) == DEVICE_PCI))

The device class is confusing. When I see DEVICE_PCI, I think this is a 
PCI device whereas here you are referring to the hostbridge.

Unfortunate, I wasn't able to comment on the original patch before it 
was committed. But I would like this to be renamed to 
DEVICE_PCI_HOSTBRIDGE for Xen 4.16. Can you send a patch?

> +    };
>   
>       naddr = dt_number_of_address(dev);
>   
> @@ -1884,7 +1889,6 @@ 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 };
>           res = dt_device_get_address(dev, i, &addr, &size);
>           if ( res )
>           {
> @@ -1898,7 +1902,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;
>   
> @@ -3056,7 +3060,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, p2m_mmio_direct_c);

It is not clear to me why you are passing p2m_mmio_direct_c and not p2mt 
here? If you really want to force a type, then I think it should be 
p2m_mmio_direct.

But then why is it a parameter of pci_host_bridge_mappings? Do you 
expect someone else to modify it?

> +#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..b81620074a91 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 pci_ecam_need_p2m_hwdom_mapping(struct domain *d,

AFAICT, this is only used by boot code. So you want to do add __init.

This is also exported but not used. I would prefer if we only exposed 
when the first external user will be introduced.

> +                                     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 1eb4daa87365..085f08e23e0c 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -18,6 +18,7 @@
>   
>   #include <xen/init.h>
>   #include <xen/pci.h>
> +#include <asm/setup.h>

For new code, we usually include xen/*.h first and then asm/*.h. They 
are then order alphabetically within themselves.

>   #include <xen/rwlock.h>
>   #include <xen/sched.h>
>   #include <xen/vmap.h>
> @@ -320,6 +321,54 @@ int pci_host_get_num_bridges(void)
>       return count;
>   }
>   
> +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,
> +        .skip_mapping = false
> +    };
> +
> +    /*
> +     * 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, but we want to

AFAIU, "assigned-addresses" is only relevant for the child of nodes with 
type="pci". This is not the case for the hostbridges. So I think you 
want to drop the mention of "assigned-addresses".


> +     * map other regions of the host bridge. The PCI aperture defined by
> +     * the "ranges" device tree property should also be skipped.
> +     */
> +    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 61a9807d3d58..6ad2b31e810d 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 a71b0eea8cb9..b5b85ccd0937 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);
>   int pci_get_host_bridge_segment(const struct dt_device_node *node,
>                                   uint16_t *segment);
> @@ -113,6 +121,8 @@ int pci_host_iterate_bridges(struct domain *d,
>                                           struct pci_host_bridge *bridge));
>   int pci_host_get_num_bridges(void);
>   
> +int pci_host_bridge_mappings(struct domain *d, p2m_type_t p2mt);
> +
>   #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:
> 

-- 
Julien Grall

Re: [PATCH v5 08/10] xen/arm: Setup MMIO range trap handlers for hardware domain

[Oleksandr Andrushchenko]

Hi, Roger!

On 13.10.21 13:11, Roger Pau Monné wrote:
> On Fri, Oct 08, 2021 at 08:55:33AM +0300, 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>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
>> Tested-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> 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 | 28 ++++++++++++++++++++++++
>>   xen/arch/arm/vpci.c                | 34 ++++++++++++++++++++++++++++++
>>   xen/arch/arm/vpci.h                |  6 ++++++
>>   xen/include/asm-arm/pci.h          |  5 +++++
>>   5 files changed, 75 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 79012bf77757..fa6fcc5e467c 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -733,6 +733,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 )
> IMO it might be better to convert the fixed array into a linked list,
> I guess this made sense when Arm had a very limited number of mmio
> trap handlers, but having to do all this accounting seems quite
> tedious every time you want to add new handlers.
Yes, I think we need to do so, but this improvement was not meant
to be in this patch.
>
>>           goto fail;
>>   
>> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
>> index 592c01aae5bb..1eb4daa87365 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -292,6 +292,34 @@ struct dt_device_node *pci_find_host_bridge_node(struct device *dev)
>>       }
>>       return bridge->dt_node;
>>   }
>> +
>> +int pci_host_iterate_bridges(struct domain *d,
>> +                             int (*cb)(struct domain *d,
>> +                                       struct pci_host_bridge *bridge))
>> +{
>> +    struct pci_host_bridge *bridge;
>> +    int err;
>> +
>> +    list_for_each_entry( bridge, &pci_host_bridges, node )
>> +    {
>> +        err = cb(d, bridge);
>> +        if ( err )
>> +            return err;
>> +    }
>> +    return 0;
>> +}
>> +
>> +int pci_host_get_num_bridges(void)
>> +{
>> +    struct pci_host_bridge *bridge;
>> +    int count = 0;
> unsigned int for both the local variable and the return type.
Ok
>
>> +
>> +    list_for_each_entry( bridge, &pci_host_bridges, node )
>> +        count++;
>> +
>> +    return count;
>> +}
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> index 76c12b92814f..6e179cd3010b 100644
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -80,17 +80,51 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
>>       .write = vpci_mmio_write,
>>   };
>>   
>> +static int vpci_setup_mmio_handler(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, NULL);
> I'm confused here, don't you need to use a slightly different handler
> for dom0 so that you can differentiate between the segments of the
> host bridges?
>
> AFAICT the translation done by vpci_mmio_handler using MMCFG_BDF
> always assume segment 0.
You are absolutely right here: I can set up hwdom specific
handlers, so I can properly translate the segment.
On the other hand, when virtual bus topology added, the SBDF
translation from virtual to physical SBDF resides in the Arm's
vpci_mmio_{read|write}, like the below:
     if ( priv->is_virt_ecam &&
          !vpci_translate_virtual_device(v->domain, &sbdf) )
             return 1;
(BTW Jan asked in some other comment why it is Arm specific:
I tend to keep it Arm specific until the point when x86 wants that
as well. Until that point the code, if moved to common, will be
unneeded and as Jan calls that "dead")
So, I think that I can extend vpci_mmio_{read|write} to account
on the hwdom like (virtual bus code is the future code):

static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
                           register_t *r, void *p)
{
...
     struct vpci_mmio_priv *priv = (struct vpci_mmio_priv *)p;

     if ( priv->is_virt_ecam )
         sbdf.sbdf = MMCFG_BDF(info->gpa); /* For virtual bus topology the segment is always 0. */
     else
     {
         sbdf.sbdf = MMCFG_BDF(info->gpa);
         sbdf.seg = priv->segment;
     }
     reg = REGISTER_OFFSET(info->gpa);

...
     /*
      * For the passed through devices we need to map their virtual SBDF
      * to the physical PCI device being passed through.
      */
     if ( priv->is_virt_ecam &&
          !vpci_translate_virtual_device(v->domain, &sbdf) )
             return 1;

Will it work for you?
>
>> +    return 0;
>> +}
>> +
>>   int domain_vpci_init(struct domain *d)
>>   {
>>       if ( !has_vpci(d) )
>>           return 0;
>>   
>> +    if ( is_hardware_domain(d) )
>> +        return pci_host_iterate_bridges(d, vpci_setup_mmio_handler);
>> +
>> +    /* Guest domains use what is programmed in their device tree. */
>>       register_mmio_handler(d, &vpci_mmio_handler,
>>                             GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
>>   
>>       return 0;
>>   }
>>   
>> +int domain_vpci_get_num_mmio_handlers(struct domain *d)
>> +{
>> +    int count;
> unsigned for both types.
Ok
>
>> +
>> +    if ( is_hardware_domain(d) )
>> +        /* For each PCI host bridge's configuration space. */
>> +        count = pci_host_get_num_bridges();
> There's no need to trap MSI-X Table/PBA accesses for dom0 I assume?
No need
>
>> +    else
>> +        /*
>> +         * VPCI_MSIX_MEM_NUM handlers for MSI-X tables per each PCI device
>> +         * being passed through. Maximum number of supported devices
>> +         * is 32 as virtual bus topology emulates the devices as embedded
>> +         * endpoints.
>> +         * +1 for a single emulated host bridge's configuration space.
>> +         */
>> +        count = VPCI_MSIX_MEM_NUM * 32 + 1;
> There's a single MSI-X mmio handler that deals with both PBA and MSIX
> tables, so I don't see the need to * VPCI_MSIX_MEM_NUM.
Yes, thank you, it is indeed, so the number can be reduced
>
> Thanks, Roger.
Thank you,
Oleksandr

Re: [PATCH v5 10/10] xen/arm: Process pending vPCI map/unmap operations

[Oleksandr Andrushchenko]

Hi, Julien!

On 13.10.21 18:30, Julien Grall wrote:
> Hi Oleksandr,
>
> On 08/10/2021 06:55, 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 changes]
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
>> Tested-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: Paul Durrant <paul@xen.org>
>>
>> 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..b246f51086e3 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>
>> @@ -2304,6 +2305,18 @@ static bool check_for_vcpu_work(void)
>>       }
>>   #endif
>>   +    if ( has_vpci(v->domain) )
>> +    {
>> +        bool pending;
>> +
>> +        local_irq_enable();
>> +        pending = vpci_process_pending(v);
>> +        local_irq_disable();
>> +
>> +        if ( pending )
>> +            return true;
>> +    }
>> +
>
> I would prefer if this addition is moved before the vcpu_ioreq__handle_completion(v). This is to avoid differences with the x86 version.
Ok, will do

Thanks,
Oleksandr

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

[Oleksandr Andrushchenko]

Hi, Julien!

On 13.10.21 19:17, Julien Grall wrote:
> Hi Oleksandr,
>
> On 08/10/2021 06:55, 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
>
> NIT: The space before "From" looks odd.
Will fix
>
>> 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.
>>
>> [1] https://urldefense.com/v3/__https://lists.xenproject.org/archives/html/xen-devel/2020-07/msg00777.html__;!!GF_29dbcQIUBPA!hDUAXuiHAx7fsX6gyMvlen2PB4kTm3vWbVFaTy19HwpFJkSbFXkOSjjp_78xX6VmHCVkcFK39w$ [lists[.]xenproject[.]org]
>> [2] https://urldefense.com/v3/__https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/host-generic-pci.txt__;!!GF_29dbcQIUBPA!hDUAXuiHAx7fsX6gyMvlen2PB4kTm3vWbVFaTy19HwpFJkSbFXkOSjjp_78xX6VmHCWryzEUIg$ [kernel[.]org]
>> [3] https://urldefense.com/v3/__https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt__;!!GF_29dbcQIUBPA!hDUAXuiHAx7fsX6gyMvlen2PB4kTm3vWbVFaTy19HwpFJkSbFXkOSjjp_78xX6VmHCWuT3i91Q$ [kernel[.]org]
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> ---
>> 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        | 57 ++++++++++++++++++------------
>>   xen/arch/arm/pci/ecam.c            | 14 ++++++++
>>   xen/arch/arm/pci/pci-host-common.c | 49 +++++++++++++++++++++++++
>>   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, 121 insertions(+), 23 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index b51176b31bef..0d673b06a2f3 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>
>> @@ -51,12 +50,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))
>> @@ -1663,10 +1656,11 @@ 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);
>> +    bool need_mapping = !mr_data->skip_mapping;
>
> Before, the variable 'need_mapping' was helpful to understand what !dt_device_for_passthrough(). Now it feels odd to read. There is only one user, so can the local variable be dropped?
Ok, I can drop the local variable and use the value directly
>
>>         if ( irq < NR_LOCAL_IRQS )
>>       {
>> @@ -1690,13 +1684,12 @@ 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;
>> -    bool need_mapping = !dt_device_for_passthrough(dev);
>> +    bool need_mapping = !mr_data->skip_mapping;
>
> Same here.
Ok, I can drop the local variable and use the value directly
>
>>       int res;
>>         /*
>> @@ -1748,23 +1741,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;
>>       }
>> @@ -1845,6 +1836,20 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
>>       int res;
>>       u64 addr, size;
>>       bool need_mapping = !dt_device_for_passthrough(dev);
>
> I find the difference between .skip_mapping and the local variable need_mapping quite puzzling. We are likely going to misuse them in the future.
>
> I think it would be clearer if the local variable is renamed to 'own_device' or similar.
Ok, so let it be own_device
>
>> +    /*
>> +     * For PCI passthrough we only need to remap to Dom0 the interrupts
>> +     * and memory ranges from "reg" property which cover controller's
>> +     * configuration registers and such. PCIe configuration space registers
>> +     * of the PCIe Root Complex and PCIe aperture should not be mapped
>> +     * automatically to Dom0.
>> +     */
>> +    struct map_range_data mr_data = {
>> +        .d = d,
>> +        .p2mt = p2mt,
>> +        .skip_mapping = !need_mapping ||
>> +                        (is_pci_passthrough_enabled() &&
>> +                         (device_get_class(dev) == DEVICE_PCI))
>
> The device class is confusing. When I see DEVICE_PCI, I think this is a PCI device whereas here you are referring to the hostbridge.
>
> Unfortunate, I wasn't able to comment on the original patch before it was committed. But I would like this to be renamed to DEVICE_PCI_HOSTBRIDGE for Xen 4.16. Can you send a patch?
Ok, I'll add a patch with s/DEVICE_PCI/DEVICE_PCI_HOSTBRIDGE
>
>> +    };
>>         naddr = dt_number_of_address(dev);
>>   @@ -1884,7 +1889,6 @@ 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 };
>>           res = dt_device_get_address(dev, i, &addr, &size);
>>           if ( res )
>>           {
>> @@ -1898,7 +1902,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;
>>   @@ -3056,7 +3060,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, p2m_mmio_direct_c);
>
> It is not clear to me why you are passing p2m_mmio_direct_c and not p2mt here?
There is no p2mt in the caller function, e.g. construct_dom0
> If you really want to force a type, then I think it should be p2m_mmio_direct.
I just followed the defaults found in:
static int __init prepare_dtb_hwdom(struct domain *d, struct kernel_info *kinfo)
{
     const p2m_type_t default_p2mt = p2m_mmio_direct_c;
which is also called from construct_dom0
>
> But then why is it a parameter of pci_host_bridge_mappings? Do you expect someone else to modify it?
No, I don't expect to modify that, I just don't want PCI code to make decisions on that.
So, I feel more comfortable if that decision is taken in construct_dom0.

So, what do we want here? Pass as parameter (then p2m_mmio_direct I guess, not p2m_mmio_direct_c)?
Or let PCI code use p2m_mmio_direct inside pci_host_bridge_mappings?
>
>> +#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..b81620074a91 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 pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
>
> AFAICT, this is only used by boot code. So you want to do add __init.
Makes sense
>
> This is also exported but not used. I would prefer if we only exposed when the first external user will be introduced.
ZynqMP is the first user yet in this patch. More to come probably later on when we add other host bridges.
>
>> + 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 1eb4daa87365..085f08e23e0c 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -18,6 +18,7 @@
>>     #include <xen/init.h>
>>   #include <xen/pci.h>
>> +#include <asm/setup.h>
>
> For new code, we usually include xen/*.h first and then asm/*.h. They are then order alphabetically within themselves.
Ok
>
>>   #include <xen/rwlock.h>
>>   #include <xen/sched.h>
>>   #include <xen/vmap.h>
>> @@ -320,6 +321,54 @@ int pci_host_get_num_bridges(void)
>>       return count;
>>   }
>>   +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,
>> +        .skip_mapping = false
>> +    };
>> +
>> +    /*
>> +     * 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, but we want to
>
> AFAIU, "assigned-addresses" is only relevant for the child of nodes with type="pci". This is not the case for the hostbridges. So I think you want to drop the mention of "assigned-addresses".
Will drop
>
>
>> +     * map other regions of the host bridge. The PCI aperture defined by
>> +     * the "ranges" device tree property should also be skipped.
>> +     */
>> +    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 61a9807d3d58..6ad2b31e810d 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 a71b0eea8cb9..b5b85ccd0937 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);
>>   int pci_get_host_bridge_segment(const struct dt_device_node *node,
>>                                   uint16_t *segment);
>> @@ -113,6 +121,8 @@ int pci_host_iterate_bridges(struct domain *d,
>>                                           struct pci_host_bridge *bridge));
>>   int pci_host_get_num_bridges(void);
>>   +int pci_host_bridge_mappings(struct domain *d, p2m_type_t p2mt);
>> +
>>   #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:
>>
>

Re: [PATCH v5 06/10] libxl: Allow removing PCI devices for all types of domains

[Roger Pau Monné]

On Fri, Oct 08, 2021 at 08:55:31AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> The PCI device remove path may now be used by PVH on ARM, so the
> assert is no longer valid.

I think ti might be worth saying that this is a preparatory change and
that PCI passthrough is not yet functional on Arm after it.

Thanks, Roger.

Re: [PATCH v5 06/10] libxl: Allow removing PCI devices for all types of domains

[Oleksandr Andrushchenko]

Hi, Roger!

On 25.10.21 16:14, Roger Pau Monné wrote:
> On Fri, Oct 08, 2021 at 08:55:31AM +0300, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> The PCI device remove path may now be used by PVH on ARM, so the
>> assert is no longer valid.
> I think ti might be worth saying that this is a preparatory change and
> that PCI passthrough is not yet functional on Arm after it.
Unfortunately this change has already landed.
>
> Thanks, Roger.
Thank you,
Oleksandr

Re: [PATCH v5 07/10] libxl: Only map legacy PCI IRQs if they are supported

[Roger Pau Monné]

On Fri, Oct 08, 2021 at 08:55:32AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Arm's PCI passthrough implementation doesn't support legacy interrupts,
> but MSI/MSI-X. This can be the case for other platforms too.
> For that reason introduce a new CONFIG_PCI_SUPP_LEGACY_IRQ and add
> it to the CFLAGS and compile the relevant code in the toolstack only if
> applicable.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> Tested-by: Rahul Singh <rahul.singh@arm.com>
> ---
> Cc: Ian Jackson <iwj@xenproject.org>
> Cc: Juergen Gross <jgross@suse.com>
> 
> Since v1:
>  - Minimized #idefery by introducing pci_supp_legacy_irq function
>    for relevant checks
> ---
>  tools/libs/light/Makefile    |  4 ++++
>  tools/libs/light/libxl_pci.c | 13 +++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
> index 7d8c51d49242..bd3f6be2a183 100644
> --- a/tools/libs/light/Makefile
> +++ b/tools/libs/light/Makefile
> @@ -46,6 +46,10 @@ CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
>  	-Wno-declaration-after-statement -Wformat-nonliteral
>  CFLAGS += -I.
>  
> +ifeq ($(CONFIG_X86),y)
> +CFLAGS += -DCONFIG_PCI_SUPP_LEGACY_IRQ
> +endif

TBH, since CONFIG_PCI_SUPP_LEGACY_IRQ is used in a single place in the
source I would consider just using CONFIG_X86, as I think it would be
clear enough that some arches support legacy interrupt while others
don't. Not sure it's worth going through the obfuscation of defining a
separate one.

> +
>  SRCS-$(CONFIG_X86) += libxl_cpuid.c
>  SRCS-$(CONFIG_X86) += libxl_x86.c
>  SRCS-$(CONFIG_X86) += libxl_psr.c
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 59f3686fc85e..4c2d7aeefbb2 100644
> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -1364,6 +1364,15 @@ static void pci_add_timeout(libxl__egc *egc, libxl__ev_time *ev,
>      pci_add_dm_done(egc, pas, rc);
>  }
>  
> +static bool pci_supp_legacy_irq(void)

A naming more inline with the PCI specification (since we insisted on
using ECAM instead of MCFG) would be intx, ie: pci_supports_intx.

Thanks, Roger.

Re: [PATCH v5 07/10] libxl: Only map legacy PCI IRQs if they are supported

[Oleksandr Andrushchenko]

Hi, Roger!

On 25.10.21 16:22, Roger Pau Monné wrote:
> On Fri, Oct 08, 2021 at 08:55:32AM +0300, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> Arm's PCI passthrough implementation doesn't support legacy interrupts,
>> but MSI/MSI-X. This can be the case for other platforms too.
>> For that reason introduce a new CONFIG_PCI_SUPP_LEGACY_IRQ and add
>> it to the CFLAGS and compile the relevant code in the toolstack only if
>> applicable.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
>> Tested-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Cc: Ian Jackson <iwj@xenproject.org>
>> Cc: Juergen Gross <jgross@suse.com>
>>
>> Since v1:
>>   - Minimized #idefery by introducing pci_supp_legacy_irq function
>>     for relevant checks
>> ---
>>   tools/libs/light/Makefile    |  4 ++++
>>   tools/libs/light/libxl_pci.c | 13 +++++++++++++
>>   2 files changed, 17 insertions(+)
>>
>> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
>> index 7d8c51d49242..bd3f6be2a183 100644
>> --- a/tools/libs/light/Makefile
>> +++ b/tools/libs/light/Makefile
>> @@ -46,6 +46,10 @@ CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
>>   	-Wno-declaration-after-statement -Wformat-nonliteral
>>   CFLAGS += -I.
>>   
>> +ifeq ($(CONFIG_X86),y)
>> +CFLAGS += -DCONFIG_PCI_SUPP_LEGACY_IRQ
>> +endif
> TBH, since CONFIG_PCI_SUPP_LEGACY_IRQ is used in a single place in the
> source I would consider just using CONFIG_X86, as I think it would be
> clear enough that some arches support legacy interrupt while others
> don't. Not sure it's worth going through the obfuscation of defining a
> separate one.
The idea behind not using CONFIG_X86 is to be arch agnostic,
so CONFIG_PCI_SUPP_LEGACY_IRQ makes more sense with this
respect
>> +
>>   SRCS-$(CONFIG_X86) += libxl_cpuid.c
>>   SRCS-$(CONFIG_X86) += libxl_x86.c
>>   SRCS-$(CONFIG_X86) += libxl_psr.c
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index 59f3686fc85e..4c2d7aeefbb2 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1364,6 +1364,15 @@ static void pci_add_timeout(libxl__egc *egc, libxl__ev_time *ev,
>>       pci_add_dm_done(egc, pas, rc);
>>   }
>>   
>> +static bool pci_supp_legacy_irq(void)
> A naming more inline with the PCI specification (since we insisted on
> using ECAM instead of MCFG) would be intx, ie: pci_supports_intx.
This will require a follow up patch if we all agree this change is needed.
@Jan, are you ok with the rename?
>
> Thanks, Roger.
Thank you,
Oleksandr

Re: [PATCH v5 08/10] xen/arm: Setup MMIO range trap handlers for hardware domain

[Roger Pau Monné]

On Mon, Oct 25, 2021 at 09:38:00AM +0000, Oleksandr Andrushchenko wrote:
> Hi, Roger!
> 
> On 13.10.21 13:11, Roger Pau Monné wrote:
> > On Fri, Oct 08, 2021 at 08:55:33AM +0300, 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>
> >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> >> Tested-by: Rahul Singh <rahul.singh@arm.com>
> >> ---
> >> 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 | 28 ++++++++++++++++++++++++
> >>   xen/arch/arm/vpci.c                | 34 ++++++++++++++++++++++++++++++
> >>   xen/arch/arm/vpci.h                |  6 ++++++
> >>   xen/include/asm-arm/pci.h          |  5 +++++
> >>   5 files changed, 75 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >> index 79012bf77757..fa6fcc5e467c 100644
> >> --- a/xen/arch/arm/domain.c
> >> +++ b/xen/arch/arm/domain.c
> >> @@ -733,6 +733,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 )
> > IMO it might be better to convert the fixed array into a linked list,
> > I guess this made sense when Arm had a very limited number of mmio
> > trap handlers, but having to do all this accounting seems quite
> > tedious every time you want to add new handlers.
> Yes, I think we need to do so, but this improvement was not meant
> to be in this patch.

Ack, just wanted to raise that this model seems to be getting more
complex than just setting up a list.

> >> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> >> index 76c12b92814f..6e179cd3010b 100644
> >> --- a/xen/arch/arm/vpci.c
> >> +++ b/xen/arch/arm/vpci.c
> >> @@ -80,17 +80,51 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
> >>       .write = vpci_mmio_write,
> >>   };
> >>   
> >> +static int vpci_setup_mmio_handler(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, NULL);
> > I'm confused here, don't you need to use a slightly different handler
> > for dom0 so that you can differentiate between the segments of the
> > host bridges?
> >
> > AFAICT the translation done by vpci_mmio_handler using MMCFG_BDF
> > always assume segment 0.
> You are absolutely right here: I can set up hwdom specific
> handlers, so I can properly translate the segment.
> On the other hand, when virtual bus topology added, the SBDF
> translation from virtual to physical SBDF resides in the Arm's
> vpci_mmio_{read|write}, like the below:
>      if ( priv->is_virt_ecam &&
>           !vpci_translate_virtual_device(v->domain, &sbdf) )
>              return 1;
> (BTW Jan asked in some other comment why it is Arm specific:
> I tend to keep it Arm specific until the point when x86 wants that
> as well. Until that point the code, if moved to common, will be
> unneeded and as Jan calls that "dead")
> So, I think that I can extend vpci_mmio_{read|write} to account
> on the hwdom like (virtual bus code is the future code):
> 
> static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>                            register_t *r, void *p)
> {
> ...
>      struct vpci_mmio_priv *priv = (struct vpci_mmio_priv *)p;
> 
>      if ( priv->is_virt_ecam )
>          sbdf.sbdf = MMCFG_BDF(info->gpa); /* For virtual bus topology the segment is always 0. */
>      else
>      {
>          sbdf.sbdf = MMCFG_BDF(info->gpa);
>          sbdf.seg = priv->segment;
>      }
>      reg = REGISTER_OFFSET(info->gpa);
> 
> ...
>      /*
>       * For the passed through devices we need to map their virtual SBDF
>       * to the physical PCI device being passed through.
>       */
>      if ( priv->is_virt_ecam &&
>           !vpci_translate_virtual_device(v->domain, &sbdf) )
>              return 1;
> 
> Will it work for you?

Right, I guess it could work as long as the differences between the
hardware domain and the unprivileged ones are not too big.

The nice part about having different handlers is that you avoid a
bunch of conditionals (ie: no need to check for is_virt_ecam) which
makes the code easier to follow. OTOH it could introduce more code
duplication.

Thanks, Roger.

Re: [PATCH v5 07/10] libxl: Only map legacy PCI IRQs if they are supported

[Roger Pau Monné]

On Mon, Oct 25, 2021 at 01:38:19PM +0000, Oleksandr Andrushchenko wrote:
> Hi, Roger!
> 
> On 25.10.21 16:22, Roger Pau Monné wrote:
> > On Fri, Oct 08, 2021 at 08:55:32AM +0300, Oleksandr Andrushchenko wrote:
> >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> >>
> >> Arm's PCI passthrough implementation doesn't support legacy interrupts,
> >> but MSI/MSI-X. This can be the case for other platforms too.
> >> For that reason introduce a new CONFIG_PCI_SUPP_LEGACY_IRQ and add
> >> it to the CFLAGS and compile the relevant code in the toolstack only if
> >> applicable.
> >>
> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> >> Tested-by: Rahul Singh <rahul.singh@arm.com>
> >> ---
> >> Cc: Ian Jackson <iwj@xenproject.org>
> >> Cc: Juergen Gross <jgross@suse.com>
> >>
> >> Since v1:
> >>   - Minimized #idefery by introducing pci_supp_legacy_irq function
> >>     for relevant checks
> >> ---
> >>   tools/libs/light/Makefile    |  4 ++++
> >>   tools/libs/light/libxl_pci.c | 13 +++++++++++++
> >>   2 files changed, 17 insertions(+)
> >>
> >> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
> >> index 7d8c51d49242..bd3f6be2a183 100644
> >> --- a/tools/libs/light/Makefile
> >> +++ b/tools/libs/light/Makefile
> >> @@ -46,6 +46,10 @@ CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
> >>   	-Wno-declaration-after-statement -Wformat-nonliteral
> >>   CFLAGS += -I.
> >>   
> >> +ifeq ($(CONFIG_X86),y)
> >> +CFLAGS += -DCONFIG_PCI_SUPP_LEGACY_IRQ
> >> +endif
> > TBH, since CONFIG_PCI_SUPP_LEGACY_IRQ is used in a single place in the
> > source I would consider just using CONFIG_X86, as I think it would be
> > clear enough that some arches support legacy interrupt while others
> > don't. Not sure it's worth going through the obfuscation of defining a
> > separate one.
> The idea behind not using CONFIG_X86 is to be arch agnostic,
> so CONFIG_PCI_SUPP_LEGACY_IRQ makes more sense with this
> respect

I'm not going to insist, but you could have avoided modifying the
Makefile at all by just using:

static bool pci_supp_legacy_irq(void)
{
#ifdef CONFIG_X86
    return true;
#else
    return false;
#endif
}

Which IMO is clearer than the current approach since it's a single
place where CONFIG_PCI_SUPP_LEGACY_IRQ gets used.

> >> +
> >>   SRCS-$(CONFIG_X86) += libxl_cpuid.c
> >>   SRCS-$(CONFIG_X86) += libxl_x86.c
> >>   SRCS-$(CONFIG_X86) += libxl_psr.c
> >> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> >> index 59f3686fc85e..4c2d7aeefbb2 100644
> >> --- a/tools/libs/light/libxl_pci.c
> >> +++ b/tools/libs/light/libxl_pci.c
> >> @@ -1364,6 +1364,15 @@ static void pci_add_timeout(libxl__egc *egc, libxl__ev_time *ev,
> >>       pci_add_dm_done(egc, pas, rc);
> >>   }
> >>   
> >> +static bool pci_supp_legacy_irq(void)
> > A naming more inline with the PCI specification (since we insisted on
> > using ECAM instead of MCFG) would be intx, ie: pci_supports_intx.
> This will require a follow up patch if we all agree this change is needed.
> @Jan, are you ok with the rename?

Didn't realize it was already committed, in which case I guess we could
leave it as-is.

Thanks, Roger.

Re: [PATCH v5 08/10] xen/arm: Setup MMIO range trap handlers for hardware domain

[Oleksandr Andrushchenko]

On 25.10.21 16:40, Roger Pau Monné wrote:
> On Mon, Oct 25, 2021 at 09:38:00AM +0000, Oleksandr Andrushchenko wrote:
>> Hi, Roger!
>>
>> On 13.10.21 13:11, Roger Pau Monné wrote:
>>> On Fri, Oct 08, 2021 at 08:55:33AM +0300, 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>
>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
>>>> Tested-by: Rahul Singh <rahul.singh@arm.com>
>>>> ---
>>>> 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 | 28 ++++++++++++++++++++++++
>>>>    xen/arch/arm/vpci.c                | 34 ++++++++++++++++++++++++++++++
>>>>    xen/arch/arm/vpci.h                |  6 ++++++
>>>>    xen/include/asm-arm/pci.h          |  5 +++++
>>>>    5 files changed, 75 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>>> index 79012bf77757..fa6fcc5e467c 100644
>>>> --- a/xen/arch/arm/domain.c
>>>> +++ b/xen/arch/arm/domain.c
>>>> @@ -733,6 +733,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 )
>>> IMO it might be better to convert the fixed array into a linked list,
>>> I guess this made sense when Arm had a very limited number of mmio
>>> trap handlers, but having to do all this accounting seems quite
>>> tedious every time you want to add new handlers.
>> Yes, I think we need to do so, but this improvement was not meant
>> to be in this patch.
> Ack, just wanted to raise that this model seems to be getting more
> complex than just setting up a list.
>
>>>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>>>> index 76c12b92814f..6e179cd3010b 100644
>>>> --- a/xen/arch/arm/vpci.c
>>>> +++ b/xen/arch/arm/vpci.c
>>>> @@ -80,17 +80,51 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
>>>>        .write = vpci_mmio_write,
>>>>    };
>>>>    
>>>> +static int vpci_setup_mmio_handler(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, NULL);
>>> I'm confused here, don't you need to use a slightly different handler
>>> for dom0 so that you can differentiate between the segments of the
>>> host bridges?
>>>
>>> AFAICT the translation done by vpci_mmio_handler using MMCFG_BDF
>>> always assume segment 0.
>> You are absolutely right here: I can set up hwdom specific
>> handlers, so I can properly translate the segment.
>> On the other hand, when virtual bus topology added, the SBDF
>> translation from virtual to physical SBDF resides in the Arm's
>> vpci_mmio_{read|write}, like the below:
>>       if ( priv->is_virt_ecam &&
>>            !vpci_translate_virtual_device(v->domain, &sbdf) )
>>               return 1;
>> (BTW Jan asked in some other comment why it is Arm specific:
>> I tend to keep it Arm specific until the point when x86 wants that
>> as well. Until that point the code, if moved to common, will be
>> unneeded and as Jan calls that "dead")
>> So, I think that I can extend vpci_mmio_{read|write} to account
>> on the hwdom like (virtual bus code is the future code):
>>
>> static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>>                             register_t *r, void *p)
>> {
>> ...
>>       struct vpci_mmio_priv *priv = (struct vpci_mmio_priv *)p;
>>
>>       if ( priv->is_virt_ecam )
>>           sbdf.sbdf = MMCFG_BDF(info->gpa); /* For virtual bus topology the segment is always 0. */
>>       else
>>       {
>>           sbdf.sbdf = MMCFG_BDF(info->gpa);
>>           sbdf.seg = priv->segment;
>>       }
>>       reg = REGISTER_OFFSET(info->gpa);
>>
>> ...
>>       /*
>>        * For the passed through devices we need to map their virtual SBDF
>>        * to the physical PCI device being passed through.
>>        */
>>       if ( priv->is_virt_ecam &&
>>            !vpci_translate_virtual_device(v->domain, &sbdf) )
>>               return 1;
>>
>> Will it work for you?
> Right, I guess it could work as long as the differences between the
> hardware domain and the unprivileged ones are not too big.
>
> The nice part about having different handlers is that you avoid a
> bunch of conditionals (ie: no need to check for is_virt_ecam) which
> makes the code easier to follow. OTOH it could introduce more code
> duplication.
Yes. This is why I prefer conditionals in this specific case. Handlers
are small, yet I do not want to have two versions of the same.
The handlers become ugly if we try dividing them into common parts
So, I'll probably introduce priv in this patch and use it for the hwdom's
segment. Later on, when virtual bus topology is added for the guests,
I'll use the same (extended) priv to pass guest's info.
>
> Thanks, Roger.
Thank you,
Oleksandr

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

[Julien Grall]

On 25/10/2021 12:37, Oleksandr Andrushchenko wrote:
> Hi, Julien!

Hi Oleksandr,

> On 13.10.21 19:17, Julien Grall wrote:
>> On 08/10/2021 06:55, Oleksandr Andrushchenko wrote:
>>> +    };
>>>          naddr = dt_number_of_address(dev);
>>>    @@ -1884,7 +1889,6 @@ 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 };
>>>            res = dt_device_get_address(dev, i, &addr, &size);
>>>            if ( res )
>>>            {
>>> @@ -1898,7 +1902,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;
>>>    @@ -3056,7 +3060,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, p2m_mmio_direct_c);
>>
>> It is not clear to me why you are passing p2m_mmio_direct_c and not p2mt here?
> There is no p2mt in the caller function, e.g. construct_dom0
>> If you really want to force a type, then I think it should be p2m_mmio_direct.
> I just followed the defaults found in:
> static int __init prepare_dtb_hwdom(struct domain *d, struct kernel_info *kinfo)
> {
>       const p2m_type_t default_p2mt = p2m_mmio_direct_c;
> which is also called from construct_dom0

We use "p2m_mmio_direct_c" (cacheable mapping) by default because we 
don't know what how the region will be accessed  (e.g. this may be an 
SRAM). With this type, there is no restriction and dom0 is responsible 
to set to proper attribute in the stage-1 page-tables.

In this hostbridge cases, I see limited reasons at the moment for 
someone to map the non-BAR regions with cacheable attributes. So I think 
it is better to chose the most restricting attribute in the stage-2 
page-table.

>>
>> But then why is it a parameter of pci_host_bridge_mappings? Do you expect someone else to modify it?
> No, I don't expect to modify that, I just don't want PCI code to make decisions on that.
> So, I feel more comfortable if that decision is taken in construct_dom0.
Only the hostbridge driver is really aware of how the region will be 
accessed. So I think it is better to let...

> 
> So, what do we want here? Pass as parameter (then p2m_mmio_direct I guess, not p2m_mmio_direct_c)?
> Or let PCI code use p2m_mmio_direct inside pci_host_bridge_mappings?

... pci_host_bridge_mappings() decide the attribute. This can 
potentially allow us to have a per-hostbridge type if needed in the future.

[...]

>>
>> This is also exported but not used. I would prefer if we only exposed when the first external user will be introduced.
> ZynqMP is the first user yet in this patch. More to come probably later on when we add other host bridges.

Ah, sorry I didn't spot the use.

Cheers,

-- 
Julien Grall

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

[Oleksandr Andrushchenko]

Hi, Julien!

On 28.10.21 14:13, Julien Grall wrote:
>
>
> On 25/10/2021 12:37, Oleksandr Andrushchenko wrote:
>> Hi, Julien!
>
> Hi Oleksandr,
>
>> On 13.10.21 19:17, Julien Grall wrote:
>>> On 08/10/2021 06:55, Oleksandr Andrushchenko wrote:
>>>> +    };
>>>>          naddr = dt_number_of_address(dev);
>>>>    @@ -1884,7 +1889,6 @@ 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 };
>>>>            res = dt_device_get_address(dev, i, &addr, &size);
>>>>            if ( res )
>>>>            {
>>>> @@ -1898,7 +1902,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;
>>>>    @@ -3056,7 +3060,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, p2m_mmio_direct_c);
>>>
>>> It is not clear to me why you are passing p2m_mmio_direct_c and not p2mt here?
>> There is no p2mt in the caller function, e.g. construct_dom0
>>> If you really want to force a type, then I think it should be p2m_mmio_direct.
>> I just followed the defaults found in:
>> static int __init prepare_dtb_hwdom(struct domain *d, struct kernel_info *kinfo)
>> {
>>       const p2m_type_t default_p2mt = p2m_mmio_direct_c;
>> which is also called from construct_dom0
>
> We use "p2m_mmio_direct_c" (cacheable mapping) by default because we don't know what how the region will be accessed  (e.g. this may be an SRAM). With this type, there is no restriction and dom0 is responsible to set to proper attribute in the stage-1 page-tables.
>
> In this hostbridge cases, I see limited reasons at the moment for someone to map the non-BAR regions with cacheable attributes. So I think it is better to chose the most restricting attribute in the stage-2 page-table.
>
>>>
>>> But then why is it a parameter of pci_host_bridge_mappings? Do you expect someone else to modify it?
>> No, I don't expect to modify that, I just don't want PCI code to make decisions on that.
>> So, I feel more comfortable if that decision is taken in construct_dom0.
> Only the hostbridge driver is really aware of how the region will be accessed. So I think it is better to let...
>
>>
>> So, what do we want here? Pass as parameter (then p2m_mmio_direct I guess, not p2m_mmio_direct_c)?
>> Or let PCI code use p2m_mmio_direct inside pci_host_bridge_mappings?
>
> ... pci_host_bridge_mappings() decide the attribute. This can potentially allow us to have a per-hostbridge type if needed in the future.
It is all clear now, thank you!
>
> [...]
>
>>>
>>> This is also exported but not used. I would prefer if we only exposed when the first external user will be introduced.
>> ZynqMP is the first user yet in this patch. More to come probably later on when we add other host bridges.
>
> Ah, sorry I didn't spot the use.
>
> Cheers,
>
Thank you,
Oleksandr

Re: [PATCH v5 07/10] libxl: Only map legacy PCI IRQs if they are supported

[Jan Beulich]

On 25.10.2021 15:38, Oleksandr Andrushchenko wrote:
> On 25.10.21 16:22, Roger Pau Monné wrote:
>> On Fri, Oct 08, 2021 at 08:55:32AM +0300, Oleksandr Andrushchenko wrote:
>>> --- a/tools/libs/light/libxl_pci.c
>>> +++ b/tools/libs/light/libxl_pci.c
>>> @@ -1364,6 +1364,15 @@ static void pci_add_timeout(libxl__egc *egc, libxl__ev_time *ev,
>>>       pci_add_dm_done(egc, pas, rc);
>>>   }
>>>   
>>> +static bool pci_supp_legacy_irq(void)
>> A naming more inline with the PCI specification (since we insisted on
>> using ECAM instead of MCFG) would be intx, ie: pci_supports_intx.
> This will require a follow up patch if we all agree this change is needed.
> @Jan, are you ok with the rename?

Sure, but my view doesn't matter all this much anyway for libxl code.

Jan

Re: [PATCH v5 07/10] libxl: Only map legacy PCI IRQs if they are supported

[Oleksandr Andrushchenko]

On 02.11.21 13:32, Jan Beulich wrote:
> On 25.10.2021 15:38, Oleksandr Andrushchenko wrote:
>> On 25.10.21 16:22, Roger Pau Monné wrote:
>>> On Fri, Oct 08, 2021 at 08:55:32AM +0300, Oleksandr Andrushchenko wrote:
>>>> --- a/tools/libs/light/libxl_pci.c
>>>> +++ b/tools/libs/light/libxl_pci.c
>>>> @@ -1364,6 +1364,15 @@ static void pci_add_timeout(libxl__egc *egc, libxl__ev_time *ev,
>>>>        pci_add_dm_done(egc, pas, rc);
>>>>    }
>>>>    
>>>> +static bool pci_supp_legacy_irq(void)
>>> A naming more inline with the PCI specification (since we insisted on
>>> using ECAM instead of MCFG) would be intx, ie: pci_supports_intx.
>> This will require a follow up patch if we all agree this change is needed.
>> @Jan, are you ok with the rename?
> Sure, but my view doesn't matter all this much anyway for libxl code.
Do you want a follow-up patch for this as the original change has been merged?
>
> Jan
>
Thanks,
Oleksandr