Re: [Qemu-devel] [PATCH 06/19] pc: acpi: move SSDT part of memhp into a custom table

[Laszlo Ersek <lersek@redhat.com>]

On 10/24/15 19:59, Michael S. Tsirkin wrote:
> On Fri, Oct 23, 2015 at 04:57:09PM +0200, Igor Mammedov wrote:
>> moves SSDT part to custom MHPT table, which is loaded
>> at runtime by OSPM if it supports ACPIv2 revision and
>> only if memory hotplug is enabled.
>> That should reduce ACPI tables blob size if memory
>> hotplug is not enabled (default case).
> 
> With seabios, it's still in reserved memory. How does it help?
> 
> And this trick likely breaks UEFI.

It does; see my response to the blurb.

In any case, I might send an email to the USWG list because I think
better cooperation is needed between the ACPI and UEFI specs for
supporting LoadTable and DataTableRegion. In particular,
EFI_ACPI_TABLE2_PROTOCOL.InstallTable() should take the EFI memory type
to install the table in. Luckily these two specs belong to the same
organization now.

Thanks
Laszlo

> I just checked, the only legal way to specify OEM
> specific tables seems to be using OEMX prefix.
> 
> But there's a decent chance this will conflict with
> people using command line flags to load their own
> tables.
> 
> This likely means this trick is off.
> 
> 
> 
>> Checked for compatibility issues with:
>>  * Windows XPsp3, Windows Server 2003: they don't load
>>    the table as OSPM only reports revision 1 as supported.
>>    And we don't care about memhp for these guests as they
>>    do not support it anyway.
>>  * Windows Server 2008, Windows Server 2008R2:
>>    works as expected
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>  hw/acpi/Makefile.objs               |   2 +-
>>  hw/acpi/memory_hotplug_acpi_table.c | 138 ++++++++++++++++++++++++++++++++++++
>>  hw/i386/acpi-build.c                | 130 +++++----------------------------
>>  include/hw/acpi/memory_hotplug.h    |   3 +
>>  4 files changed, 159 insertions(+), 114 deletions(-)
>>  create mode 100644 hw/acpi/memory_hotplug_acpi_table.c
>>
>> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
>> index 7d3230c..c04064e 100644
>> --- a/hw/acpi/Makefile.objs
>> +++ b/hw/acpi/Makefile.objs
>> @@ -1,7 +1,7 @@
>>  common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o
>>  common-obj-$(CONFIG_ACPI_X86_ICH) += ich9.o tco.o
>>  common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
>> -common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
>> +common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o memory_hotplug_acpi_table.o
>>  common-obj-$(CONFIG_ACPI) += acpi_interface.o
>>  common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
>>  common-obj-$(CONFIG_ACPI) += aml-build.o
>> diff --git a/hw/acpi/memory_hotplug_acpi_table.c b/hw/acpi/memory_hotplug_acpi_table.c
>> new file mode 100644
>> index 0000000..28da13c
>> --- /dev/null
>> +++ b/hw/acpi/memory_hotplug_acpi_table.c
>> @@ -0,0 +1,138 @@
>> +#include <stdbool.h>
>> +#include "hw/acpi/aml-build.h"
>> +#include "hw/acpi/memory_hotplug.h"
>> +#include "include/hw/acpi/pc-hotplug.h"
>> +#include "hw/boards.h"
>> +
>> +#define BASEPATH "\\_SB.PCI0." stringify(MEMORY_HOTPLUG_DEVICE) "."
>> +
>> +void build_mhpt(GArray *table_data, GArray *linker, uint32_t nr_mem,
>> +                uint16_t io_base, uint16_t io_len)
>> +{
>> +    int i;
>> +    Aml *table, *sb_scope, *dev, *method, *ifctx, *ctrl_dev;
>> +
>> +    table = init_aml_allocator();
>> +    acpi_data_push(table->buf, sizeof(AcpiTableHeader));
>> +
>> +    /* scope for memory hotplug controller device node */
>> +    assert(nr_mem <= ACPI_MAX_RAM_SLOTS);
>> +    ctrl_dev = aml_scope("\\_SB.PCI0." stringify(MEMORY_HOTPLUG_DEVICE));
>> +    {
>> +        Aml *crs, *field;
>> +
>> +        aml_append(ctrl_dev,
>> +            aml_name_decl(stringify(MEMORY_SLOTS_NUMBER), aml_int(nr_mem))
>> +        );
>> +
>> +        crs = aml_resource_template();
>> +        aml_append(crs, aml_io(AML_DECODE16, io_base, io_base, 0, io_len));
>> +        aml_append(ctrl_dev, aml_name_decl("_CRS", crs));
>> +
>> +        aml_append(ctrl_dev, aml_operation_region(
>> +            stringify(MEMORY_HOTPLUG_IO_REGION), AML_SYSTEM_IO,
>> +            io_base, io_len)
>> +        );
>> +
>> +        field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_DWORD_ACC,
>> +                          AML_PRESERVE);
>> +        aml_append(field, /* read only */
>> +            aml_named_field(stringify(MEMORY_SLOT_ADDR_LOW), 32));
>> +        aml_append(field, /* read only */
>> +            aml_named_field(stringify(MEMORY_SLOT_ADDR_HIGH), 32));
>> +        aml_append(field, /* read only */
>> +            aml_named_field(stringify(MEMORY_SLOT_SIZE_LOW), 32));
>> +        aml_append(field, /* read only */
>> +            aml_named_field(stringify(MEMORY_SLOT_SIZE_HIGH), 32));
>> +        aml_append(field, /* read only */
>> +            aml_named_field(stringify(MEMORY_SLOT_PROXIMITY), 32));
>> +        aml_append(ctrl_dev, field);
>> +
>> +        field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_BYTE_ACC,
>> +                          AML_WRITE_AS_ZEROS);
>> +        aml_append(field, aml_reserved_field(160 /* bits, Offset(20) */));
>> +        aml_append(field, /* 1 if enabled, read only */
>> +            aml_named_field(stringify(MEMORY_SLOT_ENABLED), 1));
>> +        aml_append(field,
>> +            /*(read) 1 if has a insert event. (write) 1 to clear event */
>> +            aml_named_field(stringify(MEMORY_SLOT_INSERT_EVENT), 1));
>> +        aml_append(field,
>> +            /* (read) 1 if has a remove event. (write) 1 to clear event */
>> +            aml_named_field(stringify(MEMORY_SLOT_REMOVE_EVENT), 1));
>> +        aml_append(field,
>> +            /* initiates device eject, write only */
>> +            aml_named_field(stringify(MEMORY_SLOT_EJECT), 1));
>> +        aml_append(ctrl_dev, field);
>> +
>> +        field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_DWORD_ACC,
>> +                          AML_PRESERVE);
>> +        aml_append(field, /* DIMM selector, write only */
>> +            aml_named_field(stringify(MEMORY_SLOT_SLECTOR), 32));
>> +        aml_append(field, /* _OST event code, write only */
>> +            aml_named_field(stringify(MEMORY_SLOT_OST_EVENT), 32));
>> +        aml_append(field, /* _OST status code, write only */
>> +            aml_named_field(stringify(MEMORY_SLOT_OST_STATUS), 32));
>> +        aml_append(ctrl_dev, field);
>> +    }
>> +    aml_append(table, ctrl_dev);
>> +
>> +    sb_scope = aml_scope("\\_SB");
>> +    for (i = 0; i < nr_mem; i++) {
>> +        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);
>> +        s = BASEPATH stringify(MEMORY_SLOT_CRS_METHOD);
>> +        aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>> +        aml_append(dev, method);
>> +
>> +        method = aml_method("_STA", 0);
>> +        s = BASEPATH stringify(MEMORY_SLOT_STATUS_METHOD);
>> +        aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>> +        aml_append(dev, method);
>> +
>> +        method = aml_method("_PXM", 0);
>> +        s = BASEPATH stringify(MEMORY_SLOT_PROXIMITY_METHOD);
>> +        aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>> +        aml_append(dev, method);
>> +
>> +        method = aml_method("_OST", 3);
>> +        s = BASEPATH stringify(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);
>> +        s = BASEPATH stringify(MEMORY_SLOT_EJECT_METHOD);
>> +        aml_append(method, aml_return(aml_call2(
>> +                   s, aml_name("_UID"), aml_arg(0))));
>> +        aml_append(dev, method);
>> +
>> +        aml_append(sb_scope, dev);
>> +    }
>> +
>> +    /* build Method(MEMORY_SLOT_NOTIFY_METHOD, 2) {
>> +     *     If (LEqual(Arg0, 0x00)) {Notify(MP00, Arg1)} ... }
>> +     */
>> +    method = aml_method(stringify(MEMORY_SLOT_NOTIFY_METHOD), 2);
>> +    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(sb_scope, method);
>> +    aml_append(table, sb_scope);
>> +
>> +    /* copy AML table into ACPI tables blob and patch header there */
>> +    g_array_append_vals(table_data, table->buf->data, table->buf->len);
>> +    build_header(linker, table_data,
>> +        (void *)(table_data->data + table_data->len - table->buf->len),
>> +        "MHPT", table->buf->len, 2);
>> +    free_aml_allocator();
>> +}
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 95e0c65..8add4d9 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -925,6 +925,16 @@ build_ssdt(GArray *table_data, GArray *linker,
>>      /* Reserve space for header */
>>      acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
>>  
>> +    sb_scope = aml_scope("\\_SB");
>> +    method = aml_method("_INI", 0);
>> +    ifctx = aml_if(aml_lgreater_equal(aml_name("_REV"), aml_int(2)));
>> +    if (nr_mem) {
>> +        aml_append(ifctx, aml_load_table("MHPT"));
>> +    }
>> +    aml_append(method, ifctx);
>> +    aml_append(sb_scope, method);
>> +    aml_append(ssdt, sb_scope);
>> +
>>      /* Extra PCI root buses are implemented  only for i440fx */
>>      bus = find_i440fx();
>>      if (bus) {
>> @@ -1200,119 +1210,6 @@ build_ssdt(GArray *table_data, GArray *linker,
>>          }
>>          aml_append(sb_scope, aml_name_decl("CPON", pkg));
>>  
>> -        /* build memory devices */
>> -        assert(nr_mem <= ACPI_MAX_RAM_SLOTS);
>> -        scope = aml_scope("\\_SB.PCI0." stringify(MEMORY_HOTPLUG_DEVICE));
>> -        aml_append(scope,
>> -            aml_name_decl(stringify(MEMORY_SLOTS_NUMBER), aml_int(nr_mem))
>> -        );
>> -
>> -        crs = aml_resource_template();
>> -        aml_append(crs,
>> -            aml_io(AML_DECODE16, pm->mem_hp_io_base, pm->mem_hp_io_base, 0,
>> -                   pm->mem_hp_io_len)
>> -        );
>> -        aml_append(scope, aml_name_decl("_CRS", crs));
>> -
>> -        aml_append(scope, aml_operation_region(
>> -            stringify(MEMORY_HOTPLUG_IO_REGION), AML_SYSTEM_IO,
>> -            pm->mem_hp_io_base, pm->mem_hp_io_len)
>> -        );
>> -
>> -        field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_DWORD_ACC,
>> -                          AML_PRESERVE);
>> -        aml_append(field, /* read only */
>> -            aml_named_field(stringify(MEMORY_SLOT_ADDR_LOW), 32));
>> -        aml_append(field, /* read only */
>> -            aml_named_field(stringify(MEMORY_SLOT_ADDR_HIGH), 32));
>> -        aml_append(field, /* read only */
>> -            aml_named_field(stringify(MEMORY_SLOT_SIZE_LOW), 32));
>> -        aml_append(field, /* read only */
>> -            aml_named_field(stringify(MEMORY_SLOT_SIZE_HIGH), 32));
>> -        aml_append(field, /* read only */
>> -            aml_named_field(stringify(MEMORY_SLOT_PROXIMITY), 32));
>> -        aml_append(scope, field);
>> -
>> -        field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_BYTE_ACC,
>> -                          AML_WRITE_AS_ZEROS);
>> -        aml_append(field, aml_reserved_field(160 /* bits, Offset(20) */));
>> -        aml_append(field, /* 1 if enabled, read only */
>> -            aml_named_field(stringify(MEMORY_SLOT_ENABLED), 1));
>> -        aml_append(field,
>> -            /*(read) 1 if has a insert event. (write) 1 to clear event */
>> -            aml_named_field(stringify(MEMORY_SLOT_INSERT_EVENT), 1));
>> -        aml_append(field,
>> -            /* (read) 1 if has a remove event. (write) 1 to clear event */
>> -            aml_named_field(stringify(MEMORY_SLOT_REMOVE_EVENT), 1));
>> -        aml_append(field,
>> -            /* initiates device eject, write only */
>> -            aml_named_field(stringify(MEMORY_SLOT_EJECT), 1));
>> -        aml_append(scope, field);
>> -
>> -        field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_DWORD_ACC,
>> -                          AML_PRESERVE);
>> -        aml_append(field, /* DIMM selector, write only */
>> -            aml_named_field(stringify(MEMORY_SLOT_SLECTOR), 32));
>> -        aml_append(field, /* _OST event code, write only */
>> -            aml_named_field(stringify(MEMORY_SLOT_OST_EVENT), 32));
>> -        aml_append(field, /* _OST status code, write only */
>> -            aml_named_field(stringify(MEMORY_SLOT_OST_STATUS), 32));
>> -        aml_append(scope, field);
>> -
>> -        aml_append(sb_scope, scope);
>> -
>> -        for (i = 0; i < nr_mem; i++) {
>> -            #define BASEPATH "\\_SB.PCI0." stringify(MEMORY_HOTPLUG_DEVICE) "."
>> -            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);
>> -            s = BASEPATH stringify(MEMORY_SLOT_CRS_METHOD);
>> -            aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>> -            aml_append(dev, method);
>> -
>> -            method = aml_method("_STA", 0);
>> -            s = BASEPATH stringify(MEMORY_SLOT_STATUS_METHOD);
>> -            aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>> -            aml_append(dev, method);
>> -
>> -            method = aml_method("_PXM", 0);
>> -            s = BASEPATH stringify(MEMORY_SLOT_PROXIMITY_METHOD);
>> -            aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
>> -            aml_append(dev, method);
>> -
>> -            method = aml_method("_OST", 3);
>> -            s = BASEPATH stringify(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);
>> -            s = BASEPATH stringify(MEMORY_SLOT_EJECT_METHOD);
>> -            aml_append(method, aml_return(aml_call2(
>> -                       s, aml_name("_UID"), aml_arg(0))));
>> -            aml_append(dev, method);
>> -
>> -            aml_append(sb_scope, dev);
>> -        }
>> -
>> -        /* build Method(MEMORY_SLOT_NOTIFY_METHOD, 2) {
>> -         *     If (LEqual(Arg0, 0x00)) {Notify(MP00, Arg1)} ... }
>> -         */
>> -        method = aml_method(stringify(MEMORY_SLOT_NOTIFY_METHOD), 2);
>> -        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(sb_scope, method);
>> -
>>          {
>>              Object *pci_host;
>>              PCIBus *bus = NULL;
>> @@ -1671,6 +1568,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>>      uint8_t *u;
>>      size_t aml_len = 0;
>>      GArray *tables_blob = tables->table_data;
>> +    MachineState *machine = MACHINE(qdev_get_machine());
>>  
>>      acpi_get_cpu_info(&cpu);
>>      acpi_get_pm_info(&pm);
>> @@ -1742,6 +1640,12 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>>          build_dmar_q35(tables_blob, tables->linker);
>>      }
>>  
>> +    if (machine->ram_slots) {
>> +        acpi_add_table(table_offsets, tables_blob);
>> +        build_mhpt(tables_blob, tables->linker, machine->ram_slots,
>> +                   pm.mem_hp_io_base, pm.mem_hp_io_len);
>> +    }
>> +
>>      /* Add tables supplied by user (if any) */
>>      for (u = acpi_table_first(); u; u = acpi_table_next(u)) {
>>          unsigned len = acpi_table_len(u);
>> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
>> index 1342adb..5fd2854 100644
>> --- a/include/hw/acpi/memory_hotplug.h
>> +++ b/include/hw/acpi/memory_hotplug.h
>> @@ -45,4 +45,7 @@ extern const VMStateDescription vmstate_memory_hotplug;
>>                     vmstate_memory_hotplug, MemHotplugState)
>>  
>>  void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list);
>> +
>> +void build_mhpt(GArray *table_data, GArray *linker, uint32_t nr_mem,
>> +                uint16_t io_base, uint16_t io_len);
>>  #endif
>> -- 
>> 1.8.3.1