Re: [Qemu-devel] [PATCH for-2.9 10/10] memhp: move DIMM devices into dedicated scope with related common methods

[Marcel Apfelbaum <marcel@redhat.com>]

On 12/22/2016 03:31 PM, Igor Mammedov wrote:
> On Thu, 22 Dec 2016 14:31:19 +0200
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 12/06/2016 01:32 AM, Igor Mammedov wrote:
>>> Move DIMM devices from global _SB scope to a new \_SB.MHPC
>>> container along with common methods used by DIMMs:
>>>   MCRS, MRST, MPXM, MOST, MEJ00, MSCN, MTFY
>>>
>>> this reduces AML size on 12 * #slots bytes,
>>> i.e. up to 3072 bytes for 265 slots.
>>>
>>
>> Can you please explain how the bytes number are reduced?
>> Is it because the devices path is now relative to the new container?
> yes
>
>> If so, can you give an example of before/after?
> as example:
>
> -        Device (MP02)
> -        {
> -            Name (_UID, "0x02")  // _UID: Unique ID
> -            Name (_HID, EisaId ("PNP0C80") /* Memory Device */)  // _HID: Hardware ID
> -            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> -            {
> -                Return (\_SB.PCI0.MHPD.MCRS (_UID))
> -            }
> -
> -            Method (_STA, 0, NotSerialized)  // _STA: Status
> -            {
> -                Return (\_SB.PCI0.MHPD.MRST (_UID))
> -            }
> -
> -            Method (_PXM, 0, NotSerialized)  // _PXM: Device Proximity
> -            {
> -                Return (\_SB.PCI0.MHPD.MPXM (_UID))
> -            }
> -
> -            Method (_OST, 3, NotSerialized)  // _OST: OSPM Status Indication
> -            {
> -                Return (\_SB.PCI0.MHPD.MOST (_UID, Arg0, Arg1, Arg2))
> -            }
> -
> -            Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device
> -            {
> -                Return (\_SB.PCI0.MHPD.MEJ0 (_UID, Arg0))
> -            }
> -        }
> ----
> +        Device (MP02)
> +        {
> +            Name (_UID, "0x02")  // _UID: Unique ID
> +            Name (_HID, EisaId ("PNP0C80") /* Memory Device */)  // _HID: Hardware ID
> +            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> +            {
> +                Return (MCRS (_UID))
> +            }
> +
> +            Method (_STA, 0, NotSerialized)  // _STA: Status
> +            {
> +                Return (MRST (_UID))
> +            }
> +
> +            Method (_PXM, 0, NotSerialized)  // _PXM: Device Proximity
> +            {
> +                Return (MPXM (_UID))
> +            }
> +
> +            Method (_OST, 3, NotSerialized)  // _OST: OSPM Status Indication
> +            {
> +                Return (MOST (_UID, Arg0, Arg1, Arg2))
> +            }
> +
> +            Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device
> +            {
> +                Return (MEJ0 (_UID, Arg0))
> +            }
> +        }
>

Thanks for the confirmation.

>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>>  hw/acpi/memory_hotplug.c | 190 ++++++++++++++++++++++++-----------------------
>>>  1 file changed, 97 insertions(+), 93 deletions(-)
>>>
>>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>>> index fb04d24..210073d 100644
>>> --- a/hw/acpi/memory_hotplug.c
>>> +++ b/hw/acpi/memory_hotplug.c
>>> @@ -31,6 +31,7 @@
>>>  #define MEMORY_SLOT_SCAN_METHOD      "MSCN"
>>>  #define MEMORY_HOTPLUG_DEVICE        "MHPD"
>>>  #define MEMORY_HOTPLUG_IO_LEN         24
>>> +#define MEMORY_DEVICES_CONTAINER     "\\_SB.MHPC"
>>>
>>>  static uint16_t memhp_io_base;
>>>
>>> @@ -343,9 +344,8 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>      int i;
>>>      Aml *ifctx;
>>>      Aml *method;
>>> -    Aml *sb_scope;
>>> +    Aml *dev_container;
>>>      Aml *mem_ctrl_dev;
>>> -    char *scan_path;
>>>      char *mhp_res_path;
>>>
>>>      if (!memhp_io_base) {
>>> @@ -356,24 +356,11 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>      mem_ctrl_dev = aml_device("%s", mhp_res_path);
>>>      {
>>>          Aml *crs;
>>> -        Aml *field;
>>> -        Aml *one = aml_int(1);
>>> -        Aml *zero = aml_int(0);
>>> -        Aml *ret_val = aml_local(0);
>>> -        Aml *slot_arg0 = aml_arg(0);
>>> -        Aml *slots_nr = aml_name(MEMORY_SLOTS_NUMBER);
>>> -        Aml *ctrl_lock = aml_name(MEMORY_SLOT_LOCK);
>>> -        Aml *slot_selector = aml_name(MEMORY_SLOT_SLECTOR);
>>>
>>>          aml_append(mem_ctrl_dev, aml_name_decl("_HID", aml_string("PNP0A06")));
>>
>> Do we need the above declaration twice? Here is the first occurrence.
>>
>>>          aml_append(mem_ctrl_dev,
>>>              aml_name_decl("_UID", aml_string("Memory hotplug resources")));
> [...]
>
>>> +
>>> +    dev_container = aml_device(MEMORY_DEVICES_CONTAINER);
>>> +    {
> [...]
>>> +        aml_append(dev_container, aml_name_decl("_HID", aml_string("PNP0A06")));
>>
>> And here is the second occurrence.
> It's different containers, the first one is for MMIO registers
> for x86 it's placed \_SB.PCI0.MHPD consumes resources from parent's PCI0._CRS
> and the second is for memory devices \_SB.MHPC which consumes resources
> outside of PCI0._CRS scope.
>

Understood, here is my last R-b tag, however I want to point out
that I only looked at the re-factoring steps and my acpi-based memory hotplug
knowledge is rather limited.


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

>
>>
>> Thanks,
>> Marcel
>>
>>> +        aml_append(dev_container,
>>> +            aml_name_decl("_UID", aml_string("DIMM devices")));
>>> +
>>> +        assert(nr_mem <= ACPI_MAX_RAM_SLOTS);
>>> +        aml_append(dev_container,
>>> +            aml_name_decl(MEMORY_SLOTS_NUMBER, aml_int(nr_mem))
>>> +        );
>>> +
>>> +        field = aml_field(mmio_path, AML_DWORD_ACC,
>>>                            AML_NOLOCK, AML_PRESERVE);
>>>          aml_append(field, /* read only */
>>>              aml_named_field(MEMORY_SLOT_ADDR_LOW, 32));
>>> @@ -398,9 +410,9 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>              aml_named_field(MEMORY_SLOT_SIZE_HIGH, 32));
>>>          aml_append(field, /* read only */
>>>              aml_named_field(MEMORY_SLOT_PROXIMITY, 32));
>>> -        aml_append(mem_ctrl_dev, field);
>>> +        aml_append(dev_container, field);
>>>
>>> -        field = aml_field(MEMORY_HOTPLUG_IO_REGION, AML_BYTE_ACC,
>>> +        field = aml_field(mmio_path, AML_BYTE_ACC,
>>>                            AML_NOLOCK, AML_WRITE_AS_ZEROS);
>>>          aml_append(field, aml_reserved_field(160 /* bits, Offset(20) */));
>>>          aml_append(field, /* 1 if enabled, read only */
>>> @@ -414,9 +426,9 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>          aml_append(field,
>>>              /* initiates device eject, write only */
>>>              aml_named_field(MEMORY_SLOT_EJECT, 1));
>>> -        aml_append(mem_ctrl_dev, field);
>>> +        aml_append(dev_container, field);
>>>
>>> -        field = aml_field(MEMORY_HOTPLUG_IO_REGION, AML_DWORD_ACC,
>>> +        field = aml_field(mmio_path, AML_DWORD_ACC,
>>>                            AML_NOLOCK, AML_PRESERVE);
>>>          aml_append(field, /* DIMM selector, write only */
>>>              aml_named_field(MEMORY_SLOT_SLECTOR, 32));
>>> @@ -424,7 +436,8 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>              aml_named_field(MEMORY_SLOT_OST_EVENT, 32));
>>>          aml_append(field, /* _OST status code, write only */
>>>              aml_named_field(MEMORY_SLOT_OST_STATUS, 32));
>>> -        aml_append(mem_ctrl_dev, field);
>>> +        aml_append(dev_container, field);
>>> +        g_free(mmio_path);
>>>
>>>          method = aml_method("_STA", 0, AML_NOTSERIALIZED);
>>>          ifctx = aml_if(aml_equal(slots_nr, zero));
>>> @@ -434,9 +447,9 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>          aml_append(method, ifctx);
>>>          /* present, functioning, decoding, not shown in UI */
>>>          aml_append(method, aml_return(aml_int(0xB)));
>>> -        aml_append(mem_ctrl_dev, method);
>>> +        aml_append(dev_container, method);
>>>
>>> -        aml_append(mem_ctrl_dev, aml_mutex(MEMORY_SLOT_LOCK, 0));
>>> +        aml_append(dev_container, aml_mutex(MEMORY_SLOT_LOCK, 0));
>>>
>>>          method = aml_method(MEMORY_SLOT_SCAN_METHOD, 0, AML_NOTSERIALIZED);
>>>          {
>>> @@ -492,7 +505,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>              aml_append(method, aml_release(ctrl_lock));
>>>              aml_append(method, aml_return(one));
>>>          }
>>> -        aml_append(mem_ctrl_dev, method);
>>> +        aml_append(dev_container, method);
>>>
>>>          method = aml_method(MEMORY_SLOT_STATUS_METHOD, 1, AML_NOTSERIALIZED);
>>>          {
>>> @@ -512,7 +525,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>              aml_append(method, aml_release(ctrl_lock));
>>>              aml_append(method, aml_return(ret_val));
>>>          }
>>> -        aml_append(mem_ctrl_dev, method);
>>> +        aml_append(dev_container, method);
>>>
>>>          method = aml_method(MEMORY_SLOT_CRS_METHOD, 1, AML_SERIALIZED);
>>>          {
>>> @@ -603,7 +616,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>              aml_append(method, aml_release(ctrl_lock));
>>>              aml_append(method, aml_return(mr64));
>>>          }
>>> -        aml_append(mem_ctrl_dev, method);
>>> +        aml_append(dev_container, method);
>>>
>>>          method = aml_method(MEMORY_SLOT_PROXIMITY_METHOD, 1,
>>>                              AML_NOTSERIALIZED);
>>> @@ -617,7 +630,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>              aml_append(method, aml_release(ctrl_lock));
>>>              aml_append(method, aml_return(ret_val));
>>>          }
>>> -        aml_append(mem_ctrl_dev, method);
>>> +        aml_append(dev_container, method);
>>>
>>>          method = aml_method(MEMORY_SLOT_OST_METHOD, 4, AML_NOTSERIALIZED);
>>>          {
>>> @@ -631,7 +644,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>              aml_append(method, aml_store(aml_arg(2), ost_status));
>>>              aml_append(method, aml_release(ctrl_lock));
>>>          }
>>> -        aml_append(mem_ctrl_dev, method);
>>> +        aml_append(dev_container, method);
>>>
>>>          method = aml_method(MEMORY_SLOT_EJECT_METHOD, 2, AML_NOTSERIALIZED);
>>>          {
>>> @@ -643,75 +656,66 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>>>              aml_append(method, aml_store(one, eject));
>>>              aml_append(method, aml_release(ctrl_lock));
>>>          }
>>> -        aml_append(mem_ctrl_dev, method);
>>> -    }
>>> -    aml_append(table, mem_ctrl_dev);
>>> -
>>> -    sb_scope = aml_scope("_SB");
>>> -    /* build memory devices */
>>> -    for (i = 0; i < nr_mem; i++) {
>>> -        Aml *dev;
>>> -        char *s;
>>> -
>>> -        dev = aml_device("MP%02X", i);
>>> -        aml_append(dev, aml_name_decl("_UID", aml_string("0x%02X", i)));
>>> -        aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C80")));
>>> -
>>> -        method = aml_method("_CRS", 0, AML_NOTSERIALIZED);
>>> -        s = g_strdup_printf("%s.%s", mhp_res_path, MEMORY_SLOT_CRS_METHOD);
>>> -        aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>>> -        g_free(s);
>>> -        aml_append(dev, method);
>>> -
>>> -        method = aml_method("_STA", 0, AML_NOTSERIALIZED);
>>> -        s = g_strdup_printf("%s.%s", mhp_res_path, MEMORY_SLOT_STATUS_METHOD);
>>> -        aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>>> -        g_free(s);
>>> -        aml_append(dev, method);
>>> -
>>> -        method = aml_method("_PXM", 0, AML_NOTSERIALIZED);
>>> -        s = g_strdup_printf("%s.%s", mhp_res_path,
>>> -                            MEMORY_SLOT_PROXIMITY_METHOD);
>>> -        aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>>> -        g_free(s);
>>> -        aml_append(dev, method);
>>> -
>>> -        method = aml_method("_OST", 3, AML_NOTSERIALIZED);
>>> -        s = g_strdup_printf("%s.%s", mhp_res_path, MEMORY_SLOT_OST_METHOD);
>>> -        aml_append(method, aml_return(aml_call4(
>>> -            s, aml_name("_UID"), aml_arg(0), aml_arg(1), aml_arg(2)
>>> -        )));
>>> -        g_free(s);
>>> -        aml_append(dev, method);
>>> -
>>> -        method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
>>> -        s = g_strdup_printf("%s.%s", mhp_res_path, MEMORY_SLOT_EJECT_METHOD);
>>> -        aml_append(method, aml_return(aml_call2(
>>> -                   s, aml_name("_UID"), aml_arg(0))));
>>> -        g_free(s);
>>> -        aml_append(dev, method);
>>> -
>>> -        aml_append(sb_scope, dev);
>>> -    }
>>> +        aml_append(dev_container, method);
>>> +
>>> +        /* build memory devices */
>>> +        for (i = 0; i < nr_mem; i++) {
>>> +            Aml *dev;
>>> +            const char *s;
>>> +
>>> +            dev = aml_device("MP%02X", i);
>>> +            aml_append(dev, aml_name_decl("_UID", aml_string("0x%02X", i)));
>>> +            aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C80")));
>>> +
>>> +            method = aml_method("_CRS", 0, AML_NOTSERIALIZED);
>>> +            s = MEMORY_SLOT_CRS_METHOD;
>>> +            aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>>> +            aml_append(dev, method);
>>> +
>>> +            method = aml_method("_STA", 0, AML_NOTSERIALIZED);
>>> +            s = MEMORY_SLOT_STATUS_METHOD;
>>> +            aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>>> +            aml_append(dev, method);
>>> +
>>> +            method = aml_method("_PXM", 0, AML_NOTSERIALIZED);
>>> +            s = MEMORY_SLOT_PROXIMITY_METHOD;
>>> +            aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>>> +            aml_append(dev, method);
>>> +
>>> +            method = aml_method("_OST", 3, AML_NOTSERIALIZED);
>>> +            s = MEMORY_SLOT_OST_METHOD;
>>> +            aml_append(method, aml_return(aml_call4(
>>> +                s, aml_name("_UID"), aml_arg(0), aml_arg(1), aml_arg(2)
>>> +            )));
>>> +            aml_append(dev, method);
>>> +
>>> +            method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
>>> +            s = MEMORY_SLOT_EJECT_METHOD;
>>> +            aml_append(method, aml_return(aml_call2(
>>> +                       s, aml_name("_UID"), aml_arg(0))));
>>> +            aml_append(dev, method);
>>> +
>>> +            aml_append(dev_container, dev);
>>> +        }
>>>
>>> -    /* build Method(MEMORY_SLOT_NOTIFY_METHOD, 2) {
>>> -     *     If (LEqual(Arg0, 0x00)) {Notify(MP00, Arg1)} ... }
>>> -     */
>>> -    method = aml_method(MEMORY_SLOT_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
>>> -    for (i = 0; i < nr_mem; i++) {
>>> -        ifctx = aml_if(aml_equal(aml_arg(0), aml_int(i)));
>>> -        aml_append(ifctx,
>>> -            aml_notify(aml_name("MP%.02X", i), aml_arg(1))
>>> -        );
>>> -        aml_append(method, ifctx);
>>> +        /* build Method(MEMORY_SLOT_NOTIFY_METHOD, 2) {
>>> +         *     If (LEqual(Arg0, 0x00)) {Notify(MP00, Arg1)} ... }
>>> +         */
>>> +        method = aml_method(MEMORY_SLOT_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
>>> +        for (i = 0; i < nr_mem; i++) {
>>> +            ifctx = aml_if(aml_equal(aml_arg(0), aml_int(i)));
>>> +            aml_append(ifctx,
>>> +                aml_notify(aml_name("MP%.02X", i), aml_arg(1))
>>> +            );
>>> +            aml_append(method, ifctx);
>>> +        }
>>> +        aml_append(dev_container, method);
>>>      }
>>> -    aml_append(sb_scope, method);
>>> -    aml_append(table, sb_scope);
>>> +    aml_append(table, dev_container);
>>>
>>>      method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
>>> -    scan_path = g_strdup_printf("%s.%s", mhp_res_path, MEMORY_SLOT_SCAN_METHOD);
>>> -    aml_append(method, aml_call0(scan_path));
>>> -    g_free(scan_path);
>>> +    aml_append(method,
>>> +        aml_call0(MEMORY_DEVICES_CONTAINER "." MEMORY_SLOT_SCAN_METHOD));
>>>      aml_append(table, method);
>>>
>>>      g_free(mhp_res_path);
>>>
>>
>