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....@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
>
