From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42325) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaMOx-0005HQ-LC for qemu-devel@nongnu.org; Tue, 24 Mar 2015 06:49:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaMOs-0000W7-Vv for qemu-devel@nongnu.org; Tue, 24 Mar 2015 06:49:39 -0400 Received: from [59.151.112.132] (port=34913 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaMOr-0000Vp-DF for qemu-devel@nongnu.org; Tue, 24 Mar 2015 06:49:34 -0400 Message-ID: <551140E2.7070006@cn.fujitsu.com> Date: Tue, 24 Mar 2015 18:48:02 +0800 From: Zhu Guihua MIME-Version: 1.0 References: <3509802a09b0e89aa9c169ba7a25e624e826ba5a.1426494342.git.zhugh.fnst@cn.fujitsu.com> <20150316155933.1186d483@nial.brq.redhat.com> <551130AD.1060408@cn.fujitsu.com> <20150324113156.2bf65f86@nial.brq.redhat.com> In-Reply-To: <20150324113156.2bf65f86@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/24/2015 06:31 PM, Igor Mammedov wrote: > 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? When we clear insert event, 'data & 1' was also true, so unplug operation maybe called. This is only asumed situation, because when the addr is 0x14, acpi_memory_hotplug_write() will be called only once, and the value of data is 5, we could not execute the condition of 'data & 1', the unplug operation could not be called. > >> 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; >>>> } >>>> >>>> } >>>> >> [...] > . >