On 02/29/2016 05:38 PM, Michael S. Tsirkin wrote:
> On Sun, Feb 14, 2016 at 04:51:02PM +0800, Xiao Guangrong wrote:
>> The dsm memory is used to save the input parameters and store
>> the dsm result which is filled by QEMU.
>>
>> The address of dsm memory is decided by bios and patched into
>> int32 object named "MEMA"
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>
>
> This is a bit too hacky for my taste. First, I would prefer an explicit API
> to add a DWORD. Second, I would like to avoid offset math hacks
> and make API returning offsets.
>
> Pls see below for a suggestion.
>
>
>> ---
>>   hw/acpi/nvdimm.c | 34 +++++++++++++++++++++++++++++++++-
>>   1 file changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 8568b20..bca36ae 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -29,6 +29,7 @@
>>   #include "qemu/osdep.h"
>>   #include "hw/acpi/acpi.h"
>>   #include "hw/acpi/aml-build.h"
>> +#include "hw/acpi/bios-linker-loader.h"
>>   #include "hw/nvram/fw_cfg.h"
>>   #include "hw/mem/nvdimm.h"
>>
>> @@ -406,6 +407,7 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>>   }
>>
>>   #define NVDIMM_COMMON_DSM      "NCAL"
>> +#define NVDIMM_ACPI_MEM_ADDR   "MEMA"
>>
>>   static void nvdimm_build_common_dsm(Aml *dev)
>>   {
>> @@ -470,7 +472,9 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
>>   static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>>                                 GArray *table_data, GArray *linker)
>>   {
>> -    Aml *ssdt, *sb_scope, *dev;
>> +    Aml *ssdt, *sb_scope, *dev, *mem_addr;
>> +    uint32_t zero_offset = 0;
>> +    int offset;
>>
>>       acpi_add_table(table_offsets, table_data);
>>
>> @@ -501,9 +505,37 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>>
>>       aml_append(sb_scope, dev);
>>
>> +    /*
>> +     * leave it at the end of ssdt so that we can conveniently get the
>> +     * offset of int32 object which will be patched with the real address
>> +     * of the dsm memory by BIOS.
>> +     *
>> +     * 0x32000000 is the magic number to let aml_int() create int32 object.
>> +     * It will be zeroed later to make bios_linker_loader_add_pointer()
>> +     * happy.
>> +     */
>> +    mem_addr = aml_name_decl(NVDIMM_ACPI_MEM_ADDR, aml_int(0x32000000));
>> +
>> +    aml_append(sb_scope, mem_addr);
>>       aml_append(ssdt, sb_scope);
>>       /* copy AML table into ACPI tables blob and patch header there */
>>       g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
>> +
>> +    offset = table_data->len - 4;
>> +
>> +    /*
>> +     * zero the last 4 bytes, i.e, it is the offset of
>> +     * NVDIMM_ACPI_MEM_ADDR object.
>> +     */
>> +    g_array_remove_range(table_data, offset, 4);
>> +    g_array_append_vals(table_data, &zero_offset, 4);
>> +
>> +    bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
>> +                             false /* high memory */);
>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>> +                                   NVDIMM_DSM_MEM_FILE, table_data,
>> +                                   table_data->data + offset,
>> +                                   sizeof(uint32_t));
>>       build_header(linker, table_data,
>>           (void *)(table_data->data + table_data->len - ssdt->buf->len),
>>           "SSDT", ssdt->buf->len, 1, NULL, "NVDIMM");
>> --
>> 1.8.3.1
>
>
>
> --->
>
> acpi: add build_append_named_dword, returning an offset in buffer
>
> This is a very limited form of support for runtime patching -
> similar in functionality to what we can do with ACPI_EXTRACT
> macros in python, but implemented in C.
>
> This is to allow ACPI code direct access to data tables -
> which is exactly what DataTableRegion is there for, except
> no known windows release so far implements DataTableRegion.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 1b632dc..f8998ea 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -286,4 +286,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre);
>   void
>   build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets);
>
> +int
> +build_append_named_dword(GArray *array, const char *name_format, ...);
> +
>   #endif
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 0d4b324..7f9fa65 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -262,6 +262,32 @@ static void build_append_int(GArray *table, uint64_t value)
>       }
>   }
>
> +/* Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a dword,
> + * and return the offset to 0x00000000 for runtime patching.
> + *
> + * Warning: runtime patching is best avoided. Only use this as
> + * a replacement for DataTableRegion (for guests that don't
> + * support it).
> + */
> +int
> +build_append_named_dword(GArray *array, const char *name_format, ...)
> +{
> +    int offset;
> +    va_list ap;
> +
> +    va_start(ap, name_format);
> +    build_append_namestringv(array, name_format, ap);
> +    va_end(ap);
> +

The NameOP was missed here...

The idea is great and i fixed and applied it on the top this patchset, the patch
is attached, would it be good to you?