Re: [PATCH] failover: fix unplug pending detection

[Laurent Vivier <lvivier@redhat.com>]

On 01/10/2021 07:19, Ani Sinha wrote:
> 
> 
> On Thu, 30 Sep 2021, Laurent Vivier wrote:
> 
>> On 30/09/2021 11:24, Ani Sinha wrote:
>>>
>>>
>>> On Thu, 30 Sep 2021, Laurent Vivier wrote:
>>>
>>>> Failover needs to detect the end of the PCI unplug to start migration
>>>> after the VFIO card has been unplugged.
>>>>
>>>> To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset
>>>> in
>>>> pcie_unplug_device().
>>>>
>>>> But since
>>>>       17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on
>>>> Q35")
>>>> we have switched to ACPI unplug and these functions are not called anymore
>>>> and the flag not set. So failover migration is not able to detect if card
>>>> is really unplugged and acts as it's done as soon as it's started. So it
>>>> doesn't wait the end of the unplug to start the migration. We don't see
>>>> any
>>>> problem when we test that because ACPI unplug is faster than PCIe native
>>>> hotplug and when the migration really starts the unplug operation is
>>>> already done.
>>>>
>>>> See c000a9bd06ea ("pci: mark device having guest unplug request pending")
>>>>       a99c4da9fc2a ("pci: mark devices partially unplugged")
>>>
>>> Ok so I have a basic question about partially_hotplugged flag in the
>>> device struct (there were no comments added in a99c4da9fc2a39847
>>> explaining it). It seems we return early from pcie_unplug_device() when
>>> this flag is set from failover_unplug_primary() in virtio-net. What is the
>>> purpose of this flag? It seems we are not doing a full unplug of the
>>> primary device?
>>
>> Yes, to be able to plug it back in case of migration failure we must keep all
>> the data structures.
> 
> Ok so two things here:
> (a) could you please add a comment to PCIDevice struct in pci.h to clarify
> what the flag actually means, why it is there and what it is supposed to
> do.

Will be in v3.

> (b) the naming of the variable could be something like do_partial_unplug
> or some such. This could be a separate patch.

OK, I'll do that on a separate patch: I'm already working on a patch series moving most of 
the failover code to PCI files (hotplug is a PCI feature not a virtio one).

https://patchew.org/QEMU/20210820142002.152994-1-lvivier@redhat.com/

>   >
>> But reading the code again it seems this part should be in
>> acpi_pcihp_eject_slot() rather than in acpi_pcihp_device_unplug_cb() to
>> prevent the hotplug_handler_unplug()/object_unparent()  rather than the
>> qdev_unrealize() (to be like in pcie.c).
> 
> Correct. You need to place the check earlier so as to be equivalent to
> what the native hotplug code does.
> 

Thanks,
Laurent