Patchwork [V7,11/11] xen passthrough: clean up MSI-X table handling

login
register
mail settings
Submitter Anthony PERARD
Date Feb. 17, 2012, 5:08 p.m.
Message ID <1329498525-8454-12-git-send-email-anthony.perard@citrix.com>
Download mbox | patch
Permalink /patch/141924/
State New
Headers show

Comments

Anthony PERARD - Feb. 17, 2012, 5:08 p.m.
From: Shan Haitao <maillists.shan@gmail.com>

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 <maillists.shan@gmail.com>

Consolidated duplicate code into _pt_iomem_helper(). Fixed formatting.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 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(-)
Jan Beulich - Feb. 21, 2012, 9:34 a.m.
>>> On 17.02.12 at 18:08, Anthony PERARD <anthony.perard@citrix.com> wrote:

Wouldn't thus much better be merged into the prior patch(es)? After
all you're not trying to reconstruct the Xen-side history of this code
anyway.

Jan

> From: Shan Haitao <maillists.shan@gmail.com>
> 
> 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 <maillists.shan@gmail.com>
> 
> Consolidated duplicate code into _pt_iomem_helper(). Fixed formatting.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  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
Anthony PERARD - Feb. 21, 2012, 4:15 p.m.
On Tue, Feb 21, 2012 at 09:34, Jan Beulich <JBeulich@suse.com> wrote:
> Wouldn't thus much better be merged into the prior patch(es)? After
> all you're not trying to reconstruct the Xen-side history of this code
> anyway.

Yes, that probably better than having an extra patch. There is a
"link" to the history, anyway. I'll merge the patch.

Thanks,

Patch

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