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.y...@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.y...@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.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) >> >> >> >> 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. -- Wei Yang Help you, Help me