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;
