[Qemu-devel] [PATCH V7 11/11] xen passthrough: clean up MSI-X table handling

[Anthony PERARD]

From: Shan Haitao <address@hidden>

This patch does cleaning up of QEMU MSI handling. The fixes are:
1. Changes made to MSI-X table mapping handling to eliminate the small
windows in which guest could have access to physical MSI-X table.
2. MSI-X table is mapped as read-only to QEMU, as masking of MSI-X is
already in Xen now.
3. For registers that coexists inside the MSI-X table (this could be
only PBA I think), value read from physical page would be returned.

Signed-off-by:  Shan Haitao <address@hidden>

Consolidated duplicate code into _pt_iomem_helper(). Fixed formatting.

Signed-off-by: Jan Beulich <address@hidden>
Signed-off-by: Anthony PERARD <address@hidden>
---
 hw/xen_pci_passthrough.c     |   80 ++++++++++++++++++++++++++++++++----------
 hw/xen_pci_passthrough.h     |   11 +++++-
 hw/xen_pci_passthrough_msi.c |   62 ++++----------------------------
 3 files changed, 78 insertions(+), 75 deletions(-)

diff --git a/hw/xen_pci_passthrough.c b/hw/xen_pci_passthrough.c
index 6ea2c45..fab9fbe 100644
--- a/hw/xen_pci_passthrough.c
+++ b/hw/xen_pci_passthrough.c
@@ -356,6 +356,53 @@ out:
     }
 }
 
+static int pt_iomem_helper(XenPCIPassthroughState *s, int i,
+                           pcibus_t e_base, pcibus_t e_size, int op)
+{
+    uint64_t msix_last_pfn = 0;
+    pcibus_t bar_last_pfn = 0;
+    pcibus_t msix_table_size = 0;
+    XenPTMSIX *msix = s->msix;
+    int rc = 0;
+
+    if (!pt_has_msix_mapping(s, i)) {
+        return xc_domain_memory_mapping(xen_xc, xen_domid,
+                                        PFN(e_base),
+                                        PFN(s->bases[i].access.maddr),
+                                        PFN(e_size + XC_PAGE_SIZE - 1),
+                                        op);
+
+    }
+
+    if (msix->table_off) {
+        rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+                                      PFN(e_base),
+                                      PFN(s->bases[i].access.maddr),
+                                      PFN(msix->mmio_base_addr) - PFN(e_base),
+                                      op);
+    }
+
+    msix_table_size = msix->total_entries * PCI_MSIX_ENTRY_SIZE;
+    msix_last_pfn = PFN(msix->mmio_base_addr + msix_table_size - 1);
+    bar_last_pfn = PFN(e_base + e_size - 1);
+
+    if (rc == 0 && msix_last_pfn != bar_last_pfn) {
+        assert(msix_last_pfn < bar_last_pfn);
+
+        rc = xc_domain_memory_mapping(xen_xc, xen_domid,
+                                      msix_last_pfn + 1,
+                                      PFN(s->bases[i].access.maddr
+                                          + msix->table_off
+                                          + msix_table_size
+                                          + XC_PAGE_SIZE - 1),
+                                      bar_last_pfn - msix_last_pfn,
+                                      op);
+    }
+
+    return rc;
+}
+
+
 /* ioport/iomem space*/
 static void pt_iomem_map(XenPCIPassthroughState *s, int i,
                          pcibus_t e_phys, pcibus_t e_size)
@@ -376,13 +423,10 @@ static void pt_iomem_map(XenPCIPassthroughState *s, int i,
     }
 
     if (!first_map && old_ebase != PT_PCI_BAR_UNMAPPED) {
-        pt_add_msix_mapping(s, i);
-        /* Remove old mapping */
-        rc = xc_domain_memory_mapping(xen_xc, xen_domid,
-                               old_ebase >> XC_PAGE_SHIFT,
-                               s->bases[i].access.maddr >> XC_PAGE_SHIFT,
-                               (e_size + XC_PAGE_SIZE - 1) >> XC_PAGE_SHIFT,
-                               DPCI_REMOVE_MAPPING);
+        if (pt_has_msix_mapping(s, i)) {
+            memory_region_set_enabled(&s->msix->mmio, false);
+        }
+        rc = pt_iomem_helper(s, i, old_ebase, e_size, DPCI_REMOVE_MAPPING);
         if (rc) {
             PT_ERR(&s->dev, "remove old mapping failed! (rc: %i)\n", rc);
             return;
@@ -391,21 +435,19 @@ static void pt_iomem_map(XenPCIPassthroughState *s, int i,
 
     /* map only valid guest address */
     if (e_phys != PCI_BAR_UNMAPPED) {
-        /* Create new mapping */
-        rc = xc_domain_memory_mapping(xen_xc, xen_domid,
-                                   s->bases[i].e_physbase >> XC_PAGE_SHIFT,
-                                   s->bases[i].access.maddr >> XC_PAGE_SHIFT,
-                                   (e_size+XC_PAGE_SIZE-1) >> XC_PAGE_SHIFT,
-                                   DPCI_ADD_MAPPING);
+        if (pt_has_msix_mapping(s, i)) {
+            XenPTMSIX *msix = s->msix;
 
-        if (rc) {
-            PT_ERR(&s->dev, "create new mapping failed! (rc: %i)\n", rc);
+            msix->mmio_base_addr = s->bases[i].e_physbase + msix->table_off;
+
+            memory_region_set_enabled(&msix->mmio, true);
         }
 
-        rc = pt_remove_msix_mapping(s, i);
-        if (rc != 0) {
-            PT_ERR(&s->dev, "Remove MSI-X MMIO mapping failed! (rc: %d)\n",
-                   rc);
+        rc = pt_iomem_helper(s, i, e_phys, e_size, DPCI_ADD_MAPPING);
+
+        if (rc) {
+            PT_ERR(&s->dev, "create new mapping failed! (rc: %i)\n", rc);
+            return;
         }
 
         if (old_ebase != e_phys && old_ebase != -1) {
diff --git a/hw/xen_pci_passthrough.h b/hw/xen_pci_passthrough.h
index 3d41e04..f36e0d2 100644
--- a/hw/xen_pci_passthrough.h
+++ b/hw/xen_pci_passthrough.h
@@ -30,6 +30,9 @@ void pt_log(const PCIDevice *d, const char *f, ...) 
GCC_FMT_ATTR(2, 3);
 #endif
 
 
+/* Helper */
+#define PFN(x) ((x) >> XC_PAGE_SHIFT)
+
 typedef struct XenPTRegInfo XenPTRegInfo;
 typedef struct XenPTReg XenPTReg;
 
@@ -295,7 +298,11 @@ void pt_msix_delete(XenPCIPassthroughState *s);
 int pt_msix_update(XenPCIPassthroughState *s);
 int pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
 void pt_msix_disable(XenPCIPassthroughState *s);
-int pt_add_msix_mapping(XenPCIPassthroughState *s, int bar_index);
-int pt_remove_msix_mapping(XenPCIPassthroughState *s, int bar_index);
+
+static inline bool pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
+{
+    return s->msix && s->msix->bar_index == bar;
+}
+
 
 #endif /* !QEMU_HW_XEN_PCI_PASSTHROUGH_H */
diff --git a/hw/xen_pci_passthrough_msi.c b/hw/xen_pci_passthrough_msi.c
index e848c61..e775d21 100644
--- a/hw/xen_pci_passthrough_msi.c
+++ b/hw/xen_pci_passthrough_msi.c
@@ -300,16 +300,6 @@ static int msix_set_enable(XenPCIPassthroughState *s, bool 
enabled)
                            enabled);
 }
 
-static void mask_physical_msix_entry(XenPCIPassthroughState *s,
-                                     int entry_nr, int mask)
-{
-    void *phys_off;
-
-    phys_off = s->msix->phys_iomem_base + PCI_MSIX_ENTRY_SIZE * entry_nr
-        + PCI_MSIX_ENTRY_VECTOR_CTRL;
-    *(uint32_t *)phys_off = mask;
-}
-
 static int pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr)
 {
     XenPTMSIXEntry *entry = NULL;
@@ -480,8 +470,6 @@ static void pci_msix_write(void *opaque, target_phys_addr_t 
addr,
         if (msix->enabled && !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
             pt_msix_update_one(s, entry_nr);
         }
-        mask_physical_msix_entry(s, entry_nr,
-            entry->vector_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT);
     }
 }
 
@@ -493,14 +481,19 @@ static uint64_t pci_msix_read(void *opaque, 
target_phys_addr_t addr,
     int entry_nr, offset;
 
     entry_nr = addr / PCI_MSIX_ENTRY_SIZE;
-    if (entry_nr < 0 || entry_nr >= msix->total_entries) {
+    if (entry_nr < 0) {
         PT_ERR(&s->dev, "asked MSI-X entry '%i' invalid!\n", entry_nr);
         return 0;
     }
 
     offset = addr % PCI_MSIX_ENTRY_SIZE;
 
-    return get_entry_value(&msix->msix_entry[entry_nr], offset);
+    if (addr < msix->total_entries * PCI_MSIX_ENTRY_SIZE) {
+        return get_entry_value(&msix->msix_entry[entry_nr], offset);
+    } else {
+        /* Pending Bit Array (PBA) */
+        return *(uint32_t *)(msix->phys_iomem_base + addr);
+    }
 }
 
 static const MemoryRegionOps pci_msix_ops = {
@@ -514,45 +507,6 @@ static const MemoryRegionOps pci_msix_ops = {
     },
 };
 
-int pt_add_msix_mapping(XenPCIPassthroughState *s, int bar_index)
-{
-    XenPTMSIX *msix = s->msix;
-
-    if (!(s->msix && s->msix->bar_index == bar_index)) {
-        return 0;
-    }
-
-    memory_region_set_enabled(&msix->mmio, false);
-    return xc_domain_memory_mapping(xen_xc, xen_domid,
-         s->msix->mmio_base_addr >> XC_PAGE_SHIFT,
-         (s->bases[bar_index].access.maddr + s->msix->table_off)
-           >> XC_PAGE_SHIFT,
-         (s->msix->total_entries * PCI_MSIX_ENTRY_SIZE + XC_PAGE_SIZE - 1)
-           >> XC_PAGE_SHIFT,
-         DPCI_ADD_MAPPING);
-}
-
-int pt_remove_msix_mapping(XenPCIPassthroughState *s, int bar_index)
-{
-    XenPTMSIX *msix = s->msix;
-
-    if (!(msix && msix->bar_index == bar_index)) {
-        return 0;
-    }
-
-    memory_region_set_enabled(&msix->mmio, true);
-
-    msix->mmio_base_addr = s->bases[bar_index].e_physbase + msix->table_off;
-
-    return xc_domain_memory_mapping(xen_xc, xen_domid,
-         s->msix->mmio_base_addr >> XC_PAGE_SHIFT,
-         (s->bases[bar_index].access.maddr + s->msix->table_off)
-             >> XC_PAGE_SHIFT,
-         (s->msix->total_entries * PCI_MSIX_ENTRY_SIZE + XC_PAGE_SIZE - 1)
-             >> XC_PAGE_SHIFT,
-         DPCI_REMOVE_MAPPING);
-}
-
 int pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
 {
     uint8_t id = 0;
@@ -609,7 +563,7 @@ int pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
     msix->phys_iomem_base =
         mmap(NULL,
              total_entries * PCI_MSIX_ENTRY_SIZE + msix->table_offset_adjust,
-             PROT_WRITE | PROT_READ,
+             PROT_READ,
              MAP_SHARED | MAP_LOCKED,
              fd,
              msix->table_base + table_off - msix->table_offset_adjust);
-- 
Anthony PERARD