Message ID | 20190225010708.27385-1-richardw.yang@linux.intel.com |
---|---|
State | New |
Headers | show |
Series | [v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug | expand |
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
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?
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? > >
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 >
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?
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;
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 --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,