From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36906) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaM7x-0008Uh-Dz for qemu-devel@nongnu.org; Tue, 24 Mar 2015 06:32:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaM7u-0003Eo-6X for qemu-devel@nongnu.org; Tue, 24 Mar 2015 06:32:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51514) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaM7t-0003Eb-PZ for qemu-devel@nongnu.org; Tue, 24 Mar 2015 06:32:02 -0400 Date: Tue, 24 Mar 2015 11:31:56 +0100 From: Igor Mammedov Message-ID: <20150324113156.2bf65f86@nial.brq.redhat.com> In-Reply-To: <551130AD.1060408@cn.fujitsu.com> References: <3509802a09b0e89aa9c169ba7a25e624e826ba5a.1426494342.git.zhugh.fnst@cn.fujitsu.com> <20150316155933.1186d483@nial.brq.redhat.com> <551130AD.1060408@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Zhu Guihua 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 Tue, 24 Mar 2015 17:38:53 +0800 Zhu Guihua wrote: > > 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. It's not clear to me what a problem you see here. Could you give more extended explanation, pls? > > 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; > >> } > >> > >> } > >> > [...]