Re: [Qemu-devel] [PATCH for-2.9 07/10] memhp: move GPE handler_E03 into build_memory_hotplug_aml()

[Marcel Apfelbaum <marcel@redhat.com>]

On 12/06/2016 01:32 AM, Igor Mammedov wrote:
> From this patch all the memory hotplug related AML
> bits are consolidated in one place within DSTD.
> Follow up patches will utilize that to simplify
> memory hotplug related C/AML code.


I didn't quite understand what you mean...

>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/acpi/memory_hotplug.h |  6 +++---
>  hw/acpi/memory_hotplug.c         | 42 ++++++++++++++++++++++++++--------------
>  hw/i386/acpi-build.c             |  7 ++-----
>  3 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index 6dc48d2..37e2706 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -49,9 +49,9 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list);
>
>  #define MEMORY_HOTPLUG_DEVICE        "MHPD"
>  #define MEMORY_SLOT_SCAN_METHOD      "MSCN"
> -#define MEMORY_HOTPLUG_HANDLER_PATH "\\_SB.PCI0." \
> -     MEMORY_HOTPLUG_DEVICE "." MEMORY_SLOT_SCAN_METHOD
>
>  void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> -                              uint16_t io_base, uint16_t io_len);
> +                              uint16_t io_base, uint16_t io_len,
> +                              const char *res_root,
> +                              const char *event_handler_method);
>  #endif
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 18b95f2..49e856f 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -308,18 +308,19 @@ const VMStateDescription vmstate_memory_hotplug = {
>  };
>
>  void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> -                              uint16_t io_base, uint16_t io_len)
> +                              uint16_t io_base, uint16_t io_len,
> +                              const char *res_root,
> +                              const char *event_handler_method)
>  {
>      int i;
>      Aml *ifctx;
>      Aml *method;
> -    Aml *pci_scope;
>      Aml *sb_scope;
>      Aml *mem_ctrl_dev;
> +    char *scan_path;
> +    char *mhp_res_path = g_strdup_printf("%s." MEMORY_HOTPLUG_DEVICE, res_root);
>
> -    /* scope for memory hotplug controller device node */
> -    pci_scope = aml_scope("_SB.PCI0");
> -    mem_ctrl_dev = aml_device(MEMORY_HOTPLUG_DEVICE);
> +    mem_ctrl_dev = aml_device("%s", mhp_res_path);


Will mem_ctrl_dev's name will have an extra "." suffix now?

>      {
>          Aml *crs;
>          Aml *field;
> @@ -610,47 +611,50 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>          }
>          aml_append(mem_ctrl_dev, method);
>      }
> -    aml_append(pci_scope, mem_ctrl_dev);
> -    aml_append(table, pci_scope);
> +    aml_append(table, mem_ctrl_dev);
>
>      sb_scope = aml_scope("_SB");
>      /* build memory devices */
>      for (i = 0; i < nr_mem; i++) {
> -        #define BASEPATH "\\_SB.PCI0." MEMORY_HOTPLUG_DEVICE "."
>          Aml *dev;
> -        const char *s;
> +        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 = BASEPATH MEMORY_SLOT_CRS_METHOD;
> +        s = g_strdup_printf("%s.%s", mhp_res_path, MEMORY_SLOT_CRS_METHOD);

Same question here, do we get ".." in the path? Maybe is OK?


Thanks,
Marcel

>          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 = BASEPATH MEMORY_SLOT_STATUS_METHOD;
> +        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 = BASEPATH MEMORY_SLOT_PROXIMITY_METHOD;
> +        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 = BASEPATH MEMORY_SLOT_OST_METHOD;
> -
> +        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 = BASEPATH MEMORY_SLOT_EJECT_METHOD;
> +        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);
> @@ -669,4 +673,12 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
>      }
>      aml_append(sb_scope, method);
>      aml_append(table, sb_scope);
> +
> +    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(table, method);
> +
> +    g_free(mhp_res_path);
>  }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 690e9a0..b7f4682 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1926,7 +1926,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>                         "\\_SB.PCI0", "\\_GPE._E02");
>      }
>      build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base,
> -                             pm->mem_hp_io_len);
> +                             pm->mem_hp_io_len,
> +                             "\\_SB.PCI0", "\\_GPE._E03");
>
>      scope =  aml_scope("_GPE");
>      {
> @@ -1941,10 +1942,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              aml_append(scope, method);
>          }
>
> -        method = aml_method("_E03", 0, AML_NOTSERIALIZED);
> -        aml_append(method, aml_call0(MEMORY_HOTPLUG_HANDLER_PATH));
> -        aml_append(scope, method);
> -
>          if (pcms->acpi_nvdimm_state.is_enabled) {
>              method = aml_method("_E04", 0, AML_NOTSERIALIZED);
>              aml_append(method, aml_notify(aml_name("\\_SB.NVDR"),
>