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

[PATCH v3 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 | 93 +++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c           | 57 +++------------
 xen/drivers/pci/Kconfig                 |  4 ++
 xen/drivers/vpci/Makefile               |  3 +-
 xen/drivers/vpci/header.c               | 19 ++---
 xen/drivers/vpci/msix.c                 | 25 +++++++
 xen/drivers/vpci/vpci.c                 |  3 +-
 xen/include/xen/iommu.h                 | 13 +++-
 xen/include/xen/msi-intercept.h         | 55 +++++++++++++++
 xen/include/xen/pci.h                   | 11 +--
 xen/include/xen/vpci.h                  | 23 ++++++
 xen/xsm/flask/hooks.c                   |  8 +--
 14 files changed, 241 insertions(+), 75 deletions(-)
 create mode 100644 xen/drivers/passthrough/msi-intercept.c
 create mode 100644 xen/include/xen/msi-intercept.h

-- 
2.17.1

[PATCH v3 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>
---
Changes since v2:
- This patch is added in this version to make sure iommu related code is in
  separate patch.
---
 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

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

[Jan Beulich]

On 26.04.2021 18:21, Rahul Singh wrote:
> 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>

[PATCH v3 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_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>
---
Changes since v1:
 - Rename CONFIG_HAS_PCI_MSI to CONFIG_PCI_MSI_INTERCEPT
 - Implement stub version of the unused function for ARM.
 - Move unneeded function to x86 file.

Changes since v2:
 - Rename function name as per the comments.
 - Created a separate non-arch specific file msi-intercept.c.
---
 xen/arch/x86/Kconfig                    |  1 +
 xen/drivers/passthrough/Makefile        |  1 +
 xen/drivers/passthrough/msi-intercept.c | 52 +++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c           | 16 +++-----
 xen/drivers/pci/Kconfig                 |  4 ++
 xen/drivers/vpci/Makefile               |  3 +-
 xen/drivers/vpci/header.c               | 19 ++-------
 xen/drivers/vpci/msix.c                 | 25 ++++++++++++
 xen/drivers/vpci/vpci.c                 |  3 +-
 xen/include/xen/msi-intercept.h         | 48 +++++++++++++++++++++++
 xen/include/xen/vpci.h                  | 23 +++++++++++
 xen/xsm/flask/hooks.c                   |  8 ++--
 12 files changed, 170 insertions(+), 33 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..4d72798f00 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 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..3c707706b0 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_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..1edae6d4e8
--- /dev/null
+++ b/xen/drivers/passthrough/msi-intercept.c
@@ -0,0 +1,52 @@
+/*
+ * Copyright (C) 2021 Arm Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ *
+ * 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("%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..298be21b5b 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,7 +1271,6 @@ 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);
 
@@ -1280,8 +1279,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
         printk("%pp - %pd - node %-3d - MSIs < ",
                &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);
+        pdev_dump_msi(pdev);
         printk(">\n");
     }
 
@@ -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..e6bf0b7441 100644
--- a/xen/drivers/pci/Kconfig
+++ b/xen/drivers/pci/Kconfig
@@ -1,3 +1,7 @@
 
 config HAS_PCI
 	bool
+
+config PCI_MSI_INTERCEPT
+	bool
+	depends on HAS_PCI
diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
index 55d1bdfda0..f91fa71a40 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_PCI_MSI_INTERCEPT) += msi.o msix.o
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index ba9a036202..ec735c5b4b 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, 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..3efbaebc92 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -428,6 +428,31 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
     return 0;
 }
 
+int vpci_remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem)
+{
+    const struct vpci_msix *msix = pdev->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(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);
+            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..77c105e286
--- /dev/null
+++ b/xen/include/xen/msi-intercept.h
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) 2021 Arm Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ *
+ * 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_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_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_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..2cd3647305 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_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_PCI_MSI_INTERCEPT */
 #endif
 };
 
@@ -148,6 +150,7 @@ struct vpci_vcpu {
 };
 
 #ifdef __XEN__
+#ifdef CONFIG_PCI_MSI_INTERCEPT
 void vpci_dump_msi(void);
 
 /* Make sure there's a hole in the p2m for the MSIX mmio areas. */
@@ -174,6 +177,7 @@ 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 pci_dev *pdev, struct rangeset *mem);
 
 /*
  * Helper functions to fetch MSIX related data. They are used by both the
@@ -208,6 +212,25 @@ static inline unsigned int vmsix_entry_nr(const struct vpci_msix *msix,
 {
     return entry - msix->entries;
 }
+
+static inline void vpci_msi_free(struct vpci *vpci)
+{
+    XFREE(vpci->msix);
+    XFREE(vpci->msi);
+}
+#else /* !CONFIG_PCI_MSI_INTERCEPT */
+static inline int vpci_make_msix_hole(const struct pci_dev *pdev)
+{
+    return 0;
+}
+
+static inline int vpci_remove_msix_regions(const struct pci_dev *pdev,
+                                      struct rangeset *mem)
+{
+    return 0;
+}
+static inline void vpci_msi_free(struct vpci *vpci) {}
+#endif /* CONFIG_PCI_MSI_INTERCEPT */
 #endif /* __XEN__ */
 
 #else /* !CONFIG_HAS_VPCI */
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 5a24d01f04..d87f56fc95 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_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_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_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_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 v3 2/3] xen/pci: Refactor PCI MSI intercept related code

[Roger Pau Monné]

On Mon, Apr 26, 2021 at 05:21:26PM +0100, Rahul Singh wrote:
> MSI intercept related code is not useful for ARM when MSI interrupts are
> injected via GICv3 ITS.
> 
> Therefore introducing the new flag CONFIG_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>
> ---
> Changes since v1:
>  - Rename CONFIG_HAS_PCI_MSI to CONFIG_PCI_MSI_INTERCEPT
>  - Implement stub version of the unused function for ARM.
>  - Move unneeded function to x86 file.
> 
> Changes since v2:
>  - Rename function name as per the comments.
>  - Created a separate non-arch specific file msi-intercept.c.
> ---
>  xen/arch/x86/Kconfig                    |  1 +
>  xen/drivers/passthrough/Makefile        |  1 +
>  xen/drivers/passthrough/msi-intercept.c | 52 +++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c           | 16 +++-----
>  xen/drivers/pci/Kconfig                 |  4 ++
>  xen/drivers/vpci/Makefile               |  3 +-
>  xen/drivers/vpci/header.c               | 19 ++-------
>  xen/drivers/vpci/msix.c                 | 25 ++++++++++++
>  xen/drivers/vpci/vpci.c                 |  3 +-
>  xen/include/xen/msi-intercept.h         | 48 +++++++++++++++++++++++
>  xen/include/xen/vpci.h                  | 23 +++++++++++
>  xen/xsm/flask/hooks.c                   |  8 ++--
>  12 files changed, 170 insertions(+), 33 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..4d72798f00 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 PCI_MSI_INTERCEPT
>  	select HAS_PDX
>  	select HAS_SCHED_GRANULARITY
>  	select HAS_UBSAN

This list is sorted alphabetically AFAICT, and new additions should
respect that.

> diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
> index 445690e3e5..3c707706b0 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_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..1edae6d4e8
> --- /dev/null
> +++ b/xen/drivers/passthrough/msi-intercept.c
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright (C) 2021 Arm Ltd.

Since this is code movement, I think it should keep the copyright of
the file they are moved from.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + *
> + * 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("%d ", msi->irq);
> +}
> +/*

Missing newline.

> + * 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..298be21b5b 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,7 +1271,6 @@ 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);
>  
> @@ -1280,8 +1279,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
>          printk("%pp - %pd - node %-3d - MSIs < ",
>                 &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);
> +        pdev_dump_msi(pdev);
>          printk(">\n");

Maybe the whole '- MSIs < ... >' should be removed, instead of
printing an empty list of MSIs when MSI interception is not supported?

Or else you give the impression that no MSIs are in use, when instead
Xen is not tracking them.

>      }
>  
> @@ -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..e6bf0b7441 100644
> --- a/xen/drivers/pci/Kconfig
> +++ b/xen/drivers/pci/Kconfig
> @@ -1,3 +1,7 @@
>  
>  config HAS_PCI
>  	bool
> +
> +config PCI_MSI_INTERCEPT
> +	bool
> +	depends on HAS_PCI
> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
> index 55d1bdfda0..f91fa71a40 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_PCI_MSI_INTERCEPT) += msi.o msix.o
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index ba9a036202..ec735c5b4b 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, 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..3efbaebc92 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -428,6 +428,31 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>      return 0;
>  }
>  
> +int vpci_remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem)

You could pass struct vpci here, since there's no need to pass the
pdev, but I don't really have a strong opinion.

> +{
> +    const struct vpci_msix *msix = pdev->vpci->msix;
> +    unsigned int i;
> +    int rc;

Nit: you can define rc inside the loop, like it's done for start and
end.

> +
> +    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);

You can now also move vmsix_table_{base,addr,size} to be local static
inlines to msix.c instead of being defined in the header AFAICT.

> +
> +        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..77c105e286
> --- /dev/null
> +++ b/xen/include/xen/msi-intercept.h
> @@ -0,0 +1,48 @@
> +/*
> + * Copyright (C) 2021 Arm Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + *
> + * 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_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_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_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..2cd3647305 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_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_PCI_MSI_INTERCEPT */
>  #endif
>  };
>  
> @@ -148,6 +150,7 @@ struct vpci_vcpu {
>  };
>  
>  #ifdef __XEN__
> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>  void vpci_dump_msi(void);
>  
>  /* Make sure there's a hole in the p2m for the MSIX mmio areas. */
> @@ -174,6 +177,7 @@ 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 pci_dev *pdev, struct rangeset *mem);
>  
>  /*
>   * Helper functions to fetch MSIX related data. They are used by both the
> @@ -208,6 +212,25 @@ static inline unsigned int vmsix_entry_nr(const struct vpci_msix *msix,
>  {
>      return entry - msix->entries;
>  }
> +
> +static inline void vpci_msi_free(struct vpci *vpci)
> +{
> +    XFREE(vpci->msix);
> +    XFREE(vpci->msi);
> +}
> +#else /* !CONFIG_PCI_MSI_INTERCEPT */
> +static inline int vpci_make_msix_hole(const struct pci_dev *pdev)
> +{
> +    return 0;
> +}
> +
> +static inline int vpci_remove_msix_regions(const struct pci_dev *pdev,
> +                                      struct rangeset *mem)

Nit: line seems to not be properly aligned.

Thanks, Roger.

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

[Jan Beulich]

On 28.04.2021 12:42, Roger Pau Monné wrote:
> On Mon, Apr 26, 2021 at 05:21:26PM +0100, Rahul Singh wrote:
>> --- 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 PCI_MSI_INTERCEPT
>>  	select HAS_PDX
>>  	select HAS_SCHED_GRANULARITY
>>  	select HAS_UBSAN
> 
> This list is sorted alphabetically AFAICT, and new additions should
> respect that.

Since this isn't a user visible option, perhaps it wants to be
HAS_PCI_MSI_INTERCEPT anyway?

Jan

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

[Rahul Singh]

Hi Jan,

> On 28 Apr 2021, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 28.04.2021 12:42, Roger Pau Monné wrote:
>> On Mon, Apr 26, 2021 at 05:21:26PM +0100, Rahul Singh wrote:
>>> --- 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 PCI_MSI_INTERCEPT
>>> 	select HAS_PDX
>>> 	select HAS_SCHED_GRANULARITY
>>> 	select HAS_UBSAN
>> 
>> This list is sorted alphabetically AFAICT, and new additions should
>> respect that.
> 
> Since this isn't a user visible option, perhaps it wants to be
> HAS_PCI_MSI_INTERCEPT anyway?
> 

Yes I agree with you I will change the name to HAS_PCI_MSI_INTERCEPT in next version.

Regards,
Rahul
> Jan
> 

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

[Rahul Singh]

Hi Roger,

> On 28 Apr 2021, at 11:42 am, Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> On Mon, Apr 26, 2021 at 05:21:26PM +0100, Rahul Singh wrote:
>> MSI intercept related code is not useful for ARM when MSI interrupts are
>> injected via GICv3 ITS.
>> 
>> Therefore introducing the new flag CONFIG_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>
>> ---
>> Changes since v1:
>> - Rename CONFIG_HAS_PCI_MSI to CONFIG_PCI_MSI_INTERCEPT
>> - Implement stub version of the unused function for ARM.
>> - Move unneeded function to x86 file.
>> 
>> Changes since v2:
>> - Rename function name as per the comments.
>> - Created a separate non-arch specific file msi-intercept.c.
>> ---
>> xen/arch/x86/Kconfig                    |  1 +
>> xen/drivers/passthrough/Makefile        |  1 +
>> xen/drivers/passthrough/msi-intercept.c | 52 +++++++++++++++++++++++++
>> xen/drivers/passthrough/pci.c           | 16 +++-----
>> xen/drivers/pci/Kconfig                 |  4 ++
>> xen/drivers/vpci/Makefile               |  3 +-
>> xen/drivers/vpci/header.c               | 19 ++-------
>> xen/drivers/vpci/msix.c                 | 25 ++++++++++++
>> xen/drivers/vpci/vpci.c                 |  3 +-
>> xen/include/xen/msi-intercept.h         | 48 +++++++++++++++++++++++
>> xen/include/xen/vpci.h                  | 23 +++++++++++
>> xen/xsm/flask/hooks.c                   |  8 ++--
>> 12 files changed, 170 insertions(+), 33 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..4d72798f00 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 PCI_MSI_INTERCEPT
>> 	select HAS_PDX
>> 	select HAS_SCHED_GRANULARITY
>> 	select HAS_UBSAN
> 
> This list is sorted alphabetically AFAICT, and new additions should
> respect that.

Ok. As per the Jan request, I will modify the Kconfig name HAS_PCI_MSI_INTERCEPT. 
> 
>> diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
>> index 445690e3e5..3c707706b0 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_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..1edae6d4e8
>> --- /dev/null
>> +++ b/xen/drivers/passthrough/msi-intercept.c
>> @@ -0,0 +1,52 @@
>> +/*
>> + * Copyright (C) 2021 Arm Ltd.
> 
> Since this is code movement, I think it should keep the copyright of
> the file they are moved from.
> 

Ok. 

>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * 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.
>> + *
>> + * 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("%d ", msi->irq);
>> +}
>> +/*
> 
> Missing newline.

Ack.
> 
>> + * 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..298be21b5b 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,7 +1271,6 @@ 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);
>> 
>> @@ -1280,8 +1279,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
>>         printk("%pp - %pd - node %-3d - MSIs < ",
>>                &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);
>> +        pdev_dump_msi(pdev);
>>         printk(">\n");
> 
> Maybe the whole '- MSIs < ... >' should be removed, instead of
> printing an empty list of MSIs when MSI interception is not supported?
> 
> Or else you give the impression that no MSIs are in use, when instead
> Xen is not tracking them.
> 
Ok I will remove - MSIs < … > when  MSI interception is not supported.
>>     }
>> 
>> @@ -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..e6bf0b7441 100644
>> --- a/xen/drivers/pci/Kconfig
>> +++ b/xen/drivers/pci/Kconfig
>> @@ -1,3 +1,7 @@
>> 
>> config HAS_PCI
>> 	bool
>> +
>> +config PCI_MSI_INTERCEPT
>> +	bool
>> +	depends on HAS_PCI
>> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
>> index 55d1bdfda0..f91fa71a40 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_PCI_MSI_INTERCEPT) += msi.o msix.o
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index ba9a036202..ec735c5b4b 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, 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..3efbaebc92 100644
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -428,6 +428,31 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>>     return 0;
>> }
>> 
>> +int vpci_remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem)
> 
> You could pass struct vpci here, since there's no need to pass the
> pdev, but I don't really have a strong opinion.
Ack.
> 
>> +{
>> +    const struct vpci_msix *msix = pdev->vpci->msix;
>> +    unsigned int i;
>> +    int rc;
> 
> Nit: you can define rc inside the loop, like it's done for start and
> end.

Ack.
> 
>> +
>> +    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);
> 
> You can now also move vmsix_table_{base,addr,size} to be local static
> inlines to msix.c instead of being defined in the header AFAICT.

Ok.
> 
>> +
>> +        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..77c105e286
>> --- /dev/null
>> +++ b/xen/include/xen/msi-intercept.h
>> @@ -0,0 +1,48 @@
>> +/*
>> + * Copyright (C) 2021 Arm Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * 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.
>> + *
>> + * 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_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_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_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..2cd3647305 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_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_PCI_MSI_INTERCEPT */
>> #endif
>> };
>> 
>> @@ -148,6 +150,7 @@ struct vpci_vcpu {
>> };
>> 
>> #ifdef __XEN__
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>> void vpci_dump_msi(void);
>> 
>> /* Make sure there's a hole in the p2m for the MSIX mmio areas. */
>> @@ -174,6 +177,7 @@ 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 pci_dev *pdev, struct rangeset *mem);
>> 
>> /*
>>  * Helper functions to fetch MSIX related data. They are used by both the
>> @@ -208,6 +212,25 @@ static inline unsigned int vmsix_entry_nr(const struct vpci_msix *msix,
>> {
>>     return entry - msix->entries;
>> }
>> +
>> +static inline void vpci_msi_free(struct vpci *vpci)
>> +{
>> +    XFREE(vpci->msix);
>> +    XFREE(vpci->msi);
>> +}
>> +#else /* !CONFIG_PCI_MSI_INTERCEPT */
>> +static inline int vpci_make_msix_hole(const struct pci_dev *pdev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static inline int vpci_remove_msix_regions(const struct pci_dev *pdev,
>> +                                      struct rangeset *mem)
> 
> Nit: line seems to not be properly aligned.

Ack.

Regards,
Rahul
> 
> Thanks, Roger.

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

[Jan Beulich]

On 26.04.2021 18:21, Rahul Singh wrote:
> --- 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_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_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_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_PCI_MSI_INTERCEPT
>      const struct pci_dev *pdev = data;
>      u32 machine_bdf = (pdev->seg << 16) | (pdev->bus << 8) | pdev->devfn;
>  

Is all of this really related to MSI interception? Iirc the code here
has been around for much longer, and hence is more related to MSI
support in Xen in general (as required for PV guests in particular).

Jan

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

[Rahul Singh]

Hi Jan,

> On 28 Apr 2021, at 3:06 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 26.04.2021 18:21, Rahul Singh wrote:
>> --- 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_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_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_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_PCI_MSI_INTERCEPT
>>     const struct pci_dev *pdev = data;
>>     u32 machine_bdf = (pdev->seg << 16) | (pdev->bus << 8) | pdev->devfn;
>> 
> 
> Is all of this really related to MSI interception? Iirc the code here
> has been around for much longer, and hence is more related to MSI
> support in Xen in general (as required for PV guests in particular).

Yes I agree this code is not related to MSI interception but I decide to move the code 
under CONFIG_PCI_MSI_INTERCEPT flag to gate the code for ARM as to avoid an 
extra flag. We can modify this code once we have more data how MSI will be 
supported for other architecture.  

Regards,
Rahul
> 
> Jan

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

[Jan Beulich]

On 29.04.2021 13:31, Rahul Singh wrote:
>> On 28 Apr 2021, at 3:06 pm, Jan Beulich <jbeulich@suse.com> wrote:
>> On 26.04.2021 18:21, Rahul Singh wrote:
>>> --- 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_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_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_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_PCI_MSI_INTERCEPT
>>>     const struct pci_dev *pdev = data;
>>>     u32 machine_bdf = (pdev->seg << 16) | (pdev->bus << 8) | pdev->devfn;
>>>
>>
>> Is all of this really related to MSI interception? Iirc the code here
>> has been around for much longer, and hence is more related to MSI
>> support in Xen in general (as required for PV guests in particular).
> 
> Yes I agree this code is not related to MSI interception but I decide to move the code 
> under CONFIG_PCI_MSI_INTERCEPT flag to gate the code for ARM as to avoid an 
> extra flag. We can modify this code once we have more data how MSI will be 
> supported for other architecture.  

In any event the change belongs into the other patch. My objection
there then still holds, but if I'm to be outvoted there, so be it.

Jan

[PATCH v3 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_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>
---
Changes since v2:
 - This patch is added in this version.
---
 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 ++++---
 4 files changed, 61 insertions(+), 32 deletions(-)

diff --git a/xen/drivers/passthrough/msi-intercept.c b/xen/drivers/passthrough/msi-intercept.c
index 1edae6d4e8..33ab71514d 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 298be21b5b..b1e3c711ad 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 77c105e286..38ff7a09e1 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_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..f5b57270be 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_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 */
-- 
2.17.1

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

[Roger Pau Monné]

On Mon, Apr 26, 2021 at 05:21:27PM +0100, Rahul Singh wrote:
> MSI code that implements MSI functionality to support MSI within XEN is
> not usable on ARM. Move the code under CONFIG_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>

I think this is fine, as we don't really want to add another Kconfig
option (ie: CONFIG_PCI_MSI) for just the non explicitly intercept MSI
code.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Some nits below...

> ---
> Changes since v2:
>  - This patch is added in this version.
> ---
>  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 ++++---
>  4 files changed, 61 insertions(+), 32 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/msi-intercept.c b/xen/drivers/passthrough/msi-intercept.c
> index 1edae6d4e8..33ab71514d 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 298be21b5b..b1e3c711ad 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);

There's no need to use XFREE here, plain xfree is fine since pdev is a
local variable so makes no sense to assign to NULL before returning.

> +        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 77c105e286..38ff7a09e1 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_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..f5b57270be 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_PCI_MSI_INTERCEPT
> +    struct list_head msi_list;
> +
> +    struct arch_msix *msix;
> +
>      uint8_t msi_maxvec;
> +#endif

This seems to introduce some padding between the sbdf and the msi_list
field. I guess that's better than having two different
CONFIG_PCI_MSI_INTERCEPT guarded regions?

Thanks, Roger.

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

[Jan Beulich]

On 28.04.2021 13:06, Roger Pau Monné wrote:
> On Mon, Apr 26, 2021 at 05:21:27PM +0100, Rahul Singh wrote:
>> MSI code that implements MSI functionality to support MSI within XEN is
>> not usable on ARM. Move the code under CONFIG_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>
> 
> I think this is fine, as we don't really want to add another Kconfig
> option (ie: CONFIG_PCI_MSI) for just the non explicitly intercept MSI
> code.

While a separate config option may be excessive, keying it to the
wrong one is not desirable imo. If we want to avoid having PCI_MSI,
then keeping respective bits guarded by X86 would look better to me.
I'm not convinced though that doing the separation properly right
away (see also the XSM changes in patch 2 which imo belong here)
isn't going to be better in the long run, and hence introducing
HAS_PCI_MSI right away isn't the way to go.

Jan

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

[Julien Grall]

On 28/04/2021 12:06, Roger Pau Monné wrote:
> On Mon, Apr 26, 2021 at 05:21:27PM +0100, Rahul Singh wrote:
>> MSI code that implements MSI functionality to support MSI within XEN is
>> not usable on ARM. Move the code under CONFIG_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>
> 
> I think this is fine, as we don't really want to add another Kconfig
> option (ie: CONFIG_PCI_MSI) for just the non explicitly intercept MSI
> code.

+1 This is code and therefore can be revisited once we have more data 
(e.g. a second arch that will re-use it).

> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

Cheers,

-- 
Julien Grall

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

[Rahul Singh]

Hi Roger,

> On 28 Apr 2021, at 12:06 pm, Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> On Mon, Apr 26, 2021 at 05:21:27PM +0100, Rahul Singh wrote:
>> MSI code that implements MSI functionality to support MSI within XEN is
>> not usable on ARM. Move the code under CONFIG_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>
> 
> I think this is fine, as we don't really want to add another Kconfig
> option (ie: CONFIG_PCI_MSI) for just the non explicitly intercept MSI
> code.
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 

Thanks.

> Some nits below...
> 
>> ---
>> Changes since v2:
>> - This patch is added in this version.
>> ---
>> 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 ++++---
>> 4 files changed, 61 insertions(+), 32 deletions(-)
>> 
>> diff --git a/xen/drivers/passthrough/msi-intercept.c b/xen/drivers/passthrough/msi-intercept.c
>> index 1edae6d4e8..33ab71514d 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 298be21b5b..b1e3c711ad 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);
> 
> There's no need to use XFREE here, plain xfree is fine since pdev is a
> local variable so makes no sense to assign to NULL before returning.

Ok. I will change it to xfree in next version.

> 
>> +        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 77c105e286..38ff7a09e1 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_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..f5b57270be 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_PCI_MSI_INTERCEPT
>> +    struct list_head msi_list;
>> +
>> +    struct arch_msix *msix;
>> +
>>     uint8_t msi_maxvec;
>> +#endif
> 
> This seems to introduce some padding between the sbdf and the msi_list
> field. I guess that's better than having two different
> CONFIG_PCI_MSI_INTERCEPT guarded regions?

Yes. That’s why I move all the fields related to MSI to one place to avoid the 
extra CONFIG_PCI_MSI_INTERCEPT instance.

Regards,
Rahul
> 
> Thanks, Roger.