From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53069) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaLK1-00071D-Cf for qemu-devel@nongnu.org; Tue, 24 Mar 2015 05:40:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaLJv-00010h-Ct for qemu-devel@nongnu.org; Tue, 24 Mar 2015 05:40:29 -0400 Received: from [59.151.112.132] (port=1149 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaLJu-0000wj-Gh for qemu-devel@nongnu.org; Tue, 24 Mar 2015 05:40:23 -0400 Message-ID: <551130AD.1060408@cn.fujitsu.com> Date: Tue, 24 Mar 2015 17:38:53 +0800 From: Zhu Guihua MIME-Version: 1.0 References: <3509802a09b0e89aa9c169ba7a25e624e826ba5a.1426494342.git.zhugh.fnst@cn.fujitsu.com> <20150316155933.1186d483@nial.brq.redhat.com> In-Reply-To: <20150316155933.1186d483@nial.brq.redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: mst@redhat.com, qemu-devel@nongnu.org, tangchen@cn.fujitsu.com, izumi.taku@jp.fujitsu.com, guz.fnst@cn.fujitsu.com, pbonzini@redhat.com On 03/16/2015 10:59 PM, Igor Mammedov wrote: > On Mon, 16 Mar 2015 16:58:18 +0800 > Zhu Guihua wrote: > >> This patch adds a new bit to memory hotplug IO port indicating that > actually bit was added in 2/6 where is_removing had been added. > >> EJ0 has been evaluated by guest OS. And call pc-dimm unplug cb to do >> the real removal. >> >> Signed-off-by: Zhu Guihua >> --- >> docs/specs/acpi_mem_hotplug.txt | 11 +++++++++-- >> hw/acpi/memory_hotplug.c | 21 +++++++++++++++++++-- >> hw/core/qdev.c | 2 +- >> hw/i386/acpi-build.c | 9 +++++++++ >> hw/i386/acpi-dsdt-mem-hotplug.dsl | 10 ++++++++++ >> include/hw/acpi/pc-hotplug.h | 2 ++ >> include/hw/qdev-core.h | 1 + >> trace-events | 1 + >> 8 files changed, 52 insertions(+), 5 deletions(-) >> >> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt >> index 1290994..85cd4b8 100644 >> --- a/docs/specs/acpi_mem_hotplug.txt >> +++ b/docs/specs/acpi_mem_hotplug.txt >> @@ -19,7 +19,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access): >> 1: Device insert event, used to distinguish device for which >> no device check event to OSPM was issued. >> It's valid only when bit 1 is set. >> - 2-7: reserved and should be ignored by OSPM >> + 2: Device remove event, used to distinguish device for which >> + no device check event to OSPM was issued. >> + 3-7: reserved and should be ignored by OSPM >> [0x15-0x17] reserved >> >> write access: >> @@ -35,7 +37,12 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access): >> 1: if set to 1 clears device insert event, set by OSPM >> after it has emitted device check event for the >> selected memory device >> - 2-7: reserved, OSPM must clear them before writing to register >> + 2: if set to 1 clears device remove event, set by OSPM >> + after it has emitted device check event for the >> + selected memory device. if guest fails to eject device, it >> + should send OST event about it and forget about device >> + removal. >> + 3-7: reserved, OSPM must clear them before writing to register >> >> Selecting memory device slot beyond present range has no effect on platform: >> - write accesses to memory hot-plug registers not documented above are >> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c >> index 687b2f1..d6b8c89 100644 >> --- a/hw/acpi/memory_hotplug.c >> +++ b/hw/acpi/memory_hotplug.c >> @@ -2,6 +2,7 @@ >> #include "hw/acpi/pc-hotplug.h" >> #include "hw/mem/pc-dimm.h" >> #include "hw/boards.h" >> +#include "hw/qdev-core.h" >> #include "trace.h" >> #include "qapi-event.h" >> >> @@ -91,6 +92,8 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, >> MemHotplugState *mem_st = opaque; >> MemStatus *mdev; >> ACPIOSTInfo *info; >> + DeviceState *dev = NULL; >> + HotplugHandler *hotplug_ctrl = NULL; >> >> if (!mem_st->dev_count) { >> return; >> @@ -122,19 +125,33 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, >> mdev = &mem_st->devs[mem_st->selector]; >> mdev->ost_status = data; >> trace_mhp_acpi_write_ost_status(mem_st->selector, mdev->ost_status); >> - /* TODO: implement memory removal on guest signal */ >> >> info = acpi_memory_device_status(mem_st->selector, mdev); >> qapi_event_send_acpi_device_ost(info, &error_abort); >> qapi_free_ACPIOSTInfo(info); >> break; >> - case 0x14: >> + case 0x14: /* set is_* fields */ >> mdev = &mem_st->devs[mem_st->selector]; >> if (data & 2) { /* clear insert event */ >> mdev->is_inserting = false; >> trace_mhp_acpi_clear_insert_evt(mem_st->selector); >> + } else if (data & 4) { /* request removal of device */ > fix comment to match docs above. > >> + mdev->is_removing = false; >> + trace_mhp_acpi_clear_remove_evt(mem_st->selector); > just clear event here and don't do removal part as it doesn't match > documentation you've written above regarding this field. > > It would be better to move is_removing handling from here to 2/6 > + related ASL code from DSDT which should clear it after sending device check. > >> + /* >> + * QEMU memory hot unplug is an asynchronous procedure. QEMU first >> + * calls pc-dimm unplug request cb to send a SCI to guest. When the >> + * guest OS finished handling the SCI, it evaluates ACPI EJ0, and >> + * QEMU calls pc-dimm unplug cb to remove memory device. >> + */ > something like this comment, should be in acpi_mem_hotplug.txt not here. > > > There is 'is_enabled' field, which is 1 if device is present, we can use it > for triggering actual ejecting in QEMU from EJ0(), something like: > > } else if (data & 1) { /* eject device */ I think this is not correct. When you clear insert event, the 'is_enabled' filed was also 1. And when we hot remove memory, the addr 0x14 will be written only once. Thanks, Zhu >> + dev = DEVICE(mdev->dimm); > potential NULL dereference, dimm could be NULL if guest does eject twice > or does eject of empty slot. > Perhaps add check before accessing dimm. > > if(!mdev->is_enabled) { > trace_..._ejecting_invalid_slot(...) > break; > } > >> + hotplug_ctrl = qdev_get_hotplug_handler(dev); >> + /* Call pc-dimm unplug cb. */ >> + hotplug_handler_unplug(hotplug_ctrl, dev, NULL); > It's not that we can do anything about error at this point > but instead of forgetting it silently at least log error in trace, > the best would be in addition to that send QMP event to notify mgmt > about it. (sending QMP event could be a separate patch) > > >> } >> break; >> + default: >> + break; >> } >> >> } >> [...]