Re: [SeaBIOS] [Qemu-devel] [RFC PATCH 2/9] Implement acpi-dsdt functions for memory hotplug.

[Igor Mammedov]

On 04/19/2012 04:08 PM, Vasilis Liaskovitis wrote:
> Extend the DSDT to include methods for handling memory hot-add and hot-remove
> notifications and memory device status requests. These functions are called
> from the memory device SSDT methods.
>
> Eject has only been tested with level gpe event, but will be changed to edge gpe
> event soon, according to recent master patch for other ACPI hotplug events.
> Signed-off-by: Vasilis Liaskovitis
>
> src/acpi-dsdt.dsl |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
> index 4bdc268..184daf0 100644
> --- a/src/acpi-dsdt.dsl
> +++ b/src/acpi-dsdt.dsl
> @@ -709,9 +709,72 @@ DefinitionBlock (
>               }
>               Return(One)
>           }
> }
>    /* Objects filled in by run-time generated SSDT */
>    External(MTFY, MethodObj)
>    External(MEON, PkgObj)
>    Method (CMST, 1, NotSerialized) {
>        // _STA method - return ON status of memdevice
>        // Local0 = MEON flag for this cpu
>        Store(DerefOf(Index(MEON, Arg0)), Local0)
>        If (Local0) { Return(0xF) } Else { Return(0x0) }
>    }
>    /* Memory eject notify method */
>    OperationRegion(MEMJ, SystemIO, 0xaf40, 32)
>    Field (MEMJ, ByteAcc, NoLock, Preserve)
>    {
>        MPE, 256
>    }
>    Method (MPEJ, 2, NotSerialized) {
>        // _EJ0 method - eject callback
>        Store(ShiftLeft(1,Arg0), MPE)
>        Sleep(200)
>    }

MPE is write only and only one memslot is ejected at a time. Why 256 bit-field is here then?
Could we use just 1 byte and write a slot number into it and save some io address space this way?
>    /* Memory hotplug notify method */
>    OperationRegion(MEST, SystemIO, 0xaf20, 32)

It's more a suggestion: move it a bit farther to allow maybe 1024 cpus in the future.
That will prevent compatibility a headache, if we decide to expand support to more then
256 cpus.

Or event better to make this address configurable in run-time and build this var along
with SSDT (converting along the way all other hard-coded io ports to the same generic
run-time interface). This wish is out of scope of this patch-set, but what
do you think about the idea?
>    Field (MEST, ByteAcc, NoLock, Preserve)
>    {
>        MES, 256
>    }
>    Method(MESC, 0) {
>        // Local5 = active memdevice bitmap
>        Store (MES, Local5)
>        // Local2 = last read byte from bitmap
>        Store (Zero, Local2)
>        // Local0 = memory device iterator
>        Store (Zero, Local0)
>        While (LLess(Local0, SizeOf(MEON))) {
>            // Local1 = MEON flag for this memory device
>            Store(DerefOf(Index(MEON, Local0)), Local1)
>            If (And(Local0, 0x07)) {
>                // Shift down previously read bitmap byte
>                ShiftRight(Local2, 1, Local2)
>            } Else {
>                // Read next byte from memdevice bitmap
>                Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2)
>            }
>            // Local3 = active state for this memory device
>            Store(And(Local2, 1), Local3)
>
>            If (LNotEqual(Local1, Local3)) {
>                // State change - update MEON with new state
>                Store(Local3, Index(MEON, Local0))
>                // Do MEM notify
>                If (LEqual(Local3, 1)) {
>                    MTFY(Local0, 1)
>                } Else {
>                    MTFY(Local0, 3)
>                }
>            }
>            Increment(Local0)
>        }
>        Return(One)
>    }
>
> }
> /****************************************************************
> General purpose events
>
> ****************************************************************/
>
> @@ -732,7 +795,8 @@ DefinitionBlock (
>               Return(_SB.PRSC())
>           }
>           Method(_L03) {
>        Return(0x01)
>        // Memory hotplug event
>        Return(\_SB.MESC())
>     }
>     Method(_L04) {
>         Return(0x01)

[Qemu-devel] [RFC PATCH 2/9] Implement acpi-dsdt functions for memory hotplug.

[Vasilis Liaskovitis]

Hi,

On Fri, Apr 20, 2012 at 12:55:24PM +0200, Igor Mammedov wrote:
> >    /* Memory eject notify method */
> >    OperationRegion(MEMJ, SystemIO, 0xaf40, 32)
> >    Field (MEMJ, ByteAcc, NoLock, Preserve)
> >    {
> >        MPE, 256
> >    }
> >    Method (MPEJ, 2, NotSerialized) {
> >        // _EJ0 method - eject callback
> >        Store(ShiftLeft(1,Arg0), MPE)
> >        Sleep(200)
> >    }
>
> MPE is write only and only one memslot is ejected at a time. Why 256 bit-field is here then?
> Could we use just 1 byte and write a slot number into it and save some io address space this way?

good point. This was implemented similarly to the hot-add/status register only
for symmetry, but you are right, since only one slot is ejected at a time, this
can be reduced to one byte and save space. I will update for the next version.
> >    /* Memory hotplug notify method */
> >    OperationRegion(MEST, SystemIO, 0xaf20, 32)
>
> It's more a suggestion: move it a bit farther to allow maybe 1024 cpus in the future.
> That will prevent compatibility a headache, if we decide to expand support to more then
> 256 cpus.

ok, I will move it to 0xaf80 or higher (so cpu-hotplug could be extended to at
least 1024 cpus)
> Or event better to make this address configurable in run-time and build this var along
> with SSDT (converting along the way all other hard-coded io ports to the same generic
> run-time interface). This wish is out of scope of this patch-set, but what
> do you think about the idea?

yes, that would give more flexibility and avoid more compatibility headaches.
As you say it's not a main issue for the series, but I can work on it as we start
converting hardcoded i/o ports to configurable properties.

thanks,

- Vasilis