| Message ID | 20200326054009.454477-4-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | Better handling of attempt NVLink2 unplug | expand |
On Thu, 26 Mar 2020 16:40:08 +1100 David Gibson <david@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 <david@gibson.dropbear.id.au> > --- Reviewed-by: Greg Kurz <groug@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 */
On Thu, Mar 26, 2020 at 01:18:24PM +0100, Greg Kurz wrote: > On Thu, 26 Mar 2020 16:40:08 +1100 > David Gibson <david@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. Fair point. I've added the tag and moved to ppc-for-5.0. > > Maybe add this tag ? > > Fixes: 14e714900f6b "spapr: Allow hot plug/unplug of PCI bridges and devices under PCI bridges" > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > Reviewed-by: Greg Kurz <groug@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 */ >
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 */
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, it doesn't actually stop trying to unplug the bridge anyway. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr_pci.c | 1 + 1 file changed, 1 insertion(+)