[PATCH v4 0/3] xen/pci: Make PCI passthrough code non-x86 specific

[PATCH v4 0/3] xen/pci: Make PCI passthrough code non-x86 specific

[Rahul Singh]

This patch series is preparatory work to implement the PCI passthrough support
for the ARM architecture.

Rahul Singh (3):
  xen/iommu: Move iommu_update_ire_from_msi(..) to xen/iommu.h
  xen/pci: Refactor PCI MSI intercept related code
  xen/pci: Refactor MSI code that implements MSI functionality within
    XEN

 xen/arch/x86/Kconfig                    |  1 +
 xen/drivers/passthrough/Makefile        |  1 +
 xen/drivers/passthrough/msi-intercept.c | 94 +++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c           | 61 ++++------------
 xen/drivers/pci/Kconfig                 |  4 ++
 xen/drivers/vpci/Makefile               |  3 +-
 xen/drivers/vpci/header.c               | 19 ++---
 xen/drivers/vpci/msix.c                 | 55 +++++++++++++++
 xen/drivers/vpci/vpci.c                 |  3 +-
 xen/include/xen/iommu.h                 | 13 +++-
 xen/include/xen/msi-intercept.h         | 56 +++++++++++++++
 xen/include/xen/pci.h                   | 11 +--
 xen/include/xen/vpci.h                  | 41 +++++------
 xen/xsm/flask/hooks.c                   |  8 +--
 14 files changed, 270 insertions(+), 100 deletions(-)
 create mode 100644 xen/drivers/passthrough/msi-intercept.c
 create mode 100644 xen/include/xen/msi-intercept.h

-- 
2.17.1

[PATCH v4 1/3] xen/iommu: Move iommu_update_ire_from_msi(..) to xen/iommu.h

[Rahul Singh]

Move iommu_update_ire_from_msi(..) from passthrough/pci.c to
xen/iommu.h and wrap it under CONFIG_X86 as it is referenced in x86
code only to avoid compilation error for other architecture when
HAS_PCI is enabled.

No functional change intended.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/passthrough/pci.c |  7 -------
 xen/include/xen/iommu.h       | 13 ++++++++++---
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 705137f8be..199ce08612 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1303,13 +1303,6 @@ static int __init setup_dump_pcidevs(void)
 }
 __initcall(setup_dump_pcidevs);
 
-int iommu_update_ire_from_msi(
-    struct msi_desc *msi_desc, struct msi_msg *msg)
-{
-    return iommu_intremap
-           ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
-}
-
 static int iommu_add_device(struct pci_dev *pdev)
 {
     const struct domain_iommu *hd;
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 4f0e5ac622..460755df29 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -187,8 +187,6 @@ void free_hvm_irq_dpci(struct hvm_irq_dpci *dpci);
 struct msi_desc;
 struct msi_msg;
 
-int iommu_update_ire_from_msi(struct msi_desc *msi_desc, struct msi_msg *msg);
-
 #define PT_IRQ_TIME_OUT MILLISECS(8)
 #endif /* HAS_PCI */
 
@@ -238,7 +236,6 @@ struct iommu_ops {
                            u8 devfn, device_t *dev);
 #ifdef CONFIG_HAS_PCI
     int (*get_device_group_id)(u16 seg, u8 bus, u8 devfn);
-    int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg *msg);
 #endif /* HAS_PCI */
 
     void (*teardown)(struct domain *d);
@@ -267,6 +264,7 @@ struct iommu_ops {
     int (*adjust_irq_affinities)(void);
     void (*sync_cache)(const void *addr, unsigned int size);
     void (*clear_root_pgtable)(struct domain *d);
+    int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg *msg);
 #endif /* CONFIG_X86 */
 
     int __must_check (*suspend)(void);
@@ -374,6 +372,15 @@ extern struct page_list_head iommu_pt_cleanup_list;
 
 bool arch_iommu_use_permitted(const struct domain *d);
 
+#ifdef CONFIG_X86
+static inline int iommu_update_ire_from_msi(
+    struct msi_desc *msi_desc, struct msi_msg *msg)
+{
+    return iommu_intremap
+           ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
+}
+#endif
+
 #endif /* _IOMMU_H_ */
 
 /*
-- 
2.17.1

[PATCH v4 2/3] xen/pci: Refactor PCI MSI intercept related code

[Rahul Singh]

MSI intercept related code is not useful for ARM when MSI interrupts are
injected via GICv3 ITS.

Therefore introducing the new flag CONFIG_HAS_PCI_MSI_INTERCEPT to gate
the MSI code for ARM in common code and also implemented the stub
version for the unused code for ARM to avoid compilation error when
HAS_PCI is enabled for ARM.

No functional change intended.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/arch/x86/Kconfig                    |  1 +
 xen/drivers/passthrough/Makefile        |  1 +
 xen/drivers/passthrough/msi-intercept.c | 53 ++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c           | 20 ++++-----
 xen/drivers/pci/Kconfig                 |  4 ++
 xen/drivers/vpci/Makefile               |  3 +-
 xen/drivers/vpci/header.c               | 19 ++-------
 xen/drivers/vpci/msix.c                 | 55 +++++++++++++++++++++++++
 xen/drivers/vpci/vpci.c                 |  3 +-
 xen/include/xen/msi-intercept.h         | 49 ++++++++++++++++++++++
 xen/include/xen/vpci.h                  | 41 ++++++++----------
 11 files changed, 195 insertions(+), 54 deletions(-)
 create mode 100644 xen/drivers/passthrough/msi-intercept.c
 create mode 100644 xen/include/xen/msi-intercept.h

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 32b9f23a20..a822687c2e 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -20,6 +20,7 @@ config X86
 	select HAS_NS16550
 	select HAS_PASSTHROUGH
 	select HAS_PCI
+	select HAS_PCI_MSI_INTERCEPT
 	select HAS_PDX
 	select HAS_SCHED_GRANULARITY
 	select HAS_UBSAN
diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
index 445690e3e5..eb27d10f5a 100644
--- a/xen/drivers/passthrough/Makefile
+++ b/xen/drivers/passthrough/Makefile
@@ -7,3 +7,4 @@ obj-y += iommu.o
 obj-$(CONFIG_HAS_PCI) += pci.o
 obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
 obj-$(CONFIG_HAS_PCI) += ats.o
+obj-$(CONFIG_HAS_PCI_MSI_INTERCEPT) += msi-intercept.o
diff --git a/xen/drivers/passthrough/msi-intercept.c b/xen/drivers/passthrough/msi-intercept.c
new file mode 100644
index 0000000000..ed3ec38003
--- /dev/null
+++ b/xen/drivers/passthrough/msi-intercept.c
@@ -0,0 +1,53 @@
+/*
+ * Copyright (C) 2008,  Netronome Systems, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/init.h>
+#include <xen/pci.h>
+#include <asm/msi.h>
+#include <asm/hvm/io.h>
+
+int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
+{
+    int rc;
+
+    if ( pdev->msix )
+    {
+        rc = pci_reset_msix_state(pdev);
+        if ( rc )
+            return rc;
+        msixtbl_init(d);
+    }
+
+    return 0;
+}
+
+void pdev_dump_msi(const struct pci_dev *pdev)
+{
+    const struct msi_desc *msi;
+
+    list_for_each_entry ( msi, &pdev->msi_list, list )
+        printk("- MSIs < %d >", msi->irq);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 199ce08612..237461b4ab 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -32,8 +32,8 @@
 #include <xen/softirq.h>
 #include <xen/tasklet.h>
 #include <xen/vpci.h>
+#include <xen/msi-intercept.h>
 #include <xsm/xsm.h>
-#include <asm/msi.h>
 #include "ats.h"
 
 struct pci_seg {
@@ -1271,18 +1271,16 @@ bool_t pcie_aer_get_firmware_first(const struct pci_dev *pdev)
 static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
 {
     struct pci_dev *pdev;
-    struct msi_desc *msi;
 
     printk("==== segment %04x ====\n", pseg->nr);
 
     list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
     {
-        printk("%pp - %pd - node %-3d - MSIs < ",
+        printk("%pp - %pd - node %-3d ",
                &pdev->sbdf, pdev->domain,
                (pdev->node != NUMA_NO_NODE) ? pdev->node : -1);
-        list_for_each_entry ( msi, &pdev->msi_list, list )
-               printk("%d ", msi->irq);
-        printk(">\n");
+        pdev_dump_msi(pdev);
+        printk("\n");
     }
 
     return 0;
@@ -1422,13 +1420,9 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
     ASSERT(pdev && (pdev->domain == hardware_domain ||
                     pdev->domain == dom_io));
 
-    if ( pdev->msix )
-    {
-        rc = pci_reset_msix_state(pdev);
-        if ( rc )
-            goto done;
-        msixtbl_init(d);
-    }
+    rc = pdev_msix_assign(d, pdev);
+    if ( rc )
+        goto done;
 
     pdev->fault.count = 0;
 
diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig
index 7da03fa13b..295731a3f4 100644
--- a/xen/drivers/pci/Kconfig
+++ b/xen/drivers/pci/Kconfig
@@ -1,3 +1,7 @@
 
 config HAS_PCI
 	bool
+
+config HAS_PCI_MSI_INTERCEPT
+	bool
+	depends on HAS_PCI
diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
index 55d1bdfda0..a95e6c2ca0 100644
--- a/xen/drivers/vpci/Makefile
+++ b/xen/drivers/vpci/Makefile
@@ -1 +1,2 @@
-obj-y += vpci.o header.o msi.o msix.o
+obj-y += vpci.o header.o
+obj-$(CONFIG_HAS_PCI_MSI_INTERCEPT) += msi.o msix.o
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index ba9a036202..81d3d2d17f 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -206,7 +206,6 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
     struct vpci_header *header = &pdev->vpci->header;
     struct rangeset *mem = rangeset_new(NULL, NULL, 0);
     struct pci_dev *tmp, *dev = NULL;
-    const struct vpci_msix *msix = pdev->vpci->msix;
     unsigned int i;
     int rc;
 
@@ -244,21 +243,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++ )
+    rc = vpci_remove_msix_regions(pdev->vpci, mem);
+    if ( rc )
     {
-        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);
-
-        rc = rangeset_remove_range(mem, start, end);
-        if ( rc )
-        {
-            printk(XENLOG_G_WARNING
-                   "Failed to remove MSIX table [%lx, %lx]: %d\n",
-                   start, end, rc);
-            rangeset_destroy(mem);
-            return rc;
-        }
+        rangeset_destroy(mem);
+        return rc;
     }
 
     /*
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 846f1b8d70..b62106ddf9 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -27,6 +27,36 @@
     ((addr) >= vmsix_table_addr(vpci, nr) &&                              \
      (addr) < vmsix_table_addr(vpci, nr) + vmsix_table_size(vpci, nr))
 
+/*
+ * Helper functions to fetch MSIX related data. They are used by both the
+ * emulated MSIX code and the BAR handlers.
+ */
+static inline paddr_t vmsix_table_base(const struct vpci *vpci,
+                                       unsigned int nr)
+{
+    return vpci->header.bars[vpci->msix->tables[nr] & PCI_MSIX_BIRMASK].addr;
+}
+
+static inline paddr_t vmsix_table_addr(const struct vpci *vpci,
+                                       unsigned int nr)
+{
+    return vmsix_table_base(vpci, nr) +
+           (vpci->msix->tables[nr] & ~PCI_MSIX_BIRMASK);
+}
+
+/*
+ * Note regarding the size calculation of the PBA: the spec mentions "The last
+ * QWORD will not necessarily be fully populated", so it implies that the PBA
+ * size is 64-bit aligned.
+ */
+static inline size_t vmsix_table_size(const struct vpci *vpci, unsigned int nr)
+{
+    return
+        (nr == VPCI_MSIX_TABLE) ? vpci->msix->max_entries * PCI_MSIX_ENTRY_SIZE
+                                : ROUNDUP(DIV_ROUND_UP(vpci->msix->max_entries,
+                                                       8), 8);
+}
+
 static uint32_t control_read(const struct pci_dev *pdev, unsigned int reg,
                              void *data)
 {
@@ -428,6 +458,31 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
     return 0;
 }
 
+int vpci_remove_msix_regions(const struct vpci *vpci, struct rangeset *mem)
+{
+    const struct vpci_msix *msix = vpci->msix;
+    unsigned int i;
+    int rc;
+
+    for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
+    {
+        unsigned long start = PFN_DOWN(vmsix_table_addr(vpci, i));
+        unsigned long end = PFN_DOWN(vmsix_table_addr(vpci, i) +
+                vmsix_table_size(vpci, i) - 1);
+
+        rc = rangeset_remove_range(mem, start, end);
+        if ( rc )
+        {
+            printk(XENLOG_G_WARNING
+                    "Failed to remove MSIX table [%lx, %lx]: %d\n",
+                    start, end, rc);
+            return rc;
+        }
+    }
+
+    return 0;
+}
+
 static int init_msix(struct pci_dev *pdev)
 {
     struct domain *d = pdev->domain;
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index cbd1bac7fc..85084dd924 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -48,8 +48,7 @@ void vpci_remove_device(struct pci_dev *pdev)
         xfree(r);
     }
     spin_unlock(&pdev->vpci->lock);
-    xfree(pdev->vpci->msix);
-    xfree(pdev->vpci->msi);
+    vpci_msi_free(pdev->vpci);
     xfree(pdev->vpci);
     pdev->vpci = NULL;
 }
diff --git a/xen/include/xen/msi-intercept.h b/xen/include/xen/msi-intercept.h
new file mode 100644
index 0000000000..1bf9fc4465
--- /dev/null
+++ b/xen/include/xen/msi-intercept.h
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2008,  Netronome Systems, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __XEN_MSI_INTERCEPT_H_
+#define __XEN_MSI_INTERCEPT_H_
+
+#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT
+
+#include <asm/msi.h>
+
+int pdev_msix_assign(struct domain *d, struct pci_dev *pdev);
+void pdev_dump_msi(const struct pci_dev *pdev);
+
+#else /* !CONFIG_HAS_PCI_MSI_INTERCEPT */
+
+static inline int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
+{
+    return 0;
+}
+
+static inline void pdev_dump_msi(const struct pci_dev *pdev) {}
+static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
+
+#endif /* CONFIG_HAS_PCI_MSI_INTERCEPT */
+
+#endif /* __XEN_MSI_INTERCEPT_H */
+
+/*
+ * 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 9f5b5d52e1..4197407464 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -91,6 +91,7 @@ struct vpci {
         /* FIXME: currently there's no support for SR-IOV. */
     } header;
 
+#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT
     /* MSI data. */
     struct vpci_msi {
       /* Address. */
@@ -136,6 +137,7 @@ struct vpci {
             struct vpci_arch_msix_entry arch;
         } entries[];
     } *msix;
+#endif /* CONFIG_HAS_PCI_MSI_INTERCEPT */
 #endif
 };
 
@@ -148,6 +150,7 @@ struct vpci_vcpu {
 };
 
 #ifdef __XEN__
+#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT
 void vpci_dump_msi(void);
 
 /* Make sure there's a hole in the p2m for the MSIX mmio areas. */
@@ -174,40 +177,32 @@ int __must_check vpci_msix_arch_disable_entry(struct vpci_msix_entry *entry,
                                               const struct pci_dev *pdev);
 void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry);
 int vpci_msix_arch_print(const struct vpci_msix *msix);
+int vpci_remove_msix_regions(const struct vpci *vpci, struct rangeset *mem);
 
-/*
- * Helper functions to fetch MSIX related data. They are used by both the
- * emulated MSIX code and the BAR handlers.
- */
-static inline paddr_t vmsix_table_base(const struct vpci *vpci, unsigned int nr)
+static inline unsigned int vmsix_entry_nr(const struct vpci_msix *msix,
+                                          const struct vpci_msix_entry *entry)
 {
-    return vpci->header.bars[vpci->msix->tables[nr] & PCI_MSIX_BIRMASK].addr;
+    return entry - msix->entries;
 }
 
-static inline paddr_t vmsix_table_addr(const struct vpci *vpci, unsigned int nr)
+static inline void vpci_msi_free(struct vpci *vpci)
 {
-    return vmsix_table_base(vpci, nr) +
-           (vpci->msix->tables[nr] & ~PCI_MSIX_BIRMASK);
+    XFREE(vpci->msix);
+    XFREE(vpci->msi);
 }
-
-/*
- * Note regarding the size calculation of the PBA: the spec mentions "The last
- * QWORD will not necessarily be fully populated", so it implies that the PBA
- * size is 64-bit aligned.
- */
-static inline size_t vmsix_table_size(const struct vpci *vpci, unsigned int nr)
+#else /* !CONFIG_HAS_PCI_MSI_INTERCEPT */
+static inline int vpci_make_msix_hole(const struct pci_dev *pdev)
 {
-    return
-        (nr == VPCI_MSIX_TABLE) ? vpci->msix->max_entries * PCI_MSIX_ENTRY_SIZE
-                                : ROUNDUP(DIV_ROUND_UP(vpci->msix->max_entries,
-                                                       8), 8);
+    return 0;
 }
 
-static inline unsigned int vmsix_entry_nr(const struct vpci_msix *msix,
-                                          const struct vpci_msix_entry *entry)
+static inline int vpci_remove_msix_regions(const struct vpci *vpci,
+                                           struct rangeset *mem)
 {
-    return entry - msix->entries;
+    return 0;
 }
+static inline void vpci_msi_free(struct vpci *vpci) {}
+#endif /* CONFIG_HAS_PCI_MSI_INTERCEPT */
 #endif /* __XEN__ */
 
 #else /* !CONFIG_HAS_VPCI */
-- 
2.17.1

[PATCH v4 3/3] xen/pci: Refactor MSI code that implements MSI functionality within XEN

[Rahul Singh]

MSI code that implements MSI functionality to support MSI within XEN is
not usable on ARM. Move the code under CONFIG_HAS_PCI_MSI_INTERCEPT flag
to gate the code for ARM.

Currently, we have no idea how MSI functionality will be supported for
other architecture therefore we have decided to move the code under
CONFIG_PCI_MSI_INTERCEPT. We know this is not the right flag to gate the
code but to avoid an extra flag we decided to use this.

No functional change intended.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/msi-intercept.c | 41 +++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c           | 34 ++++----------------
 xen/include/xen/msi-intercept.h         |  7 +++++
 xen/include/xen/pci.h                   | 11 ++++---
 xen/xsm/flask/hooks.c                   |  8 ++---
 5 files changed, 65 insertions(+), 36 deletions(-)

diff --git a/xen/drivers/passthrough/msi-intercept.c b/xen/drivers/passthrough/msi-intercept.c
index ed3ec38003..12187bc9b9 100644
--- a/xen/drivers/passthrough/msi-intercept.c
+++ b/xen/drivers/passthrough/msi-intercept.c
@@ -19,6 +19,47 @@
 #include <asm/msi.h>
 #include <asm/hvm/io.h>
 
+int pdev_msi_init(struct pci_dev *pdev)
+{
+    unsigned int pos;
+
+    INIT_LIST_HEAD(&pdev->msi_list);
+
+    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
+    if ( pos )
+    {
+        uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
+
+        pdev->msi_maxvec = multi_msi_capable(ctrl);
+    }
+
+    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
+    if ( pos )
+    {
+        struct arch_msix *msix = xzalloc(struct arch_msix);
+        uint16_t ctrl;
+
+        if ( !msix )
+            return -ENOMEM;
+
+        spin_lock_init(&msix->table_lock);
+
+        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
+        msix->nr_entries = msix_table_size(ctrl);
+
+        pdev->msix = msix;
+    }
+
+    return 0;
+}
+
+void pdev_msi_deinit(struct pci_dev *pdev)
+{
+    XFREE(pdev->msix);
+}
+
 int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
 {
     int rc;
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 237461b4ab..a3ec85c293 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -314,6 +314,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
 {
     struct pci_dev *pdev;
     unsigned int pos;
+    int rc;
 
     list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
         if ( pdev->bus == bus && pdev->devfn == devfn )
@@ -327,35 +328,12 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
     *((u8*) &pdev->bus) = bus;
     *((u8*) &pdev->devfn) = devfn;
     pdev->domain = NULL;
-    INIT_LIST_HEAD(&pdev->msi_list);
-
-    pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                              PCI_CAP_ID_MSI);
-    if ( pos )
-    {
-        uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
 
-        pdev->msi_maxvec = multi_msi_capable(ctrl);
-    }
-
-    pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                              PCI_CAP_ID_MSIX);
-    if ( pos )
+    rc = pdev_msi_init(pdev);
+    if ( rc )
     {
-        struct arch_msix *msix = xzalloc(struct arch_msix);
-        uint16_t ctrl;
-
-        if ( !msix )
-        {
-            xfree(pdev);
-            return NULL;
-        }
-        spin_lock_init(&msix->table_lock);
-
-        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
-        msix->nr_entries = msix_table_size(ctrl);
-
-        pdev->msix = msix;
+        xfree(pdev);
+        return NULL;
     }
 
     list_add(&pdev->alldevs_list, &pseg->alldevs_list);
@@ -449,7 +427,7 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
     }
 
     list_del(&pdev->alldevs_list);
-    xfree(pdev->msix);
+    pdev_msi_deinit(pdev);
     xfree(pdev);
 }
 
diff --git a/xen/include/xen/msi-intercept.h b/xen/include/xen/msi-intercept.h
index 1bf9fc4465..7b094e08f7 100644
--- a/xen/include/xen/msi-intercept.h
+++ b/xen/include/xen/msi-intercept.h
@@ -21,16 +21,23 @@
 
 #include <asm/msi.h>
 
+int pdev_msi_init(struct pci_dev *pdev);
+void pdev_msi_deinit(struct pci_dev *pdev);
 int pdev_msix_assign(struct domain *d, struct pci_dev *pdev);
 void pdev_dump_msi(const struct pci_dev *pdev);
 
 #else /* !CONFIG_HAS_PCI_MSI_INTERCEPT */
+static inline int pdev_msi_init(struct pci_dev *pdev)
+{
+    return 0;
+}
 
 static inline int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
 {
     return 0;
 }
 
+static inline void pdev_msi_deinit(struct pci_dev *pdev) {}
 static inline void pdev_dump_msi(const struct pci_dev *pdev) {}
 static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
 
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 8e3d4d9454..01a92ce9e6 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -79,10 +79,6 @@ struct pci_dev {
     struct list_head alldevs_list;
     struct list_head domain_list;
 
-    struct list_head msi_list;
-
-    struct arch_msix *msix;
-
     struct domain *domain;
 
     const union {
@@ -94,7 +90,14 @@ struct pci_dev {
         pci_sbdf_t sbdf;
     };
 
+#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT
+    struct list_head msi_list;
+
+    struct arch_msix *msix;
+
     uint8_t msi_maxvec;
+#endif
+
     uint8_t phantom_stride;
 
     nodeid_t node; /* NUMA node */
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 5a24d01f04..394455cc42 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -21,7 +21,7 @@
 #include <xen/guest_access.h>
 #include <xen/xenoprof.h>
 #include <xen/iommu.h>
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT
 #include <asm/msi.h>
 #endif
 #include <public/xen.h>
@@ -114,7 +114,7 @@ static int get_irq_sid(int irq, u32 *sid, struct avc_audit_data *ad)
         }
         return security_irq_sid(irq, sid);
     }
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT
     {
         struct irq_desc *desc = irq_to_desc(irq);
         if ( desc->msi_desc && desc->msi_desc->dev ) {
@@ -874,7 +874,7 @@ static int flask_map_domain_pirq (struct domain *d)
 static int flask_map_domain_msi (struct domain *d, int irq, const void *data,
                                  u32 *sid, struct avc_audit_data *ad)
 {
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT
     const struct msi_info *msi = data;
     u32 machine_bdf = (msi->seg << 16) | (msi->bus << 8) | msi->devfn;
 
@@ -940,7 +940,7 @@ static int flask_unmap_domain_pirq (struct domain *d)
 static int flask_unmap_domain_msi (struct domain *d, int irq, const void *data,
                                    u32 *sid, struct avc_audit_data *ad)
 {
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT
     const struct pci_dev *pdev = data;
     u32 machine_bdf = (pdev->seg << 16) | (pdev->bus << 8) | pdev->devfn;
 
-- 
2.17.1

Re: [PATCH v4 2/3] xen/pci: Refactor PCI MSI intercept related code

[Jan Beulich]

On 29.04.2021 16:46, Rahul Singh wrote:
> --- /dev/null
> +++ b/xen/drivers/passthrough/msi-intercept.c
> @@ -0,0 +1,53 @@
> +/*
> + * Copyright (C) 2008,  Netronome Systems, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/init.h>
> +#include <xen/pci.h>
> +#include <asm/msi.h>
> +#include <asm/hvm/io.h>
> +
> +int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    if ( pdev->msix )
> +    {
> +        rc = pci_reset_msix_state(pdev);
> +        if ( rc )
> +            return rc;
> +        msixtbl_init(d);
> +    }
> +
> +    return 0;
> +}
> +
> +void pdev_dump_msi(const struct pci_dev *pdev)
> +{
> +    const struct msi_desc *msi;
> +
> +    list_for_each_entry ( msi, &pdev->msi_list, list )
> +        printk("- MSIs < %d >", msi->irq);

Only the %d and a blank should be part of the format string inside the
loop body; the rest wants printing exactly once.

> +static inline size_t vmsix_table_size(const struct vpci *vpci, unsigned int nr)
> +{
> +    return
> +        (nr == VPCI_MSIX_TABLE) ? vpci->msix->max_entries * PCI_MSIX_ENTRY_SIZE
> +                                : ROUNDUP(DIV_ROUND_UP(vpci->msix->max_entries,
> +                                                       8), 8);

I'm afraid I don't view this as an acceptable way of wrapping lines.
How about

    return (nr == VPCI_MSIX_TABLE)
           ? vpci->msix->max_entries * PCI_MSIX_ENTRY_SIZE
           : ROUNDUP(DIV_ROUND_UP(vpci->msix->max_entries, 8), 8);

> @@ -428,6 +458,31 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>      return 0;
>  }
>  
> +int vpci_remove_msix_regions(const struct vpci *vpci, struct rangeset *mem)
> +{
> +    const struct vpci_msix *msix = vpci->msix;
> +    unsigned int i;
> +    int rc;
> +
> +    for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
> +    {
> +        unsigned long start = PFN_DOWN(vmsix_table_addr(vpci, i));
> +        unsigned long end = PFN_DOWN(vmsix_table_addr(vpci, i) +
> +                vmsix_table_size(vpci, i) - 1);
> +
> +        rc = rangeset_remove_range(mem, start, end);
> +        if ( rc )
> +        {
> +            printk(XENLOG_G_WARNING
> +                    "Failed to remove MSIX table [%lx, %lx]: %d\n",
> +                    start, end, rc);

Indentation looks to be off by one space on the last two lines.

> --- /dev/null
> +++ b/xen/include/xen/msi-intercept.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright (C) 2008,  Netronome Systems, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __XEN_MSI_INTERCEPT_H_
> +#define __XEN_MSI_INTERCEPT_H_
> +
> +#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT
> +
> +#include <asm/msi.h>
> +
> +int pdev_msix_assign(struct domain *d, struct pci_dev *pdev);
> +void pdev_dump_msi(const struct pci_dev *pdev);
> +
> +#else /* !CONFIG_HAS_PCI_MSI_INTERCEPT */
> +
> +static inline int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
> +{
> +    return 0;
> +}
> +
> +static inline void pdev_dump_msi(const struct pci_dev *pdev) {}
> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}

I don't think this last one is intercept related (and hence doesn't belong
here)?

> @@ -148,6 +150,7 @@ struct vpci_vcpu {
>  };
>  
>  #ifdef __XEN__
> +#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT

Since both start and ...

> +static inline void vpci_msi_free(struct vpci *vpci) {}
> +#endif /* CONFIG_HAS_PCI_MSI_INTERCEPT */
>  #endif /* __XEN__ */

... end look to match, may I suggest to simply replace the __XEN__ ones,
as the test harness isn't supposed to (randomly) define CONFIG_*? Or
alternatively at least combine both #ifdef-s?

Jan

Re: [PATCH v4 3/3] xen/pci: Refactor MSI code that implements MSI functionality within XEN

[Jan Beulich]

On 29.04.2021 16:46, Rahul Singh wrote:
> MSI code that implements MSI functionality to support MSI within XEN is
> not usable on ARM. Move the code under CONFIG_HAS_PCI_MSI_INTERCEPT flag
> to gate the code for ARM.
> 
> Currently, we have no idea how MSI functionality will be supported for
> other architecture therefore we have decided to move the code under
> CONFIG_PCI_MSI_INTERCEPT. We know this is not the right flag to gate the
> code but to avoid an extra flag we decided to use this.

My objection remains: Actively putting code under the wrong gating
CONFIG_* is imo quite a bit worse than keeping it under a too wide one
(e.g. CONFIG_X86), if introducing a separate CONFIG_HAS_PCI_MSI is
deemed undesirable for whatever reason. Otherwise every abuse of
CONFIG_PCI_MSI_INTERCEPT ought to get a comment to the effect of this
being an abuse, which in particular for code you move into
xen/drivers/passthrough/msi-intercept.c would end up sufficiently odd.
(As a minor extra remark, putting deliberately misplaced code at the
top of a file rather than at its bottom is likely to add to possible
confusion down the road.)

Jan

Re: [PATCH v4 2/3] xen/pci: Refactor PCI MSI intercept related code

[Rahul Singh]

Hi Jan,

> On 3 May 2021, at 3:38 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 29.04.2021 16:46, Rahul Singh wrote:
>> --- /dev/null
>> +++ b/xen/drivers/passthrough/msi-intercept.c
>> @@ -0,0 +1,53 @@
>> +/*
>> + * Copyright (C) 2008,  Netronome Systems, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope 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.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program; If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/init.h>
>> +#include <xen/pci.h>
>> +#include <asm/msi.h>
>> +#include <asm/hvm/io.h>
>> +
>> +int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
>> +{
>> +    int rc;
>> +
>> +    if ( pdev->msix )
>> +    {
>> +        rc = pci_reset_msix_state(pdev);
>> +        if ( rc )
>> +            return rc;
>> +        msixtbl_init(d);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +void pdev_dump_msi(const struct pci_dev *pdev)
>> +{
>> +    const struct msi_desc *msi;
>> +
>> +    list_for_each_entry ( msi, &pdev->msi_list, list )
>> +        printk("- MSIs < %d >", msi->irq);
> 
> Only the %d and a blank should be part of the format string inside the
> loop body; the rest wants printing exactly once.

Yes I agree I missed this I will fix this in next patch.
 
> 
>> +static inline size_t vmsix_table_size(const struct vpci *vpci, unsigned int nr)
>> +{
>> +    return
>> +        (nr == VPCI_MSIX_TABLE) ? vpci->msix->max_entries * PCI_MSIX_ENTRY_SIZE
>> +                                : ROUNDUP(DIV_ROUND_UP(vpci->msix->max_entries,
>> +                                                       8), 8);
> 
> I'm afraid I don't view this as an acceptable way of wrapping lines.
> How about
> 
>    return (nr == VPCI_MSIX_TABLE)
>           ? vpci->msix->max_entries * PCI_MSIX_ENTRY_SIZE
>           : ROUNDUP(DIV_ROUND_UP(vpci->msix->max_entries, 8), 8);

Ok.

> 
>> @@ -428,6 +458,31 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>>     return 0;
>> }
>> 
>> +int vpci_remove_msix_regions(const struct vpci *vpci, struct rangeset *mem)
>> +{
>> +    const struct vpci_msix *msix = vpci->msix;
>> +    unsigned int i;
>> +    int rc;
>> +
>> +    for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
>> +    {
>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(vpci, i));
>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(vpci, i) +
>> +                vmsix_table_size(vpci, i) - 1);
>> +
>> +        rc = rangeset_remove_range(mem, start, end);
>> +        if ( rc )
>> +        {
>> +            printk(XENLOG_G_WARNING
>> +                    "Failed to remove MSIX table [%lx, %lx]: %d\n",
>> +                    start, end, rc);
> 
> Indentation looks to be off by one space on the last two lines.

Ok.

> 
>> --- /dev/null
>> +++ b/xen/include/xen/msi-intercept.h
>> @@ -0,0 +1,49 @@
>> +/*
>> + * Copyright (C) 2008,  Netronome Systems, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope 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.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program; If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __XEN_MSI_INTERCEPT_H_
>> +#define __XEN_MSI_INTERCEPT_H_
>> +
>> +#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT
>> +
>> +#include <asm/msi.h>
>> +
>> +int pdev_msix_assign(struct domain *d, struct pci_dev *pdev);
>> +void pdev_dump_msi(const struct pci_dev *pdev);
>> +
>> +#else /* !CONFIG_HAS_PCI_MSI_INTERCEPT */
>> +
>> +static inline int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static inline void pdev_dump_msi(const struct pci_dev *pdev) {}
>> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
> 
> I don't think this last one is intercept related (and hence doesn't belong
> here)?
> 
Ok I will move this to next patch in series.
>> @@ -148,6 +150,7 @@ struct vpci_vcpu {
>> };
>> 
>> #ifdef __XEN__
>> +#ifdef CONFIG_HAS_PCI_MSI_INTERCEPT
> 
> Since both start and ...
> 
>> +static inline void vpci_msi_free(struct vpci *vpci) {}
>> +#endif /* CONFIG_HAS_PCI_MSI_INTERCEPT */
>> #endif /* __XEN__ */
> 
> ... end look to match, may I suggest to simply replace the __XEN__ ones,
> as the test harness isn't supposed to (randomly) define CONFIG_*? Or
> alternatively at least combine both #ifdef-s?

Ok I will replace the line "#ifdef __XEN__"  with  " #ifdef CONFIG_HAS_PCI_MSI_INTERCEPT”

Regards,
Rahul

> 
> Jan
> 

Re: [PATCH v4 3/3] xen/pci: Refactor MSI code that implements MSI functionality within XEN

[Rahul Singh]

Hi Jan,

> On 3 May 2021, at 3:46 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 29.04.2021 16:46, Rahul Singh wrote:
>> MSI code that implements MSI functionality to support MSI within XEN is
>> not usable on ARM. Move the code under CONFIG_HAS_PCI_MSI_INTERCEPT flag
>> to gate the code for ARM.
>> 
>> Currently, we have no idea how MSI functionality will be supported for
>> other architecture therefore we have decided to move the code under
>> CONFIG_PCI_MSI_INTERCEPT. We know this is not the right flag to gate the
>> code but to avoid an extra flag we decided to use this.
> 
> My objection remains: Actively putting code under the wrong gating
> CONFIG_* is imo quite a bit worse than keeping it under a too wide one
> (e.g. CONFIG_X86), if introducing a separate CONFIG_HAS_PCI_MSI is
> deemed undesirable for whatever reason. Otherwise every abuse of
> CONFIG_PCI_MSI_INTERCEPT ought to get a comment to the effect of this
> being an abuse, which in particular for code you move into
> xen/drivers/passthrough/msi-intercept.c would end up sufficiently odd.
> (As a minor extra remark, putting deliberately misplaced code at the
> top of a file rather than at its bottom is likely to add to possible
> confusion down the road.)
> 

I understand that this is not the correct flag to gate the code. If we choose to
move the code under CONFIG_X86 there will be #ifdef in the common file
"passthrough/pci.c” that I think will make code harder to understand. The only
option left is to introduce the new CONFIG_HAS_PCI_MSI  option and new
non-arch files (msi.c, msi.h). Move all non-intercept-related code to those files.

As I mention earlier also this is code and as of now we have no data on how MSI
will be supported for non-x86 architecture that’s why we decided it is better to move
the code under CONFIG_PCI_MSI_INTERCEPT and later point in time we can
modify the code once non-x86 architecture implements MSI functionality in XEN if required.

I will move the code at the bottom of the file to avoid confusion.

Regards,
Rahul
> Jan