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

[Michael S. Tsirkin]

On Mon, Oct 27, 2014 at 05:04:03PM +0200, Marcel Apfelbaum wrote:
> 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 <address@hidden>
> > 
> > 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

Let's try the non silent error.
If someone is unhappy we can make it silent again, but
if it succeeds we'll have to keep it working forever.


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