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

[Michael S. Tsirkin]

On Thu, Jul 05, 2018 at 08:11:48PM +0200, Cédric Le Goater 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")

I don't see it in that commit. I think it was part of the original
split by Avi.

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

What kind of testing did you do on this patch? Could you include
that info in the commit log pls?

I think you need to at least add/remove some devices, then migrate.

> ---
>  hw/pci/pci.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 80bc45930dee..78bf74e19f22 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -191,7 +191,6 @@ static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num);
>  static void pci_update_mappings(PCIDevice *d);
>  static void pci_irq_handler(void *opaque, int irq_num, int level);
>  static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, Error 
> **);
> -static void pci_del_option_rom(PCIDevice *pdev);
>  
>  static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET;
>  static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU;
> @@ -1096,7 +1095,6 @@ static void pci_qdev_unrealize(DeviceState *dev, Error 
> **errp)
>      PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
>  
>      pci_unregister_io_regions(pci_dev);
> -    pci_del_option_rom(pci_dev);
>  
>      if (pc->exit) {
>          pc->exit(pci_dev);
> @@ -2262,15 +2260,6 @@ static void pci_add_option_rom(PCIDevice *pdev, bool 
> is_default_rom,
>      pci_register_bar(pdev, PCI_ROM_SLOT, 0, &pdev->rom);
>  }
>  
> -static void pci_del_option_rom(PCIDevice *pdev)
> -{
> -    if (!pdev->has_rom)
> -        return;
> -
> -    vmstate_unregister_ram(&pdev->rom, &pdev->qdev);
> -    pdev->has_rom = false;
> -}
> -
>  /*
>   * On success, pci_add_capability() returns a positive value
>   * that the offset of the pci capability.
> -- 
> 2.13.6