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 <marce...@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
