[RFC,for-5.1,3/4] spapr: Fix failure path for attempting to hot unplug PCI bridges
diff mbox series

Message ID 20200326054009.454477-4-david@gibson.dropbear.id.au
State New
Headers show
Series
  • Better handling of attempt NVLink2 unplug
Related show

Commit Message

David Gibson March 26, 2020, 5:40 a.m. UTC
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(+)

Comments

Greg Kurz March 26, 2020, 12:18 p.m. UTC | #1
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 */
David Gibson March 26, 2020, 11:54 p.m. UTC | #2
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 */
>

Patch
diff mbox series

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 */