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 <gr...@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
signature.asc
Description: PGP signature
