Re: [Qemu-devel] [PATCH 33/35] pc: ACPI BIOS: reserve SRAT entry for hotplug mem hole

[Igor Mammedov]

On Wed, 28 May 2014 18:38:13 +0200
Vasilis Liaskovitis <address@hidden> 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 <address@hidden> 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 <address@hidden> 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
>