On Tue, 24 Mar 2015 17:38:53 +0800
Zhu Guihua <zhugh.f...@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.f...@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.f...@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;
> >> }
> >>
> >> }
> >>
> [...]
