diff mbox

[Fix] Qemu/Xen: Fix early freeing MSIX MMIO memory region

Message ID 1443595897-405-1-git-send-email-tianyu.lan@intel.com
State New
Headers show

Commit Message

Lan Tianyu Sept. 30, 2015, 6:51 a.m. UTC
MSIX MMIO memory region is added to pt device's obj as property.
When pt device is unplugged, all properties will be deleted and
memory region's obj is needed at that point(refer object_finalize_child_property()).
But current code frees MSIX MMIO memory region in the xen_pt_msix_delete()
before deleting pt device's properties, this will cause segment fault.
Reproduce the bug via hotplugging device frequently.

This patch is to fix the issue via moving MSIX MMIO memory region into
struct XenPCIPassthroughState and free it together with pt device's obj.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 hw/xen/xen_pt.c     |    4 ++--
 hw/xen/xen_pt.h     |    2 +-
 hw/xen/xen_pt_msi.c |    6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

Comments

Stefano Stabellini Oct. 5, 2015, 4:53 p.m. UTC | #1
Hi Lan, thanks for the patch.

On Wed, 30 Sep 2015, Lan Tianyu wrote:
> MSIX MMIO memory region is added to pt device's obj as property.

msix->mmio is added to XenPCIPassthroughState's object as property


> When pt device is unplugged, all properties will be deleted and
> memory region's obj is needed at that point(refer object_finalize_child_property()).

object_finalize_child_property is called for XenPCIPassthroughState's
object, which calls object_property_del_all, which is going to try to
delete (again) msix->mmio. But the whole msix struct could have already
been freed by xen_pt_msix_delete.


> But current code frees MSIX MMIO memory region in the xen_pt_msix_delete()
> before deleting pt device's properties, this will cause segment fault.
> Reproduce the bug via hotplugging device frequently.

What is the line of code under object_property_del_all that actually
causes the seg fault?


> This patch is to fix the issue via moving MSIX MMIO memory region into
> struct XenPCIPassthroughState and free it together with pt device's obj.

Given that all the MSI-X related info are in XenPTMSIX, I would prefer
to keep the mmio memory region there, if possible.

Couldn't you just unhook msix->mmio from XenPCIPassthroughState's object
in xen_pt_msix_delete?  Calling object_property_del_child or
object_unparent?


> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>
>  hw/xen/xen_pt.c     |    4 ++--
>  hw/xen/xen_pt.h     |    2 +-
>  hw/xen/xen_pt_msi.c |    6 +++---
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 2b54f52..0c11069 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -587,11 +587,11 @@ static void xen_pt_region_update(XenPCIPassthroughState *s,
>      };
>  
>      bar = xen_pt_bar_from_region(s, mr);
> -    if (bar == -1 && (!s->msix || &s->msix->mmio != mr)) {
> +    if (bar == -1 && (!s->msix || &s->msix_mmio != mr)) {
>          return;
>      }
>  
> -    if (s->msix && &s->msix->mmio == mr) {
> +    if (s->msix && &s->msix_mmio == mr) {
>          if (adding) {
>              s->msix->mmio_base_addr = sec->offset_within_address_space;
>              rc = xen_pt_msix_update_remap(s, s->msix->bar_index);
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 3bc22eb..3569c2c 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -199,7 +199,6 @@ typedef struct XenPTMSIX {
>      uint64_t table_base;
>      uint32_t table_offset_adjust; /* page align mmap */
>      uint64_t mmio_base_addr;
> -    MemoryRegion mmio;
>      void *phys_iomem_base;
>      XenPTMSIXEntry msix_entry[0];
>  } XenPTMSIX;
> @@ -222,6 +221,7 @@ struct XenPCIPassthroughState {
>  
>      MemoryRegion bar[PCI_NUM_REGIONS - 1];
>      MemoryRegion rom;
> +    MemoryRegion msix_mmio;
>  
>      MemoryListener memory_listener;
>      MemoryListener io_listener;
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index e3d7194..ae39ab3 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -558,7 +558,7 @@ int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
>          msix->msix_entry[i].pirq = XEN_PT_UNASSIGNED_PIRQ;
>      }
>  
> -    memory_region_init_io(&msix->mmio, OBJECT(s), &pci_msix_ops,
> +    memory_region_init_io(&s->msix_mmio, OBJECT(s), &pci_msix_ops,
>                            s, "xen-pci-pt-msix",
>                            (total_entries * PCI_MSIX_ENTRY_SIZE
>                             + XC_PAGE_SIZE - 1)
> @@ -599,7 +599,7 @@ int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
>                 msix->phys_iomem_base);
>  
>      memory_region_add_subregion_overlap(&s->bar[bar_index], table_off,
> -                                        &msix->mmio,
> +                                        &s->msix_mmio,
>                                          2); /* Priority: pci default + 1 */
>  
>      return 0;
> @@ -626,7 +626,7 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s)
>                 + msix->table_offset_adjust);
>      }
>  
> -    memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
> +    memory_region_del_subregion(&s->bar[msix->bar_index], &s->msix_mmio);
>  
>      g_free(s->msix);
>      s->msix = NULL;
> -- 
> 1.7.9.5
>
diff mbox

Patch

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 2b54f52..0c11069 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -587,11 +587,11 @@  static void xen_pt_region_update(XenPCIPassthroughState *s,
     };
 
     bar = xen_pt_bar_from_region(s, mr);
-    if (bar == -1 && (!s->msix || &s->msix->mmio != mr)) {
+    if (bar == -1 && (!s->msix || &s->msix_mmio != mr)) {
         return;
     }
 
-    if (s->msix && &s->msix->mmio == mr) {
+    if (s->msix && &s->msix_mmio == mr) {
         if (adding) {
             s->msix->mmio_base_addr = sec->offset_within_address_space;
             rc = xen_pt_msix_update_remap(s, s->msix->bar_index);
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 3bc22eb..3569c2c 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -199,7 +199,6 @@  typedef struct XenPTMSIX {
     uint64_t table_base;
     uint32_t table_offset_adjust; /* page align mmap */
     uint64_t mmio_base_addr;
-    MemoryRegion mmio;
     void *phys_iomem_base;
     XenPTMSIXEntry msix_entry[0];
 } XenPTMSIX;
@@ -222,6 +221,7 @@  struct XenPCIPassthroughState {
 
     MemoryRegion bar[PCI_NUM_REGIONS - 1];
     MemoryRegion rom;
+    MemoryRegion msix_mmio;
 
     MemoryListener memory_listener;
     MemoryListener io_listener;
diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index e3d7194..ae39ab3 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -558,7 +558,7 @@  int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
         msix->msix_entry[i].pirq = XEN_PT_UNASSIGNED_PIRQ;
     }
 
-    memory_region_init_io(&msix->mmio, OBJECT(s), &pci_msix_ops,
+    memory_region_init_io(&s->msix_mmio, OBJECT(s), &pci_msix_ops,
                           s, "xen-pci-pt-msix",
                           (total_entries * PCI_MSIX_ENTRY_SIZE
                            + XC_PAGE_SIZE - 1)
@@ -599,7 +599,7 @@  int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
                msix->phys_iomem_base);
 
     memory_region_add_subregion_overlap(&s->bar[bar_index], table_off,
-                                        &msix->mmio,
+                                        &s->msix_mmio,
                                         2); /* Priority: pci default + 1 */
 
     return 0;
@@ -626,7 +626,7 @@  void xen_pt_msix_delete(XenPCIPassthroughState *s)
                + msix->table_offset_adjust);
     }
 
-    memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
+    memory_region_del_subregion(&s->bar[msix->bar_index], &s->msix_mmio);
 
     g_free(s->msix);
     s->msix = NULL;