qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Fix PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memor


From: Stefano Stabellini
Subject: Re: [Qemu-devel] [Fix PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region
Date: Mon, 5 Oct 2015 17:53:12 +0100
User-agent: Alpine 2.02 (DEB 1266 2009-07-14)

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 <address@hidden>
>
>  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
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]