|
| From: | Daniel Henrique Barboza |
| Subject: | Re: [PATCH v3 2/7] spapr_pci.c: simplify spapr_pci_unplug_request() function handling |
| Date: | Tue, 16 Feb 2021 14:44:44 -0300 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 |
On 2/16/21 2:16 PM, Greg Kurz wrote:
On Tue, 16 Feb 2021 13:09:43 -0300 Daniel Henrique Barboza <danielhb413@gmail.com> wrote:On 2/16/21 12:50 PM, Greg Kurz wrote:On Thu, 11 Feb 2021 19:52:41 -0300 Daniel Henrique Barboza <danielhb413@gmail.com> wrote:When hotunplugging a PCI function we'll branch out the logic in two cases, function zero and non-zero. If non-zero, we'll call spapr_drc_detach() and nothing else. If it's function zero, we'll loop it once between all the functions in the slot to call spapr_drc_detach() on them, and afterwards we'll do another backwards loop where we'll signal the event to the guest. We can simplify this logic. We can ignore all the DRC handling for non-zero functions, since we'll end up doing that regardless when unplugging function zero. And for function zero, everything can be done in a single loop, since tt doesn't matter if we end up marking the function DRCs as unplug pending in backwards order or not, as long as we call spapr_drc_detach() before issuing the hotunplug event to the guest. This will also avoid a possible scenario where the user starts to hotunplug the slot, starting with a non-zero function, and then delays/forgets to hotunplug function zero afterwards. This would keep the function DRC marked as unplug requested indefinitely.... or until the guest is reset, which will no longer happen with this patch applied, i.e. breaks the long standing policy that machine reset causes pending hot-unplug requests to succeed. I don't see an obvious reason to special case non-zero PCI functions.It's not possible to hotunplug the non-zero functions during machine reset for multifunction PCI devices. We need to unplug the entire slot, and that will only happen when function zero is unplugged. In fact, I think bad things will happen in this case you mentioned if we are forcing the removal of non-zero functions without function zero (spoiler: didn't test it).I've tested with the aggregation of two e1000e emulated devices: device_add e1000e,addr=10.1,id=netfn1 device_add e1000e,multifunction=on,addr=10.0,id=netfn0 And I don't quite see what "bad things" could happen. We're resetting the machine to a stable state and the new OS instance will just not see the removed function (just like only function netfn0 got added).
Interesting. Thanks for looking this up. Given that the intention of this patch was a simplification of the existing design, without changing what we currently do regarding PCI functions and unplug, and apparently it just did that, let's drop it. DHB
What I'm doing in this patch is making it clearer that non-zero functions does not matter for the unplug of multifunction PCI devices. We'll detach the whole slot when function zero is unplugged, regardless of the unplug state of other functions.I understand that hot-unplug of non-zero functions is special cased while the guest OS is running, but this doesn't really applies if the guest is rebooted. Code simplification is not a good reason enough, at least for me, to alter the "reset complete all pending hotplugs" general rule.The only reason why I didn't make 'device_del' to error out when used with a non-zero function is because we allowed this in the past and it would break user ABI. Otherwise, FWIW, "device_del <non-zero function>" is doing nothing since commit "spapr_pci: remove all child functions in function zero unplug". Thanks, DHBSigned-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/ppc/spapr_pci.c | 44 ++++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index f1c7479816..1791d98a49 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1709,38 +1709,26 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler, return; }- /* ensure any other present functions are pending unplug */- if (PCI_FUNC(pdev->devfn) == 0) { - for (i = 1; i < 8; i++) { - func_drc = drc_from_devfn(phb, chassis, PCI_DEVFN(slotnr, i)); - func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc); - state = func_drck->dr_entity_sense(func_drc); - if (state == SPAPR_DR_ENTITY_SENSE_PRESENT - && !spapr_drc_unplug_requested(func_drc)) { - /* - * 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); - } - } + /* + * The hotunplug itself will occur when unplugging function 0, + * regardless of marking any other functions DRCs as pending + * unplug beforehand (since 02a1536eee33). + */ + if (PCI_FUNC(pdev->devfn) != 0) { + return; }- spapr_drc_detach(drc);+ for (i = 7; i >= 0; i--) { + func_drc = drc_from_devfn(phb, chassis, PCI_DEVFN(slotnr, i)); + func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc); + state = func_drck->dr_entity_sense(func_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 = drc_from_devfn(phb, chassis, PCI_DEVFN(slotnr, i)); - func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc); - state = func_drck->dr_entity_sense(func_drc); - if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) { - spapr_hotplug_req_remove_by_index(func_drc); + if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) { + /* Mark the DRC as requested unplug if needed. */ + if (!spapr_drc_unplug_requested(func_drc)) { + spapr_drc_detach(func_drc); } + spapr_hotplug_req_remove_by_index(func_drc); } } }
| [Prev in Thread] | Current Thread | [Next in Thread] |