Re: [PATCH v3 2/7] spapr_pci.c: simplify spapr_pci_unplug_request() function handling

[Greg Kurz <groug@kaod.org>]

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).

> 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 <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);
> >>               }
> >>           }
> >>       }
> >