[PATCH v8 0/5] PCI devices passthrough on Arm

[PATCH v8 0/5] PCI devices passthrough on Arm

[Bertrand Marquis]

Hello All,

This serie is a follow up on Rahul serie where we included various fixes
required after review on the mailing list and a new patch to move some
of the x86 ecam related code to the common vpci code.

Most of the patches of the original serie have been merged and this
serie includes only 2 of the original patches reworked and 3 new patches:

Move some ECAM related functions from x86 to generic vpci
implementation:
- move vcpi mmcfg_{read/write} and vpci_access_allowed to common vpci.c.
- use ecam instead of mmcfg in common code.

Enable the existing x86 virtual PCI support for ARM:
- Add VPCI trap handler for each of the PCI device added for config
  space access.
- Register the trap handler in XEN for each of the host bridge PCI ECAM
  config space access.

Modify libxl function to take the whole domain config as argument:
- libxl__arch_domain_init_hw_description
- libxl__prepare_dt

Emulated PCI device tree node in libxl:
- Create a virtual PCI device tree node in libxl to enable the guest OS
  to discover the virtual PCI during guest boot.

The patch modifying xc_domain_ioport_permission has been removed from
the serie.

Bertrand Marquis (1):
  xen/vpci: Move ecam access functions to common code

Michal Orzel (2):
  tools/libxl: Modify libxl__arch_domain_init_hw_description...
  tools/libxl_arm: Modify libxl__prepare_dtb...

Rahul Singh (2):
  xen/arm: Enable the existing x86 virtual PCI support for ARM
  arm/libxl: Emulated PCI device tree node in libxl

 tools/libs/light/libxl_arch.h   |   2 +-
 tools/libs/light/libxl_arm.c    | 111 +++++++++++++++++++++++++++++++-
 tools/libs/light/libxl_create.c |   5 ++
 tools/libs/light/libxl_dom.c    |   2 +-
 tools/libs/light/libxl_x86.c    |   2 +-
 xen/arch/arm/Makefile           |   1 +
 xen/arch/arm/domain.c           |   4 ++
 xen/arch/arm/vpci.c             |  77 ++++++++++++++++++++++
 xen/arch/arm/vpci.h             |  36 +++++++++++
 xen/arch/x86/hvm/io.c           |  46 ++-----------
 xen/drivers/passthrough/pci.c   |  13 ++++
 xen/drivers/vpci/header.c       |   2 +-
 xen/drivers/vpci/vpci.c         |  64 ++++++++++++++++++
 xen/include/asm-arm/domain.h    |   1 +
 xen/include/asm-x86/pci.h       |   2 -
 xen/include/public/arch-arm.h   |  17 +++++
 xen/include/xen/pci.h           |   2 +
 xen/include/xen/vpci.h          |  12 ++++
 18 files changed, 349 insertions(+), 50 deletions(-)
 create mode 100644 xen/arch/arm/vpci.c
 create mode 100644 xen/arch/arm/vpci.h

-- 
2.25.1

[PATCH v8 1/5] xen/vpci: Move ecam access functions to common code

[Bertrand Marquis]

PCI standard is using ECAM and not MCFG which is coming from ACPI[1].
Use ECAM/ecam instead of MCFG in common code and in new functions added
in common vpci code by this patch.

Move vpci_access_allowed from arch/x86/hvm/io.c to drivers/vpci/vpci.c.

Create vpci_ecam_{read,write} in drivers/vpci/vpci.c that
contains the common code to perform these operations, changed
vpci_mmcfg_{read,write} accordingly to make use of these functions.

The vpci_ecam_{read,write} functions are returning false on error and
true on success. As the x86 code was previously always returning
X86EMUL_OKAY the return code is ignored. A comment has been added in
the code to show that this is intentional.

Those functions will be used in a following patch inside by arm vpci
implementation.

Rename MMCFG_BDF to VPCI_ECAM_BDF and move it to vpci.h.
This macro is only used by functions calling vpci_ecam helpers.

No functional change intended with this patch.

[1] https://wiki.osdev.org/PCI_Express

Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes in v8:
- move comment in vpci.h for ecam
- fix indent of vpci_ecam declarations in vpci.h
- fix code typo: VCPI instead of VPCI
Changes in v7:
- Rename vpci_ecam_access_allowed to vpci_access_allowed
- Rename vpci_ecam_mmio_{read/write} to vpci_ecam_{read/write}
- Replace comment in x86/hvm/io.c with one suggested by Roger
- Remove 32bit comments and fixes from this patch and move it to the next
one to keep only the moving+renaming in this patch
- Change return type of vpci_ecam_{read/write} to bool
- rename ECAM_BDF to VPCI_ECAM_BDF and move it to vpci.h
Changes in v6: Patch added
---
 xen/arch/x86/hvm/io.c     | 46 ++++-----------------------------
 xen/drivers/vpci/vpci.c   | 54 +++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/pci.h |  2 --
 xen/include/xen/vpci.h    | 12 +++++++++
 4 files changed, 71 insertions(+), 43 deletions(-)

diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 046a8eb4ed..93f1d1503f 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -260,20 +260,6 @@ unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
     return CF8_ADDR_LO(cf8) | (addr & 3);
 }
 
-/* Do some sanity checks. */
-static bool vpci_access_allowed(unsigned int reg, unsigned int len)
-{
-    /* Check access size. */
-    if ( len != 1 && len != 2 && len != 4 && len != 8 )
-        return false;
-
-    /* Check that access is size aligned. */
-    if ( (reg & (len - 1)) )
-        return false;
-
-    return true;
-}
-
 /* vPCI config space IO ports handlers (0xcf8/0xcfc). */
 static bool vpci_portio_accept(const struct hvm_io_handler *handler,
                                const ioreq_t *p)
@@ -394,7 +380,7 @@ static unsigned int vpci_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
                                            paddr_t addr, pci_sbdf_t *sbdf)
 {
     addr -= mmcfg->addr;
-    sbdf->bdf = MMCFG_BDF(addr);
+    sbdf->bdf = VPCI_ECAM_BDF(addr);
     sbdf->bus += mmcfg->start_bus;
     sbdf->seg = mmcfg->segment;
 
@@ -434,25 +420,8 @@ static int vpci_mmcfg_read(struct vcpu *v, unsigned long addr,
     reg = vpci_mmcfg_decode_addr(mmcfg, addr, &sbdf);
     read_unlock(&d->arch.hvm.mmcfg_lock);
 
-    if ( !vpci_access_allowed(reg, len) ||
-         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
-        return X86EMUL_OKAY;
-
-    /*
-     * According to the PCIe 3.1A specification:
-     *  - Configuration Reads and Writes must usually be DWORD or smaller
-     *    in size.
-     *  - Because Root Complex implementations are not required to support
-     *    accesses to a RCRB that cross DW boundaries [...] software
-     *    should take care not to cause the generation of such accesses
-     *    when accessing a RCRB unless the Root Complex will support the
-     *    access.
-     *  Xen however supports 8byte accesses by splitting them into two
-     *  4byte accesses.
-     */
-    *data = vpci_read(sbdf, reg, min(4u, len));
-    if ( len == 8 )
-        *data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
+    /* Failed reads are not propagated to the caller */
+    vpci_ecam_read(sbdf, reg, len, data);
 
     return X86EMUL_OKAY;
 }
@@ -476,13 +445,8 @@ static int vpci_mmcfg_write(struct vcpu *v, unsigned long addr,
     reg = vpci_mmcfg_decode_addr(mmcfg, addr, &sbdf);
     read_unlock(&d->arch.hvm.mmcfg_lock);
 
-    if ( !vpci_access_allowed(reg, len) ||
-         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
-        return X86EMUL_OKAY;
-
-    vpci_write(sbdf, reg, min(4u, len), data);
-    if ( len == 8 )
-        vpci_write(sbdf, reg + 4, 4, data >> 32);
+    /* Failed writes are not propagated to the caller */
+    vpci_ecam_write(sbdf, reg, len, data);
 
     return X86EMUL_OKAY;
 }
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index cbd1bac7fc..ef690f15a9 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -478,6 +478,60 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
     spin_unlock(&pdev->vpci->lock);
 }
 
+/* Helper function to check an access size and alignment on vpci space. */
+bool vpci_access_allowed(unsigned int reg, unsigned int len)
+{
+    /* Check access size. */
+    if ( len != 1 && len != 2 && len != 4 && len != 8 )
+        return false;
+
+    /* Check that access is size aligned. */
+    if ( (reg & (len - 1)) )
+        return false;
+
+    return true;
+}
+
+bool vpci_ecam_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
+                         unsigned long data)
+{
+    if ( !vpci_access_allowed(reg, len) ||
+         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
+        return false;
+
+    vpci_write(sbdf, reg, min(4u, len), data);
+    if ( len == 8 )
+        vpci_write(sbdf, reg + 4, 4, data >> 32);
+
+    return true;
+}
+
+bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
+                        unsigned long *data)
+{
+    if ( !vpci_access_allowed(reg, len) ||
+         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
+        return false;
+
+    /*
+     * According to the PCIe 3.1A specification:
+     *  - Configuration Reads and Writes must usually be DWORD or smaller
+     *    in size.
+     *  - Because Root Complex implementations are not required to support
+     *    accesses to a RCRB that cross DW boundaries [...] software
+     *    should take care not to cause the generation of such accesses
+     *    when accessing a RCRB unless the Root Complex will support the
+     *    access.
+     *  Xen however supports 8byte accesses by splitting them into two
+     *  4byte accesses.
+     */
+    *data = vpci_read(sbdf, reg, min(4u, len));
+    if ( len == 8 )
+        *data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
+
+    return true;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/pci.h b/xen/include/asm-x86/pci.h
index edd7c3e71a..443f25347d 100644
--- a/xen/include/asm-x86/pci.h
+++ b/xen/include/asm-x86/pci.h
@@ -6,8 +6,6 @@
 #define CF8_ADDR_HI(cf8) (  ((cf8) & 0x0f000000) >> 16)
 #define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
 
-#define MMCFG_BDF(addr)  ( ((addr) & 0x0ffff000) >> 12)
-
 #define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \
                         || id == 0x01268086 || id == 0x01028086 \
                         || id == 0x01128086 || id == 0x01228086 \
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 9f5b5d52e1..6746c2589a 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -19,6 +19,8 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
 #define VPCI_PRIORITY_MIDDLE    "5"
 #define VPCI_PRIORITY_LOW       "9"
 
+#define VPCI_ECAM_BDF(addr)     (((addr) & 0x0ffff000) >> 12)
+
 #define REGISTER_VPCI_INIT(x, p)                \
   static vpci_register_init_t *const x##_entry  \
                __used_section(".data.vpci." p) = x
@@ -208,6 +210,16 @@ static inline unsigned int vmsix_entry_nr(const struct vpci_msix *msix,
 {
     return entry - msix->entries;
 }
+
+bool vpci_access_allowed(unsigned int reg, unsigned int len);
+
+/* ECAM mmio read/write helpers */
+bool vpci_ecam_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
+                     unsigned long data);
+
+bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
+                    unsigned long *data);
+
 #endif /* __XEN__ */
 
 #else /* !CONFIG_HAS_VPCI */
-- 
2.25.1

[PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Bertrand Marquis]

From: Rahul Singh <rahul.singh@arm.com>

The existing VPCI support available for X86 is adapted for Arm.
When the device is added to XEN via the hyper call
“PHYSDEVOP_pci_device_add”, VPCI handler for the config space
access is added to the Xen to emulate the PCI devices config space.

A MMIO trap handler for the PCI ECAM space is registered in XEN
so that when guest is trying to access the PCI config space,XEN
will trap the access and emulate read/write using the VPCI and
not the real PCI hardware.

For Dom0less systems scan_pci_devices() would be used to discover the
PCI device in XEN and VPCI handler will be added during XEN boots.

This patch is also doing some small fixes to fix compilation errors on
arm32 of vpci and prevent 64bit accesses on 32bit:
- use %zu instead of lu in header.c for print
- prevent 64bit accesses in vpci_access_allowed
- ifdef out using CONFIG_64BIT handling of len 8 in
vpci_ecam_{read/write}

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
Changes in v8:
- move pci_add_handlers call to right place so that it is called only
once and cleanup pdev->domain on error.
- fix code typo: VCPI instead of VPCI
Changes in v7:
- adapt to changes in vpci generic functions (name and type)
- remove call to pci_cleanup_msi on error exit
- move pci_add_handlers to be only done when pdev->domain is not NULL
- Remove cast to unsigned long in header.c and use %zu for print
- Fix non ascii chars in arch-arm.h
- Use CONFIG_64BIT inside vpci_access_allowed to prevent 64bit access on
32bit platforms
- Use CONFIG_64BIT to compile out 64bit cases in vpci_ecam_{read/write}
Changes in v6:
- Use new vpci_ecam_ helpers for PCI access
- Do not set XEN_DOMCTL_CDF_vpci for dom0 for now (will be done in a
future patch once everything is ready)
- rename REGISTER_OFFSET to ECAM_REG_OFFSET and move it to pci.h
- remove not needed local variables in vpci_mmio_write, the one in read
has been kept to ensure proper compilation on arm32
- move call to vpci_add_handlers before iommu init to simplify exit path
- move call to pci_cleanup_msi in the out section of pci_add_device if
pdev is not NULL and on error
- initialize pdev to NULL to handle properly exit path call of
pci_cleanup_msi
- keep has_vpci to return false for now as CFG_vpci has been removed.
Added a comment on top of the definition.
- fix compilation errors on arm32 (print in header.c, cast missing in
mmio_write.
- local variable was kept in vpci_mmio_read on arm to prevent a cast
error in arm32.
Change in v5:
- Add pci_cleanup_msi(pdev) incleanup part.
- Added Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Change in v4:
- Move addition of XEN_DOMCTL_CDF_vpci flag to separate patch
Change in v3:
- Use is_pci_passthrough_enabled() in place of pci_passthrough_enabled variable
- Reject XEN_DOMCTL_CDF_vpci for x86 in arch_sanitise_domain_config()
- Remove IS_ENABLED(CONFIG_HAS_VPCI) from has_vpci()
Change in v2:
- Add new XEN_DOMCTL_CDF_vpci flag
- modify has_vpci() to include XEN_DOMCTL_CDF_vpci
- enable vpci support when pci-passthough option is enabled.
---
---
 xen/arch/arm/Makefile         |  1 +
 xen/arch/arm/domain.c         |  4 ++
 xen/arch/arm/vpci.c           | 77 +++++++++++++++++++++++++++++++++++
 xen/arch/arm/vpci.h           | 36 ++++++++++++++++
 xen/drivers/passthrough/pci.c | 13 ++++++
 xen/drivers/vpci/header.c     |  2 +-
 xen/drivers/vpci/vpci.c       | 10 +++++
 xen/include/asm-arm/domain.h  |  1 +
 xen/include/public/arch-arm.h |  7 ++++
 xen/include/xen/pci.h         |  2 +
 10 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/vpci.c
 create mode 100644 xen/arch/arm/vpci.h

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 64518848b2..07f634508e 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -7,6 +7,7 @@ ifneq ($(CONFIG_NO_PLAT),y)
 obj-y += platforms/
 endif
 obj-$(CONFIG_TEE) += tee/
+obj-$(CONFIG_HAS_VPCI) += vpci.o
 
 obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
 obj-y += bootfdt.init.o
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index eef0661beb..96e1b23550 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -39,6 +39,7 @@
 #include <asm/vgic.h>
 #include <asm/vtimer.h>
 
+#include "vpci.h"
 #include "vuart.h"
 
 DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
@@ -773,6 +774,9 @@ int arch_domain_create(struct domain *d,
     if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
         goto fail;
 
+    if ( (rc = domain_vpci_init(d)) != 0 )
+        goto fail;
+
     return 0;
 
 fail:
diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
new file mode 100644
index 0000000000..8f40a0dec6
--- /dev/null
+++ b/xen/arch/arm/vpci.c
@@ -0,0 +1,77 @@
+/*
+ * xen/arch/arm/vpci.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <xen/sched.h>
+#include <xen/vpci.h>
+
+#include <asm/mmio.h>
+
+static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
+                          register_t *r, void *p)
+{
+    pci_sbdf_t sbdf;
+    /* data is needed to prevent a pointer cast on 32bit */
+    unsigned long data;
+
+    /* We ignore segment part and always handle segment 0 */
+    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
+
+    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
+                        1U << info->dabt.size, &data) )
+    {
+        *r = data;
+        return 1;
+    }
+
+    *r = ~0ul;
+
+    return 0;
+}
+
+static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
+                           register_t r, void *p)
+{
+    pci_sbdf_t sbdf;
+
+    /* We ignore segment part and always handle segment 0 */
+    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
+
+    return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa),
+                           1U << info->dabt.size, r);
+}
+
+static const struct mmio_handler_ops vpci_mmio_handler = {
+    .read  = vpci_mmio_read,
+    .write = vpci_mmio_write,
+};
+
+int domain_vpci_init(struct domain *d)
+{
+    if ( !has_vpci(d) )
+        return 0;
+
+    register_mmio_handler(d, &vpci_mmio_handler,
+                          GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
+
diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h
new file mode 100644
index 0000000000..d8a7b0e3e8
--- /dev/null
+++ b/xen/arch/arm/vpci.h
@@ -0,0 +1,36 @@
+/*
+ * xen/arch/arm/vpci.h
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __ARCH_ARM_VPCI_H__
+#define __ARCH_ARM_VPCI_H__
+
+#ifdef CONFIG_HAS_VPCI
+int domain_vpci_init(struct domain *d);
+#else
+static inline int domain_vpci_init(struct domain *d)
+{
+    return 0;
+}
+#endif
+
+#endif /* __ARCH_ARM_VPCI_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 3aa8c3175f..35e0190796 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
     if ( !pdev->domain )
     {
         pdev->domain = hardware_domain;
+#ifdef CONFIG_ARM
+        /*
+         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
+         * when Dom0 inform XEN to add the PCI devices in XEN.
+         */
+        ret = vpci_add_handlers(pdev);
+        if ( ret )
+        {
+            printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
+            pdev->domain = NULL;
+            goto out;
+        }
+#endif
         ret = iommu_add_device(pdev);
         if ( ret )
         {
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index f8cd55e7c0..40ff79c33f 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -373,7 +373,7 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg,
         /* If the value written is the current one avoid printing a warning. */
         if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
             gprintk(XENLOG_WARNING,
-                    "%pp: ignored BAR %lu write with memory decoding enabled\n",
+                    "%pp: ignored BAR %zu write with memory decoding enabled\n",
                     &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
         return;
     }
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index ef690f15a9..decf7d87a1 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -485,6 +485,12 @@ bool vpci_access_allowed(unsigned int reg, unsigned int len)
     if ( len != 1 && len != 2 && len != 4 && len != 8 )
         return false;
 
+#ifndef CONFIG_64BIT
+    /* Prevent 64bit accesses on 32bit */
+    if ( len == 8 )
+        return false;
+#endif
+
     /* Check that access is size aligned. */
     if ( (reg & (len - 1)) )
         return false;
@@ -500,8 +506,10 @@ bool vpci_ecam_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
         return false;
 
     vpci_write(sbdf, reg, min(4u, len), data);
+#ifdef CONFIG_64BIT
     if ( len == 8 )
         vpci_write(sbdf, reg + 4, 4, data >> 32);
+#endif
 
     return true;
 }
@@ -526,8 +534,10 @@ bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
      *  4byte accesses.
      */
     *data = vpci_read(sbdf, reg, min(4u, len));
+#ifdef CONFIG_64BIT
     if ( len == 8 )
         *data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
+#endif
 
     return true;
 }
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 14e575288f..9b3647587a 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -263,6 +263,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
 
 #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag)
 
+/* vPCI is not available on Arm */
 #define has_vpci(d)    ({ (void)(d); false; })
 
 #endif /* __ASM_DOMAIN_H__ */
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 96ead3de07..b4c615bcdf 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -418,6 +418,13 @@ typedef uint64_t xen_callback_t;
 #define GUEST_GICV3_GICR0_BASE     xen_mk_ullong(0x03020000) /* vCPU0..127 */
 #define GUEST_GICV3_GICR0_SIZE     xen_mk_ullong(0x01000000)
 
+/*
+ * 256 MB is reserved for VPCI configuration space based on calculation
+ * 256 buses x 32 devices x 8 functions x 4 KB = 256 MB
+ */
+#define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)
+#define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)
+
 /* ACPI tables physical address */
 #define GUEST_ACPI_BASE xen_mk_ullong(0x20000000)
 #define GUEST_ACPI_SIZE xen_mk_ullong(0x02000000)
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 70ac25345c..b6d7e454f8 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -40,6 +40,8 @@
 #define PCI_SBDF3(s,b,df) \
     ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) })
 
+#define ECAM_REG_OFFSET(addr)  ((addr) & 0x00000fff)
+
 typedef union {
     uint32_t sbdf;
     struct {
-- 
2.25.1

[PATCH v8 3/5] tools/libxl: Modify libxl__arch_domain_init_hw_description...

[Bertrand Marquis]

From: Michal Orzel <michal.orzel@arm.com>

... to take a second argument of type libxl_domain_config*
rather than libxl_domain_build_info*.

We need to pass the whole libxl_domain_config
structure as this will be needed later on to modify
the libxl__prepare_dtb function to also take
libxl_domain_config.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Suggested-by: Ian Jackson <iwj@xenproject.org>
Reviewed-by: Ian Jackson <iwj@xenproject.org>
---
Changes in v8: add Signed-off Bertrand Marquis
Changes in v7: Patch added
---
 tools/libs/light/libxl_arch.h | 2 +-
 tools/libs/light/libxl_arm.c  | 3 ++-
 tools/libs/light/libxl_dom.c  | 2 +-
 tools/libs/light/libxl_x86.c  | 2 +-
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
index 8527fc5c6c..1522ecb97f 100644
--- a/tools/libs/light/libxl_arch.h
+++ b/tools/libs/light/libxl_arch.h
@@ -38,7 +38,7 @@ int libxl__arch_domain_create(libxl__gc *gc,
 /* setup arch specific hardware description, i.e. DTB on ARM */
 _hidden
 int libxl__arch_domain_init_hw_description(libxl__gc *gc,
-                                           libxl_domain_build_info *info,
+                                           libxl_domain_config *d_config,
                                            libxl__domain_build_state *state,
                                            struct xc_dom_image *dom);
 /* finalize arch specific hardware description. */
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index a7801558cf..d5771f98dd 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -1016,12 +1016,13 @@ out:
 }
 
 int libxl__arch_domain_init_hw_description(libxl__gc *gc,
-                                           libxl_domain_build_info *info,
+                                           libxl_domain_config *d_config,
                                            libxl__domain_build_state *state,
                                            struct xc_dom_image *dom)
 {
     int rc;
     uint64_t val;
+    libxl_domain_build_info *const info = &d_config->b_info;
 
     if (info->type != LIBXL_DOMAIN_TYPE_PVH) {
         LOG(ERROR, "Unsupported Arm guest type %s",
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index e9f58ee4b2..fe9f760f71 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -571,7 +571,7 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
         LOG(ERROR, "xc_dom_parse_image failed");
         goto out;
     }
-    if ( (ret = libxl__arch_domain_init_hw_description(gc, info, state, dom)) != 0 ) {
+    if ( (ret = libxl__arch_domain_init_hw_description(gc, d_config, state, dom)) != 0 ) {
         LOGE(ERROR, "libxl__arch_domain_init_hw_description failed");
         goto out;
     }
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 6083878315..1feadebb18 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -568,7 +568,7 @@ int libxl__arch_extra_memory(libxl__gc *gc,
 }
 
 int libxl__arch_domain_init_hw_description(libxl__gc *gc,
-                                           libxl_domain_build_info *info,
+                                           libxl_domain_config *d_config,
                                            libxl__domain_build_state *state,
                                            struct xc_dom_image *dom)
 {
-- 
2.25.1

[PATCH v8 4/5] tools/libxl_arm: Modify libxl__prepare_dtb...

[Bertrand Marquis]

From: Michal Orzel <michal.orzel@arm.com>

... to take a second argument of type libxl_domain_config*
rather than libxl_domain_build_info*.

This change will be needed to get access from
libxl__prepare_dtb to "num_pcidevs" field of
libxl_domain_config to check whether to create
a vPCI DT node or not.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Suggested-by: Ian Jackson <iwj@xenproject.org>
Reviewed-by: Ian Jackson <iwj@xenproject.org>
---
Changes in v8: add Signed-off Bertrand Marquis
Changes in v7: Patch added
---
 tools/libs/light/libxl_arm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index d5771f98dd..fdae129605 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -878,7 +878,7 @@ static int copy_partial_fdt(libxl__gc *gc, void *fdt, void *pfdt)
 
 #define FDT_MAX_SIZE (1<<20)
 
-static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info,
+static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config,
                               libxl__domain_build_state *state,
                               struct xc_dom_image *dom)
 {
@@ -887,6 +887,7 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info,
     int rc, res;
     size_t fdt_size = 0;
     int pfdt_size = 0;
+    libxl_domain_build_info *const info = &d_config->b_info;
 
     const libxl_version_info *vers;
     const struct arch_info *ainfo;
@@ -1042,7 +1043,7 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
     if (rc)
         return rc;
 
-    rc = libxl__prepare_dtb(gc, info, state, dom);
+    rc = libxl__prepare_dtb(gc, d_config, state, dom);
     if (rc) goto out;
 
     if (!libxl_defbool_val(info->acpi)) {
-- 
2.25.1

[PATCH v8 5/5] arm/libxl: Emulated PCI device tree node in libxl

[Bertrand Marquis]

From: Rahul Singh <rahul.singh@arm.com>

libxl will create an emulated PCI device tree node in the device tree to
enable the guest OS to discover the virtual PCI during guest boot.
Emulated PCI device tree node will only be created when there is any
device assigned to guest.

A new area has been reserved in the arm guest physical map at
which the VPCI bus is declared in the device tree (reg and ranges
parameters of the node).

Note that currently we are using num_pcidevs instead of
c_info->passthrough to decide whether to create a vPCI DT node.
This will be insufficient if and when ARM does PCI hotplug.
Add this note inside libxl_create.c where c_info->passthrough
is set.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Acked-by: Ian Jackson <iwj@xenproject.org>
---
Changes in v8: add Signed-off Bertrand Marquis
Changes in v7:
-remove arch_arm.vpci
-create vpci DT note if (d_config->num_pcidevs)
Changes in v6:
According to https://marc.info/?l=xen-devel&m=163415838129479&w=2:
-do not set XEN_DOMCTL_CDF_vpci
-do not enable VPCI support (by setting b_info.arch_arm.vpci)
-do not define LIBXL_HAVE_BUILDINFO_ARM_VPCI
-keep b_info.arch_arm.vpci, make_vpci_node and its helpers
Change in v5:
- Move setting the arch_arm.vpci and XEN_DOMCTL_CDF_vpci to libxl_arm.c
Change in v4:
- Gate code for x86 for setting the XEN_DOMCTL_CDF_vpci for x86.
Change in v3:
- Make GUEST_VPCI_MEM_ADDR address 2MB aligned
Change in v2:
- enable doamin_vpci_init() when XEN_DOMCTL_CDF_vpci is set for domain.
---
---
 tools/libs/light/libxl_arm.c    | 103 ++++++++++++++++++++++++++++++++
 tools/libs/light/libxl_create.c |   5 ++
 xen/include/public/arch-arm.h   |  10 ++++
 3 files changed, 118 insertions(+)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index fdae129605..eef1de0939 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -284,6 +284,59 @@ static int fdt_property_reg_placeholder(libxl__gc *gc, void *fdt,
     return fdt_property(fdt, "reg", regs, sizeof(regs));
 }
 
+static int fdt_property_values(libxl__gc *gc, void *fdt,
+                               const char *name,
+                               unsigned num_cells, ...)
+{
+    uint32_t prop[num_cells];
+    be32 *cells = &prop[0];
+    int i;
+    va_list ap;
+    uint32_t arg;
+
+    va_start(ap, num_cells);
+    for (i = 0 ; i < num_cells; i++) {
+        arg = va_arg(ap, uint32_t);
+        set_cell(&cells, 1, arg);
+    }
+    va_end(ap);
+
+    return fdt_property(fdt, name, prop, sizeof(prop));
+}
+
+static int fdt_property_vpci_ranges(libxl__gc *gc, void *fdt,
+                                    unsigned addr_cells,
+                                    unsigned size_cells,
+                                    unsigned num_regs, ...)
+{
+    uint32_t regs[num_regs*((addr_cells*2)+size_cells+1)];
+    be32 *cells = &regs[0];
+    int i;
+    va_list ap;
+    uint64_t arg;
+
+    va_start(ap, num_regs);
+    for (i = 0 ; i < num_regs; i++) {
+        /* Set the memory bit field */
+        arg = va_arg(ap, uint32_t);
+        set_cell(&cells, 1, arg);
+
+        /* Set the vpci bus address */
+        arg = addr_cells ? va_arg(ap, uint64_t) : 0;
+        set_cell(&cells, addr_cells , arg);
+
+        /* Set the cpu bus address where vpci address is mapped */
+        set_cell(&cells, addr_cells, arg);
+
+        /* Set the vpci size requested */
+        arg = size_cells ? va_arg(ap, uint64_t) : 0;
+        set_cell(&cells, size_cells, arg);
+    }
+    va_end(ap);
+
+    return fdt_property(fdt, "ranges", regs, sizeof(regs));
+}
+
 static int make_root_properties(libxl__gc *gc,
                                 const libxl_version_info *vers,
                                 void *fdt)
@@ -687,6 +740,53 @@ static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
+static int make_vpci_node(libxl__gc *gc, void *fdt,
+                          const struct arch_info *ainfo,
+                          struct xc_dom_image *dom)
+{
+    int res;
+    const uint64_t vpci_ecam_base = GUEST_VPCI_ECAM_BASE;
+    const uint64_t vpci_ecam_size = GUEST_VPCI_ECAM_SIZE;
+    const char *name = GCSPRINTF("pcie@%"PRIx64, vpci_ecam_base);
+
+    res = fdt_begin_node(fdt, name);
+    if (res) return res;
+
+    res = fdt_property_compat(gc, fdt, 1, "pci-host-ecam-generic");
+    if (res) return res;
+
+    res = fdt_property_string(fdt, "device_type", "pci");
+    if (res) return res;
+
+    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
+            GUEST_ROOT_SIZE_CELLS, 1, vpci_ecam_base, vpci_ecam_size);
+    if (res) return res;
+
+    res = fdt_property_values(gc, fdt, "bus-range", 2, 0, 255);
+    if (res) return res;
+
+    res = fdt_property_cell(fdt, "#address-cells", 3);
+    if (res) return res;
+
+    res = fdt_property_cell(fdt, "#size-cells", 2);
+    if (res) return res;
+
+    res = fdt_property_string(fdt, "status", "okay");
+    if (res) return res;
+
+    res = fdt_property_vpci_ranges(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
+        GUEST_ROOT_SIZE_CELLS, 2,
+        GUEST_VPCI_ADDR_TYPE_MEM, GUEST_VPCI_MEM_ADDR, GUEST_VPCI_MEM_SIZE,
+        GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM, GUEST_VPCI_PREFETCH_MEM_ADDR,
+        GUEST_VPCI_PREFETCH_MEM_SIZE);
+    if (res) return res;
+
+    res = fdt_end_node(fdt);
+    if (res) return res;
+
+    return 0;
+}
+
 static const struct arch_info *get_arch_info(libxl__gc *gc,
                                              const struct xc_dom_image *dom)
 {
@@ -991,6 +1091,9 @@ next_resize:
         if (info->tee == LIBXL_TEE_TYPE_OPTEE)
             FDT( make_optee_node(gc, fdt) );
 
+        if (d_config->num_pcidevs)
+            FDT( make_vpci_node(gc, fdt, ainfo, dom) );
+
         if (pfdt)
             FDT( copy_partial_fdt(gc, fdt, pfdt) );
 
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 6ebb2bfc76..5a61d01722 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1096,6 +1096,11 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
         goto error_out;
     }
 
+    /*
+     * Note: libxl_arm directly examines num_pcidevs to decide whether
+     * to create a vPCI DT node, rather than using c_info->passthrough.
+     * This will be insufficient if and when ARM does PCI hotplug.
+     */
     bool need_pt = d_config->num_pcidevs || d_config->num_dtdevs;
     if (c_info->passthrough == LIBXL_PASSTHROUGH_DEFAULT) {
         c_info->passthrough = need_pt
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index b4c615bcdf..94b31511dd 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -433,6 +433,11 @@ typedef uint64_t xen_callback_t;
 #define GUEST_PL011_BASE    xen_mk_ullong(0x22000000)
 #define GUEST_PL011_SIZE    xen_mk_ullong(0x00001000)
 
+/* Guest PCI-PCIe memory space where config space and BAR will be available.*/
+#define GUEST_VPCI_ADDR_TYPE_MEM            xen_mk_ullong(0x02000000)
+#define GUEST_VPCI_MEM_ADDR                 xen_mk_ullong(0x23000000)
+#define GUEST_VPCI_MEM_SIZE                 xen_mk_ullong(0x10000000)
+
 /*
  * 16MB == 4096 pages reserved for guest to use as a region to map its
  * grant table in.
@@ -453,6 +458,11 @@ typedef uint64_t xen_callback_t;
 #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */
 #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
 
+/* 4GB @ 4GB Prefetch Memory for VPCI */
+#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM   xen_mk_ullong(0x42000000)
+#define GUEST_VPCI_PREFETCH_MEM_ADDR        xen_mk_ullong(0x100000000)
+#define GUEST_VPCI_PREFETCH_MEM_SIZE        xen_mk_ullong(0x100000000)
+
 #define GUEST_RAM1_BASE   xen_mk_ullong(0x0200000000) /* 1016GB of RAM @ 8GB */
 #define GUEST_RAM1_SIZE   xen_mk_ullong(0xfe00000000)
 
-- 
2.25.1

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Julien Grall]

Hi Bertrand,

On 15/10/2021 17:51, Bertrand Marquis wrote:
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 3aa8c3175f..35e0190796 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>       if ( !pdev->domain )
>       {
>           pdev->domain = hardware_domain;
> +#ifdef CONFIG_ARM
> +        /*
> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
> +         * when Dom0 inform XEN to add the PCI devices in XEN.
> +         */
> +        ret = vpci_add_handlers(pdev);

I don't seem to find the code to remove __init_hwdom in this series. Are 
you intending to fix it separately?

With that addressed:

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

Cheers,


-- 
Julien Grall

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Bertrand Marquis]

Hi Julien,

> On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 15/10/2021 17:51, Bertrand Marquis wrote:
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 3aa8c3175f..35e0190796 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>      if ( !pdev->domain )
>>      {
>>          pdev->domain = hardware_domain;
>> +#ifdef CONFIG_ARM
>> +        /*
>> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
>> +         * when Dom0 inform XEN to add the PCI devices in XEN.
>> +         */
>> +        ret = vpci_add_handlers(pdev);
> 
> I don't seem to find the code to remove __init_hwdom in this series. Are you intending to fix it separately?

Yes I think it is better to fix that in a new patch as it will require some discussion as it will impact the x86 code if I just remove the flag now.


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

Thanks
Bertrand

> Cheers,
> 
> 
> -- 
> Julien Grall

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Julien Grall]

On 15/10/2021 18:33, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

> 
>> On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Bertrand,
>>
>> On 15/10/2021 17:51, Bertrand Marquis wrote:
>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>>> index 3aa8c3175f..35e0190796 100644
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>       if ( !pdev->domain )
>>>       {
>>>           pdev->domain = hardware_domain;
>>> +#ifdef CONFIG_ARM
>>> +        /*
>>> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
>>> +         * when Dom0 inform XEN to add the PCI devices in XEN.
>>> +         */
>>> +        ret = vpci_add_handlers(pdev);
>>
>> I don't seem to find the code to remove __init_hwdom in this series. Are you intending to fix it separately?
> 
> Yes I think it is better to fix that in a new patch as it will require some discussion as it will impact the x86 code if I just remove the flag now.
For the future patch series, may I ask to keep track of outstanding 
issues in the commit message (if you don't plan to address them before 
commiting) or after --- (if they are meant to be addressed before 
commiting)?

In this case, the impact on Arm is this would result to an hypervisor 
crash if called. If we drop __init_hwdom, the impact on x86 is Xen text 
will slightly be bigger after the boot time.

AFAICT, the code is not reachable on Arm (?). Therefore, one could argue 
we this can wait after the week-end as this is a latent bug. Yet, I am 
not really comfortable to see knowningly buggy code merged.

Stefano, would you be willing to remove __init_hwdom while committing 
it? If not, can you update the commit message and mention this patch 
doesn't work as intended?

Cheers,

-- 
Julien Grall

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Stefano Stabellini]

On Fri, 15 Oct 2021, Julien Grall wrote:
> On 15/10/2021 18:33, Bertrand Marquis wrote:
> > > On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote:
> > > 
> > > Hi Bertrand,
> > > 
> > > On 15/10/2021 17:51, Bertrand Marquis wrote:
> > > > diff --git a/xen/drivers/passthrough/pci.c
> > > > b/xen/drivers/passthrough/pci.c
> > > > index 3aa8c3175f..35e0190796 100644
> > > > --- a/xen/drivers/passthrough/pci.c
> > > > +++ b/xen/drivers/passthrough/pci.c
> > > > @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> > > >       if ( !pdev->domain )
> > > >       {
> > > >           pdev->domain = hardware_domain;
> > > > +#ifdef CONFIG_ARM
> > > > +        /*
> > > > +         * On ARM PCI devices discovery will be done by Dom0. Add vpci
> > > > handler
> > > > +         * when Dom0 inform XEN to add the PCI devices in XEN.
> > > > +         */
> > > > +        ret = vpci_add_handlers(pdev);
> > > 
> > > I don't seem to find the code to remove __init_hwdom in this series. Are
> > > you intending to fix it separately?
> > 
> > Yes I think it is better to fix that in a new patch as it will require some
> > discussion as it will impact the x86 code if I just remove the flag now.
> For the future patch series, may I ask to keep track of outstanding issues in
> the commit message (if you don't plan to address them before commiting) or
> after --- (if they are meant to be addressed before commiting)?
> 
> In this case, the impact on Arm is this would result to an hypervisor crash if
> called. If we drop __init_hwdom, the impact on x86 is Xen text will slightly
> be bigger after the boot time.
> 
> AFAICT, the code is not reachable on Arm (?). Therefore, one could argue we
> this can wait after the week-end as this is a latent bug. Yet, I am not really
> comfortable to see knowningly buggy code merged.
> 
> Stefano, would you be willing to remove __init_hwdom while committing it? If
> not, can you update the commit message and mention this patch doesn't work as
> intended?

I prefer not to do a change like this on commit as it affects x86.

I added a note in the commit message about it. I also added Roger's ack
that was given to the previous version. FYI this is the only outstanding
TODO as far as I am aware (there are other pending patch series of
course).

After reviewing the whole series again, checking it against all the
reviewers comments, and making it go through gitlab-ci, I committed the
series.

Thank you all for all the efforts that went into this. We made it :-)

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Roger Pau Monné]

On Fri, Oct 15, 2021 at 12:47:17PM -0700, Stefano Stabellini wrote:
> On Fri, 15 Oct 2021, Julien Grall wrote:
> > On 15/10/2021 18:33, Bertrand Marquis wrote:
> > > > On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote:
> > > > 
> > > > Hi Bertrand,
> > > > 
> > > > On 15/10/2021 17:51, Bertrand Marquis wrote:
> > > > > diff --git a/xen/drivers/passthrough/pci.c
> > > > > b/xen/drivers/passthrough/pci.c
> > > > > index 3aa8c3175f..35e0190796 100644
> > > > > --- a/xen/drivers/passthrough/pci.c
> > > > > +++ b/xen/drivers/passthrough/pci.c
> > > > > @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> > > > >       if ( !pdev->domain )
> > > > >       {
> > > > >           pdev->domain = hardware_domain;
> > > > > +#ifdef CONFIG_ARM
> > > > > +        /*
> > > > > +         * On ARM PCI devices discovery will be done by Dom0. Add vpci
> > > > > handler
> > > > > +         * when Dom0 inform XEN to add the PCI devices in XEN.
> > > > > +         */
> > > > > +        ret = vpci_add_handlers(pdev);
> > > > 
> > > > I don't seem to find the code to remove __init_hwdom in this series. Are
> > > > you intending to fix it separately?
> > > 
> > > Yes I think it is better to fix that in a new patch as it will require some
> > > discussion as it will impact the x86 code if I just remove the flag now.
> > For the future patch series, may I ask to keep track of outstanding issues in
> > the commit message (if you don't plan to address them before commiting) or
> > after --- (if they are meant to be addressed before commiting)?
> > 
> > In this case, the impact on Arm is this would result to an hypervisor crash if
> > called. If we drop __init_hwdom, the impact on x86 is Xen text will slightly
> > be bigger after the boot time.
> > 
> > AFAICT, the code is not reachable on Arm (?). Therefore, one could argue we
> > this can wait after the week-end as this is a latent bug. Yet, I am not really
> > comfortable to see knowningly buggy code merged.
> > 
> > Stefano, would you be willing to remove __init_hwdom while committing it? If
> > not, can you update the commit message and mention this patch doesn't work as
> > intended?
> 
> I prefer not to do a change like this on commit as it affects x86.
> 
> I added a note in the commit message about it. I also added Roger's ack
> that was given to the previous version. FYI this is the only outstanding
> TODO as far as I am aware (there are other pending patch series of
> course).
> 
> After reviewing the whole series again, checking it against all the
> reviewers comments, and making it go through gitlab-ci, I committed the
> series.

Hello,

Maybe I'm being pedantic, or there was some communication outside the
mailing list, but I think strictly speaking you are missing an Ack
from either Jan or Paul for the xen/drivers/passthrough/pci.c change.

IMHO seeing how that chunk moved from 3 different places in just one
afternoon also doesn't give me a lot of confidence. It's Arm only code
at the end, so it's not going to effect the existing x86 support and
I'm not specially worried, but I would like to avoid having to move it
again.

Regards, Roger.

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Jan Beulich]

On 16.10.2021 12:28, Roger Pau Monné wrote:
> On Fri, Oct 15, 2021 at 12:47:17PM -0700, Stefano Stabellini wrote:
>> On Fri, 15 Oct 2021, Julien Grall wrote:
>>> On 15/10/2021 18:33, Bertrand Marquis wrote:
>>>>> On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote:
>>>>>
>>>>> Hi Bertrand,
>>>>>
>>>>> On 15/10/2021 17:51, Bertrand Marquis wrote:
>>>>>> diff --git a/xen/drivers/passthrough/pci.c
>>>>>> b/xen/drivers/passthrough/pci.c
>>>>>> index 3aa8c3175f..35e0190796 100644
>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>>>>       if ( !pdev->domain )
>>>>>>       {
>>>>>>           pdev->domain = hardware_domain;
>>>>>> +#ifdef CONFIG_ARM
>>>>>> +        /*
>>>>>> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci
>>>>>> handler
>>>>>> +         * when Dom0 inform XEN to add the PCI devices in XEN.
>>>>>> +         */
>>>>>> +        ret = vpci_add_handlers(pdev);
>>>>>
>>>>> I don't seem to find the code to remove __init_hwdom in this series. Are
>>>>> you intending to fix it separately?
>>>>
>>>> Yes I think it is better to fix that in a new patch as it will require some
>>>> discussion as it will impact the x86 code if I just remove the flag now.
>>> For the future patch series, may I ask to keep track of outstanding issues in
>>> the commit message (if you don't plan to address them before commiting) or
>>> after --- (if they are meant to be addressed before commiting)?
>>>
>>> In this case, the impact on Arm is this would result to an hypervisor crash if
>>> called. If we drop __init_hwdom, the impact on x86 is Xen text will slightly
>>> be bigger after the boot time.
>>>
>>> AFAICT, the code is not reachable on Arm (?). Therefore, one could argue we
>>> this can wait after the week-end as this is a latent bug. Yet, I am not really
>>> comfortable to see knowningly buggy code merged.
>>>
>>> Stefano, would you be willing to remove __init_hwdom while committing it? If
>>> not, can you update the commit message and mention this patch doesn't work as
>>> intended?
>>
>> I prefer not to do a change like this on commit as it affects x86.
>>
>> I added a note in the commit message about it. I also added Roger's ack
>> that was given to the previous version. FYI this is the only outstanding
>> TODO as far as I am aware (there are other pending patch series of
>> course).
>>
>> After reviewing the whole series again, checking it against all the
>> reviewers comments, and making it go through gitlab-ci, I committed the
>> series.
> 
> Hello,
> 
> Maybe I'm being pedantic, or there was some communication outside the
> mailing list, but I think strictly speaking you are missing an Ack
> from either Jan or Paul for the xen/drivers/passthrough/pci.c change.
> 
> IMHO seeing how that chunk moved from 3 different places in just one
> afternoon also doesn't give me a lot of confidence. It's Arm only code
> at the end, so it's not going to effect the existing x86 support and
> I'm not specially worried, but I would like to avoid having to move it
> again.

+1

I'll be replying to the patch itself for the technical aspects. As per
context still visible above this code path is supposedly unreachable
right now, which makes me wonder even more: Why the rush? Depending on
the answer plus considering the __hwdom_init issue, Ian, I'm inclined
to suggest a revert.

Jan

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Jan Beulich]

On 15.10.2021 18:51, Bertrand Marquis wrote:
> --- /dev/null
> +++ b/xen/arch/arm/vpci.c
> @@ -0,0 +1,77 @@
> +/*
> + * xen/arch/arm/vpci.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <xen/sched.h>
> +#include <xen/vpci.h>
> +
> +#include <asm/mmio.h>
> +
> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> +                          register_t *r, void *p)
> +{
> +    pci_sbdf_t sbdf;
> +    /* data is needed to prevent a pointer cast on 32bit */
> +    unsigned long data;
> +
> +    /* We ignore segment part and always handle segment 0 */
> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
> +
> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
> +                        1U << info->dabt.size, &data) )
> +    {

Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
the virtual one. The function then calls vpci_read(), which in turn
will call vpci_read_hw() in a number of situations (first and foremost
whenever pci_get_pdev_by_domain() returns NULL). That function as well
as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
physical one; I'm unable to spot any translation. Yet I do recall
seeing assignment of a virtual device and function number somewhere
(perhaps another of the related series), so the model also doesn't
look to assume 1:1 mapping of SBDF.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      if ( !pdev->domain )
>      {
>          pdev->domain = hardware_domain;
> +#ifdef CONFIG_ARM
> +        /*
> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
> +         * when Dom0 inform XEN to add the PCI devices in XEN.
> +         */
> +        ret = vpci_add_handlers(pdev);
> +        if ( ret )
> +        {
> +            printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
> +            pdev->domain = NULL;
> +            goto out;
> +        }
> +#endif
>          ret = iommu_add_device(pdev);
>          if ( ret )
>          {

Upon failure, vpci_add_handlers() will itself call vpci_remove_handlers().
What about iommu_add_device() failure? The device will have ->domain
zapped, but all vPCI handlers still in place. This aspect of insufficient
error cleanup was pointed out already in review of v1.

Furthermore already in v1 I questioned why this would be Arm-specific: On
x86 this code path is going to be taken for all devices Xen wasn't able
to discover at boot (anything on segments we wouldn't consider config
space access safe on without reassurance by Dom0 plus SR-IOV VFs, at the
very least). Hence it is my understanding that something along these
lines is actually also needed for PVH Dom0. I've just checked, and
according to my mailbox that comment was actually left unresponded to.

Roger, am I missing anything here as to PVH Dom0 getting handlers put in
place?

Of course as soon as the CONFIG_ARM conditionals were dropped, the
__hwdom_init issue would become an "active" one.

Jan

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Jan Beulich]

On 15.10.2021 20:38, Julien Grall wrote:
> 
> 
> On 15/10/2021 18:33, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>
>>> On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote:
>>>
>>> Hi Bertrand,
>>>
>>> On 15/10/2021 17:51, Bertrand Marquis wrote:
>>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>>>> index 3aa8c3175f..35e0190796 100644
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>>       if ( !pdev->domain )
>>>>       {
>>>>           pdev->domain = hardware_domain;
>>>> +#ifdef CONFIG_ARM
>>>> +        /*
>>>> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
>>>> +         * when Dom0 inform XEN to add the PCI devices in XEN.
>>>> +         */
>>>> +        ret = vpci_add_handlers(pdev);
>>>
>>> I don't seem to find the code to remove __init_hwdom in this series. Are you intending to fix it separately?
>>
>> Yes I think it is better to fix that in a new patch as it will require some discussion as it will impact the x86 code if I just remove the flag now.
> For the future patch series, may I ask to keep track of outstanding 
> issues in the commit message (if you don't plan to address them before 
> commiting) or after --- (if they are meant to be addressed before 
> commiting)?
> 
> In this case, the impact on Arm is this would result to an hypervisor 
> crash if called. If we drop __init_hwdom, the impact on x86 is Xen text 
> will slightly be bigger after the boot time.
> 
> AFAICT, the code is not reachable on Arm (?).

Which re-raises my question towards testing of what is being added in
this series. Supported also by the typo in v7 patch 1, which suggests
that version wasn't even build-tested.

Jan

> Therefore, one could argue 
> we this can wait after the week-end as this is a latent bug. Yet, I am 
> not really comfortable to see knowningly buggy code merged.
> 
> Stefano, would you be willing to remove __init_hwdom while committing 
> it? If not, can you update the commit message and mention this patch 
> doesn't work as intended?
> 
> Cheers,
> 

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Roger Pau Monné]

On Mon, Oct 18, 2021 at 09:47:06AM +0200, Jan Beulich wrote:
> On 15.10.2021 18:51, Bertrand Marquis wrote:
> > --- /dev/null
> > +++ b/xen/arch/arm/vpci.c
> > @@ -0,0 +1,77 @@
> > +/*
> > + * xen/arch/arm/vpci.c
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +#include <xen/sched.h>
> > +#include <xen/vpci.h>
> > +
> > +#include <asm/mmio.h>
> > +
> > +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> > +                          register_t *r, void *p)
> > +{
> > +    pci_sbdf_t sbdf;
> > +    /* data is needed to prevent a pointer cast on 32bit */
> > +    unsigned long data;
> > +
> > +    /* We ignore segment part and always handle segment 0 */
> > +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
> > +
> > +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
> > +                        1U << info->dabt.size, &data) )
> > +    {
> 
> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
> the virtual one. The function then calls vpci_read(), which in turn
> will call vpci_read_hw() in a number of situations (first and foremost
> whenever pci_get_pdev_by_domain() returns NULL). That function as well
> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
> physical one; I'm unable to spot any translation. Yet I do recall
> seeing assignment of a virtual device and function number somewhere
> (perhaps another of the related series), so the model also doesn't
> look to assume 1:1 mapping of SBDF.
> 
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> >      if ( !pdev->domain )
> >      {
> >          pdev->domain = hardware_domain;
> > +#ifdef CONFIG_ARM
> > +        /*
> > +         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
> > +         * when Dom0 inform XEN to add the PCI devices in XEN.
> > +         */
> > +        ret = vpci_add_handlers(pdev);
> > +        if ( ret )
> > +        {
> > +            printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
> > +            pdev->domain = NULL;
> > +            goto out;
> > +        }
> > +#endif
> >          ret = iommu_add_device(pdev);
> >          if ( ret )
> >          {
> 
> Upon failure, vpci_add_handlers() will itself call vpci_remove_handlers().
> What about iommu_add_device() failure? The device will have ->domain
> zapped, but all vPCI handlers still in place. This aspect of insufficient
> error cleanup was pointed out already in review of v1.
> 
> Furthermore already in v1 I questioned why this would be Arm-specific: On
> x86 this code path is going to be taken for all devices Xen wasn't able
> to discover at boot (anything on segments we wouldn't consider config
> space access safe on without reassurance by Dom0 plus SR-IOV VFs, at the
> very least).

My original plans for SR-IOV VFs on PVH dom0 involved trapping
accesses to the SR-IOV capability and detecting the creation of VFs
without the need for dom0 to notify them to Xen. This would avoid dom0
having to call PHYSDEVOP_pci_device_add for that case.

I might be confused, but I think we also spoke about other (non SR-IOV
related) cases where PCI devices might appear after certain actions by
dom0, so I think we need to keep the PHYSDEVOP_pci_device_add for PVH
dom0.

> Hence it is my understanding that something along these
> lines is actually also needed for PVH Dom0. I've just checked, and
> according to my mailbox that comment was actually left unresponded to.
> 
> Roger, am I missing anything here as to PVH Dom0 getting handlers put in
> place?

No, I think we will need those, likewise for run-time reported MCFG
regions.

Thanks, Roger.

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Bertrand Marquis]

Hi Jan,

> On 18 Oct 2021, at 08:51, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 15.10.2021 20:38, Julien Grall wrote:
>> 
>> 
>> On 15/10/2021 18:33, Bertrand Marquis wrote:
>>> Hi Julien,
>> 
>> Hi Bertrand,
>> 
>>> 
>>>> On 15 Oct 2021, at 18:25, Julien Grall <julien@xen.org> wrote:
>>>> 
>>>> Hi Bertrand,
>>>> 
>>>> On 15/10/2021 17:51, Bertrand Marquis wrote:
>>>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>>>>> index 3aa8c3175f..35e0190796 100644
>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>>>      if ( !pdev->domain )
>>>>>      {
>>>>>          pdev->domain = hardware_domain;
>>>>> +#ifdef CONFIG_ARM
>>>>> +        /*
>>>>> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
>>>>> +         * when Dom0 inform XEN to add the PCI devices in XEN.
>>>>> +         */
>>>>> +        ret = vpci_add_handlers(pdev);
>>>> 
>>>> I don't seem to find the code to remove __init_hwdom in this series. Are you intending to fix it separately?
>>> 
>>> Yes I think it is better to fix that in a new patch as it will require some discussion as it will impact the x86 code if I just remove the flag now.
>> For the future patch series, may I ask to keep track of outstanding 
>> issues in the commit message (if you don't plan to address them before 
>> commiting) or after --- (if they are meant to be addressed before 
>> commiting)?
>> 
>> In this case, the impact on Arm is this would result to an hypervisor 
>> crash if called. If we drop __init_hwdom, the impact on x86 is Xen text 
>> will slightly be bigger after the boot time.
>> 
>> AFAICT, the code is not reachable on Arm (?).
> 
> Which re-raises my question towards testing of what is being added in
> this series. Supported also by the typo in v7 patch 1, which suggests
> that version wasn't even build-tested.

This was an honest mistake, we did build locally but without VPCI activated.
Once I discovered this by rerunning all tests (including one modifying the
code to activate VPCI), I signalled it on the mailing list and it was fixed in v8.

We did a lot of tests and tried to be as careful as possible but on the last rush
before the feature freeze deadline those can happen.

Regards
Bertrand

> 
> Jan
> 
>> Therefore, one could argue 
>> we this can wait after the week-end as this is a latent bug. Yet, I am 
>> not really comfortable to see knowningly buggy code merged.
>> 
>> Stefano, would you be willing to remove __init_hwdom while committing 
>> it? If not, can you update the commit message and mention this patch 
>> doesn't work as intended?
>> 
>> Cheers,

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Jan Beulich]

On 18.10.2021 10:03, Roger Pau Monné wrote:
> On Mon, Oct 18, 2021 at 09:47:06AM +0200, Jan Beulich wrote:
>> On 15.10.2021 18:51, Bertrand Marquis wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/arm/vpci.c
>>> @@ -0,0 +1,77 @@
>>> +/*
>>> + * xen/arch/arm/vpci.c
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +#include <xen/sched.h>
>>> +#include <xen/vpci.h>
>>> +
>>> +#include <asm/mmio.h>
>>> +
>>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>>> +                          register_t *r, void *p)
>>> +{
>>> +    pci_sbdf_t sbdf;
>>> +    /* data is needed to prevent a pointer cast on 32bit */
>>> +    unsigned long data;
>>> +
>>> +    /* We ignore segment part and always handle segment 0 */
>>> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
>>> +
>>> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>>> +                        1U << info->dabt.size, &data) )
>>> +    {
>>
>> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
>> the virtual one. The function then calls vpci_read(), which in turn
>> will call vpci_read_hw() in a number of situations (first and foremost
>> whenever pci_get_pdev_by_domain() returns NULL). That function as well
>> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
>> physical one; I'm unable to spot any translation. Yet I do recall
>> seeing assignment of a virtual device and function number somewhere
>> (perhaps another of the related series), so the model also doesn't
>> look to assume 1:1 mapping of SBDF.
>>
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>      if ( !pdev->domain )
>>>      {
>>>          pdev->domain = hardware_domain;
>>> +#ifdef CONFIG_ARM
>>> +        /*
>>> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
>>> +         * when Dom0 inform XEN to add the PCI devices in XEN.
>>> +         */
>>> +        ret = vpci_add_handlers(pdev);
>>> +        if ( ret )
>>> +        {
>>> +            printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
>>> +            pdev->domain = NULL;
>>> +            goto out;
>>> +        }
>>> +#endif
>>>          ret = iommu_add_device(pdev);
>>>          if ( ret )
>>>          {
>>
>> Upon failure, vpci_add_handlers() will itself call vpci_remove_handlers().
>> What about iommu_add_device() failure? The device will have ->domain
>> zapped, but all vPCI handlers still in place. This aspect of insufficient
>> error cleanup was pointed out already in review of v1.
>>
>> Furthermore already in v1 I questioned why this would be Arm-specific: On
>> x86 this code path is going to be taken for all devices Xen wasn't able
>> to discover at boot (anything on segments we wouldn't consider config
>> space access safe on without reassurance by Dom0 plus SR-IOV VFs, at the
>> very least).
> 
> My original plans for SR-IOV VFs on PVH dom0 involved trapping
> accesses to the SR-IOV capability and detecting the creation of VFs
> without the need for dom0 to notify them to Xen. This would avoid dom0
> having to call PHYSDEVOP_pci_device_add for that case.
> 
> I might be confused, but I think we also spoke about other (non SR-IOV
> related) cases where PCI devices might appear after certain actions by
> dom0, so I think we need to keep the PHYSDEVOP_pci_device_add for PVH
> dom0.

Right, hotplugged ones, which I forgot to mention in my earlier reply.

>> Hence it is my understanding that something along these
>> lines is actually also needed for PVH Dom0. I've just checked, and
>> according to my mailbox that comment was actually left unresponded to.
>>
>> Roger, am I missing anything here as to PVH Dom0 getting handlers put in
>> place?
> 
> No, I think we will need those, likewise for run-time reported MCFG
> regions.

Yes, this was what I referred to by "without reassurance by Dom0".
Thanks for confirming.

Jan

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Oleksandr Andrushchenko]

Hi, Jan!

On 18.10.21 10:47, Jan Beulich wrote:
> On 15.10.2021 18:51, Bertrand Marquis wrote:
>> --- /dev/null
>> +++ b/xen/arch/arm/vpci.c
>> @@ -0,0 +1,77 @@
>> +/*
>> + * xen/arch/arm/vpci.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <xen/sched.h>
>> +#include <xen/vpci.h>
>> +
>> +#include <asm/mmio.h>
>> +
>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>> +                          register_t *r, void *p)
>> +{
>> +    pci_sbdf_t sbdf;
>> +    /* data is needed to prevent a pointer cast on 32bit */
>> +    unsigned long data;
>> +
>> +    /* We ignore segment part and always handle segment 0 */
>> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
>> +
>> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>> +                        1U << info->dabt.size, &data) )
>> +    {
> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
> the virtual one.
Not really yet
>   The function then calls vpci_read(), which in turn
> will call vpci_read_hw() in a number of situations (first and foremost
> whenever pci_get_pdev_by_domain() returns NULL). That function as well
> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
> physical one; I'm unable to spot any translation. Yet I do recall
> seeing assignment of a virtual device and function number somewhere
> (perhaps another of the related series), so the model also doesn't
> look to assume 1:1 mapping of SBDF.
>
At the time of this patch we do not yet have a virtual topology
implemented which is added at a later stage.
So, when a DomU performs PCI enumeration it sees all the real
PCI HW and thus discovers all PCI devices with their *real SBDFs*,
e.g. just like we pass through all the topology to the guest.
So, in the question, SBDFs are physical

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Bertrand Marquis]

Hi Jan,

> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 15.10.2021 18:51, Bertrand Marquis wrote:
>> --- /dev/null
>> +++ b/xen/arch/arm/vpci.c
>> @@ -0,0 +1,77 @@
>> +/*
>> + * xen/arch/arm/vpci.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <xen/sched.h>
>> +#include <xen/vpci.h>
>> +
>> +#include <asm/mmio.h>
>> +
>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>> +                          register_t *r, void *p)
>> +{
>> +    pci_sbdf_t sbdf;
>> +    /* data is needed to prevent a pointer cast on 32bit */
>> +    unsigned long data;
>> +
>> +    /* We ignore segment part and always handle segment 0 */
>> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
>> +
>> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>> +                        1U << info->dabt.size, &data) )
>> +    {
> 
> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
> the virtual one. The function then calls vpci_read(), which in turn
> will call vpci_read_hw() in a number of situations (first and foremost
> whenever pci_get_pdev_by_domain() returns NULL). That function as well
> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
> physical one; I'm unable to spot any translation. Yet I do recall
> seeing assignment of a virtual device and function number somewhere
> (perhaps another of the related series), so the model also doesn't
> look to assume 1:1 mapping of SBDF.

This question was answered by Oleksandr I think.

> 
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>     if ( !pdev->domain )
>>     {
>>         pdev->domain = hardware_domain;
>> +#ifdef CONFIG_ARM
>> +        /*
>> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
>> +         * when Dom0 inform XEN to add the PCI devices in XEN.
>> +         */
>> +        ret = vpci_add_handlers(pdev);
>> +        if ( ret )
>> +        {
>> +            printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
>> +            pdev->domain = NULL;
>> +            goto out;
>> +        }
>> +#endif
>>         ret = iommu_add_device(pdev);
>>         if ( ret )
>>         {
> 
> Upon failure, vpci_add_handlers() will itself call vpci_remove_handlers().
> What about iommu_add_device() failure? The device will have ->domain
> zapped, but all vPCI handlers still in place. This aspect of insufficient
> error cleanup was pointed out already in review of v1.

Yes a call to vpci_remove_device should be made on the error path out if
iommu_add_device is failing. This should also be done in fact in 
pci_remove_device before cleanup the msi.
We will push a patch with a proposal for a fix for this.

> 
> Furthermore already in v1 I questioned why this would be Arm-specific: On
> x86 this code path is going to be taken for all devices Xen wasn't able
> to discover at boot (anything on segments we wouldn't consider config
> space access safe on without reassurance by Dom0 plus SR-IOV VFs, at the
> very least). Hence it is my understanding that something along these
> lines is actually also needed for PVH Dom0. I've just checked, and
> according to my mailbox that comment was actually left unresponded to.
> 
> Roger, am I missing anything here as to PVH Dom0 getting handlers put in
> place?

From Roger answer I understood that it will be needed (in the future). 
When and if this is needed, the ifdef CONFIG_ARM can be removed
but this would change x86 code behaviour so I do not think it would
have been right to do that in this serie.

> 
> Of course as soon as the CONFIG_ARM conditionals were dropped, the
> __hwdom_init issue would become an "active" one.

We will push a proposal for a fix for that.
If I understand Roger right, vpci_add_handler will also be needed in runtime
on x86 in the future so maybe it would even be right to remove the flag altogether ?

Regards
Bertrand

> 
> Jan
> 

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Jan Beulich]

On 18.10.2021 12:11, Bertrand Marquis wrote:
>> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> wrote:
>> On 15.10.2021 18:51, Bertrand Marquis wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/arm/vpci.c
>>> @@ -0,0 +1,77 @@
>>> +/*
>>> + * xen/arch/arm/vpci.c
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +#include <xen/sched.h>
>>> +#include <xen/vpci.h>
>>> +
>>> +#include <asm/mmio.h>
>>> +
>>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>>> +                          register_t *r, void *p)
>>> +{
>>> +    pci_sbdf_t sbdf;
>>> +    /* data is needed to prevent a pointer cast on 32bit */
>>> +    unsigned long data;
>>> +
>>> +    /* We ignore segment part and always handle segment 0 */
>>> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
>>> +
>>> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>>> +                        1U << info->dabt.size, &data) )
>>> +    {
>>
>> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
>> the virtual one. The function then calls vpci_read(), which in turn
>> will call vpci_read_hw() in a number of situations (first and foremost
>> whenever pci_get_pdev_by_domain() returns NULL). That function as well
>> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
>> physical one; I'm unable to spot any translation. Yet I do recall
>> seeing assignment of a virtual device and function number somewhere
>> (perhaps another of the related series), so the model also doesn't
>> look to assume 1:1 mapping of SBDF.
> 
> This question was answered by Oleksandr I think.

Yes; I continue to be sure though that I saw devfn allocation logic in
one of the many patches that are related here. And I'm relatively sure
that there no adjustment to logic here was made (but since it's hard
to pick the right patch out of the huge pile without knowing its title,
I can't reasonably go check).

>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>     if ( !pdev->domain )
>>>     {
>>>         pdev->domain = hardware_domain;
>>> +#ifdef CONFIG_ARM
>>> +        /*
>>> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
>>> +         * when Dom0 inform XEN to add the PCI devices in XEN.
>>> +         */
>>> +        ret = vpci_add_handlers(pdev);
>>> +        if ( ret )
>>> +        {
>>> +            printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
>>> +            pdev->domain = NULL;
>>> +            goto out;
>>> +        }
>>> +#endif
>>>         ret = iommu_add_device(pdev);
>>>         if ( ret )
>>>         {
>>
>> Upon failure, vpci_add_handlers() will itself call vpci_remove_handlers().
>> What about iommu_add_device() failure? The device will have ->domain
>> zapped, but all vPCI handlers still in place. This aspect of insufficient
>> error cleanup was pointed out already in review of v1.
> 
> Yes a call to vpci_remove_device should be made on the error path out if
> iommu_add_device is failing. This should also be done in fact in 
> pci_remove_device before cleanup the msi.
> We will push a patch with a proposal for a fix for this.
> 
>>
>> Furthermore already in v1 I questioned why this would be Arm-specific: On
>> x86 this code path is going to be taken for all devices Xen wasn't able
>> to discover at boot (anything on segments we wouldn't consider config
>> space access safe on without reassurance by Dom0 plus SR-IOV VFs, at the
>> very least). Hence it is my understanding that something along these
>> lines is actually also needed for PVH Dom0. I've just checked, and
>> according to my mailbox that comment was actually left unresponded to.
>>
>> Roger, am I missing anything here as to PVH Dom0 getting handlers put in
>> place?
> 
> From Roger answer I understood that it will be needed (in the future). 
> When and if this is needed, the ifdef CONFIG_ARM can be removed
> but this would change x86 code behaviour so I do not think it would
> have been right to do that in this serie.

I view this differently: This change {c,sh}ould have been broken out
into one changing x86 behavior first, which Arm then would simply
have adopted. I don't find it unusual for issues to be found in
existing code when making that fit for another architecture. As a
result ...

>> Of course as soon as the CONFIG_ARM conditionals were dropped, the
>> __hwdom_init issue would become an "active" one.
> 
> We will push a proposal for a fix for that.
> If I understand Roger right, vpci_add_handler will also be needed in runtime
> on x86 in the future so maybe it would even be right to remove the flag altogether ?

... I view these as going hand in hand: Removing the annotation
altogether is the way to go, yes, because on x86 the call will also
need to be made.

Ian, considering that PVH Dom0 is still experimental on x86, and
considering that the patch here was committed prematurely anyway,
would you be willing to release-ack a patch dropping the "#ifdef
CONFIG_ARM" alongside other necessary adjustments here, provided
maintainers have reached agreement?

Jan

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM [and 1 more messages]

[Ian Jackson]

Julien Grall writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"):
> AFAICT, the code is not reachable on Arm (?). Therefore, one could argue 
> we this can wait after the week-end as this is a latent bug. Yet, I am 
> not really comfortable to see knowningly buggy code merged.

I agree that merging something that is known to be wrong would be
quite irregular, at least without a compelling reason.

Jan Beulich writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"):
> On 16.10.2021 12:28, Roger Pau Monné wrote:
> > Maybe I'm being pedantic, or there was some communication outside the
> > mailing list, but I think strictly speaking you are missing an Ack
> > from either Jan or Paul for the xen/drivers/passthrough/pci.c change.
> > 
> > IMHO seeing how that chunk moved from 3 different places in just one
> > afternoon also doesn't give me a lot of confidence. It's Arm only code
> > at the end, so it's not going to effect the existing x86 support and
> > I'm not specially worried, but I would like to avoid having to move it
> > again.
> 
> +1
> 
> I'll be replying to the patch itself for the technical aspects. As per
> context still visible above this code path is supposedly unreachable
> right now, which makes me wonder even more: Why the rush? Depending on
> the answer plus considering the __hwdom_init issue, Ian, I'm inclined
> to suggest a revert.

I don't want to be waving hammers about at this stage, and I haven't
looked at the technical details myself, but:

Can I ask the ARM folks to make sure that this situation is sorted out
ASAP ?  Say, by the end of Thursday ?

By sorted out I mean that the __init_hwdom issue is fixed and that
the overall changes to xen/drivers/passthrough/pci.c have been
properly approved.

Furthermore, I think these followup patches should go in all in one
go, as a small series, when everyone is OK with it, rather than
dribbling in.  That will make it easier to see the wood for the trees
(and it would also make a revert less complicated).

Jan, are you OK with this approach ?

Thanks,
Ian.

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM [and 1 more messages]

[Bertrand Marquis]

Hi Ian,

> On 18 Oct 2021, at 11:38, Ian Jackson <iwj@xenproject.org> wrote:
> 
> Julien Grall writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"):
>> AFAICT, the code is not reachable on Arm (?). Therefore, one could argue 
>> we this can wait after the week-end as this is a latent bug. Yet, I am 
>> not really comfortable to see knowningly buggy code merged.
> 
> I agree that merging something that is known to be wrong would be
> quite irregular, at least without a compelling reason.
> 
> Jan Beulich writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"):
>> On 16.10.2021 12:28, Roger Pau Monn� wrote:
>>> Maybe I'm being pedantic, or there was some communication outside the
>>> mailing list, but I think strictly speaking you are missing an Ack
>>> from either Jan or Paul for the xen/drivers/passthrough/pci.c change.
>>> 
>>> IMHO seeing how that chunk moved from 3 different places in just one
>>> afternoon also doesn't give me a lot of confidence. It's Arm only code
>>> at the end, so it's not going to effect the existing x86 support and
>>> I'm not specially worried, but I would like to avoid having to move it
>>> again.
>> 
>> +1
>> 
>> I'll be replying to the patch itself for the technical aspects. As per
>> context still visible above this code path is supposedly unreachable
>> right now, which makes me wonder even more: Why the rush? Depending on
>> the answer plus considering the __hwdom_init issue, Ian, I'm inclined
>> to suggest a revert.
> 
> I don't want to be waving hammers about at this stage, and I haven't
> looked at the technical details myself, but:
> 
> Can I ask the ARM folks to make sure that this situation is sorted out
> ASAP ?  Say, by the end of Thursday ?

Sure.

Cheers
Bertrand

> 
> By sorted out I mean that the __init_hwdom issue is fixed and that
> the overall changes to xen/drivers/passthrough/pci.c have been
> properly approved.
> 
> Furthermore, I think these followup patches should go in all in one
> go, as a small series, when everyone is OK with it, rather than
> dribbling in.  That will make it easier to see the wood for the trees
> (and it would also make a revert less complicated).
> 
> Jan, are you OK with this approach ?
> 
> Thanks,
> Ian.

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM [and 1 more messages]

[Jan Beulich]

On 18.10.2021 12:38, Ian Jackson wrote:
> Julien Grall writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"):
>> AFAICT, the code is not reachable on Arm (?). Therefore, one could argue 
>> we this can wait after the week-end as this is a latent bug. Yet, I am 
>> not really comfortable to see knowningly buggy code merged.
> 
> I agree that merging something that is known to be wrong would be
> quite irregular, at least without a compelling reason.
> 
> Jan Beulich writes ("Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM"):
>> On 16.10.2021 12:28, Roger Pau Monné wrote:
>>> Maybe I'm being pedantic, or there was some communication outside the
>>> mailing list, but I think strictly speaking you are missing an Ack
>>> from either Jan or Paul for the xen/drivers/passthrough/pci.c change.
>>>
>>> IMHO seeing how that chunk moved from 3 different places in just one
>>> afternoon also doesn't give me a lot of confidence. It's Arm only code
>>> at the end, so it's not going to effect the existing x86 support and
>>> I'm not specially worried, but I would like to avoid having to move it
>>> again.
>>
>> +1
>>
>> I'll be replying to the patch itself for the technical aspects. As per
>> context still visible above this code path is supposedly unreachable
>> right now, which makes me wonder even more: Why the rush? Depending on
>> the answer plus considering the __hwdom_init issue, Ian, I'm inclined
>> to suggest a revert.
> 
> I don't want to be waving hammers about at this stage, and I haven't
> looked at the technical details myself, but:
> 
> Can I ask the ARM folks to make sure that this situation is sorted out
> ASAP ?  Say, by the end of Thursday ?
> 
> By sorted out I mean that the __init_hwdom issue is fixed and that
> the overall changes to xen/drivers/passthrough/pci.c have been
> properly approved.
> 
> Furthermore, I think these followup patches should go in all in one
> go, as a small series, when everyone is OK with it, rather than
> dribbling in.  That will make it easier to see the wood for the trees
> (and it would also make a revert less complicated).
> 
> Jan, are you OK with this approach ?

Yes. FTR I'm not fussed about "all in one go" vs "dribbling in".

Jan

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Oleksandr Andrushchenko]

On 18.10.21 13:29, Jan Beulich wrote:
> On 18.10.2021 12:11, Bertrand Marquis wrote:
>>> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 15.10.2021 18:51, Bertrand Marquis wrote:
>>>> --- /dev/null
>>>> +++ b/xen/arch/arm/vpci.c
>>>> @@ -0,0 +1,77 @@
>>>> +/*
>>>> + * xen/arch/arm/vpci.c
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License as published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +#include <xen/sched.h>
>>>> +#include <xen/vpci.h>
>>>> +
>>>> +#include <asm/mmio.h>
>>>> +
>>>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>>>> +                          register_t *r, void *p)
>>>> +{
>>>> +    pci_sbdf_t sbdf;
>>>> +    /* data is needed to prevent a pointer cast on 32bit */
>>>> +    unsigned long data;
>>>> +
>>>> +    /* We ignore segment part and always handle segment 0 */
>>>> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
>>>> +
>>>> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>>>> +                        1U << info->dabt.size, &data) )
>>>> +    {
>>> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
>>> the virtual one. The function then calls vpci_read(), which in turn
>>> will call vpci_read_hw() in a number of situations (first and foremost
>>> whenever pci_get_pdev_by_domain() returns NULL). That function as well
>>> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
>>> physical one; I'm unable to spot any translation. Yet I do recall
>>> seeing assignment of a virtual device and function number somewhere
>>> (perhaps another of the related series), so the model also doesn't
>>> look to assume 1:1 mapping of SBDF.
>> This question was answered by Oleksandr I think.
> Yes; I continue to be sure though that I saw devfn allocation logic in
> one of the many patches that are related here. And I'm relatively sure
> that there no adjustment to logic here was made (but since it's hard
> to pick the right patch out of the huge pile without knowing its title,
> I can't reasonably go check).
Offtop: I was somehow dropped from the Cc..

Please see:

[PATCH v3 10/11] vpci: Add initial support for virtual PCI bus topology
[PATCH v3 11/11] xen/arm: Translate virtual PCI bus topology for guests

[1] https://patchwork.kernel.org/project/xen-devel/list/?series=555481

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Jan Beulich]

On 18.10.2021 16:07, Oleksandr Andrushchenko wrote:
> On 18.10.21 13:29, Jan Beulich wrote:
>> On 18.10.2021 12:11, Bertrand Marquis wrote:
>>>> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 15.10.2021 18:51, Bertrand Marquis wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/arm/vpci.c
>>>>> @@ -0,0 +1,77 @@
>>>>> +/*
>>>>> + * xen/arch/arm/vpci.c
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License as published by
>>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>>> + * (at your option) any later version.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> + * GNU General Public License for more details.
>>>>> + */
>>>>> +#include <xen/sched.h>
>>>>> +#include <xen/vpci.h>
>>>>> +
>>>>> +#include <asm/mmio.h>
>>>>> +
>>>>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>>>>> +                          register_t *r, void *p)
>>>>> +{
>>>>> +    pci_sbdf_t sbdf;
>>>>> +    /* data is needed to prevent a pointer cast on 32bit */
>>>>> +    unsigned long data;
>>>>> +
>>>>> +    /* We ignore segment part and always handle segment 0 */
>>>>> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
>>>>> +
>>>>> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>>>>> +                        1U << info->dabt.size, &data) )
>>>>> +    {
>>>> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
>>>> the virtual one. The function then calls vpci_read(), which in turn
>>>> will call vpci_read_hw() in a number of situations (first and foremost
>>>> whenever pci_get_pdev_by_domain() returns NULL). That function as well
>>>> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
>>>> physical one; I'm unable to spot any translation. Yet I do recall
>>>> seeing assignment of a virtual device and function number somewhere
>>>> (perhaps another of the related series), so the model also doesn't
>>>> look to assume 1:1 mapping of SBDF.
>>> This question was answered by Oleksandr I think.
>> Yes; I continue to be sure though that I saw devfn allocation logic in
>> one of the many patches that are related here. And I'm relatively sure
>> that there no adjustment to logic here was made (but since it's hard
>> to pick the right patch out of the huge pile without knowing its title,
>> I can't reasonably go check).
> Offtop: I was somehow dropped from the Cc..
> 
> Please see:
> 
> [PATCH v3 10/11] vpci: Add initial support for virtual PCI bus topology
> [PATCH v3 11/11] xen/arm: Translate virtual PCI bus topology for guests
> 
> [1] https://patchwork.kernel.org/project/xen-devel/list/?series=555481

Ah yes, this way I can find them in my mailbox. And indeed - no translation
from virtual to physical SBDF on the config space read/write paths afaics.

Thanks for the pointer,
Jan

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Oleksandr Andrushchenko]

On 18.10.21 17:19, Jan Beulich wrote:
> On 18.10.2021 16:07, Oleksandr Andrushchenko wrote:
>> On 18.10.21 13:29, Jan Beulich wrote:
>>> On 18.10.2021 12:11, Bertrand Marquis wrote:
>>>>> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 15.10.2021 18:51, Bertrand Marquis wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/xen/arch/arm/vpci.c
>>>>>> @@ -0,0 +1,77 @@
>>>>>> +/*
>>>>>> + * xen/arch/arm/vpci.c
>>>>>> + *
>>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>>> + * it under the terms of the GNU General Public License as published by
>>>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>>>> + * (at your option) any later version.
>>>>>> + *
>>>>>> + * This program is distributed in the hope that it will be useful,
>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>>> + * GNU General Public License for more details.
>>>>>> + */
>>>>>> +#include <xen/sched.h>
>>>>>> +#include <xen/vpci.h>
>>>>>> +
>>>>>> +#include <asm/mmio.h>
>>>>>> +
>>>>>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>>>>>> +                          register_t *r, void *p)
>>>>>> +{
>>>>>> +    pci_sbdf_t sbdf;
>>>>>> +    /* data is needed to prevent a pointer cast on 32bit */
>>>>>> +    unsigned long data;
>>>>>> +
>>>>>> +    /* We ignore segment part and always handle segment 0 */
>>>>>> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
>>>>>> +
>>>>>> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>>>>>> +                        1U << info->dabt.size, &data) )
>>>>>> +    {
>>>>> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
>>>>> the virtual one. The function then calls vpci_read(), which in turn
>>>>> will call vpci_read_hw() in a number of situations (first and foremost
>>>>> whenever pci_get_pdev_by_domain() returns NULL). That function as well
>>>>> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
>>>>> physical one; I'm unable to spot any translation. Yet I do recall
>>>>> seeing assignment of a virtual device and function number somewhere
>>>>> (perhaps another of the related series), so the model also doesn't
>>>>> look to assume 1:1 mapping of SBDF.
>>>> This question was answered by Oleksandr I think.
>>> Yes; I continue to be sure though that I saw devfn allocation logic in
>>> one of the many patches that are related here. And I'm relatively sure
>>> that there no adjustment to logic here was made (but since it's hard
>>> to pick the right patch out of the huge pile without knowing its title,
>>> I can't reasonably go check).
>> Offtop: I was somehow dropped from the Cc..
>>
>> Please see:
>>
>> [PATCH v3 10/11] vpci: Add initial support for virtual PCI bus topology
>> [PATCH v3 11/11] xen/arm: Translate virtual PCI bus topology for guests
>>
>> [1] https://urldefense.com/v3/__https://patchwork.kernel.org/project/xen-devel/list/?series=555481__;!!GF_29dbcQIUBPA!mM8A39p8nk4UMK3YeKMMW9ua9BHj1UOWzaQcyx7G46YPdudxMpD3huqZfih0Uc8S-GyWXD_mPg$ [patchwork[.]kernel[.]org]
> Ah yes, this way I can find them in my mailbox. And indeed - no translation
> from virtual to physical SBDF on the config space read/write paths afaics.
There are translations for both read and write [2] such as:

+    /*
+     * 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 && !pci_translate_virtual_device(v->domain, &sbdf) )
+            return 1;
+
>
> Thanks for the pointer,
> Jan
>
[2] https://patchwork.kernel.org/project/xen-devel/patch/20210930075223.860329-12-andr2000@gmail.com/

Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Jan Beulich]

On 18.10.2021 16:37, Oleksandr Andrushchenko wrote:
> 
> 
> On 18.10.21 17:19, Jan Beulich wrote:
>> On 18.10.2021 16:07, Oleksandr Andrushchenko wrote:
>>> On 18.10.21 13:29, Jan Beulich wrote:
>>>> On 18.10.2021 12:11, Bertrand Marquis wrote:
>>>>>> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> On 15.10.2021 18:51, Bertrand Marquis wrote:
>>>>>>> --- /dev/null
>>>>>>> +++ b/xen/arch/arm/vpci.c
>>>>>>> @@ -0,0 +1,77 @@
>>>>>>> +/*
>>>>>>> + * xen/arch/arm/vpci.c
>>>>>>> + *
>>>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>>>> + * it under the terms of the GNU General Public License as published by
>>>>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>>>>> + * (at your option) any later version.
>>>>>>> + *
>>>>>>> + * This program is distributed in the hope that it will be useful,
>>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>>>> + * GNU General Public License for more details.
>>>>>>> + */
>>>>>>> +#include <xen/sched.h>
>>>>>>> +#include <xen/vpci.h>
>>>>>>> +
>>>>>>> +#include <asm/mmio.h>
>>>>>>> +
>>>>>>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>>>>>>> +                          register_t *r, void *p)
>>>>>>> +{
>>>>>>> +    pci_sbdf_t sbdf;
>>>>>>> +    /* data is needed to prevent a pointer cast on 32bit */
>>>>>>> +    unsigned long data;
>>>>>>> +
>>>>>>> +    /* We ignore segment part and always handle segment 0 */
>>>>>>> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
>>>>>>> +
>>>>>>> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>>>>>>> +                        1U << info->dabt.size, &data) )
>>>>>>> +    {
>>>>>> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
>>>>>> the virtual one. The function then calls vpci_read(), which in turn
>>>>>> will call vpci_read_hw() in a number of situations (first and foremost
>>>>>> whenever pci_get_pdev_by_domain() returns NULL). That function as well
>>>>>> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
>>>>>> physical one; I'm unable to spot any translation. Yet I do recall
>>>>>> seeing assignment of a virtual device and function number somewhere
>>>>>> (perhaps another of the related series), so the model also doesn't
>>>>>> look to assume 1:1 mapping of SBDF.
>>>>> This question was answered by Oleksandr I think.
>>>> Yes; I continue to be sure though that I saw devfn allocation logic in
>>>> one of the many patches that are related here. And I'm relatively sure
>>>> that there no adjustment to logic here was made (but since it's hard
>>>> to pick the right patch out of the huge pile without knowing its title,
>>>> I can't reasonably go check).
>>> Offtop: I was somehow dropped from the Cc..
>>>
>>> Please see:
>>>
>>> [PATCH v3 10/11] vpci: Add initial support for virtual PCI bus topology
>>> [PATCH v3 11/11] xen/arm: Translate virtual PCI bus topology for guests
>>>
>>> [1] https://urldefense.com/v3/__https://patchwork.kernel.org/project/xen-devel/list/?series=555481__;!!GF_29dbcQIUBPA!mM8A39p8nk4UMK3YeKMMW9ua9BHj1UOWzaQcyx7G46YPdudxMpD3huqZfih0Uc8S-GyWXD_mPg$ [patchwork[.]kernel[.]org]
>> Ah yes, this way I can find them in my mailbox. And indeed - no translation
>> from virtual to physical SBDF on the config space read/write paths afaics.
> There are translations for both read and write [2] such as:
> 
> +    /*
> +     * 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 && !pci_translate_virtual_device(v->domain, &sbdf) )
> +            return 1;
> +

Oh, that's before you even call vpci_read(). I would have expected this
to live in common vPCI code ...

Jan