On Mon, 2014-11-03 at 14:40 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marce...@redhat.com> writes:
>
> > On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote:
> >> Marcel Apfelbaum <marce...@redhat.com> 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 <ebl...@redhat.com>
> >> > Signed-off-by: Marcel Apfelbaum <marce...@redhat.com>
> >> > ---
> >> > 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.
Thanks,
Marcel
