From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43280) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ymyab-0003cL-TM for qemu-devel@nongnu.org; Tue, 28 Apr 2015 02:01:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YmyaW-0005tj-Us for qemu-devel@nongnu.org; Tue, 28 Apr 2015 02:01:49 -0400 Received: from [59.151.112.132] (port=11257 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YmyaS-0005qn-Gd for qemu-devel@nongnu.org; Tue, 28 Apr 2015 02:01:44 -0400 Message-ID: <553F1F77.6070405@cn.fujitsu.com> Date: Tue, 28 Apr 2015 13:49:43 +0800 From: Zhu Guihua MIME-Version: 1.0 References: <20150427210912-mutt-send-email-mst@redhat.com> In-Reply-To: <20150427210912-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 6/8] acpi: fix "Memory device control fields" register List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: imammedo@redhat.com, qemu-devel@nongnu.org, tangchen@cn.fujitsu.com, izumi.taku@jp.fujitsu.com, guz.fnst@cn.fujitsu.com, pbonzini@redhat.com On 04/28/2015 03:09 AM, Michael S. Tsirkin wrote: > On Mon, Apr 27, 2015 at 04:47:20PM +0800, Zhu Guihua wrote: >> 0 bit in Memory device control fields must be cleared before writing to >> register. But now this field isn't cleared when other fields are written. >> >> To solve this bug, This patch fixes UpdateRule to WriteAsZeros in "Memory >> device control fields" register. >> >> Reviewed-by: Igor Mammedov >> Signed-off-by: Zhu Guihua > Applied, thanks! > Please check pci branch in my tree as I had to > resolve some trivial conflicts. What about other un-applied patches in this series ? Should I rebase them on your pci branch and resend then again ? Thanks, Zhu > >> --- >> docs/specs/acpi_mem_hotplug.txt | 5 ++++- >> hw/i386/acpi-build.c | 2 +- >> 2 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt >> index dcc4808..68daa14 100644 >> --- a/docs/specs/acpi_mem_hotplug.txt >> +++ b/docs/specs/acpi_mem_hotplug.txt >> @@ -31,7 +31,10 @@ 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 >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index dc1b8e8..eb66fef 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -924,7 +924,7 @@ build_ssdt(GArray *table_data, GArray *linker, >> aml_append(scope, field); >> >> field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_byte_acc, >> - aml_preserve); >> + 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)); >> -- >> 1.9.3 > On Mon, Apr 27, 2015 at 04:47:21PM +0800, Zhu Guihua wrote: >> - implements QEMU hardware part of memory hot unplug protocol >> described at "docs/spec/acpi_mem_hotplug.txt" >> - handles memory remove notification event >> - handles device eject notification >> >> Reviewed-by: Igor Mammedov >> 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 | 12 ++++++++++++ >> hw/i386/acpi-dsdt-mem-hotplug.dsl | 13 ++++++++++++- >> include/hw/acpi/pc-hotplug.h | 3 +++ >> include/hw/qdev-core.h | 1 + >> trace-events | 3 +++ >> 8 files changed, 61 insertions(+), 5 deletions(-) >> >> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt >> index 68daa14..3df3620 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 eject request to OSPM was issued. >> + 3-7: reserved and should be ignored by OSPM >> [0x15-0x17] reserved >> >> write access: >> @@ -38,7 +40,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 eject request 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 >> >> 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 07e281f..35bbfeb 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; >> @@ -128,13 +131,29 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, >> 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) { >> + mdev->is_removing = false; >> + trace_mhp_acpi_clear_remove_evt(mem_st->selector); >> + } else if (data & 8) { >> + if (!mdev->is_enabled) { >> + trace_mhp_acpi_ejecting_invalid_slot(mem_st->selector); >> + break; >> + } >> + >> + dev = DEVICE(mdev->dimm); >> + hotplug_ctrl = qdev_get_hotplug_handler(dev); >> + /* call pc-dimm unplug cb */ >> + hotplug_handler_unplug(hotplug_ctrl, dev, NULL); >> + trace_mhp_acpi_pc_dimm_deleted(mem_st->selector); >> } >> break; >> + default: >> + break; >> } >> >> } >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> index 6e6a65d..b0f0f84 100644 >> --- a/hw/core/qdev.c >> +++ b/hw/core/qdev.c >> @@ -273,7 +273,7 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id, >> dev->alias_required_for_version = required_for_version; >> } >> >> -static HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev) >> +HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev) >> { >> HotplugHandler *hotplug_ctrl = NULL; >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index eb66fef..96c1db3 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -931,6 +931,12 @@ build_ssdt(GArray *table_data, GArray *linker, >> aml_append(field, >> /*(read) 1 if has a insert event. (write) 1 to clear event */ >> aml_named_field(stringify(MEMORY_SLOT_INSERT_EVENT), 1)); >> + aml_append(field, >> + /* (read) 1 if has a remove event. (write) 1 to clear event */ >> + aml_named_field(stringify(MEMORY_SLOT_REMOVE_EVENT), 1)); >> + aml_append(field, >> + /* initiates device eject, write only */ >> + aml_named_field(stringify(MEMORY_SLOT_EJECT), 1)); >> aml_append(scope, field); >> >> field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_dword_acc, >> @@ -975,6 +981,12 @@ build_ssdt(GArray *table_data, GArray *linker, >> ))); >> aml_append(dev, method); >> >> + method = aml_method("_EJ0", 1); >> + s = BASEPATH stringify(MEMORY_SLOT_EJECT_METHOD); >> + aml_append(method, aml_return(aml_call2( >> + s, aml_name("_UID"), aml_arg(0)))); >> + aml_append(dev, method); >> + >> aml_append(sb_scope, dev); >> } >> >> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl >> index 1e9ec39..c2bb6a1 100644 >> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl >> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl >> @@ -29,6 +29,8 @@ >> External(MEMORY_SLOT_PROXIMITY, FieldUnitObj) // read only >> External(MEMORY_SLOT_ENABLED, FieldUnitObj) // 1 if enabled, read only >> External(MEMORY_SLOT_INSERT_EVENT, FieldUnitObj) // (read) 1 if has a insert event. (write) 1 to clear event >> + External(MEMORY_SLOT_REMOVE_EVENT, FieldUnitObj) // (read) 1 if has a remove event. (write) 1 to clear event >> + External(MEMORY_SLOT_EJECT, FieldUnitObj) // initiates device eject, write only >> External(MEMORY_SLOT_SLECTOR, FieldUnitObj) // DIMM selector, write only >> External(MEMORY_SLOT_OST_EVENT, FieldUnitObj) // _OST event code, write only >> External(MEMORY_SLOT_OST_STATUS, FieldUnitObj) // _OST status code, write only >> @@ -55,8 +57,10 @@ >> If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory device needs check >> MEMORY_SLOT_NOTIFY_METHOD(Local0, 1) >> Store(1, MEMORY_SLOT_INSERT_EVENT) >> + } Elseif (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // Ejection request >> + MEMORY_SLOT_NOTIFY_METHOD(Local0, 3) >> + Store(1, MEMORY_SLOT_REMOVE_EVENT) >> } >> - // TODO: handle memory eject request >> Add(Local0, One, Local0) // goto next DIMM >> } >> Release(MEMORY_SLOT_LOCK) >> @@ -156,5 +160,12 @@ >> Store(Arg2, MEMORY_SLOT_OST_STATUS) >> Release(MEMORY_SLOT_LOCK) >> } >> + >> + Method(MEMORY_SLOT_EJECT_METHOD, 2) { >> + Acquire(MEMORY_SLOT_LOCK, 0xFFFF) >> + Store(ToInteger(Arg0), MEMORY_SLOT_SLECTOR) // select DIMM >> + Store(1, MEMORY_SLOT_EJECT) >> + Release(MEMORY_SLOT_LOCK) >> + } >> } // Device() >> } // Scope() >> diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h >> index efa6ed7..0513c1c 100644 >> --- a/include/hw/acpi/pc-hotplug.h >> +++ b/include/hw/acpi/pc-hotplug.h >> @@ -43,6 +43,8 @@ >> #define MEMORY_SLOT_PROXIMITY MPX >> #define MEMORY_SLOT_ENABLED MES >> #define MEMORY_SLOT_INSERT_EVENT MINS >> +#define MEMORY_SLOT_REMOVE_EVENT MRMV >> +#define MEMORY_SLOT_EJECT MEJ >> #define MEMORY_SLOT_SLECTOR MSEL >> #define MEMORY_SLOT_OST_EVENT MOEV >> #define MEMORY_SLOT_OST_STATUS MOSC >> @@ -51,6 +53,7 @@ >> #define MEMORY_SLOT_CRS_METHOD MCRS >> #define MEMORY_SLOT_OST_METHOD MOST >> #define MEMORY_SLOT_PROXIMITY_METHOD MPXM >> +#define MEMORY_SLOT_EJECT_METHOD MEJ0 >> #define MEMORY_SLOT_NOTIFY_METHOD MTFY >> #define MEMORY_SLOT_SCAN_METHOD MSCN >> >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> index 4e673f9..5b7acf1 100644 >> --- a/include/hw/qdev-core.h >> +++ b/include/hw/qdev-core.h >> @@ -266,6 +266,7 @@ int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT; >> void qdev_init_nofail(DeviceState *dev); >> void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id, >> int required_for_version); >> +HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev); >> void qdev_unplug(DeviceState *dev, Error **errp); >> void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev, >> DeviceState *dev, Error **errp); >> diff --git a/trace-events b/trace-events >> index 30eba92..46f6ef0 100644 >> --- a/trace-events >> +++ b/trace-events >> @@ -1562,6 +1562,7 @@ vfio_put_base_device(int fd) "close vdev->fd=%d" >> >> #hw/acpi/memory_hotplug.c >> mhp_acpi_invalid_slot_selected(uint32_t slot) "0x%"PRIx32 >> +mhp_acpi_ejecting_invalid_slot(uint32_t slot) "0x%"PRIx32 >> mhp_acpi_read_addr_lo(uint32_t slot, uint32_t addr) "slot[0x%"PRIx32"] addr lo: 0x%"PRIx32 >> mhp_acpi_read_addr_hi(uint32_t slot, uint32_t addr) "slot[0x%"PRIx32"] addr hi: 0x%"PRIx32 >> mhp_acpi_read_size_lo(uint32_t slot, uint32_t size) "slot[0x%"PRIx32"] size lo: 0x%"PRIx32 >> @@ -1572,6 +1573,8 @@ mhp_acpi_write_slot(uint32_t slot) "set active slot: 0x%"PRIx32 >> mhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "slot[0x%"PRIx32"] OST EVENT: 0x%"PRIx32 >> mhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "slot[0x%"PRIx32"] OST STATUS: 0x%"PRIx32 >> mhp_acpi_clear_insert_evt(uint32_t slot) "slot[0x%"PRIx32"] clear insert event" >> +mhp_acpi_clear_remove_evt(uint32_t slot) "slot[0x%"PRIx32"] clear remove event" >> +mhp_acpi_pc_dimm_deleted(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm deleted" >> >> # hw/i386/pc.c >> mhp_pc_dimm_assigned_slot(int slot) "0x%d" >> -- >> 1.9.3 > On Mon, Apr 27, 2015 at 04:47:22PM +0800, Zhu Guihua wrote: >> When memory hot unplug fails, this patch adds support to send >> QMP event to notify mgmt about this failure. >> >> Reviewed-by: Igor Mammedov >> Signed-off-by: Zhu Guihua >> --- >> docs/qmp/qmp-events.txt | 17 +++++++++++++++++ >> hw/acpi/memory_hotplug.c | 10 +++++++++- >> qapi/event.json | 14 ++++++++++++++ >> trace-events | 1 + >> 4 files changed, 41 insertions(+), 1 deletion(-) >> >> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt >> index d759d19..3be468f 100644 >> --- a/docs/qmp/qmp-events.txt >> +++ b/docs/qmp/qmp-events.txt >> @@ -226,6 +226,23 @@ Example: >> { "event": "GUEST_PANICKED", >> "data": { "action": "pause" } } >> >> +MEM_HOT_UNPLUG_ERROR >> +-------------------- >> +Emitted when memory hot unplug error occurs. >> + >> +Data: >> + >> +- "device": device name (json-string) >> +- "msg": Informative message (e.g., reason for the error) (json-string) >> + >> +Example: >> + >> +{ "event": "MEM_HOT_UNPLUG_ERROR" >> + "data": { "device": "dimm1", >> + "msg": "acpi: device unplug for unsupported device" >> + }, >> + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } >> + >> NIC_RX_FILTER_CHANGED >> --------------------- >> >> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c >> index 35bbfeb..34cef1e 100644 >> --- a/hw/acpi/memory_hotplug.c >> +++ b/hw/acpi/memory_hotplug.c >> @@ -94,6 +94,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, >> ACPIOSTInfo *info; >> DeviceState *dev = NULL; >> HotplugHandler *hotplug_ctrl = NULL; >> + Error *local_err = NULL; >> >> if (!mem_st->dev_count) { >> return; >> @@ -148,7 +149,14 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, >> dev = DEVICE(mdev->dimm); >> hotplug_ctrl = qdev_get_hotplug_handler(dev); >> /* call pc-dimm unplug cb */ >> - hotplug_handler_unplug(hotplug_ctrl, dev, NULL); >> + hotplug_handler_unplug(hotplug_ctrl, dev, &local_err); >> + if (local_err) { >> + trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector); >> + qapi_event_send_mem_unplug_error(dev->id, >> + error_get_pretty(local_err), >> + &error_abort); >> + break; >> + } >> trace_mhp_acpi_pc_dimm_deleted(mem_st->selector); >> } >> break; >> diff --git a/qapi/event.json b/qapi/event.json >> index c51dc49..378dda5 100644 >> --- a/qapi/event.json >> +++ b/qapi/event.json >> @@ -330,3 +330,17 @@ >> ## >> { 'event': 'VSERPORT_CHANGE', >> 'data': { 'id': 'str', 'open': 'bool' } } >> + >> +## >> +# @MEM_UNPLUG_ERROR >> +# >> +# Emitted when memory hot unplug error occurs. >> +# >> +# @device: device name >> +# >> +# @msg: Informative message >> +# >> +# Since: 2.4 >> +## >> +{ 'event': 'MEM_UNPLUG_ERROR', >> + 'data': { 'device': 'str', 'msg': 'str' } } >> diff --git a/trace-events b/trace-events >> index 46f6ef0..11387c3 100644 >> --- a/trace-events >> +++ b/trace-events >> @@ -1575,6 +1575,7 @@ mhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "slot[0x%"PRIx32"] OST STA >> mhp_acpi_clear_insert_evt(uint32_t slot) "slot[0x%"PRIx32"] clear insert event" >> mhp_acpi_clear_remove_evt(uint32_t slot) "slot[0x%"PRIx32"] clear remove event" >> mhp_acpi_pc_dimm_deleted(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm deleted" >> +mhp_acpi_pc_dimm_delete_failed(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm delete failed" >> >> # hw/i386/pc.c >> mhp_pc_dimm_assigned_slot(int slot) "0x%d" >> -- >> 1.9.3 > > . >