From patchwork Wed Mar 25 10:35:28 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Igor Mammedov X-Patchwork-Id: 454354 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 6A746140119 for ; Wed, 25 Mar 2015 21:36:10 +1100 (AEDT) Received: from localhost ([::1]:37916 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaifQ-0001qL-Lh for incoming@patchwork.ozlabs.org; Wed, 25 Mar 2015 06:36:08 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42266) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yaif1-0001T5-JO for qemu-devel@nongnu.org; Wed, 25 Mar 2015 06:35:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yaiey-0003EP-7L for qemu-devel@nongnu.org; Wed, 25 Mar 2015 06:35:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34889) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yaiey-0003Do-0d for qemu-devel@nongnu.org; Wed, 25 Mar 2015 06:35:40 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t2PAZW4u032414 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 25 Mar 2015 06:35:32 -0400 Received: from nial.brq.redhat.com (nial.brq.redhat.com [10.34.26.56]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2PAZTUa000980; Wed, 25 Mar 2015 06:35:30 -0400 Date: Wed, 25 Mar 2015 11:35:28 +0100 From: Igor Mammedov To: Zhu Guihua Message-ID: <20150325113528.56791a6f@nial.brq.redhat.com> In-Reply-To: <551140E2.7070006@cn.fujitsu.com> 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> <551140E2.7070006@cn.fujitsu.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 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 Subject: Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Tue, 24 Mar 2015 18:48:02 +0800 Zhu Guihua wrote: > > 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. Looks like there is a bug that wouldn't allow us to reuse MEMORY_SLOT_ENABLED bit for removal, acpi_mem_hotplug.txt clearly states that 0 bit must be cleared and marks it as reserved. It's caused by Field(MEMORY_HOTPLUG_IO_REGION, ByteAcc, NoLock, Preserve) { ^^^^^^^^ Offset(20), MEMORY_SLOT_ENABLED, 1, // 1 if enabled, read only MEMORY_SLOT_INSERT_EVENT, 1, // (read) 1 if has a insert event. (write) 1 to clear event } the fact that flags field is declared with "Preserve" update rule[1]. We can't fix it since that old guest (with Preserve UpdateRule) migrated to new QEMU will always write to 1st bit whatever value it has had the rest of reserved bits would be always 0 since old guest doesn't use them on read side. However since read/write semantics of fields are different, I'd fix UpdateRule to WriteAsZeros and update protocol documentation as follow: 1. ACPI 5.1: 19.5.46 Field > > > > >> 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; > >>>> } > >>>> > >>>> } > >>>> > >> [...] > > . > > > diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt index 1290994..e2cfbfc 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: @@ -31,11 +33,19 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access): [0xc-0x13] reserved, writes into it are ignored [0x14] Memory device control fields bits: - 0: reserved, OSPM must clear it before writing to register + 0: reserved, OSPM must clear it before writing to register. + Due to BUG in versions prior 2.4 that field isn't + cleared when other fields are written.Keep it reserved + and don't try to reuse it. 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 + 3: if set to 1 initiates device eject, set by OSPM when it + triggers memory device removal and calls _EJ0 method + 4-7: reserved, OSPM must clear them before writing to register and extend aml_field() to support UpdateRule + modify acpi-build.c as follow: diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index d0a5c85..4e81dda 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -922,7 +922,8 @@ build_ssdt(GArray *table_data, GArray *linker, aml_named_field(stringify(MEMORY_SLOT_PROXIMITY), 32)); aml_append(scope, field); - field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_byte_acc); + field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_byte_acc, + aml_write_as_zeros); aml_append(field, aml_reserved_field(160 /* bits, Offset(20) */)); aml_append(field, /* 1 if enabled, read only */ aml_named_field(stringify(MEMORY_SLOT_ENABLED), 1));