Re: [Qemu-devel] [PATCH] call HotplugHandler->plug() as the last step in device realization

[Pierre Morel <pmorel@linux.ibm.com>]

On 17/10/2018 12:56, Cornelia Huck wrote:
> On Tue, 16 Oct 2018 15:33:40 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
>> When [2] was fixed it was agreed that adding and calling post_plug()
>> callback after device_reset() was low risk approach to hotfix issue
>> right before release. So it was merged instead of moving already
>> existing plug() callback after device_reset() is called which would
>> be more risky and require all plug() callbacks audit.
>>
>> Looking at the current plug() callbacks, it doesn't seem that moving
>> plug() callback after device_reset() is breaking anything, so here
>> goes agreed upon [3] proper fix which essentially reverts [1][2]
>> and moves plug() callback after device_reset().
>> This way devices always comes to plug() stage, after it's been fully
>> initialized (including being reset), which fixes race condition [2]
>> without need for an extra post_plug() callback.
>>
>>   1. (25e897881 "qdev: add HotplugHandler->post_plug() callback")
>>   2. (8449bcf94 "virtio-scsi: fix hotplug ->reset() vs event race")
>>   3. https://www.mail-archive.com/qemu-devel@nongnu.org/msg549915.html
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>> TODO:
>>   remove usage of Error** from plug() callback, we need to factor out
>>   pre_plug part from plug() callbacks, before proceeding with it.
>>   DavidH has recently finished it for pc-dimm/memory_devices, cpus
>>   mostly have pre_plug parts factored out, but there still are parts
>>   that could fail so it needs some more work to eliminate failure points
>>   from plug() callbacks. Meanwhile, I'll plan to treat other misc
>>   handlers (pci[e]/acpi/usb/...) and introduce pre_plug() where
>>   necessary.
>> ---
>>   include/hw/hotplug.h  | 11 -----------
>>   hw/core/hotplug.c     | 10 ----------
>>   hw/core/qdev.c        | 16 ++++++----------
>>   hw/scsi/virtio-scsi.c | 11 +----------
>>   4 files changed, 7 insertions(+), 41 deletions(-)
> 
> I've looked at the s390x users of this interface, and it seems to be
> sane. The one I'm a bit unsure about is zPCI with its bridge
> enumeration code as introduced in d2f07120a35a ("s390x/pci: handle
> PCIBridge bus number"). I *think* that one is fine as well. Pierre, can
> you confirm?

Yes, I can confirm.
We have no problem with this patch in VFIO_PCI nor in VIRTIO_PCI with 
zPCI emulation, for devices bridge or bus.

I played the patch and tested locally. all fine.

For the S390:
Tested-by: Pierre Morel<pmorel@linux.ibm.com>

also
Acked-by: Pierre Morel<pmorel@linux.ibm.com>

Regards,
Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany