On Mon, Feb 18, 2019 at 02:53:36PM +0100, Igor Mammedov wrote: >On Mon, 18 Feb 2019 13:21:29 +0000 >Wei Yang <richard.weiy...@gmail.com> 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 <richard.weiy...@gmail.com> 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 <richardw.y...@linux.intel.com> 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 imamm...@redhat.com] >> >> >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 <richardw.y...@linux.intel.com> >> >> >> CC: Igor Mammedov <imamm...@redhat.com> >> >> >> CC: Eric Blake <ebl...@redhat.com> >> >> >> >> >> >> --- >> >> >> v2: >> >> >> * rephrase change log >> >> >one usually adds commenter's name here >> >> > * like this (some...@foo.bar) >> >> >or >> >> > * (some...@foo.bar) >> >> > - entry 1 >> >> > - entry 2 >> >> > * (someone-e...@foo.bar) >> >> > - ... >> >> > >> >> >> >> 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