Re: [Qemu-devel] [PATCH v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug

[Wei Yang]

On Wed, Feb 27, 2019 at 07:04:02PM +0100, Igor Mammedov wrote:
>On Mon, 25 Feb 2019 09:15:34 +0800
>Wei Yang <address@hidden> wrote:
>
>> On Mon, Feb 25, 2019 at 09:07:08AM +0800, Wei Yang wrote:
>> >Currently we do device realization like below:
>> >
>> >   hotplug_handler_pre_plug()
>> >   dc->realize()
>> >   hotplug_handler_plug()
>> >
>> >Before we do device realization and plug, we should allocate necessary
>> >resources and check if memory-hotplug-support property is enabled.
>> >
>> >At the piix4 and ich9, the memory-hotplug-support property is checked at
>> >plug stage. This means that device has been realized and mapped into guest
>> >address space 'pc_dimm_plug()' by the time acpi plug handler is called,
>> >where it might fail and crash QEMU due to reaching g_assert_not_reached()
>> >(piix4) or error_abort (ich9).
>> >
>> >Fix it by checking if memory hotplug is enabled at pre_plug stage
>> >where we can gracefully abort hotplug request.
>> >
>> >Signed-off-by: Wei Yang <address@hidden>
>> >CC: Igor Mammedov <address@hidden>
>> >CC: Eric Blake <address@hidden>
>> >Signed-off-by: Wei Yang <address@hidden>
>> >
>> >---
>> >v5:
>> >   * rebase on latest upstream
>> >   * remove a comment before hotplug_handler_pre_plug
>> >   * fix alignment for ich9_pm_device_pre_plug_cb
>> >v4:
>> >   * fix code alignment of piix4_device_pre_plug_cb
>> >v3:
>> >   * replace acpi_memory_hotplug with memory-hotplug-support in changelog
>> >   * fix code alignment of ich9_pm_device_pre_plug_cb
>> >   * print which device type memory-hotplug-support is disabled in
>> >     ich9_pm_device_pre_plug_cb and piix4_device_pre_plug_cb
>> >v2:
>> >   * (address@hidden)
>> >     - Almost the whole third paragraph
>> >   * apply this change to ich9
>> >   * use hotplug_handler_pre_plug() instead of open-coding check
>> >---
>> > hw/acpi/ich9.c         | 15 +++++++++++++--
>> > hw/acpi/piix4.c        | 13 ++++++++++---
>> > hw/i386/pc.c           |  2 ++
>> > hw/isa/lpc_ich9.c      |  1 +
>> > include/hw/acpi/ich9.h |  2 ++
>> > 5 files changed, 28 insertions(+), 5 deletions(-)
>> >
>> >diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> >index c5d8646abc..e53dfe1ee3 100644
>> >--- a/hw/acpi/ich9.c
>> >+++ b/hw/acpi/ich9.c
>> >@@ -483,13 +483,24 @@ void ich9_pm_add_properties(Object *obj, 
>> >ICH9LPCPMRegs *pm, Error **errp)
>> >                              NULL);
>> > }
>> > 
>> >+void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState 
>> >*dev,
>> >+                                Error **errp)
>> >+{
>> >+    ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
>> >+
>> >+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>> >+        !lpc->pm.acpi_memory_hotplug.is_enabled)
>> >+        error_setg(errp,
>> >+                   "memory hotplug is not enabled: 
>> >%s.memory-hotplug-support "
>> >+                   "is not set", object_get_typename(OBJECT(lpc)));
>> >+}
>> >+
>> > void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>> >                             Error **errp)
>> > {
>> >     ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
>> > 
>> >-    if (lpc->pm.acpi_memory_hotplug.is_enabled &&
>> >-        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> >+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> >         if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
>> >             nvdimm_acpi_plug_cb(hotplug_dev, dev);
>> >         } else {
>> >diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> >index df8c0db909..8b9654e437 100644
>> >--- a/hw/acpi/piix4.c
>> >+++ b/hw/acpi/piix4.c
>> >@@ -374,9 +374,16 @@ static void piix4_pm_powerdown_req(Notifier *n, void 
>> >*opaque)
>> > static void piix4_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>> >                                     DeviceState *dev, Error **errp)  
>> 
>> Hmm, I found one interesting thing.
>> 
>> This function is introduced in
>> 
>>     commit ec266f408882fd38475f72c4e864ed576228643b
>>     pci/pcihp: perform check for bus capability in pre_plug handler
>> 
>> This is just implemented in piix4, but don't has the counterpart in ich9.
>> 
>> So I suggest to have a follow up patch to create the counterpart for ich9 and
>> then apply this patch on top of that.
>not sure what do you mean, could you be more specific?
>

Let me reply here.

Everything starts from device_set_realized().

    device_set_realized()
       hotplug_handler_pre_plug()
       dc->realize()
       hotplug_handler_plug()

There is two choice of HotplugHandler :

    * dev's bus hotplug_handler
    * machine_hotplug_handler

Take PIIX as an example, machine_hotplug_handler and pci_bus hotplug_handler
is:

    pc_machine_device_[pre_]plug_cb
    piix4_device_[pre_]plug_cb

if I am correct.

Now back to your question.

Commit ec266f408882 introduced piix4_device_pre_plug_cb() to move the check in
pre_plug handler for PIIX. Then I am wondering the counter part of ich9, But I
don't see something for PCI_DEVICE in ich9_pm_device_plug_cb(). So on ich9,
PCI_DEVICE is not pluggable? Maybe I am lost here.

Or in other words, in case other machine supports PCI_DEVICE hotplug, do we
need to move similar check to pre_plug too?


-- 
Wei Yang
Help you, Help me