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

[Michael Roth]

Quoting David Gibson (2016-03-03 19:18:09)
> 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.

Well, as far as PAPR goes nothing seems to forbid it, but for
passthrough devices in particular there seem to be cases where
drivers (or maybe the actual hardware?) expect function 0 to be
present. I believe it was with some Broadcom bnx2x adapters where
we saw some issues.

Some of it may be due assumptions based around non-passthrough
usage, but I thought I'd seen some verbage in PCI spec that lent
some weight to this being a reasonable assumption. There's this
from PCIe 3.1, 7.5.1.5 at least:

"Multi-Function Device – When Set, indicates that the Device
may contain multiple Functions, but not necessarily. Software is
permitted to probe for Functions other than Function 0. When
Clear, software must not probe for Functions other than Function
0 unless explicitly indicated by another mechanism, such as an
ARI or SR-IOV Capability structure."

So for generic PCI rescan (which we still use currently) that could
be an issue. But yah, I can see why for firmware-configured devices
that in particular might not be an issue, since we wouldn't need to
probe in the guest.

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

Thanks!

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