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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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