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

[Alex Williamson <alex.williamson@redhat.com>]

On Thu,  5 Jul 2018 20:11:48 +0200
Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org>
> ---
>  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,

Alex