Re: [Qemu-devel] [PATCH v4 2/2] hw/pci: fixed hotplug crash when using rombar=0 with devices having romfile

[Markus Armbruster]

Marcel Apfelbaum <address@hidden> writes:

> On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote:
>> Marcel Apfelbaum <address@hidden> writes:
>> 
>> > Hot-plugging a device that has a romfile (either supplied by user
>> > or built-in) using rombar=0 option is a user error,
>> > do not allow the device to be hot-plugged.
>> >
>> > Reviewed-by: Eric Blake <address@hidden>
>> > Signed-off-by: Marcel Apfelbaum <address@hidden>
>> > ---
>> >  hw/pci/pci.c | 9 +++++++++
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> > index 36226eb..371699c 100644
>> > --- a/hw/pci/pci.c
>> > +++ b/hw/pci/pci.c
>> > @@ -1942,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);
>> > +
>> > +        /*
>> > +         * Hot-plugged devices can't use the option ROM
>> > +         * if the rom bar is disabled.
>> > +         */
>> > +        if (DEVICE(pdev)->hotplugged) {
>> > +            return -1;
>> > +        }
>> > +
>> >          if (class == 0x0300) {
>> >              rom_add_vga(pdev->romfile);
>> >          } else {
>> 
>> Unlike the function's other unsuccessful returns, this one is silent.
>> Intentional?
> Yes, the first version included an error message, but was not accepted
> as the reviewers preferred "silent drop" instead.
> The main reason was that a proper error propagation mechanism
> should be used.
> At the time of the patch there was not such an option, but now there is.
> I can add it on top of your series, preferably after is merged.

My rebased "pci: Convert core to realize" has this hunk:

    @@ -1948,7 +1955,9 @@ static int pci_add_option_rom(PCIDevice *pdev, bool 
is_default_rom)
              * if the rom bar is disabled.
              */
             if (DEVICE(pdev)->hotplugged) {
    -            return -1;
    +            error_setg(errp, "Hot-plugged device without ROM bar"
    +                       " can't have an option ROM");
    +            return;
             }

             if (class == 0x0300) {

Bad, because the patch does two separate things: fix a failure not to be
silent, and convert to realize.  Needs to be split.  Begs the question
how to order the parts.  I'd prefer to put the fix first, and get it
into 2.2.  What do you think?