Re: [Qemu-devel] [PATCH] pci: remove pci_del_option_rom()

[Peter Xu]

On Thu, Jul 05, 2018 at 11:44:44PM +0200, Paolo Bonzini wrote:
> On 05/07/2018 21:30, Alex Williamson wrote:
> > On Thu,  5 Jul 2018 20:11:48 +0200
> > Cédric Le Goater <address@hidden> wrote:
> > 
> >> PCI devices needing a ROM allocate an optional MemoryRegion with
> >> pci_add_option_rom(). pci_del_option_rom() does the cleanup when the
> >> device is destroyed. The only action taken by this routine is to call
> >> vmstate_unregister_ram() which clears the id string of the optional
> >> ROM RAMBlock and now, also flags the RAMBlock as non-migratable. This
> >> was recently added by commit b895de502717 ("migration: discard
> >> non-migratable RAMBlocks"), .
> >>
> >> VFIO devices do their own allocation of the PCI ROM region. It is
> >> initialized in vfio_pci_size_rom() in which the PCI attribute
> >> 'has_rom' is set to true but the RAMBlock of the ROM region is not
> >> allocated. When the associated PCI device is deleted,
> >> pci_del_option_rom() calls vmstate_unregister_ram() which tries to
> >> flag a NULL RAMBlock because 'has_rom' is set, leading to a SEGV .
> >>
> >> The use of vmstate_unregister_ram() in the PCI device was added in
> >> commit b0e56e0b63f350691b52d3e75e89bb64143fbeff ("unset RAMBlock idstr
> >> when unregister MemoryRegion") and from the archive in
> >> http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00282.html, it
> >> seems that it was trying to fix a reference count issue.
> >>
> >> vmstate_unregister_ram() being a work around, let's remove it to fix
> >> the current SEGV issue and let's try to find a fix for the initial ref
> >> count issue if we can reproduce.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  hw/pci/pci.c | 11 -----------
> >>  1 file changed, 11 deletions(-)
> > 
> > Looking back through git history, I think vfio sets PCIDevice.has_rom
> > because we needed that to have memory_region_destroy() called, but that
> > changed with Paolo's:
> > 
> > 469b046ead06 ("memory: remove memory_region_destroy")
> > 
> > Now the MemoryRegion gets freed automagically, so maybe the better
> > option is that vfio-pci should not set has_rom to keep it out of this
> > path.  I don't see that has_rom serves any other purpose.  Thanks,
> 
> That would work for me too!

Paolo,

A question about memory region auto destruction (which might not
related to this patch): I see that we have object_property_add_child()
in memory_region_do_init() to achieve the auto destruction but only if
the "name" of memory region is specified.  Could we just do that
unconditionally (though we might of course need to generate some of
the names), or is there a reason not to do so?

Also, not sure whether it's good we add some more comments to
memory_region_do_init() to mention about its auto destruction
capability, and if the "name" check is needed, possibly we comment
that as well (I can do that after I understand it well, if you like)?

Thanks,

-- 
Peter Xu