Marcel Apfelbaum <marce...@redhat.com> writes:
> 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.
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.
