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

[Wei Yang]

On Mon, Feb 18, 2019 at 02:53:36PM +0100, Igor Mammedov wrote:
>On Mon, 18 Feb 2019 13:21:29 +0000
>Wei Yang <address@hidden> wrote:
>
>> On Mon, Feb 18, 2019 at 01:56:02PM +0100, Igor Mammedov wrote:
>> >On Mon, 18 Feb 2019 12:13:24 +0000
>> >Wei Yang <address@hidden> wrote:
>> >
>> >> On Mon, Feb 18, 2019 at 10:50:34AM +0100, Igor Mammedov wrote:
>> >> >On Mon, 18 Feb 2019 09:13:33 +0800
>> >> >Wei Yang <address@hidden> 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 acpi_memory_hotplug property is checked in
>> >> >                             ^^^^^^^ is field name and not a property so
>> >> >s/acpi_memory_hotplug/memory-hotplug-support/
>> >> >s/in/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).
>> >> >
>> >> >
>> >> >> This patch abstract the check on acpi_memory_hotplug capacity in
>> >> >> pre_plug stage.
>> >> >maybe better would be:
>> >> >"Fix it by checking if memory hotplug is enabled at pre_plug stage
>> >> >where we can gracefully abort hotplug request."
>> >> >
>> >> >> [changelog rephrase from address@hidden
>> >> >this provides zero information for a commit reader,
>> >> >it should go under --- to chagelog
>> >> > 
>> >> 
>> >> Ok, maybe different community has different convention.
>> >> 
>> >> Linux kernel mm subsystem maintainer suggest me to add above line.
>> >> 
>> >> >> Signed-off-by: Wei Yang <address@hidden>
>> >> >> CC: Igor Mammedov <address@hidden>
>> >> >> CC: Eric Blake <address@hidden>
>> >> >> 
>> >> >> ---
>> >> >> v2:
>> >> >>    * rephrase change log
>> >> >one usually adds commenter's name here
>> >> >   * like this (address@hidden)
>> >> >or
>> >> >   * (address@hidden)
>> >> >     - entry 1
>> >> >     - entry 2
>> >> >   * (address@hidden)
>> >> >     - ...
>> >> >
>> >> 
>> >> Well, I would change to this style.
>> >> 
>> >> >>    * apply this change to ich9
>> >> >>    * use hotplug_handler_pre_plug() instead of open-coding check
>> >> >> ---
>> >> >>  hw/acpi/ich9.c         | 14 ++++++++++++--
>> >> >>  hw/acpi/piix4.c        | 14 ++++++++++++--
>> >> >>  hw/i386/pc.c           |  5 +++++
>> >> >>  hw/isa/lpc_ich9.c      |  1 +
>> >> >>  include/hw/acpi/ich9.h |  2 ++
>> >> >>  5 files changed, 32 insertions(+), 4 deletions(-)
>> >> >> 
>> >> >> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> >> >> index c5d8646abc..906a10f09a 100644
>> >> >> --- a/hw/acpi/ich9.c
>> >> >> +++ b/hw/acpi/ich9.c
>> >> >> @@ -483,13 +483,23 @@ 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)
>> >> >broken alignment?
>> >> >
>> >> >run /scripts/checkpatch.pl on patches before submitting them
>> >> 
>> >> I copied this from ich9_pm_device_plug_cb(), so thought this is the
>> >> correct style.
>> >> 
>> >> Will adjust this according to checkpatch.pl.
>> >
>> >see CODING_STYLE
>> >
>> 
>> Went throught this file, but not find the description of function
>> definition's second line.
>A patch fixing CODING_STYLE that is welcome
>

Hmm... writing document seems more difficult than writing code to me :-)

Let me have a try to see if my English is not that bad.

-- 
Wei Yang
Help you, Help me