| Message ID | 20190822195918.3307-1-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] spapr_pci: remove all child functions in function zero unplug | expand |
On 8/22/19 2:59 PM, Daniel Henrique Barboza wrote: > There is nothing wrong with how sPAPR handles multifunction PCI > hot unplugs. The problem is that x86 does it simpler. Instead of > removing each non-zero function and then removing function zero, > x86 can remove any function of the slot to trigger the hot unplug. > > +++ b/hw/ppc/spapr_pci.c > @@ -1700,11 +1700,13 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler, > state = func_drck->dr_entity_sense(func_drc); > if (state == SPAPR_DR_ENTITY_SENSE_PRESENT > && !spapr_drc_unplug_requested(func_drc)) { > - error_setg(errp, > - "PCI: slot %d, function %d still present. " > - "Must unplug all non-0 functions first.", > - slotnr, i); > - return; > + /* > + * Attempting to remove function 0 of a multifunction > + * device will will cascade into removing all child > + * functions, even if their unplug weren't requested s/weren't/wasn't/ > + * beforehand. > + */ > + spapr_drc_detach(func_drc); > } > } > } >
On Thu, Aug 22, 2019 at 04:11:45PM -0500, Eric Blake wrote: > On 8/22/19 2:59 PM, Daniel Henrique Barboza wrote: > > There is nothing wrong with how sPAPR handles multifunction PCI > > hot unplugs. The problem is that x86 does it simpler. Instead of > > removing each non-zero function and then removing function zero, > > x86 can remove any function of the slot to trigger the hot unplug. > > > > > +++ b/hw/ppc/spapr_pci.c > > @@ -1700,11 +1700,13 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler, > > state = func_drck->dr_entity_sense(func_drc); > > if (state == SPAPR_DR_ENTITY_SENSE_PRESENT > > && !spapr_drc_unplug_requested(func_drc)) { > > - error_setg(errp, > > - "PCI: slot %d, function %d still present. " > > - "Must unplug all non-0 functions first.", > > - slotnr, i); > > - return; > > + /* > > + * Attempting to remove function 0 of a multifunction > > + * device will will cascade into removing all child > > + * functions, even if their unplug weren't requested > > s/weren't/wasn't/ Actually, I think this is technically a subjunctive, which would make it "were" rather than "was". Modern English usage doesn't really do the subjunctive, though. </more-pedantic-than-thou> With my maintainer hat on, rather than pedant hat, the meaning is clear so I really don't care, especially from a contributer whose first language isn't English. > > > + * beforehand. > > + */ > > + spapr_drc_detach(func_drc); > > } > > } > > } > > >
On Thu, Aug 22, 2019 at 04:59:18PM -0300, Daniel Henrique Barboza wrote: > There is nothing wrong with how sPAPR handles multifunction PCI > hot unplugs. The problem is that x86 does it simpler. Instead of > removing each non-zero function and then removing function zero, > x86 can remove any function of the slot to trigger the hot unplug. > > Libvirt will be directly impacted by this difference, in the > (hopefully soon) PCI Multifunction hot plug/unplug support. For > hot plugs, both x86 and sPAPR will operate the same way: a XML > with all desired functions to be added, then consecutive hotplugs > of all non-zero functions first, zero last. For hot unplugs, at > least in the current state, a XML with the devices to be removed > must also be provided because of how sPAPR operates - x86 does > not need it - since any function unplug will unplug the whole > PCIe slot. This difference puts extra strain in the management > layer, which needs to either handle both archs differently in > the unplug scenario or choose treat x86 like sPAPR, forcing x86 > users to cope with sPAPR internals. > > This patch changes spapr_pci_unplug_request to handle the > unplug of function zero differently. When removing function zero, > instead of error-ing out if there are any remaining function > DRCs which needs detaching, detach those. This has no effect in > any existing scripts that are detaching the non-zero functions > before function zero, and can be used by management as a shortcut > to remove the whole PCI multifunction device without specifying > each child function. Makes sense to me, applied to ppc-for-4.2. > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/ppc/spapr_pci.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index deb0b0c80c..9f176f463e 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1700,11 +1700,13 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler, > state = func_drck->dr_entity_sense(func_drc); > if (state == SPAPR_DR_ENTITY_SENSE_PRESENT > && !spapr_drc_unplug_requested(func_drc)) { > - error_setg(errp, > - "PCI: slot %d, function %d still present. " > - "Must unplug all non-0 functions first.", > - slotnr, i); > - return; > + /* > + * Attempting to remove function 0 of a multifunction > + * device will will cascade into removing all child > + * functions, even if their unplug weren't requested > + * beforehand. > + */ > + spapr_drc_detach(func_drc); > } > } > }
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index deb0b0c80c..9f176f463e 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1700,11 +1700,13 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler, state = func_drck->dr_entity_sense(func_drc); if (state == SPAPR_DR_ENTITY_SENSE_PRESENT && !spapr_drc_unplug_requested(func_drc)) { - error_setg(errp, - "PCI: slot %d, function %d still present. " - "Must unplug all non-0 functions first.", - slotnr, i); - return; + /* + * Attempting to remove function 0 of a multifunction + * device will will cascade into removing all child + * functions, even if their unplug weren't requested + * beforehand. + */ + spapr_drc_detach(func_drc); } } }
There is nothing wrong with how sPAPR handles multifunction PCI hot unplugs. The problem is that x86 does it simpler. Instead of removing each non-zero function and then removing function zero, x86 can remove any function of the slot to trigger the hot unplug. Libvirt will be directly impacted by this difference, in the (hopefully soon) PCI Multifunction hot plug/unplug support. For hot plugs, both x86 and sPAPR will operate the same way: a XML with all desired functions to be added, then consecutive hotplugs of all non-zero functions first, zero last. For hot unplugs, at least in the current state, a XML with the devices to be removed must also be provided because of how sPAPR operates - x86 does not need it - since any function unplug will unplug the whole PCIe slot. This difference puts extra strain in the management layer, which needs to either handle both archs differently in the unplug scenario or choose treat x86 like sPAPR, forcing x86 users to cope with sPAPR internals. This patch changes spapr_pci_unplug_request to handle the unplug of function zero differently. When removing function zero, instead of error-ing out if there are any remaining function DRCs which needs detaching, detach those. This has no effect in any existing scripts that are detaching the non-zero functions before function zero, and can be used by management as a shortcut to remove the whole PCI multifunction device without specifying each child function. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/ppc/spapr_pci.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)