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

[Marcel Apfelbaum]

On Mon, 2014-11-03 at 16:54 +0100, Markus Armbruster wrote:
> 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.
I thought your patches will be part of 2.2.
I have nothing against it.

Thanks,
Marcel