On Thu, Mar 03, 2016 at 08:50:26PM -0600, Michael Roth wrote: > 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.
Ok.
>
> >
> > >
> > > 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 <mdr...@linux.vnet.ibm.com>
> > > ---
> > > 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
signature.asc
Description: PGP signature
