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 16:52 +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 27, 2014 at 03:44:04PM +0200, Marcel Apfelbaum wrote:
> > 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>
> 
> How about failing adding the device instead?
> Return error from pci_add_option_rom, and check at
> calling sites?

This was was the prev version of this patch has done,
we have only one calling site:  pci_qdev_init.
I could tweak the prev version to return error on both
rom_add_vga/rom_add_option, but I was under the impression
that silently drop the romfile was discussion's decision.

I am fine both ways, as it is a user error and hopefully
used correctly by libvirt. I only want to avoid the crash.

Thanks,
Marcel

> 
> > ---
> > 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;
> >  }
> > @@ -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 {
> > -- 
> > 1.8.3.1