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

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.

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