On Tue, 26 Feb 2019 11:37:32 +0800
Wei Yang <richardw.y...@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.y...@linux.intel.com>
> >>CC: Igor Mammedov <imamm...@redhat.com>
> >>CC: Eric Blake <ebl...@redhat.com>
> >>Signed-off-by: Wei Yang <richardw.y...@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:
> >> * (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?
>
>
