| Message ID | 3509802a09b0e89aa9c169ba7a25e624e826ba5a.1426494342.git.zhugh.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 16 Mar 2015 16:58:18 +0800 Zhu Guihua <zhugh.fnst@cn.fujitsu.com> 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 <zhugh.fnst@cn.fujitsu.com> > --- > 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 */ > + 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/hw/core/qdev.c b/hw/core/qdev.c > index 6be5866..4676ffb 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 d0a5c85..1ba6102 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -929,6 +929,9 @@ 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(scope, field); > > field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_dword_acc); > @@ -972,6 +975,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..ef847e2 100644 > --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl > +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl > @@ -29,6 +29,7 @@ > 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_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,6 +56,8 @@ > 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) clear removing field here. > } > // TODO: handle memory eject request > Add(Local0, One, Local0) // goto next DIMM > @@ -156,5 +159,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(One, MEMORY_SLOT_REMOVE_EVENT) redo it using enabled field. Otherwise it could cause guest/QEMU crash: - if 1st memory was asked to be removed - then OSPM processes it but has not called _EJ0 yet leaving is_removed set - then QEMU adds/removes another DIMM triggering slots scan which would issue second Notify(remove) since is_removed is still set - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0() > + Release(MEMORY_SLOT_LOCK) > + } > } // Device() > } // Scope() > diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h > index efa6ed7..680810b 100644 > --- a/include/hw/acpi/pc-hotplug.h > +++ b/include/hw/acpi/pc-hotplug.h > @@ -43,6 +43,7 @@ > #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_SLECTOR MSEL > #define MEMORY_SLOT_OST_EVENT MOEV > #define MEMORY_SLOT_OST_STATUS MOSC > @@ -51,6 +52,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..e552355 100644 > --- a/trace-events > +++ b/trace-events > @@ -1572,6 +1572,7 @@ 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" > > # hw/i386/pc.c > mhp_pc_dimm_assigned_slot(int slot) "0x%d"
On 03/16/2015 10:59 PM, Igor Mammedov wrote: [...] > > diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl > index 1e9ec39..ef847e2 100644 > --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl > +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl > @@ -29,6 +29,7 @@ > 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_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,6 +56,8 @@ > 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) > clear removing field here. You mean clear remove event here? > >> } >> // TODO: handle memory eject request >> Add(Local0, One, Local0) // goto next DIMM >> @@ -156,5 +159,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(One, MEMORY_SLOT_REMOVE_EVENT) > redo it using enabled field. Otherwise it could cause guest/QEMU crash: > > - if 1st memory was asked to be removed > - then OSPM processes it but has not called _EJ0 yet leaving is_removed set > - then QEMU adds/removes another DIMM triggering slots scan > which would issue second Notify(remove) since is_removed is still set > - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0() > If OSPM processed the ejection request but not called _EJ0, the device will still be present in qemu, we must handle this. So I think OSPM issues double EJ0 maybe reasonable in this situation. What's your opinion? Thanks, Zhu >> + Release(MEMORY_SLOT_LOCK) >> + } >> } // Device() >> } // Scope() >> diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h >> index efa6ed7..680810b 100644 >> --- a/include/hw/acpi/pc-hotplug.h >> +++ b/include/hw/acpi/pc-hotplug.h >> @@ -43,6 +43,7 @@ >> #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_SLECTOR MSEL >> #define MEMORY_SLOT_OST_EVENT MOEV >> #define MEMORY_SLOT_OST_STATUS MOSC >> @@ -51,6 +52,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..e552355 100644 >> --- a/trace-events >> +++ b/trace-events >> @@ -1572,6 +1572,7 @@ 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" >> >> # hw/i386/pc.c >> mhp_pc_dimm_assigned_slot(int slot) "0x%d" > . >
On Mon, 23 Mar 2015 18:59:28 +0800 Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote: > > On 03/16/2015 10:59 PM, Igor Mammedov wrote: > [...] > > > > diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl > > index 1e9ec39..ef847e2 100644 > > --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl > > +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl > > @@ -29,6 +29,7 @@ > > 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_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,6 +56,8 @@ > > 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) > > clear removing field here. > > You mean clear remove event here? yes > > > > >> } > >> // TODO: handle memory eject request > >> Add(Local0, One, Local0) // goto next DIMM > >> @@ -156,5 +159,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(One, MEMORY_SLOT_REMOVE_EVENT) > > redo it using enabled field. Otherwise it could cause guest/QEMU crash: > > > > - if 1st memory was asked to be removed > > - then OSPM processes it but has not called _EJ0 yet leaving is_removed set > > - then QEMU adds/removes another DIMM triggering slots scan > > which would issue second Notify(remove) since is_removed is still set > > - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0() > > > > If OSPM processed the ejection request but not called _EJ0, the device > will still be present in qemu, > we must handle this. There is nothing to handle in this case, if OSPM hasn't called _EJ0 then nothing happens and device stays in QEMU. >So I think OSPM issues double EJ0 maybe reasonable > in this situation. > What's your opinion? the first _EJ0 must do ejection, as for the second I think it should be NOP. > > Thanks, > Zhu > [...]
On 03/23/2015 08:47 PM, Igor Mammedov wrote: > On Mon, 23 Mar 2015 18:59:28 +0800 > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote: > >> On 03/16/2015 10:59 PM, Igor Mammedov wrote: >> [...] >>> >>> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl >>> index 1e9ec39..ef847e2 100644 >>> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl >>> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl >>> @@ -29,6 +29,7 @@ >>> 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_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,6 +56,8 @@ >>> 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) >>> clear removing field here. >> You mean clear remove event here? > yes I tested this method, clear remove event here will lead to guest kernel panic. >>>> } >>>> // TODO: handle memory eject request >>>> Add(Local0, One, Local0) // goto next DIMM >>>> @@ -156,5 +159,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(One, MEMORY_SLOT_REMOVE_EVENT) >>> redo it using enabled field. Otherwise it could cause guest/QEMU crash: >>> >>> - if 1st memory was asked to be removed >>> - then OSPM processes it but has not called _EJ0 yet leaving is_removed set >>> - then QEMU adds/removes another DIMM triggering slots scan >>> which would issue second Notify(remove) since is_removed is still set >>> - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0() >>> >> If OSPM processed the ejection request but not called _EJ0, the device >> will still be present in qemu, >> we must handle this. > There is nothing to handle in this case, if OSPM hasn't called _EJ0 then > nothing happens and device stays in QEMU. > >> So I think OSPM issues double EJ0 maybe reasonable >> in this situation. >> What's your opinion? > the first _EJ0 must do ejection, as for the second I think it should be NOP. So we should judge the enabled field to check whether the device is present before issuing Notify(remove)? Thanks, Zhu >> Thanks, >> Zhu >> > [...] > . >
On 03/16/2015 10:59 PM, Igor Mammedov wrote: > On Mon, 16 Mar 2015 16:58:18 +0800 > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> 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 <zhugh.fnst@cn.fujitsu.com> >> --- >> 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. 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; >> } >> >> } >> [...]
On Tue, 24 Mar 2015 17:34:29 +0800 Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote: > > On 03/23/2015 08:47 PM, Igor Mammedov wrote: > > On Mon, 23 Mar 2015 18:59:28 +0800 > > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote: > > > >> On 03/16/2015 10:59 PM, Igor Mammedov wrote: > >> [...] > >>> > >>> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl > >>> index 1e9ec39..ef847e2 100644 > >>> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl > >>> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl > >>> @@ -29,6 +29,7 @@ > >>> 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_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,6 +56,8 @@ > >>> 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) > >>> clear removing field here. > >> You mean clear remove event here? > > yes > > I tested this method, clear remove event here will lead to guest > kernel panic. it shouldn't cause panic if it only clears flag in QEMU (that's what it should do). > > >>>> } > >>>> // TODO: handle memory eject request > >>>> Add(Local0, One, Local0) // goto next DIMM > >>>> @@ -156,5 +159,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(One, MEMORY_SLOT_REMOVE_EVENT) > >>> redo it using enabled field. Otherwise it could cause guest/QEMU crash: > >>> > >>> - if 1st memory was asked to be removed > >>> - then OSPM processes it but has not called _EJ0 yet leaving is_removed set > >>> - then QEMU adds/removes another DIMM triggering slots scan > >>> which would issue second Notify(remove) since is_removed is still set > >>> - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0() > >>> > >> If OSPM processed the ejection request but not called _EJ0, the device > >> will still be present in qemu, > >> we must handle this. > > There is nothing to handle in this case, if OSPM hasn't called _EJ0 then > > nothing happens and device stays in QEMU. > > > >> So I think OSPM issues double EJ0 maybe reasonable > >> in this situation. > >> What's your opinion? > > the first _EJ0 must do ejection, as for the second I think it should be NOP. > > So we should judge the enabled field to check whether the device is > present before > issuing Notify(remove)? I wouldn't check if device is present. I'd unconditionally clear it and make sure on QEMU side that operation is NOP if device is not present. > > Thanks, > Zhu > > >> Thanks, > >> Zhu > >> > > [...] > > . > > >
On Tue, 24 Mar 2015 17:38:53 +0800 Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote: > > On 03/16/2015 10:59 PM, Igor Mammedov wrote: > > On Mon, 16 Mar 2015 16:58:18 +0800 > > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> 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 <zhugh.fnst@cn.fujitsu.com> > >> --- > >> 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; > >> } > >> > >> } > >> > [...]
On 03/24/2015 06:31 PM, Igor Mammedov wrote: > On Tue, 24 Mar 2015 17:38:53 +0800 > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote: > >> On 03/16/2015 10:59 PM, Igor Mammedov wrote: >>> On Mon, 16 Mar 2015 16:58:18 +0800 >>> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> 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 <zhugh.fnst@cn.fujitsu.com> >>>> --- >>>> 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; >>>> } >>>> >>>> } >>>> >> [...] > . >
On 03/24/2015 06:26 PM, Igor Mammedov wrote: > On Tue, 24 Mar 2015 17:34:29 +0800 > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote: > >> On 03/23/2015 08:47 PM, Igor Mammedov wrote: >>> On Mon, 23 Mar 2015 18:59:28 +0800 >>> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote: >>> >>>> On 03/16/2015 10:59 PM, Igor Mammedov wrote: >>>> [...] >>>>> >>>>> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl >>>>> index 1e9ec39..ef847e2 100644 >>>>> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl >>>>> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl >>>>> @@ -29,6 +29,7 @@ >>>>> 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_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,6 +56,8 @@ >>>>> 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) >>>>> clear removing field here. >>>> You mean clear remove event here? >>> yes >> I tested this method, clear remove event here will lead to guest >> kernel panic. > it shouldn't cause panic if it only clears flag in QEMU > (that's what it should do). > > >>>>>> } >>>>>> // TODO: handle memory eject request >>>>>> Add(Local0, One, Local0) // goto next DIMM >>>>>> @@ -156,5 +159,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(One, MEMORY_SLOT_REMOVE_EVENT) >>>>> redo it using enabled field. Otherwise it could cause guest/QEMU crash: >>>>> >>>>> - if 1st memory was asked to be removed >>>>> - then OSPM processes it but has not called _EJ0 yet leaving is_removed set >>>>> - then QEMU adds/removes another DIMM triggering slots scan >>>>> which would issue second Notify(remove) since is_removed is still set >>>>> - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0() >>>>> >>>> If OSPM processed the ejection request but not called _EJ0, the device >>>> will still be present in qemu, >>>> we must handle this. >>> There is nothing to handle in this case, if OSPM hasn't called _EJ0 then >>> nothing happens and device stays in QEMU. >>> >>>> So I think OSPM issues double EJ0 maybe reasonable >>>> in this situation. >>>> What's your opinion? >>> the first _EJ0 must do ejection, as for the second I think it should be NOP. >> So we should judge the enabled field to check whether the device is >> present before >> issuing Notify(remove)? > I wouldn't check if device is present. > I'd unconditionally clear it and make sure on QEMU side that > operation is NOP if device is not present. I'm sorry that I have not fully understood your meaning about 'redo it using enabled field'. How to do it? MEMORY_SLOT_ENABLED is read only, how can I use it to handle EJ0? Thanks, Zhu
On Wed, 25 Mar 2015 14:13:23 +0800 Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote: > > On 03/24/2015 06:26 PM, Igor Mammedov wrote: > > On Tue, 24 Mar 2015 17:34:29 +0800 > > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote: > > > >> On 03/23/2015 08:47 PM, Igor Mammedov wrote: > >>> On Mon, 23 Mar 2015 18:59:28 +0800 > >>> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote: > >>> > >>>> On 03/16/2015 10:59 PM, Igor Mammedov wrote: > >>>> [...] > >>>>> > >>>>> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl > >>>>> index 1e9ec39..ef847e2 100644 > >>>>> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl > >>>>> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl > >>>>> @@ -29,6 +29,7 @@ > >>>>> 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_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,6 +56,8 @@ > >>>>> 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) > >>>>> clear removing field here. > >>>> You mean clear remove event here? > >>> yes > >> I tested this method, clear remove event here will lead to guest > >> kernel panic. > > it shouldn't cause panic if it only clears flag in QEMU > > (that's what it should do). > > > > > >>>>>> } > >>>>>> // TODO: handle memory eject request > >>>>>> Add(Local0, One, Local0) // goto next DIMM > >>>>>> @@ -156,5 +159,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(One, MEMORY_SLOT_REMOVE_EVENT) > >>>>> redo it using enabled field. Otherwise it could cause guest/QEMU crash: > >>>>> > >>>>> - if 1st memory was asked to be removed > >>>>> - then OSPM processes it but has not called _EJ0 yet leaving is_removed set > >>>>> - then QEMU adds/removes another DIMM triggering slots scan > >>>>> which would issue second Notify(remove) since is_removed is still set > >>>>> - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0() > >>>>> > >>>> If OSPM processed the ejection request but not called _EJ0, the device > >>>> will still be present in qemu, > >>>> we must handle this. > >>> There is nothing to handle in this case, if OSPM hasn't called _EJ0 then > >>> nothing happens and device stays in QEMU. > >>> > >>>> So I think OSPM issues double EJ0 maybe reasonable > >>>> in this situation. > >>>> What's your opinion? > >>> the first _EJ0 must do ejection, as for the second I think it should be NOP. > >> So we should judge the enabled field to check whether the device is > >> present before > >> issuing Notify(remove)? > > I wouldn't check if device is present. > > I'd unconditionally clear it and make sure on QEMU side that > > operation is NOP if device is not present. > > I'm sorry that I have not fully understood your meaning about > 'redo it using enabled field'. How to do it? > > MEMORY_SLOT_ENABLED is read only, how can I use it to handle EJ0? it is reserved in write side if you look at spec, but appears that we can't reuse MEMORY_SLOT_ENABLED due to bug in already released QEMU versions. See another reply where I explained it in more details and suggested how QSPM<->QEMU protocol should be amended. > > Thanks, > Zhu > > >
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 */ + mdev->is_removing = false; + trace_mhp_acpi_clear_remove_evt(mem_st->selector); + /* + * 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. + */ + dev = DEVICE(mdev->dimm); + hotplug_ctrl = qdev_get_hotplug_handler(dev); + /* Call pc-dimm unplug cb. */ + hotplug_handler_unplug(hotplug_ctrl, dev, NULL); } break; + default: + break; } } diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 6be5866..4676ffb 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 d0a5c85..1ba6102 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -929,6 +929,9 @@ 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(scope, field); field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_dword_acc); @@ -972,6 +975,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..ef847e2 100644 --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl @@ -29,6 +29,7 @@ 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_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,6 +56,8 @@ 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) } // TODO: handle memory eject request Add(Local0, One, Local0) // goto next DIMM @@ -156,5 +159,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(One, MEMORY_SLOT_REMOVE_EVENT) + Release(MEMORY_SLOT_LOCK) + } } // Device() } // Scope() diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h index efa6ed7..680810b 100644 --- a/include/hw/acpi/pc-hotplug.h +++ b/include/hw/acpi/pc-hotplug.h @@ -43,6 +43,7 @@ #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_SLECTOR MSEL #define MEMORY_SLOT_OST_EVENT MOEV #define MEMORY_SLOT_OST_STATUS MOSC @@ -51,6 +52,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..e552355 100644 --- a/trace-events +++ b/trace-events @@ -1572,6 +1572,7 @@ 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" # hw/i386/pc.c mhp_pc_dimm_assigned_slot(int slot) "0x%d"
This patch adds a new bit to memory hotplug IO port indicating that EJ0 has been evaluated by guest OS. And call pc-dimm unplug cb to do the real removal. Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> --- 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(-)