On Tue, 16 Feb 2021 13:09:43 -0300
Daniel Henrique Barboza <danielhb...@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 <danielhb...@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).
> 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,
>
>
> DHB
>
>
>
> >
> >> Signed-off-by: Daniel Henrique Barboza <danielhb...@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);
> >> }
> >> }
> >> }
> >
