| Message ID | 1457042136-4255-1-git-send-email-mdroth@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <mdroth@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. > > 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); > + } > + } > + } > } > } >
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 <mdroth@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
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 <mdroth@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); > > > + } > > > + } > > > + } > > > } > > > } > > > > > >
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); + } + } + } } }
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. 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 <mdroth@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. hw/ppc/spapr_pci.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 86 insertions(+), 7 deletions(-)