[PATCH v1 0/3] vpci/msix: separate out common MSI-X code

[PATCH v1 0/3] vpci/msix: separate out common MSI-X code

[Mykyta Poturai]

This is preparatory work to make vPCI MSI-X functional on Arm. Split the
vPCI MSI-X code so that intercepting the MSI-X table and PBA stays x86
only, while the MSI-X capability handling becomes common. No functional
change intended.

This is a continuation of work done by Rahul from February 2022[1].

The approach is inverted, instead of moving out the x86 parts, move out
the common ones that will be needed on Arm. Subsequent arm patches for
reference can be found here[2].

[1] https://lore.kernel.org/xen-devel/cover.1644937405.git.rahul.singh@arm.com/T/#t
[2] https://github.com/stewdk/xen/tree/pcie-passthrough-arm-vpci-v26

Rahul Singh (1):
  vpci/msix: move helpers to common

Stewart Hildebrand (2):
  vpci/msix: move msix region removal to msix.c
  vpci/msix: move common msix code to common file

 xen/arch/x86/include/asm/msi.h      |  21 ---
 xen/arch/x86/msi.c                  |   2 +-
 xen/drivers/passthrough/amd/iommu.h |   2 +-
 xen/drivers/vpci/Makefile           |   6 +-
 xen/drivers/vpci/header.c           |  30 +----
 xen/drivers/vpci/msi.c              |   3 +-
 xen/drivers/vpci/msix.c             | 197 ++++++---------------------
 xen/drivers/vpci/msix_common.c      | 202 ++++++++++++++++++++++++++++
 xen/drivers/vpci/private.h          |   2 +
 xen/include/xen/msi.h               |  21 +++
 xen/include/xen/vpci.h              |   5 +
 11 files changed, 282 insertions(+), 209 deletions(-)
 create mode 100644 xen/drivers/vpci/msix_common.c

-- 
2.55.0

[PATCH v1 1/3] vpci/msix: move helpers to common

[Mykyta Poturai]

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

vPCI MSI-X helpers to be used on Arm are moved to a common header.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 xen/arch/x86/include/asm/msi.h      | 21 ---------------------
 xen/arch/x86/msi.c                  |  2 +-
 xen/drivers/passthrough/amd/iommu.h |  2 +-
 xen/drivers/vpci/msi.c              |  3 +--
 xen/drivers/vpci/msix.c             |  2 +-
 xen/include/xen/msi.h               | 21 +++++++++++++++++++++
 6 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 6fb663b2e7..818fa72928 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -147,27 +147,6 @@ int msi_free_irq(struct msi_desc *entry);
  */
 #define NR_HP_RESERVED_VECTORS 	20
 
-#define msi_control_reg(base)		((base) + PCI_MSI_FLAGS)
-#define msi_lower_address_reg(base)	((base) + PCI_MSI_ADDRESS_LO)
-#define msi_upper_address_reg(base)	((base) + PCI_MSI_ADDRESS_HI)
-#define msi_data_reg(base, is64bit)	\
-	((base) + ((is64bit) ? PCI_MSI_DATA_64 : PCI_MSI_DATA_32))
-#define msi_mask_bits_reg(base, is64bit) \
-	((base) + PCI_MSI_MASK_BIT - ((is64bit) ? 0 : 4))
-#define msi_pending_bits_reg(base, is64bit) \
-	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
-#define multi_msi_capable(control) \
-	(1U << MASK_EXTR(control, PCI_MSI_FLAGS_QMASK))
-#define multi_msi_enable(control, num) \
-	((control) |= MASK_INSR(fls(num) - 1, PCI_MSI_FLAGS_QSIZE))
-#define is_64bit_address(control)	(!!((control) & PCI_MSI_FLAGS_64BIT))
-#define is_mask_bit_support(control)	(!!((control) & PCI_MSI_FLAGS_MASKBIT))
-
-#define msix_control_reg(base)		((base) + PCI_MSIX_FLAGS)
-#define msix_table_offset_reg(base)	((base) + PCI_MSIX_TABLE)
-#define msix_pba_offset_reg(base)	((base) + PCI_MSIX_PBA)
-#define msix_table_size(control) 	(((control) & PCI_MSIX_FLAGS_QSIZE) + 1)
-
 /*
  * MSI Defined Data Structures
  */
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 8831ce068c..5e4e89b3c0 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -21,6 +21,7 @@
 #include <xen/iocap.h>
 #include <xen/keyhandler.h>
 #include <xen/pfn.h>
+#include <xen/msi.h>
 
 #include <asm/apic.h>
 #include <asm/genapic.h>
@@ -29,7 +30,6 @@
 #include <asm/irq-vectors.h>
 #include <asm/smp.h>
 #include <asm/desc.h>
-#include <asm/msi.h>
 #include <asm/fixmap.h>
 #include <asm/p2m.h>
 
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index 52f748310b..ba22506f58 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -26,8 +26,8 @@
 #include <xen/tasklet.h>
 #include <xen/sched.h>
 #include <xen/domain_page.h>
+#include <xen/msi.h>
 
-#include <asm/msi.h>
 #include <asm/apicdef.h>
 
 #include "iommu-defs.h"
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index f736054b2b..0ed39a6de9 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -18,11 +18,10 @@
 
 #include "private.h"
 
+#include <xen/msi.h>
 #include <xen/sched.h>
 #include <xen/softirq.h>
 
-#include <asm/msi.h>
-
 static uint32_t cf_check control_read(
     const struct pci_dev *pdev, unsigned int reg, void *data)
 {
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 5b01859acf..05056b5ea1 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -21,9 +21,9 @@
 
 #include <xen/io.h>
 #include <xen/lib.h>
+#include <xen/msi.h>
 #include <xen/sched.h>
 
-#include <asm/msi.h>
 #include <asm/p2m.h>
 #include <asm/x86_emulate.h>
 
diff --git a/xen/include/xen/msi.h b/xen/include/xen/msi.h
index c903d0050c..4e922965de 100644
--- a/xen/include/xen/msi.h
+++ b/xen/include/xen/msi.h
@@ -7,6 +7,27 @@
 
 #include <asm/msi.h>
 
+#define msi_control_reg(base)		((base) + PCI_MSI_FLAGS)
+#define msi_lower_address_reg(base)	((base) + PCI_MSI_ADDRESS_LO)
+#define msi_upper_address_reg(base)	((base) + PCI_MSI_ADDRESS_HI)
+#define msi_data_reg(base, is64bit)	\
+	((base) + ((is64bit) ? PCI_MSI_DATA_64 : PCI_MSI_DATA_32))
+#define msi_mask_bits_reg(base, is64bit) \
+	((base) + PCI_MSI_MASK_BIT - ((is64bit) ? 0 : 4))
+#define msi_pending_bits_reg(base, is64bit) \
+	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
+#define multi_msi_capable(control) \
+	(1U << MASK_EXTR(control, PCI_MSI_FLAGS_QMASK))
+#define multi_msi_enable(control, num) \
+	((control) |= MASK_INSR(fls(num) - 1, PCI_MSI_FLAGS_QSIZE))
+#define is_64bit_address(control)	(!!((control) & PCI_MSI_FLAGS_64BIT))
+#define is_mask_bit_support(control)	(!!((control) & PCI_MSI_FLAGS_MASKBIT))
+
+#define msix_control_reg(base)		((base) + PCI_MSIX_FLAGS)
+#define msix_table_offset_reg(base)	((base) + PCI_MSIX_TABLE)
+#define msix_pba_offset_reg(base)	((base) + PCI_MSIX_PBA)
+#define msix_table_size(control) 	(((control) & PCI_MSIX_FLAGS_QSIZE) + 1)
+
 int pdev_msix_assign(struct domain *d, struct pci_dev *pdev);
 int pdev_msi_init(struct pci_dev *pdev);
 void pdev_msi_deinit(struct pci_dev *pdev);
-- 
2.55.0

[PATCH v1 2/3] vpci/msix: move msix region removal to msix.c

[Mykyta Poturai]

From: Stewart Hildebrand <stewart.hildebrand@amd.com>

This code won't be used on Arm.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 xen/drivers/vpci/header.c  | 30 +++++-------------------------
 xen/drivers/vpci/msix.c    | 35 +++++++++++++++++++++++++++++++++++
 xen/drivers/vpci/private.h |  2 ++
 3 files changed, 42 insertions(+), 25 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 7e5f4b8a00..9e19e41d0a 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -310,7 +310,6 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
     struct vpci_header *header = &pdev->vpci->header;
     struct pci_dev *tmp;
     const struct domain *d;
-    const struct vpci_msix *msix = pdev->vpci->msix;
     unsigned int i, j;
     int rc;
 
@@ -405,30 +404,11 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         }
     }
 
-    /* Remove any MSIX regions if present. */
-    for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
-    {
-        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
-        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
-                                     vmsix_table_size(pdev->vpci, i) - 1);
-
-        for ( j = 0; j < ARRAY_SIZE(header->bars); j++ )
-        {
-            const struct vpci_bar *bar = &header->bars[j];
-
-            if ( rangeset_is_empty(bar->mem) )
-                continue;
-
-            rc = rangeset_remove_range(bar->mem, start, end);
-            if ( rc )
-            {
-                gprintk(XENLOG_WARNING,
-                       "%pp: failed to remove MSIX table [%lx, %lx]: %d\n",
-                        &pdev->sbdf, start, end, rc);
-                return rc;
-            }
-        }
-    }
+#ifdef CONFIG_HAS_PCI_MSI
+    rc = vpci_remove_msix_regions(pdev);
+    if ( rc )
+        return rc;
+#endif
 
     /*
      * Check for overlaps with other BARs. Note that only BARs that are
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 05056b5ea1..4a1e7561f6 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -590,6 +590,41 @@ static const struct hvm_mmio_ops vpci_msix_table_ops = {
     .write = msix_write,
 };
 
+int vpci_remove_msix_regions(const struct pci_dev *pdev)
+{
+    const struct vpci_msix *msix = pdev->vpci->msix;
+    struct vpci_header *header = &pdev->vpci->header;
+    unsigned int i, j;
+    int rc;
+
+    /* Remove any MSIX regions if present. */
+    for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
+    {
+        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
+        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
+                                     vmsix_table_size(pdev->vpci, i) - 1);
+
+        for ( j = 0; j < ARRAY_SIZE(header->bars); j++ )
+        {
+            const struct vpci_bar *bar = &header->bars[j];
+
+            if ( rangeset_is_empty(bar->mem) )
+                continue;
+
+            rc = rangeset_remove_range(bar->mem, start, end);
+            if ( rc )
+            {
+                gprintk(XENLOG_WARNING,
+                       "%pp: failed to remove MSIX table [%lx, %lx]: %d\n",
+                        &pdev->sbdf, start, end, rc);
+                return rc;
+            }
+        }
+    }
+
+    return 0;
+}
+
 int vpci_make_msix_hole(const struct pci_dev *pdev)
 {
     struct domain *d = pdev->domain;
diff --git a/xen/drivers/vpci/private.h b/xen/drivers/vpci/private.h
index 2907f6b40f..a163efe8ff 100644
--- a/xen/drivers/vpci/private.h
+++ b/xen/drivers/vpci/private.h
@@ -87,6 +87,8 @@ void cf_check vpci_hw_write16(
     const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data);
 
 #ifdef __XEN__
+int vpci_remove_msix_regions(const struct pci_dev *pdev);
+
 /* Make sure there's a hole in the p2m for the MSIX mmio areas. */
 int vpci_make_msix_hole(const struct pci_dev *pdev);
 
-- 
2.55.0

[PATCH v1 3/3] vpci/msix: move common msix code to common file

[Mykyta Poturai]

From: Stewart Hildebrand <stewart.hildebrand@amd.com>

vPCI MSI-X code dealing with intercepting the MSI-X tables and PBA won't
be used on Arm. Moving this code would result in a large diffstat, so
instead we move the common code to a common msix file to achieve a
smaller diffstat.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
 xen/drivers/vpci/Makefile      |   6 +-
 xen/drivers/vpci/msix.c        | 160 +-------------------------
 xen/drivers/vpci/msix_common.c | 202 +++++++++++++++++++++++++++++++++
 xen/include/xen/vpci.h         |   5 +
 4 files changed, 215 insertions(+), 158 deletions(-)
 create mode 100644 xen/drivers/vpci/msix_common.c

diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
index 9793a4f9b0..b4026cec81 100644
--- a/xen/drivers/vpci/Makefile
+++ b/xen/drivers/vpci/Makefile
@@ -1,3 +1,7 @@
 obj-y += cap.o
 obj-y += vpci.o header.o rebar.o
-obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o
+obj-$(CONFIG_HAS_PCI_MSI) += msi.o
+obj-$(CONFIG_HAS_PCI_MSI) += msix_common.o
+ifeq ($(CONFIG_X86),y)
+obj-$(CONFIG_HAS_PCI_MSI) += msix.o
+endif
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 4a1e7561f6..ab0853f70d 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -36,16 +36,6 @@
      PFN_DOWN(addr) <= PFN_DOWN(vmsix_table_addr(vpci, nr) +              \
                                 vmsix_table_size(vpci, nr) - 1))
 
-static uint32_t cf_check control_read(
-    const struct pci_dev *pdev, unsigned int reg, void *data)
-{
-    const struct vpci_msix *msix = data;
-
-    return (msix->max_entries - 1) |
-           (msix->enabled ? PCI_MSIX_FLAGS_ENABLE : 0) |
-           (msix->masked ? PCI_MSIX_FLAGS_MASKALL : 0);
-}
-
 static void update_entry(struct vpci_msix_entry *entry,
                          const struct pci_dev *pdev, unsigned int nr)
 {
@@ -74,7 +64,7 @@ static void update_entry(struct vpci_msix_entry *entry,
     entry->updated = false;
 }
 
-static void cf_check control_write(
+void vpci_msix_arch_control_write(
     const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
 {
     struct vpci_msix *msix = data;
@@ -83,9 +73,6 @@ static void cf_check control_write(
     unsigned int i;
     int rc;
 
-    if ( new_masked == msix->masked && new_enabled == msix->enabled )
-        return;
-
     /*
      * According to the PCI 3.0 specification, switching the enable bit to 1
      * or the function mask bit to 0 should cause all the cached addresses
@@ -138,20 +125,6 @@ static void cf_check control_write(
             }
         }
     }
-
-    /* Make sure domU doesn't enable INTx while enabling MSI-X. */
-    if ( new_enabled && !msix->enabled && !is_hardware_domain(pdev->domain) )
-    {
-        pci_intx(pdev, false);
-        pdev->vpci->header.guest_cmd |= PCI_COMMAND_INTX_DISABLE;
-    }
-
-    msix->masked = new_masked;
-    msix->enabled = new_enabled;
-
-    val = control_read(pdev, reg, data);
-    if ( pci_msi_conf_write_intercept(msix->pdev, reg, 2, &val) >= 0 )
-        pci_conf_write16(pdev->sbdf, reg, val);
 }
 
 static struct vpci_msix *msix_find(const struct domain *d, unsigned long addr)
@@ -694,151 +667,24 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
     return 0;
 }
 
-static int cf_check cleanup_msix(const struct pci_dev *pdev, bool hide)
+void vpci_msix_arch_cleanup(struct vpci *vpci)
 {
-    int rc;
-    struct vpci *vpci = pdev->vpci;
-    const unsigned int msix_pos = pdev->msix_pos;
-
     if ( vpci->msix )
     {
         list_del(&vpci->msix->next);
         for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
             if ( vpci->msix->table[i] )
                 iounmap(vpci->msix->table[i]);
-
-        XFREE(vpci->msix);
-    }
-
-    if ( !hide )
-        return 0;
-
-    rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
-    if ( rc )
-    {
-        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
-               pdev->domain, &pdev->sbdf, rc);
-        ASSERT_UNREACHABLE();
-        return rc;
     }
-
-    /*
-     * Unprivileged domains have a deny by default register access policy, no
-     * need to add any further handlers for them.
-     */
-    if ( !is_hardware_domain(pdev->domain) )
-        return 0;
-
-    /*
-     * The driver may not traverse the capability list and think device
-     * supports MSIX by default. So here let the control register of MSIX
-     * be Read-Only is to ensure MSIX disabled.
-     */
-    rc = vpci_add_register(vpci, vpci_hw_read16, NULL,
-                           msix_control_reg(msix_pos), 2, NULL);
-    if ( rc )
-        printk(XENLOG_ERR "%pd %pp: fail to add MSIX ctrl handler rc=%d\n",
-               pdev->domain, &pdev->sbdf, rc);
-
-    return rc;
 }
 
-static int cf_check init_msix(struct pci_dev *pdev)
+void vpci_msix_arch_register(struct vpci_msix *msix, struct domain *d)
 {
-    struct domain *d = pdev->domain;
-    unsigned int msix_offset, i, max_entries;
-    uint16_t control;
-    struct vpci_msix *msix;
-    int rc;
-
-    msix_offset = pdev->msix_pos;
-    if ( !msix_offset )
-        return 0;
-
-    control = pci_conf_read16(pdev->sbdf, msix_control_reg(msix_offset));
-
-    max_entries = msix_table_size(control);
-
-    msix = xzalloc_flex_struct(struct vpci_msix, entries, max_entries);
-    if ( !msix )
-        return -ENOMEM;
-
-    msix->tables[VPCI_MSIX_TABLE] =
-        pci_conf_read32(pdev->sbdf, msix_table_offset_reg(msix_offset));
-    msix->tables[VPCI_MSIX_PBA] =
-        pci_conf_read32(pdev->sbdf, msix_pba_offset_reg(msix_offset));
-
-    /* Check that the referenced BAR(s) regions are valid. */
-    for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
-    {
-        const char *name = (i == VPCI_MSIX_TABLE) ? "vector" : "PBA";
-        const struct vpci_bar *bars = pdev->vpci->header.bars;
-        unsigned int bir = msix->tables[i] & PCI_MSIX_BIRMASK;
-        unsigned int type;
-        unsigned int offset = msix->tables[i] & ~PCI_MSIX_BIRMASK;
-        unsigned int size =
-            (i == VPCI_MSIX_TABLE) ? max_entries * PCI_MSIX_ENTRY_SIZE
-                                   : ROUNDUP(DIV_ROUND_UP(max_entries, 8), 8);
-
-        if ( bir >= ARRAY_SIZE(pdev->vpci->header.bars) )
-        {
-            printk(XENLOG_ERR DEV_BUG_PREFIX
-                   "%pp: MSI-X %s table with out of range BIR %u\n",
-                   &pdev->sbdf, name, bir);
- invalid:
-            xfree(msix);
-            return -ENODEV;
-        }
-
-        type = bars[bir].type;
-        if ( type != VPCI_BAR_MEM32 && type != VPCI_BAR_MEM64_LO )
-        {
-            printk(XENLOG_ERR DEV_BUG_PREFIX
-                   "%pp: MSI-X %s table at invalid BAR%u with type %u\n",
-                   &pdev->sbdf, name, bir, type);
-            goto invalid;
-        }
-
-        if ( (uint64_t)offset + size > bars[bir].size )
-        {
-            printk(XENLOG_ERR DEV_BUG_PREFIX
-                   "%pp: MSI-X %s table offset %#x size %#x outside of BAR%u size %#lx\n",
-                   &pdev->sbdf, name, offset, size, bir, bars[bir].size);
-            goto invalid;
-        }
-    }
-
-    rc = vpci_add_register(pdev->vpci, control_read, control_write,
-                           msix_control_reg(msix_offset), 2, msix);
-    if ( rc )
-    {
-        xfree(msix);
-        return rc;
-    }
-
-    msix->max_entries = max_entries;
-    msix->pdev = pdev;
-
-    for ( i = 0; i < max_entries; i++)
-    {
-        msix->entries[i].masked = true;
-        vpci_msix_arch_init_entry(&msix->entries[i]);
-    }
-
     if ( list_empty(&d->arch.hvm.msix_tables) )
         register_mmio_handler(d, &vpci_msix_table_ops);
 
-    pdev->vpci->msix = msix;
     list_add(&msix->next, &d->arch.hvm.msix_tables);
-
-    /*
-     * vPCI header initialization will have mapped the whole BAR into the
-     * p2m, as MSI-X capability was not yet initialized.  Carve a hole for
-     * the MSI-X table here, so that Xen can trap accesses.
-     */
-    return vpci_make_msix_hole(pdev);
 }
-REGISTER_VPCI_CAP(MSIX, init_msix, cleanup_msix);
 
 /*
  * Local variables:
diff --git a/xen/drivers/vpci/msix_common.c b/xen/drivers/vpci/msix_common.c
new file mode 100644
index 0000000000..b38de48579
--- /dev/null
+++ b/xen/drivers/vpci/msix_common.c
@@ -0,0 +1,202 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Handlers for accesses to the MSI-X capability structure and the memory
+ * region.
+ *
+ * Copyright (C) 2017 Citrix Systems R&D
+ */
+
+#include "private.h"
+
+#include <xen/io.h>
+#include <xen/lib.h>
+#include <xen/msi.h>
+#include <xen/sched.h>
+
+#include <asm/p2m.h>
+
+static uint32_t cf_check control_read(
+    const struct pci_dev *pdev, unsigned int reg, void *data)
+{
+    const struct vpci_msix *msix = data;
+
+    return (msix->max_entries - 1) |
+           (msix->enabled ? PCI_MSIX_FLAGS_ENABLE : 0) |
+           (msix->masked ? PCI_MSIX_FLAGS_MASKALL : 0);
+}
+
+static void cf_check control_write(
+    const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
+{
+    struct vpci_msix *msix = data;
+    bool new_masked = val & PCI_MSIX_FLAGS_MASKALL;
+    bool new_enabled = val & PCI_MSIX_FLAGS_ENABLE;
+
+    if ( new_masked == msix->masked && new_enabled == msix->enabled )
+        return;
+
+    vpci_msix_arch_control_write(pdev, reg, val, data);
+
+    /* Make sure domU doesn't enable INTx while enabling MSI-X. */
+    if ( new_enabled && !msix->enabled && !is_hardware_domain(pdev->domain) )
+    {
+        pci_intx(pdev, false);
+        pdev->vpci->header.guest_cmd |= PCI_COMMAND_INTX_DISABLE;
+    }
+
+    msix->masked = new_masked;
+    msix->enabled = new_enabled;
+
+    val = control_read(pdev, reg, data);
+    if ( pci_msi_conf_write_intercept(msix->pdev, reg, 2, &val) >= 0 )
+        pci_conf_write16(pdev->sbdf, reg, val);
+}
+
+static int cf_check cleanup_msix(const struct pci_dev *pdev, bool hide)
+{
+    int rc;
+    struct vpci *vpci = pdev->vpci;
+    const unsigned int msix_pos = pdev->msix_pos;
+
+    vpci_msix_arch_cleanup(vpci);
+
+    if ( vpci->msix )
+        XFREE(vpci->msix);
+
+    if ( !hide )
+        return 0;
+
+    rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
+               pdev->domain, &pdev->sbdf, rc);
+        ASSERT_UNREACHABLE();
+        return rc;
+    }
+
+    /*
+     * Unprivileged domains have a deny by default register access policy, no
+     * need to add any further handlers for them.
+     */
+    if ( !is_hardware_domain(pdev->domain) )
+        return 0;
+
+    /*
+     * The driver may not traverse the capability list and think device
+     * supports MSIX by default. So here let the control register of MSIX
+     * be Read-Only is to ensure MSIX disabled.
+     */
+    rc = vpci_add_register(vpci, vpci_hw_read16, NULL,
+                           msix_control_reg(msix_pos), 2, NULL);
+    if ( rc )
+        printk(XENLOG_ERR "%pd %pp: fail to add MSIX ctrl handler rc=%d\n",
+               pdev->domain, &pdev->sbdf, rc);
+
+    return rc;
+}
+
+static int cf_check init_msix(struct pci_dev *pdev)
+{
+    struct domain *d = pdev->domain;
+    unsigned int msix_offset, i, max_entries;
+    uint16_t control;
+    struct vpci_msix *msix;
+    int rc;
+
+    msix_offset = pdev->msix_pos;
+    if ( !msix_offset )
+        return 0;
+
+    control = pci_conf_read16(pdev->sbdf, msix_control_reg(msix_offset));
+
+    max_entries = msix_table_size(control);
+
+    msix = xzalloc_flex_struct(struct vpci_msix, entries, max_entries);
+    if ( !msix )
+        return -ENOMEM;
+
+    msix->tables[VPCI_MSIX_TABLE] =
+        pci_conf_read32(pdev->sbdf, msix_table_offset_reg(msix_offset));
+    msix->tables[VPCI_MSIX_PBA] =
+        pci_conf_read32(pdev->sbdf, msix_pba_offset_reg(msix_offset));
+
+    /* Check that the referenced BAR(s) regions are valid. */
+    for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
+    {
+        const char *name = (i == VPCI_MSIX_TABLE) ? "vector" : "PBA";
+        const struct vpci_bar *bars = pdev->vpci->header.bars;
+        unsigned int bir = msix->tables[i] & PCI_MSIX_BIRMASK;
+        unsigned int type;
+        unsigned int offset = msix->tables[i] & ~PCI_MSIX_BIRMASK;
+        unsigned int size =
+            (i == VPCI_MSIX_TABLE) ? max_entries * PCI_MSIX_ENTRY_SIZE
+                                   : ROUNDUP(DIV_ROUND_UP(max_entries, 8), 8);
+
+        if ( bir >= ARRAY_SIZE(pdev->vpci->header.bars) )
+        {
+            printk(XENLOG_ERR DEV_BUG_PREFIX
+                   "%pp: MSI-X %s table with out of range BIR %u\n",
+                   &pdev->sbdf, name, bir);
+ invalid:
+            xfree(msix);
+            return -ENODEV;
+        }
+
+        type = bars[bir].type;
+        if ( type != VPCI_BAR_MEM32 && type != VPCI_BAR_MEM64_LO )
+        {
+            printk(XENLOG_ERR DEV_BUG_PREFIX
+                   "%pp: MSI-X %s table at invalid BAR%u with type %u\n",
+                   &pdev->sbdf, name, bir, type);
+            goto invalid;
+        }
+
+        if ( (uint64_t)offset + size > bars[bir].size )
+        {
+            printk(XENLOG_ERR DEV_BUG_PREFIX
+                   "%pp: MSI-X %s table offset %#x size %#x outside of BAR%u size %#lx\n",
+                   &pdev->sbdf, name, offset, size, bir, bars[bir].size);
+            goto invalid;
+        }
+    }
+
+    rc = vpci_add_register(pdev->vpci, control_read, control_write,
+                           msix_control_reg(msix_offset), 2, msix);
+    if ( rc )
+    {
+        xfree(msix);
+        return rc;
+    }
+
+    msix->max_entries = max_entries;
+    msix->pdev = pdev;
+
+    for ( i = 0; i < max_entries; i++)
+    {
+        msix->entries[i].masked = true;
+        vpci_msix_arch_init_entry(&msix->entries[i]);
+    }
+
+    pdev->vpci->msix = msix;
+
+    vpci_msix_arch_register(msix, d);
+
+    /*
+     * vPCI header initialization will have mapped the whole BAR into the
+     * p2m, as MSI-X capability was not yet initialized.  Carve a hole for
+     * the MSI-X table here, so that Xen can trap accesses.
+     */
+    return vpci_make_msix_hole(pdev);
+}
+REGISTER_VPCI_CAP(MSIX, init_msix, cleanup_msix);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 877aa391d1..07b388a998 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -199,6 +199,11 @@ bool vpci_ecam_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
 bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
                     unsigned long *data);
 
+void vpci_msix_arch_control_write(
+    const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data);
+void vpci_msix_arch_cleanup(struct vpci *vpci);
+void vpci_msix_arch_register(struct vpci_msix *msix, struct domain *d);
+
 #endif /* __XEN__ */
 
 #else /* !CONFIG_HAS_VPCI */
-- 
2.55.0

Re: [PATCH v1 0/3] vpci/msix: separate out common MSI-X code

[Jan Beulich]

On 26.10.2026 13:27, Mykyta Poturai wrote:
> This is preparatory work to make vPCI MSI-X functional on Arm. Split the
> vPCI MSI-X code so that intercepting the MSI-X table and PBA stays x86
> only,

This likely is related to peculiarities of Arm's interrupt handling, yet
on the surface I can't see why not intercepting MSI-X table accesses
would be safe. Would it be possible to extend background here a little?

> while the MSI-X capability handling becomes common. No functional
> change intended.
> 
> This is a continuation of work done by Rahul from February 2022[1].
> 
> The approach is inverted, instead of moving out the x86 parts, move out
> the common ones that will be needed on Arm. Subsequent arm patches for
> reference can be found here[2].
> 
> [1] https://lore.kernel.org/xen-devel/cover.1644937405.git.rahul.singh@arm.com/T/#t
> [2] https://github.com/stewdk/xen/tree/pcie-passthrough-arm-vpci-v26
> 
> Rahul Singh (1):
>   vpci/msix: move helpers to common
> 
> Stewart Hildebrand (2):
>   vpci/msix: move msix region removal to msix.c
>   vpci/msix: move common msix code to common file
> 
>  xen/arch/x86/include/asm/msi.h      |  21 ---
>  xen/arch/x86/msi.c                  |   2 +-
>  xen/drivers/passthrough/amd/iommu.h |   2 +-
>  xen/drivers/vpci/Makefile           |   6 +-
>  xen/drivers/vpci/header.c           |  30 +----
>  xen/drivers/vpci/msi.c              |   3 +-
>  xen/drivers/vpci/msix.c             | 197 ++++++---------------------
>  xen/drivers/vpci/msix_common.c      | 202 ++++++++++++++++++++++++++++

Nit: New file's names want to use dashes in favor of underscores.

Jan

Re: [PATCH v1 1/3] vpci/msix: move helpers to common

[Andrew Cooper]

On 26/10/2026 12:27 pm, Mykyta Poturai wrote:
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 8831ce068c..5e4e89b3c0 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -21,6 +21,7 @@
>  #include <xen/iocap.h>
>  #include <xen/keyhandler.h>
>  #include <xen/pfn.h>
> +#include <xen/msi.h>

Please keep the headers sorted.

Probably best fixed up on commit, by whomever takes this patch.

~Andrew

Re: [PATCH v1 3/3] vpci/msix: move common msix code to common file

[Andrew Cooper]

On 26/10/2026 12:28 pm, Mykyta Poturai wrote:
> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
>
> vPCI MSI-X code dealing with intercepting the MSI-X tables and PBA won't
> be used on Arm. Moving this code would result in a large diffstat, so
> instead we move the common code to a common msix file to achieve a
> smaller diffstat.
>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> ---
>  xen/drivers/vpci/Makefile      |   6 +-
>  xen/drivers/vpci/msix.c        | 160 +-------------------------
>  xen/drivers/vpci/msix_common.c | 202 +++++++++++++++++++++++++++++++++
>  xen/include/xen/vpci.h         |   5 +
>  4 files changed, 215 insertions(+), 158 deletions(-)
>  create mode 100644 xen/drivers/vpci/msix_common.c

Please arrange things to make the best structural sense after the
change, not to optimise for minimised diffstat.

The diffstat is an instantaneous review burden.  Good longterm structure
is far more important.

~Andrew

Re: [PATCH v1 0/3] vpci/msix: separate out common MSI-X code

[Mykyta Poturai]

On 10/26/26 2:34 PM, Jan Beulich wrote:
> On 26.10.2026 13:27, Mykyta Poturai wrote:
>> This is preparatory work to make vPCI MSI-X functional on Arm. Split the
>> vPCI MSI-X code so that intercepting the MSI-X table and PBA stays x86
>> only,
>
> This likely is related to peculiarities of Arm's interrupt handling, yet
> on the surface I can't see why not intercepting MSI-X table accesses
> would be safe. Would it be possible to extend background here a little?
>

Hi,

On Arm most of the work is done by the ITS, so the only thing the device
needs to write to the doorbell register is the EventID. Interrupt
translation is fully configured in the ITS; it will reject invalid
EventIDs and also has a sideband DeviceID, so misconfigured devices
cannot trigger each other's interrupts. Memory-wise everything is gated
by the IOMMU, so a wrong doorbell address would not pose any risk either.

So there is no need or benefit in translating anything here.

>> while the MSI-X capability handling becomes common. No functional
>> change intended.
>>
>> This is a continuation of work done by Rahul from February 2022[1].
>>
>> The approach is inverted, instead of moving out the x86 parts, move out
>> the common ones that will be needed on Arm. Subsequent arm patches for
>> reference can be found here[2].
>>
>> [1] https://lore.kernel.org/xen-devel/cover.1644937405.git.rahul.singh@arm.com/T/#t
>> [2] https://github.com/stewdk/xen/tree/pcie-passthrough-arm-vpci-v26
>>
>> Rahul Singh (1):
>>    vpci/msix: move helpers to common
>>
>> Stewart Hildebrand (2):
>>    vpci/msix: move msix region removal to msix.c
>>    vpci/msix: move common msix code to common file
>>
>>   xen/arch/x86/include/asm/msi.h      |  21 ---
>>   xen/arch/x86/msi.c                  |   2 +-
>>   xen/drivers/passthrough/amd/iommu.h |   2 +-
>>   xen/drivers/vpci/Makefile           |   6 +-
>>   xen/drivers/vpci/header.c           |  30 +----
>>   xen/drivers/vpci/msi.c              |   3 +-
>>   xen/drivers/vpci/msix.c             | 197 ++++++---------------------
>>   xen/drivers/vpci/msix_common.c      | 202 ++++++++++++++++++++++++++++
>
> Nit: New file's names want to use dashes in favor of underscores.
>
> Jan

Re: [PATCH v1 0/3] vpci/msix: separate out common MSI-X code

[Jan Beulich]

On 27.10.2026 10:39, Mykyta Poturai wrote:
> On 10/26/26 2:34 PM, Jan Beulich wrote:
>> On 26.10.2026 13:27, Mykyta Poturai wrote:
>>> This is preparatory work to make vPCI MSI-X functional on Arm. Split the
>>> vPCI MSI-X code so that intercepting the MSI-X table and PBA stays x86
>>> only,
>>
>> This likely is related to peculiarities of Arm's interrupt handling, yet
>> on the surface I can't see why not intercepting MSI-X table accesses
>> would be safe. Would it be possible to extend background here a little?
> 
> On Arm most of the work is done by the ITS, so the only thing the device
> needs to write to the doorbell register is the EventID. Interrupt
> translation is fully configured in the ITS; it will reject invalid
> EventIDs and also has a sideband DeviceID, so misconfigured devices
> cannot trigger each other's interrupts. Memory-wise everything is gated
> by the IOMMU, so a wrong doorbell address would not pose any risk either.

And a guest fiddling with the mask bits behind Xen's back isn't a problem
either?

Jan

Re: [PATCH v1 0/3] vpci/msix: separate out common MSI-X code

[Mykyta Poturai]

On 10/27/26 12:21 PM, Jan Beulich wrote:
> On 27.10.2026 10:39, Mykyta Poturai wrote:
>> On 10/26/26 2:34 PM, Jan Beulich wrote:
>>> On 26.10.2026 13:27, Mykyta Poturai wrote:
>>>> This is preparatory work to make vPCI MSI-X functional on Arm. Split the
>>>> vPCI MSI-X code so that intercepting the MSI-X table and PBA stays x86
>>>> only,
>>>
>>> This likely is related to peculiarities of Arm's interrupt handling, yet
>>> on the surface I can't see why not intercepting MSI-X table accesses
>>> would be safe. Would it be possible to extend background here a little?
>>
>> On Arm most of the work is done by the ITS, so the only thing the device
>> needs to write to the doorbell register is the EventID. Interrupt
>> translation is fully configured in the ITS; it will reject invalid
>> EventIDs and also has a sideband DeviceID, so misconfigured devices
>> cannot trigger each other's interrupts. Memory-wise everything is gated
>> by the IOMMU, so a wrong doorbell address would not pose any risk either.
> 
> And a guest fiddling with the mask bits behind Xen's back isn't a problem
> either?
> 
> Jan

No. Masking/unmasking an interrupt the guest has already mapped is just 
the guest doing its own configuration. Unmasking an unmapped interrupt 
will result in it being discarded by the ITS. So I don't see a problem 
here either.

Re: [PATCH v1 0/3] vpci/msix: separate out common MSI-X code

[Jan Beulich]

On 28.10.2026 09:43, Mykyta Poturai wrote:
> On 10/27/26 12:21 PM, Jan Beulich wrote:
>> On 27.10.2026 10:39, Mykyta Poturai wrote:
>>> On 10/26/26 2:34 PM, Jan Beulich wrote:
>>>> On 26.10.2026 13:27, Mykyta Poturai wrote:
>>>>> This is preparatory work to make vPCI MSI-X functional on Arm. Split the
>>>>> vPCI MSI-X code so that intercepting the MSI-X table and PBA stays x86
>>>>> only,
>>>>
>>>> This likely is related to peculiarities of Arm's interrupt handling, yet
>>>> on the surface I can't see why not intercepting MSI-X table accesses
>>>> would be safe. Would it be possible to extend background here a little?
>>>
>>> On Arm most of the work is done by the ITS, so the only thing the device
>>> needs to write to the doorbell register is the EventID. Interrupt
>>> translation is fully configured in the ITS; it will reject invalid
>>> EventIDs and also has a sideband DeviceID, so misconfigured devices
>>> cannot trigger each other's interrupts. Memory-wise everything is gated
>>> by the IOMMU, so a wrong doorbell address would not pose any risk either.
>>
>> And a guest fiddling with the mask bits behind Xen's back isn't a problem
>> either?
> 
> No. Masking/unmasking an interrupt the guest has already mapped is just 
> the guest doing its own configuration. Unmasking an unmapped interrupt 
> will result in it being discarded by the ITS. So I don't see a problem 
> here either.

IOW unlike on x86, on Arm Xen never has a need to (transiently) mask an
interrupt that the guest wants / considers unmasked?

Jan

Re: [PATCH v1 0/3] vpci/msix: separate out common MSI-X code

[Mykyta Poturai]

On 10/28/26 10:58 AM, Jan Beulich wrote:
> On 28.10.2026 09:43, Mykyta Poturai wrote:
>> On 10/27/26 12:21 PM, Jan Beulich wrote:
>>> On 27.10.2026 10:39, Mykyta Poturai wrote:
>>>> On 10/26/26 2:34 PM, Jan Beulich wrote:
>>>>> On 26.10.2026 13:27, Mykyta Poturai wrote:
>>>>>> This is preparatory work to make vPCI MSI-X functional on Arm. Split the
>>>>>> vPCI MSI-X code so that intercepting the MSI-X table and PBA stays x86
>>>>>> only,
>>>>>
>>>>> This likely is related to peculiarities of Arm's interrupt handling, yet
>>>>> on the surface I can't see why not intercepting MSI-X table accesses
>>>>> would be safe. Would it be possible to extend background here a little?
>>>>
>>>> On Arm most of the work is done by the ITS, so the only thing the device
>>>> needs to write to the doorbell register is the EventID. Interrupt
>>>> translation is fully configured in the ITS; it will reject invalid
>>>> EventIDs and also has a sideband DeviceID, so misconfigured devices
>>>> cannot trigger each other's interrupts. Memory-wise everything is gated
>>>> by the IOMMU, so a wrong doorbell address would not pose any risk either.
>>>
>>> And a guest fiddling with the mask bits behind Xen's back isn't a problem
>>> either?
>>
>> No. Masking/unmasking an interrupt the guest has already mapped is just
>> the guest doing its own configuration. Unmasking an unmapped interrupt
>> will result in it being discarded by the ITS. So I don't see a problem
>> here either.
> 
> IOW unlike on x86, on Arm Xen never has a need to (transiently) mask an
> interrupt that the guest wants / considers unmasked?

When such a need arises, the interrupt can and should be masked in the 
ITS. On Arm the MSI-X entry carries almost nothing - everything related 
to routing, mapping and masking is done in the ITS, which Xen will fully 
emulate.