From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48037) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ymyx8-0004pb-Ia for qemu-devel@nongnu.org; Tue, 28 Apr 2015 02:25:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ymyx5-0008Ey-8L for qemu-devel@nongnu.org; Tue, 28 Apr 2015 02:25:06 -0400 Received: from [59.151.112.132] (port=6353 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ymyx2-0008DS-U7 for qemu-devel@nongnu.org; Tue, 28 Apr 2015 02:25:03 -0400 Message-ID: <553F279F.1070501@cn.fujitsu.com> Date: Tue, 28 Apr 2015 14:24:31 +0800 From: Zhu Guihua MIME-Version: 1.0 References: <20150427210912-mutt-send-email-mst@redhat.com> <553F1F77.6070405@cn.fujitsu.com> In-Reply-To: <553F1F77.6070405@cn.fujitsu.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: qemu-devel@nongnu.org, tangchen@cn.fujitsu.com, pbonzini@redhat.com, izumi.taku@jp.fujitsu.com, guz.fnst@cn.fujitsu.com, imammedo@redhat.com On 04/28/2015 01:49 PM, Zhu Guihua wrote: > > 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 ? Oh, I am sorry to look at this carelessly. All patches in this series were applied in your tree, thanks. And I have checked that, memory hot unplug works as expected. Regards, 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 >> >> . >> > > > . >