diff mbox series

[v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug

Message ID 20190225010708.27385-1-richardw.yang@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug | expand

Commit Message

Wei Yang Feb. 25, 2019, 1:07 a.m. UTC 
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 <richardw.yang@linux.intel.com>
CC: Igor Mammedov <imammedo@redhat.com>
CC: Eric Blake <eblake@redhat.com>
Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

---
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:
   * (imammedo@redhat.com)
     - 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(-)

Comments

Wei Yang Feb. 25, 2019, 1:15 a.m. UTC | #1 
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 <richardw.yang@linux.intel.com>
>CC: Igor Mammedov <imammedo@redhat.com>
>CC: Eric Blake <eblake@redhat.com>
>Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>
>---
>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:
>   * (imammedo@redhat.com)
>     - 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.

Is my understanding correct?

> {
>+    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
>+
>     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>         acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
>-    } else if (!object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>+               !s->acpi_memory_hotplug.is_enabled) {
>+        error_setg(errp,
>+                   "memory hotplug is not enabled: %s.memory-hotplug-support "
>+                   "is not set", object_get_typename(OBJECT(s)));
>+    } else if (
>                !object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>         error_setg(errp, "acpi: device pre plug request for not supported"
>                    " device type: %s", object_get_typename(OBJECT(dev)));
>@@ -388,8 +395,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> {
>     PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> 
>-    if (s->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 {
>@@ -704,6 +710,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
>     dc->user_creatable = false;
>     dc->hotpluggable = false;
>     hc->pre_plug = piix4_device_pre_plug_cb;
>+    hc->pre_plug = piix4_device_pre_plug_cb;
>     hc->plug = piix4_device_plug_cb;
>     hc->unplug_request = piix4_device_unplug_request_cb;
>     hc->unplug = piix4_device_unplug_cb;
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index 578f772e7c..64d9e756fa 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -2083,6 +2083,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>         return;
>     }
> 
>+    hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>+
>     if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
>         error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>         return;
>diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>index e692b9fdc1..ac44aa53be 100644
>--- a/hw/isa/lpc_ich9.c
>+++ b/hw/isa/lpc_ich9.c
>@@ -805,6 +805,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
>      * pc_q35_init()
>      */
>     dc->user_creatable = false;
>+    hc->pre_plug = ich9_pm_device_pre_plug_cb;
>     hc->plug = ich9_pm_device_plug_cb;
>     hc->unplug_request = ich9_pm_device_unplug_request_cb;
>     hc->unplug = ich9_pm_device_unplug_cb;
>diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
>index 59aeb06393..41568d1837 100644
>--- a/include/hw/acpi/ich9.h
>+++ b/include/hw/acpi/ich9.h
>@@ -74,6 +74,8 @@ extern const VMStateDescription vmstate_ich9_pm;
> 
> void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp);
> 
>+void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>+                                Error **errp);
> void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>                             Error **errp);
> void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>-- 
>2.19.1
Wei Yang Feb. 26, 2019, 3:37 a.m. UTC | #2 
On Mon, Feb 25, 2019 at 09:15:34AM +0800, Wei Yang 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 <richardw.yang@linux.intel.com>
>>CC: Igor Mammedov <imammedo@redhat.com>
>>CC: Eric Blake <eblake@redhat.com>
>>Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>
>>---
>>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:
>>   * (imammedo@redhat.com)
>>     - 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)
>

Where will we invoke this check for pci devices?

pc_machine_device_pre_plug_cb() seems has no connection with this function. So
how this pre_plug handler takes effect?

Do I miss something?
Igor Mammedov Feb. 27, 2019, 6 p.m. UTC | #3 
On Tue, 26 Feb 2019 11:37:32 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Mon, Feb 25, 2019 at 09:15:34AM +0800, Wei Yang 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 <richardw.yang@linux.intel.com>
> >>CC: Igor Mammedov <imammedo@redhat.com>
> >>CC: Eric Blake <eblake@redhat.com>
> >>Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >>
> >>---
> >>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:
> >>   * (imammedo@redhat.com)
> >>     - 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)  
> >  
> 
> Where will we invoke this check for pci devices?
> 
> pc_machine_device_pre_plug_cb() seems has no connection with this function. So
> how this pre_plug handler takes effect?
hotplug handler doesn't have to be machine, on contrary it's typically a bus owner
see hw/core/qdev.c: device_set_realized() and qdev_get_hotplug_handler()

you also might want to check relevant
 'PATCH v1 0/3] qdev: Hotplug handler chaining' thread
that's hopefully to be merged soon

> 
> Do I miss something?
> 
>
Igor Mammedov Feb. 27, 2019, 6:04 p.m. UTC | #4 
On Mon, 25 Feb 2019 09:15:34 +0800
Wei Yang <richardw.yang@linux.intel.com> 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 <richardw.yang@linux.intel.com>
> >CC: Igor Mammedov <imammedo@redhat.com>
> >CC: Eric Blake <eblake@redhat.com>
> >Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >
> >---
> >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:
> >   * (imammedo@redhat.com)
> >     - 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?

> 
> Is my understanding correct?
> 
> > {
> >+    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> >+
> >     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> >         acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
> >-    } else if (!object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> >+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> >+               !s->acpi_memory_hotplug.is_enabled) {
> >+        error_setg(errp,
> >+                   "memory hotplug is not enabled: %s.memory-hotplug-support "
> >+                   "is not set", object_get_typename(OBJECT(s)));
> >+    } else if (
> >                !object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >         error_setg(errp, "acpi: device pre plug request for not supported"
> >                    " device type: %s", object_get_typename(OBJECT(dev)));
> >@@ -388,8 +395,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> > {
> >     PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> > 
> >-    if (s->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 {
> >@@ -704,6 +710,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
> >     dc->user_creatable = false;
> >     dc->hotpluggable = false;
> >     hc->pre_plug = piix4_device_pre_plug_cb;
> >+    hc->pre_plug = piix4_device_pre_plug_cb;
> >     hc->plug = piix4_device_plug_cb;
> >     hc->unplug_request = piix4_device_unplug_request_cb;
> >     hc->unplug = piix4_device_unplug_cb;
> >diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >index 578f772e7c..64d9e756fa 100644
> >--- a/hw/i386/pc.c
> >+++ b/hw/i386/pc.c
> >@@ -2083,6 +2083,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >         return;
> >     }
> > 
> >+    hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
> >+
> >     if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
> >         error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> >         return;
> >diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> >index e692b9fdc1..ac44aa53be 100644
> >--- a/hw/isa/lpc_ich9.c
> >+++ b/hw/isa/lpc_ich9.c
> >@@ -805,6 +805,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
> >      * pc_q35_init()
> >      */
> >     dc->user_creatable = false;
> >+    hc->pre_plug = ich9_pm_device_pre_plug_cb;
> >     hc->plug = ich9_pm_device_plug_cb;
> >     hc->unplug_request = ich9_pm_device_unplug_request_cb;
> >     hc->unplug = ich9_pm_device_unplug_cb;
> >diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> >index 59aeb06393..41568d1837 100644
> >--- a/include/hw/acpi/ich9.h
> >+++ b/include/hw/acpi/ich9.h
> >@@ -74,6 +74,8 @@ extern const VMStateDescription vmstate_ich9_pm;
> > 
> > void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp);
> > 
> >+void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >+                                Error **errp);
> > void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >                             Error **errp);
> > void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> >-- 
> >2.19.1  
>
Wei Yang Feb. 28, 2019, 1:13 a.m. UTC | #5 
On Wed, Feb 27, 2019 at 07:04:02PM +0100, Igor Mammedov wrote:
>On Mon, 25 Feb 2019 09:15:34 +0800
>Wei Yang <richardw.yang@linux.intel.com> 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 <richardw.yang@linux.intel.com>
>> >CC: Igor Mammedov <imammedo@redhat.com>
>> >CC: Eric Blake <eblake@redhat.com>
>> >Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> >
>> >---
>> >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:
>> >   * (imammedo@redhat.com)
>> >     - 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?
Igor Mammedov Feb. 28, 2019, 2:12 p.m. UTC | #6 
On Thu, 28 Feb 2019 09:13:25 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Wed, Feb 27, 2019 at 07:04:02PM +0100, Igor Mammedov wrote:
> >On Mon, 25 Feb 2019 09:15:34 +0800
> >Wei Yang <richardw.yang@linux.intel.com> 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 <richardw.yang@linux.intel.com>
> >> >CC: Igor Mammedov <imammedo@redhat.com>
> >> >CC: Eric Blake <eblake@redhat.com>
> >> >Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >> >
> >> >---
> >> >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:
> >> >   * (imammedo@redhat.com)
> >> >     - 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?
it depends, in case of ICH9. I think acpi hotplug is not used (thankfully)
it uses native PCI-E hotplug path. Following pops out when looking for it.
 hw/pci/pcie.c:void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
 hw/pci/pcie_port.c:    hc->pre_plug = pcie_cap_slot_pre_plug_cb;
Wei Yang March 1, 2019, 2:32 a.m. UTC | #7 
On Thu, Feb 28, 2019 at 03:12:21PM +0100, Igor Mammedov wrote:
>On Thu, 28 Feb 2019 09:13:25 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> On Wed, Feb 27, 2019 at 07:04:02PM +0100, Igor Mammedov wrote:
>> >On Mon, 25 Feb 2019 09:15:34 +0800
>> >Wei Yang <richardw.yang@linux.intel.com> 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 <richardw.yang@linux.intel.com>
>> >> >CC: Igor Mammedov <imammedo@redhat.com>
>> >> >CC: Eric Blake <eblake@redhat.com>
>> >> >Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> >> >
>> >> >---
>> >> >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:
>> >> >   * (imammedo@redhat.com)
>> >> >     - 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?
>it depends, in case of ICH9. I think acpi hotplug is not used (thankfully)
>it uses native PCI-E hotplug path. Following pops out when looking for it.
> hw/pci/pcie.c:void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> hw/pci/pcie_port.c:    hc->pre_plug = pcie_cap_slot_pre_plug_cb;
>

So in case of ICH9, acpi hotplug is not involved for PCI_DEVICE.

Thanks for your time. I need to resend this patch, because of my
misunderstanding.
diff mbox series

Patch

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)
 {
+    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
         acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
-    } else if (!object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
+               !s->acpi_memory_hotplug.is_enabled) {
+        error_setg(errp,
+                   "memory hotplug is not enabled: %s.memory-hotplug-support "
+                   "is not set", object_get_typename(OBJECT(s)));
+    } else if (
                !object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         error_setg(errp, "acpi: device pre plug request for not supported"
                    " device type: %s", object_get_typename(OBJECT(dev)));
@@ -388,8 +395,7 @@  static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
 {
     PIIX4PMState *s = PIIX4_PM(hotplug_dev);
 
-    if (s->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 {
@@ -704,6 +710,7 @@  static void piix4_pm_class_init(ObjectClass *klass, void *data)
     dc->user_creatable = false;
     dc->hotpluggable = false;
     hc->pre_plug = piix4_device_pre_plug_cb;
+    hc->pre_plug = piix4_device_pre_plug_cb;
     hc->plug = piix4_device_plug_cb;
     hc->unplug_request = piix4_device_unplug_request_cb;
     hc->unplug = piix4_device_unplug_cb;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 578f772e7c..64d9e756fa 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2083,6 +2083,8 @@  static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         return;
     }
 
+    hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
+
     if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
         error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
         return;
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index e692b9fdc1..ac44aa53be 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -805,6 +805,7 @@  static void ich9_lpc_class_init(ObjectClass *klass, void *data)
      * pc_q35_init()
      */
     dc->user_creatable = false;
+    hc->pre_plug = ich9_pm_device_pre_plug_cb;
     hc->plug = ich9_pm_device_plug_cb;
     hc->unplug_request = ich9_pm_device_unplug_request_cb;
     hc->unplug = ich9_pm_device_unplug_cb;
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 59aeb06393..41568d1837 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -74,6 +74,8 @@  extern const VMStateDescription vmstate_ich9_pm;
 
 void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp);
 
+void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                Error **errp);
 void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                             Error **errp);
 void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,