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 14:40 +0100, Markus Armbruster wrote:
>> 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?
>
> If I understand your question correctly:
> I would first convert to realize, then add the fix.
> The reason for it is pretty simple: Conversion to realize
> gives us the error flow propagation we need.

I'd do it the other way round, because

1. Before your series, pci_add_option_rom() can already fail.  It always
reports an error when it fails.  Good, except the caller ignores
failure.

2. Your PATCH 1/2 fixes the caller.  Good.

3. Your PATCH 2/2 adds a failure that doesn't report an error.  Bad,
because it leaves the user guessing what went wrong.  I view that as a
bug.

I'd like this bug to be fixed for 2.2.  Since Michael wants to delay my
"pci: Partial conversion to 2.3, that means fixing it before conversion
to realize.