On Wed, 28 May 2014 18:38:13 +0200 Vasilis Liaskovitis <vasilis.liaskovi...@profitbricks.com> wrote:
> On Wed, May 28, 2014 at 03:26:42PM +0200, Igor Mammedov wrote: > > 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. > > I think the original intent of MRMV is for it to be a read-only field > indicating the > is_removing field of the memory hotplug hardware. So it's not the same > semantics > as MEJE, which is writable and means "remove the device, it's safe". > But you are right that it could be a singlebit field. The same register [0x14] is control register when writing there, so we can use-reuse the same bit position as MRMV for signaling QEMU to perform eject on writing 1 there, similarly like it's done for insert event. > > thanks, > > - Vasilis >