[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-6.0 1/9] spapr: Do PCI device hotplug sanity checks at pr
From: |
David Gibson |
Subject: |
Re: [PATCH for-6.0 1/9] spapr: Do PCI device hotplug sanity checks at pre-plug only |
Date: |
Mon, 23 Nov 2020 15:50:41 +1100 |
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
signature.asc
Description: PGP signature
- [PATCH for-6.0 0/9] spapr: Perform hotplug sanity checks at pre-plug, Greg Kurz, 2020/11/20
- [PATCH for-6.0 3/9] spapr: Fix pre-2.10 dummy ICP hack, Greg Kurz, 2020/11/20
- [PATCH for-6.0 1/9] spapr: Do PCI device hotplug sanity checks at pre-plug only, Greg Kurz, 2020/11/20
- Re: [PATCH for-6.0 1/9] spapr: Do PCI device hotplug sanity checks at pre-plug only,
David Gibson <=
- [PATCH for-6.0 6/9] spapr: Make PHB placement functions and spapr_pre_plug_phb() return status, Greg Kurz, 2020/11/20
- [PATCH for-6.0 2/9] spapr: Do NVDIMM/PC-DIMM device hotplug sanity checks at pre-plug only, Greg Kurz, 2020/11/20
- [PATCH for-6.0 7/9] spapr: Do PHB hoplug sanity check at pre-plug, Greg Kurz, 2020/11/20
- [PATCH for-6.0 5/9] spapr: Simplify error path of spapr_core_plug(), Greg Kurz, 2020/11/20