On Wed, 28 May 2014 14:23:13 +0200
Vasilis Liaskovitis <vasilis.liaskovi...@profitbricks.com> wrote:
> On Wed, May 28, 2014 at 10:07:22AM +0200, Igor Mammedov wrote:
> > On Tue, 27 May 2014 17:57:31 +0200
> > Anshul Makkar <anshul.mak...@profitbricks.com> wrote:
> >
> > > Hi,
> > >
> > > I tested the hot unplug patch and doesn't seem to work properly with
> > > Debian
> > > 6 and Ubuntu host.
> > >
> > > Scenario:
> > > I added 3 dimm devices of 1G each:
> > >
> > > object_add memory-ram,id=ram0,size=1G, device_add
> > > dimm,id=dimm1,memdev=ram0
> > >
> > > object_add memory-ram,id=ram1,size=1G, device_add
> > > dimm,id=dimm2,memdev=ram1
> > >
> > > object_add memory-ram,id=ram2,size=1G, device_add
> > > dimm,id=dimm3,memdev=ram2
> > >
> > > device_del dimm3: I get the OST EVENT EJECT 0x3 and OST STATUS as 0x84(IN
> > > PROGRESS) If I check on the guest, the device has been successfully
> > > removed. But no OST EJECT SUCCESS event was received.
> > I think there should be a SUCCESS event,
> > it should be investigated from the guest side first, OST support in kernel
> > is relatively new.
>
> When testing older guest kernel (3.11), _OST success events are not sent
> from the guest. I haven't tried newer versions yet.
>
> In terms of OSPM _OST behaviour, I am not sure if returning OST success status
> on succcesful removal is *required*. Figure 6-37, page 306 of ACPI spec5.0
> shows that on succcesfull OS ejection ejection, _EJ0 is evaluated. Evaluating
> _OST does not seem to be a requirement, is it? (cc'ing linux-acpi for input)
>
> In linux guests, on successful removal, _EJ0 is always evaluated. I believe we
> should be handling _EJ0 and doing the dimm removal (object_unparent) there.
> Currently OST successes are never received and dimm devices remain in QEMU
> even
> when successfully ejected from guest.
> E.g. a quick patch for _EJ0 handling, on top of Hu Tao's series:
>
> acpi, memory-hotplug: Add _EJ0 handling
>
> ---
> docs/specs/acpi_mem_hotplug.txt | 3 ++-
> hw/acpi/memory_hotplug.c | 13 +++++++------
> hw/i386/ssdt-misc.dsl | 3 ++-
> include/hw/acpi/memory_hotplug.h | 1 +
> 4 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
> index 1290994..1352962 100644
> --- a/docs/specs/acpi_mem_hotplug.txt
> +++ b/docs/specs/acpi_mem_hotplug.txt
> @@ -28,7 +28,8 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte
> access):
> region will read/store data from/to selected memory device.
> [0x4-0x7] OST event code reported by OSPM
> [0x8-0xb] OST status code reported by OSPM
> - [0xc-0x13] reserved, writes into it are ignored
> + [0xc] EJ device if written to
> + [0xd-0x13] reserved, writes into it are ignored
> [0x14] Memory device control fields
> bits:
> 0: reserved, OSPM must clear it before writing to register
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 8aa829d..d3edd28 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -93,9 +93,6 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr
> addr, uint64_t data,
> case 0x03: /* EJECT */
> switch (mdev->ost_status) {
> case 0x0: /* SUCCESS */
> - object_unparent(OBJECT(mdev->dimm));
> - mdev->is_removing = false;
> - mdev->dimm = NULL;
> break;
> case 0x1: /* FAILURE */
> case 0x2: /* UNRECOGNIZED NOTIFY */
> @@ -115,9 +112,6 @@ static void acpi_memory_hotplug_write(void *opaque,
> hwaddr addr, uint64_t data,
> case 0x103: /* OSPM EJECT */
> switch (mdev->ost_status) {
> case 0x0: /* SUCCESS */
> - object_unparent(OBJECT(mdev->dimm));
> - mdev->is_removing = false;
> - mdev->dimm = NULL;
> break;
> case 0x84: /* EJECTION IN PROGRESS */
> mdev->is_enabled = false;
> @@ -137,6 +131,12 @@ static void acpi_memory_hotplug_write(void *opaque,
> hwaddr addr, uint64_t data,
> mdev->is_enabled = false;
> }
> break;
> + case 0x0c:
> + mdev = &mem_st->devs[mem_st->selector];
> + object_unparent(OBJECT(mdev->dimm));
> + mdev->is_removing = false;
> + mdev->dimm = NULL;
> + break;
> }
> }
>
> @@ -238,6 +238,7 @@ static const VMStateDescription vmstate_memhp_sts = {
> VMSTATE_BOOL(is_inserting, MemStatus),
> VMSTATE_UINT32(ost_event, MemStatus),
> VMSTATE_UINT32(ost_status, MemStatus),
> + VMSTATE_UINT32(ej_status, MemStatus),
> VMSTATE_END_OF_LIST()
> }
> };
> diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> index 927e503..d20c6f0 100644
> --- a/hw/i386/ssdt-misc.dsl
> +++ b/hw/i386/ssdt-misc.dsl
> @@ -163,6 +163,7 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC",
> "BXSSDTSUSP", 0x1)
> MSEL, 32, // DIMM selector, write only
> MOEV, 32, // _OST event code, write only
> MOSC, 32, // _OST status code, write only
> + MEJE, 8, // if written to, eject DIMM
> }
>
> Method(MESC, 0, Serialized) {
> @@ -283,7 +284,7 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC",
> "BXSSDTSUSP", 0x1)
> Method(MDEJ, 2) {
> Acquire(MLCK, 0xFFFF)
> Store(ToInteger(Arg0), MSEL) // select DIMM
> - Store(One, MRMV)
> + Store(1, MEJE)
Is there a reason to replace MRMV with MEJE and use whole byte instead of 1 bit
field?
It could be used the same way as MEJE is used.
> Release(MLCK)
> }
> } // Device()
> diff --git a/include/hw/acpi/memory_hotplug.h
> b/include/hw/acpi/memory_hotplug.h
> index 66dbe96..6a9f378 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -14,6 +14,7 @@ typedef struct MemStatus {
> bool is_removing;
> uint32_t ost_event;
> uint32_t ost_status;
> + uint32_t ej_status;
> } MemStatus;
>
> typedef struct MemHotplugState {
