Re: [Qemu-devel] [PATCH for-2.6] spapr_pci: fix multifunction hotplug

[David Gibson]

On Thu, Mar 03, 2016 at 03:55:36PM -0600, Michael Roth wrote:
> Since 3f1e147, QEMU has adopted a convention of supporting function
> hotplug by deferring hotplug events until func 0 is hotplugged.
> This is likely how management tools like libvirt would expose
> such support going forward.
> 
> Since sPAPR guests rely on per-func events rather than
> slot-based, our protocol has been to hotplug func 0 *first* to
> avoid cases where devices appear within guests without func 0
> present to avoid undefined behavior.

Hmm.. I would have thought PAPR guests would be able to cope with a
non-zero function device being plugged on its own.

> 
> To remain compatible with new convention, defer hotplug in a
> similar manner, but then generate events in 0-first order as we
> did in the past. Once func 0 present, fail any attempts to plug
> additional functions (as we do with PCIe).
> 
> For unplug, defer unplug operations in a similar manner, but
> generate unplug events such that function 0 is removed last in guest.
> 
> Signed-off-by: Michael Roth <address@hidden>
> ---
> Note: I'm not super-certain this is 2.6 material/soft-freeze material,
> as the current implementation does "work" if one orders device_adds
> in the manner enforced by this patch. The main reason I'm tagging as
> 2.6 is to avoid a future compatibility issue if/when libvirt adds support
> for multifunction hotplug in the manner suggested by 3f1e147. This does
> however guard a bit better against user error.

On balance, I think it is, since it does improve behaviour, rather
than add functionality.  I've added it to my ppc-for-2.6 branch.

> 
>  hw/ppc/spapr_pci.c | 93 
> ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 86 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e8edad3..ab6dece 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1142,14 +1142,21 @@ static void 
> spapr_phb_remove_pci_device(sPAPRDRConnector *drc,
>      drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, 
> errp);
>  }
>  
> -static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
> -                                               PCIDevice *pdev)
> +static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb,
> +                                                    uint32_t busnr,
> +                                                    int32_t devfn)
>  {
> -    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
>      return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
>                                      (phb->index << 16) |
>                                      (busnr << 8) |
> -                                    pdev->devfn);
> +                                    devfn);
> +}
> +
> +static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
> +                                               PCIDevice *pdev)
> +{
> +    uint32_t busnr = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))));
> +    return spapr_phb_get_pci_func_drc(phb, busnr, pdev->devfn);
>  }
>  
>  static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
> @@ -1173,6 +1180,8 @@ static void spapr_phb_hot_plug_child(HotplugHandler 
> *plug_handler,
>      PCIDevice *pdev = PCI_DEVICE(plugged_dev);
>      sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
>      Error *local_err = NULL;
> +    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
> @@ -1190,13 +1199,44 @@ static void spapr_phb_hot_plug_child(HotplugHandler 
> *plug_handler,
>  
>      g_assert(drc);
>  
> +    /* Following the QEMU convention used for PCIe multifunction
> +     * hotplug, we do not allow functions to be hotplugged to a
> +     * slot that already has function 0 present
> +     */
> +    if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] &&
> +        PCI_FUNC(pdev->devfn) != 0) {
> +        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> +                   " additional functions can no longer be exposed to 
> guest.",
> +                   slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name);
> +        return;
> +    }
> +
>      spapr_phb_add_pci_device(drc, phb, pdev, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
> -    if (plugged_dev->hotplugged) {
> -        spapr_hotplug_req_add_by_index(drc);
> +
> +    /* If this is function 0, signal hotplug for all the device functions.
> +     * Otherwise defer sending the hotplug event.
> +     */
> +    if (plugged_dev->hotplugged && PCI_FUNC(pdev->devfn) == 0) {
> +        int i;
> +
> +        for (i = 0; i < 8; i++) {
> +            sPAPRDRConnector *func_drc;
> +            sPAPRDRConnectorClass *func_drck;
> +            sPAPRDREntitySense state;
> +
> +            func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
> +                                                  PCI_DEVFN(slotnr, i));
> +            func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
> +            func_drck->entity_sense(func_drc, &state);
> +
> +            if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) {
> +                spapr_hotplug_req_add_by_index(func_drc);
> +            }
> +        }
>      }
>  }
>  
> @@ -1219,12 +1259,51 @@ static void spapr_phb_hot_unplug_child(HotplugHandler 
> *plug_handler,
>  
>      drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>      if (!drck->release_pending(drc)) {
> +        PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
> +        uint32_t slotnr = PCI_SLOT(pdev->devfn);
> +        sPAPRDRConnector *func_drc;
> +        sPAPRDRConnectorClass *func_drck;
> +        sPAPRDREntitySense state;
> +        int i;
> +
> +        /* ensure any other present functions are pending unplug */
> +        if (PCI_FUNC(pdev->devfn) == 0) {
> +            for (i = 1; i < 8; i++) {
> +                func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
> +                                                      PCI_DEVFN(slotnr, i));
> +                func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
> +                func_drck->entity_sense(func_drc, &state);
> +                if (state == SPAPR_DR_ENTITY_SENSE_PRESENT
> +                    && !func_drck->release_pending(func_drc)) {
> +                    error_setg(errp,
> +                               "PCI: slot %d, function %d still present. "
> +                               "Must unplug all non-0 functions first.",
> +                               slotnr, i);
> +                    return;
> +                }
> +            }
> +        }
> +
>          spapr_phb_remove_pci_device(drc, phb, pdev, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>              return;
>          }
> -        spapr_hotplug_req_remove_by_index(drc);
> +
> +        /* if this isn't func 0, defer unplug event. otherwise signal removal
> +         * for all present functions
> +         */
> +        if (PCI_FUNC(pdev->devfn) == 0) {
> +            for (i = 7; i >= 0; i--) {
> +                func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
> +                                                      PCI_DEVFN(slotnr, i));
> +                func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
> +                func_drck->entity_sense(func_drc, &state);
> +                if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) {
> +                    spapr_hotplug_req_remove_by_index(func_drc);
> +                }
> +            }
> +        }
>      }
>  }
>  

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