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

[Paolo Bonzini]

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