Re: [Qemu-devel] [PATCH v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug
On Thu, Feb 28, 2019 at 03:12:21PM +0100, Igor Mammedov wrote:
>On Thu, 28 Feb 2019 09:13:25 +0800
>Wei Yang 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 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
>> >> >CC: Igor Mammedov
>> >> >CC: Eric Blake
>> >> >Signed-off-by: Wei Yang
>> >> >
>> >> >---
>> >> >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:
>> >> > * (imamm...@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
Re: [Qemu-devel] [PATCH v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug
On Thu, 28 Feb 2019 09:13:25 +0800
Wei Yang 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 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
> >> >CC: Igor Mammedov
> >> >CC: Eric Blake
> >> >Signed-off-by: Wei Yang
> >> >
> >> >---
> >> >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:
> >> > * (imamm...@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
>
Re: [Qemu-devel] [PATCH v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug
On Wed, Feb 27, 2019 at 07:04:02PM +0100, Igor Mammedov wrote:
>On Mon, 25 Feb 2019 09:15:34 +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
>> >CC: Igor Mammedov
>> >CC: Eric Blake
>> >Signed-off-by: Wei Yang
>> >
>> >---
>> >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:
>> > * (imamm...@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
Re: [Qemu-devel] [PATCH v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug
On Mon, 25 Feb 2019 09:15:34 +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
> >CC: Igor Mammedov
> >CC: Eric Blake
> >Signed-off-by: Wei Yang
> >
> >---
> >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:
> > * (imamm...@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
>
Re: [Qemu-devel] [PATCH v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug
On Tue, 26 Feb 2019 11:37:32 +0800
Wei Yang 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
> >>CC: Igor Mammedov
> >>CC: Eric Blake
> >>Signed-off-by: Wei Yang
> >>
> >>---
> >>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:
> >> * (imamm...@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?
>
>
Re: [Qemu-devel] [PATCH v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug
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
>>CC: Igor Mammedov
>>CC: Eric Blake
>>Signed-off-by: Wei Yang
>>
>>---
>>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:
>> * (imamm...@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?
--
Wei Yang
Help you, Help me
Re: [Qemu-devel] [PATCH v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug
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
>CC: Igor Mammedov
>CC: Eric Blake
>Signed-off-by: Wei Yang
>
>---
>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:
> * (imamm...@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
[Qemu-devel] [PATCH v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug
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
CC: Igor Mammedov
CC: Eric Blake
Signed-off-by: Wei Yang
---
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:
* (imamm...@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)
{
+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,
