Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Mon, 2014-10-27 at 15:59 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > Combining rombar=0 with romfile=<path> is an user error,
> > silently dropping the romfile is a reasonable thing to do.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> > v1 -> v2:
> >  After a discussion with Michael, Paolo and Alex, this
> >  patch silent drops the romfile instead of not allowing
> >  the hotplug.
> >  
> >  An OK from libvirt will be nice.
> >
> >  hw/pci/pci.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 6ce75aa..cd7a403 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1776,7 +1776,12 @@ static int pci_qdev_init(DeviceState *qdev)
> >          pci_dev->romfile = g_strdup(pc->romfile);
> >          is_default_rom = true;
> >      }
> > -    pci_add_option_rom(pci_dev, is_default_rom);
> > +
> > +    rc = pci_add_option_rom(pci_dev, is_default_rom);
> > +    if (rc != 0) {
> > +        pci_unregister_device(DEVICE(pci_dev));
> > +        return rc;
> > +    }
> >  
> >      return 0;
> >  }
> 
> Looks like this part isn't covered by the commit message.
> 
> Before: errors in pci_add_option_rom() are reported, but the function
> succeeds anyway.  After: it fails.  Is this a silent bug fix?
Yes, it made sense for V1, less for V2, but I wanted to keep it.
Should I split in 2 patches?

> 
> > @@ -1937,6 +1942,15 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> >           * for 0.11 compatibility.
> >           */
> >          int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> > +
> > +        /*
> > +         * For hot-plugged devices silently ignore the option ROM
> > +         * if the rom bar is disabled.
> > +         */
> > +        if (DEVICE(pdev)->hotplugged) {
> > +            return 0;
> > +        }
> > +
> >          if (class == 0x0300) {
> >              rom_add_vga(pdev->romfile);
> >          } else {
> 
> This part is covered by the commit message.