Patchwork [RFC,2/9,SeaBIOS] Implement acpi-dsdt functions for memory hotplug.

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 <vasilis.liaskovitis@profitbricks.com>
---
 src/acpi-dsdt.dsl |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 66 insertions(+), 2 deletions(-)

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<vasilis.liaskovitis@profitbricks.com>
> ---
>   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)

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