On Thu, 26 Mar 2020 16:40:08 +1100
David Gibson <da...@gibson.dropbear.id.au> wrote:
> For various technical reasons we can't currently allow unplug a PCI to PCI
> bridge on the pseries machine. spapr_pci_unplug_request() correctly
> generates an error message if that's attempted.
>
> But.. if the given errp is not error_abort or error_fatal,
Which is the always case when trying to unplug a device AFAICT:
void qdev_unplug(DeviceState *dev, Error **errp)
{
DeviceClass *dc = DEVICE_GET_CLASS(dev);
HotplugHandler *hotplug_ctrl;
HotplugHandlerClass *hdc;
Error *local_err = NULL;
[...]
hdc = HOTPLUG_HANDLER_GET_CLASS(hotplug_ctrl);
if (hdc->unplug_request) {
hotplug_handler_unplug_request(hotplug_ctrl, dev, &local_err);
And anyway, spapr_pci_unplug_request() shouldn't rely on the caller
passing &error_fatal or &error_abort to do the right thing. Calling
error_setg() without returning right away is a dangerous practice
since it would cause a subsequent call to error_setg() with the
same errp to abort QEMU.
> it doesn't actually stop trying to unplug the bridge anyway.
>
This looks like a bug fix that could go to 5.0 IMHO.
Maybe add this tag ?
Fixes: 14e714900f6b "spapr: Allow hot plug/unplug of PCI bridges and devices
under PCI bridges"
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
Reviewed-by: Greg Kurz <gr...@kaod.org>
> hw/ppc/spapr_pci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 709a52780d..55ca9dee1e 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1663,6 +1663,7 @@ static void spapr_pci_unplug_request(HotplugHandler
> *plug_handler,
>
> if (pc->is_bridge) {
> error_setg(errp, "PCI: Hot unplug of PCI bridges not supported");
> + return;
> }
>
> /* ensure any other present functions are pending unplug */
