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

[Vasilis Liaskovitis]

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.

thanks,

- Vasilis