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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5085 bytes --]

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;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]