| Message ID | 1414431282-9839-3-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
Marcel Apfelbaum <marcel.a@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 <eblake@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel.a@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?
On Mon, Nov 03, 2014 at 01:03:30PM +0100, Markus Armbruster wrote: > Marcel Apfelbaum <marcel.a@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 <eblake@redhat.com> > > Signed-off-by: Marcel Apfelbaum <marcel.a@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? Marcel, Markus, as I applied the patch already, if you want to fix things up pls send a new patch on top. Thanks!
On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote: > Marcel Apfelbaum <marcel.a@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 <eblake@redhat.com> > > Signed-off-by: Marcel Apfelbaum <marcel.a@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. Thanks, Marcel
Marcel Apfelbaum <marcel.a@redhat.com> writes: > On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote: >> Marcel Apfelbaum <marcel.a@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 <eblake@redhat.com> >> > Signed-off-by: Marcel Apfelbaum <marcel.a@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?
On Mon, 2014-11-03 at 14:40 +0100, Markus Armbruster wrote: > Marcel Apfelbaum <marcel.a@redhat.com> writes: > > > On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote: > >> Marcel Apfelbaum <marcel.a@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 <eblake@redhat.com> > >> > Signed-off-by: Marcel Apfelbaum <marcel.a@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
Marcel Apfelbaum <marcel.a@redhat.com> writes: > On Mon, 2014-11-03 at 14:40 +0100, Markus Armbruster wrote: >> Marcel Apfelbaum <marcel.a@redhat.com> writes: >> >> > On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote: >> >> Marcel Apfelbaum <marcel.a@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 <eblake@redhat.com> >> >> > Signed-off-by: Marcel Apfelbaum <marcel.a@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.
On Mon, 2014-11-03 at 16:54 +0100, Markus Armbruster wrote: > Marcel Apfelbaum <marcel.a@redhat.com> writes: > > > On Mon, 2014-11-03 at 14:40 +0100, Markus Armbruster wrote: > >> Marcel Apfelbaum <marcel.a@redhat.com> writes: > >> > >> > On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote: > >> >> Marcel Apfelbaum <marcel.a@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 <eblake@redhat.com> > >> >> > Signed-off-by: Marcel Apfelbaum <marcel.a@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. I thought your patches will be part of 2.2. I have nothing against it. Thanks, Marcel
On Mon, Nov 03, 2014 at 04:54:25PM +0100, Markus Armbruster wrote: > Marcel Apfelbaum <marcel.a@redhat.com> writes: > > > On Mon, 2014-11-03 at 14:40 +0100, Markus Armbruster wrote: > >> Marcel Apfelbaum <marcel.a@redhat.com> writes: > >> > >> > On Mon, 2014-11-03 at 13:03 +0100, Markus Armbruster wrote: > >> >> Marcel Apfelbaum <marcel.a@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 <eblake@redhat.com> > >> >> > Signed-off-by: Marcel Apfelbaum <marcel.a@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. I didn't look at this deeply yes, so I don't know how hard would it be to fix without pulling in realize. If easy, go for it. If hard, we can pull in realize for 2.2.
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 {