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

[David Hildenbrand <david@redhat.com>]

On 16.10.18 15:56, Igor Mammedov 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.

Saw this mail just now. I guess we should do more. Especially what seems
to be fragile is errors during unrealize() and unplug().

Errors during unplug() should only ever happen if it was not triggered
via unplug_request(). Otherwise, unplug_request() should check for all
possible errors and unplug() will not result in errors.

Also, errors during unrealize() should definitely be avoided. Or even
forbidden. E.g. looking at hw/core/qdev.c:device_set_realized,

1. failing to unrealize might already have resulted in the unplug
handler getting called.
2. will result in a unparent of the device and therefore removal
3. will have already eventually unrealized child devices or buses.

To summarize, failing in unrealize() is a bad idea and might leave the
rest of the system in a very unpredictable state. Failing in unplug() is
a bad idea unless we don't have a unplug_request().

> 
> Forgot to CC spapr/s390 folks to give it an extra scrutiny.
> 


-- 

Thanks,

David / dhildenb