diff mbox series

[for-6.0,1/9] spapr: Do PCI device hotplug sanity checks at pre-plug only

Message ID 20201120234208.683521-2-groug@kaod.org (mailing list archive)
State New, archived
Headers show
Series spapr: Perform hotplug sanity checks at pre-plug | expand

Commit Message

Greg Kurz Nov. 20, 2020, 11:42 p.m. UTC 
The PHB acts as the hotplug handler for PCI devices. It does some
sanity checks on DR enablement, PCI bridge chassis numbers and
multifunction. These checks are currently performed at plug time,
but they would best sit in a pre-plug handler in order to error
out as early as possible.

Create a spapr_pci_pre_plug() handler and move all the checking
there. Add a check that the associated DRC doesn't already have
an attached device. This is equivalent to the slot availability
check performed by do_pci_register_device() upon realization of
the PCI device.

This allows to pass &error_abort to spapr_drc_attach() and to end
up with a plug handler that doesn't need to report errors anymore.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_pci.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

Comments

David Gibson Nov. 23, 2020, 4:50 a.m. UTC | #1 
On Sat, Nov 21, 2020 at 12:42:00AM +0100, Greg Kurz wrote:
> The PHB acts as the hotplug handler for PCI devices. It does some
> sanity checks on DR enablement, PCI bridge chassis numbers and
> multifunction. These checks are currently performed at plug time,
> but they would best sit in a pre-plug handler in order to error
> out as early as possible.
> 
> Create a spapr_pci_pre_plug() handler and move all the checking
> there. Add a check that the associated DRC doesn't already have
> an attached device. This is equivalent to the slot availability
> check performed by do_pci_register_device() upon realization of
> the PCI device.
> 
> This allows to pass &error_abort to spapr_drc_attach() and to end
> up with a plug handler that doesn't need to report errors anymore.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/spapr_pci.c | 43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 88ce87f130a5..2829f298d9c1 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1532,8 +1532,8 @@ static bool bridge_has_valid_chassis_nr(Object *bridge, Error **errp)
>      return true;
>  }
>  
> -static void spapr_pci_plug(HotplugHandler *plug_handler,
> -                           DeviceState *plugged_dev, Error **errp)
> +static void spapr_pci_pre_plug(HotplugHandler *plug_handler,
> +                               DeviceState *plugged_dev, Error **errp)
>  {
>      SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
>      PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> @@ -1542,9 +1542,6 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
>      PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
>      uint32_t slotnr = PCI_SLOT(pdev->devfn);
>  
> -    /* if DR is disabled we don't need to do anything in the case of
> -     * hotplug or coldplug callbacks
> -     */
>      if (!phb->dr_enabled) {
>          /* if this is a hotplug operation initiated by the user
>           * we need to let them know it's not enabled
> @@ -1552,17 +1549,14 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
>          if (plugged_dev->hotplugged) {
>              error_setg(errp, QERR_BUS_NO_HOTPLUG,
>                         object_get_typename(OBJECT(phb)));
> +            return;
>          }
> -        return;
>      }
>  
> -    g_assert(drc);
> -
>      if (pc->is_bridge) {
>          if (!bridge_has_valid_chassis_nr(OBJECT(plugged_dev), errp)) {
>              return;
>          }
> -        spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
>      }
>  
>      /* Following the QEMU convention used for PCIe multifunction
> @@ -1574,13 +1568,41 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
>          error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
>                     " additional functions can no longer be exposed to guest.",
>                     slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name);
> +    }
> +
> +    if (drc && drc->dev) {
> +        error_setg(errp, "PCI: slot %d already occupied by %s", slotnr,
> +                   pci_get_function_0(PCI_DEVICE(drc->dev))->name);
>          return;
>      }
> +}
> +
> +static void spapr_pci_plug(HotplugHandler *plug_handler,
> +                           DeviceState *plugged_dev, Error **errp)
> +{
> +    SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
> +    PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(plugged_dev);
> +    SpaprDrc *drc = drc_from_dev(phb, pdev);
> +    uint32_t slotnr = PCI_SLOT(pdev->devfn);
>  
> -    if (!spapr_drc_attach(drc, DEVICE(pdev), errp)) {
> +    /*
> +     * If DR is disabled we don't need to do anything in the case of
> +     * hotplug or coldplug callbacks.
> +     */
> +    if (!phb->dr_enabled) {
>          return;
>      }
>  
> +    g_assert(drc);
> +
> +    if (pc->is_bridge) {
> +        spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
> +    }
> +
> +    /* spapr_pci_pre_plug() already checked the DRC is attachable */
> +    spapr_drc_attach(drc, DEVICE(pdev), &error_abort);
> +
>      /* If this is function 0, signal hotplug for all the device functions.
>       * Otherwise defer sending the hotplug event.
>       */
> @@ -2223,6 +2245,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>      /* Supported by TYPE_SPAPR_MACHINE */
>      dc->user_creatable = true;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    hp->pre_plug = spapr_pci_pre_plug;
>      hp->plug = spapr_pci_plug;
>      hp->unplug = spapr_pci_unplug;
>      hp->unplug_request = spapr_pci_unplug_request;
diff mbox series

Patch

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 88ce87f130a5..2829f298d9c1 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1532,8 +1532,8 @@  static bool bridge_has_valid_chassis_nr(Object *bridge, Error **errp)
     return true;
 }
 
-static void spapr_pci_plug(HotplugHandler *plug_handler,
-                           DeviceState *plugged_dev, Error **errp)
+static void spapr_pci_pre_plug(HotplugHandler *plug_handler,
+                               DeviceState *plugged_dev, Error **errp)
 {
     SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
     PCIDevice *pdev = PCI_DEVICE(plugged_dev);
@@ -1542,9 +1542,6 @@  static void spapr_pci_plug(HotplugHandler *plug_handler,
     PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
     uint32_t slotnr = PCI_SLOT(pdev->devfn);
 
-    /* if DR is disabled we don't need to do anything in the case of
-     * hotplug or coldplug callbacks
-     */
     if (!phb->dr_enabled) {
         /* if this is a hotplug operation initiated by the user
          * we need to let them know it's not enabled
@@ -1552,17 +1549,14 @@  static void spapr_pci_plug(HotplugHandler *plug_handler,
         if (plugged_dev->hotplugged) {
             error_setg(errp, QERR_BUS_NO_HOTPLUG,
                        object_get_typename(OBJECT(phb)));
+            return;
         }
-        return;
     }
 
-    g_assert(drc);
-
     if (pc->is_bridge) {
         if (!bridge_has_valid_chassis_nr(OBJECT(plugged_dev), errp)) {
             return;
         }
-        spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
     }
 
     /* Following the QEMU convention used for PCIe multifunction
@@ -1574,13 +1568,41 @@  static void spapr_pci_plug(HotplugHandler *plug_handler,
         error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
                    " additional functions can no longer be exposed to guest.",
                    slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name);
+    }
+
+    if (drc && drc->dev) {
+        error_setg(errp, "PCI: slot %d already occupied by %s", slotnr,
+                   pci_get_function_0(PCI_DEVICE(drc->dev))->name);
         return;
     }
+}
+
+static void spapr_pci_plug(HotplugHandler *plug_handler,
+                           DeviceState *plugged_dev, Error **errp)
+{
+    SpaprPhbState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
+    PCIDevice *pdev = PCI_DEVICE(plugged_dev);
+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(plugged_dev);
+    SpaprDrc *drc = drc_from_dev(phb, pdev);
+    uint32_t slotnr = PCI_SLOT(pdev->devfn);
 
-    if (!spapr_drc_attach(drc, DEVICE(pdev), errp)) {
+    /*
+     * If DR is disabled we don't need to do anything in the case of
+     * hotplug or coldplug callbacks.
+     */
+    if (!phb->dr_enabled) {
         return;
     }
 
+    g_assert(drc);
+
+    if (pc->is_bridge) {
+        spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
+    }
+
+    /* spapr_pci_pre_plug() already checked the DRC is attachable */
+    spapr_drc_attach(drc, DEVICE(pdev), &error_abort);
+
     /* If this is function 0, signal hotplug for all the device functions.
      * Otherwise defer sending the hotplug event.
      */
@@ -2223,6 +2245,7 @@  static void spapr_phb_class_init(ObjectClass *klass, void *data)
     /* Supported by TYPE_SPAPR_MACHINE */
     dc->user_creatable = true;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    hp->pre_plug = spapr_pci_pre_plug;
     hp->plug = spapr_pci_plug;
     hp->unplug = spapr_pci_unplug;
     hp->unplug_request = spapr_pci_unplug_request;