On Tue, 6 Oct 2015, Paolo Bonzini wrote: > On 05/10/2015 18:53, Stefano Stabellini wrote: > >> 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? > > This is the right thing to do, but there are two separate things to fix. > > One is the use-after-free of msix->mmio, the other is that freeing > s->msix and in general xen_pt_config_delete should be done from the > .instance_finalize callback. This is documented in docs/memory.txt. > > This is an attempt at a patch (not even compiled): > > diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c > index e3d7194..e476bac 100644 > --- a/hw/xen/xen_pt_msi.c > +++ b/hw/xen/xen_pt_msi.c > @@ -610,7 +610,7 @@ error_out: > return rc; > } > > -void xen_pt_msix_delete(XenPCIPassthroughState *s) > +void xen_pt_msix_unmap(XenPCIPassthroughState *s) > { > XenPTMSIX *msix = s->msix; > > @@ -627,6 +627,17 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s) > } > > memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio); > +} > + > +void xen_pt_msix_delete(XenPCIPassthroughState *s) > +{ > + XenPTMSIX *msix = s->msix; > + > + if (!msix) { > + return; > + } > + > + object_unparent(&msix->mmio); > > g_free(s->msix); > s->msix = NULL; > > where xen_pt_config_unmap would be called from xen_pt_destroy, and the call > to xen_pt_config_delete would be moved to xen_pci_passthrough_info's > instance_finalize member.
Thanks for the explanation and the code. It makes sense to me. Lan, could you please write up a patch based on this approach and test it?