Re: [Qemu-devel] [PATCH v5] i386, acpi: check acpi_memory_hotplug capacity in pre_plug

[Wei Yang <richardw.yang@linux.intel.com>]

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.

-- 
Wei Yang
Help you, Help me