[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with
From: |
Marcel Apfelbaum |
Subject: |
Re: [Qemu-devel] [PATCH v2] hw/pci: fixed crash when using rombar=0 with romfile=path for hotplugged devices |
Date: |
Mon, 27 Oct 2014 17:04:58 +0200 |
On Mon, 2014-10-27 at 15:59 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <address@hidden> 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 <address@hidden>
> > ---
> > 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.