Re: [PATCH 3/6] nvdimm acpi: introduce patched dsm memory

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

On 01/06/2016 11:23 PM, Igor Mammedov wrote:
> On Tue,  5 Jan 2016 02:52:05 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> 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
>> int64 object returned by "MEMA" method
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>>   hw/acpi/aml-build.c         | 12 ++++++++++++
>>   hw/acpi/nvdimm.c            | 24 ++++++++++++++++++++++--
>>   include/hw/acpi/aml-build.h |  1 +
>>   3 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 78e1290..83eadb3 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -394,6 +394,18 @@ Aml *aml_int(const uint64_t val)
>>   }
>>
>>   /*
>> + * ACPI 1.0b: 16.2.3 Data Objects Encoding:
>> + * encode: QWordConst
>> + */
>> +Aml *aml_int64(const uint64_t val)
>> +{
>> +    Aml *var = aml_alloc();
>> +    build_append_byte(var->buf, 0x0E); /* QWordPrefix */
>> +    build_append_int_noprefix(var->buf, val, 8);
>> +    return var;
>> +}
>> +
>> +/*
>>    * helper to construct NameString, which returns Aml object
>>    * for using with aml_append or other aml_* terms
>>    */
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index bc7cd8f..a72104c 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -28,6 +28,7 @@
>>
>>   #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"
>>
>> @@ -402,7 +403,8 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
>>                       state->dsm_mem->len);
>>   }
>>
>> -#define NVDIMM_COMMON_DSM      "NCAL"
>> +#define NVDIMM_GET_DSM_MEM      "MEMA"
>> +#define NVDIMM_COMMON_DSM       "NCAL"
>>
>>   static void nvdimm_build_common_dsm(Aml *dev)
>>   {
>> @@ -468,7 +470,8 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>>                                 GArray *table_data, GArray *linker,
>>                                 uint8_t revision)
>>   {
>> -    Aml *ssdt, *sb_scope, *dev;
>> +    Aml *ssdt, *sb_scope, *dev, *method;
>> +    int offset;
>>
>>       acpi_add_table(table_offsets, table_data);
>>
>> @@ -499,9 +502,26 @@ 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 int64 object returned by the function which will be
>> +     * patched with the real address of the dsm memory by BIOS.
>> +     */
>> +    method = aml_method(NVDIMM_GET_DSM_MEM, 0, AML_NOTSERIALIZED);
>> +    aml_append(method, aml_return(aml_int64(0x0)));
> there is no need in dedicated aml_int64(), you can use aml_int(0x6400000000) trick

We can not do this due to the trick in  bios_linker_loader_add_pointer() which will
issue a COMMAND_ADD_POINTER to BIOS, however, this request does:
         /*
          * COMMAND_ADD_POINTER - patch the table (originating from
          * @dest_file) at @pointer.offset, by adding a pointer to the table
          * originating from @src_file. 1,2,4 or 8 byte unsigned
          * addition is used depending on @pointer.size.
          */

that means the new-offset = old-offset + the address of the new table allocated by BIOS.

So we expect 0 offset here.

>
>> +    aml_append(sb_scope, method);
>>       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 - 8;
>> +
>> +    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(uint64_t));
> this offset magic will break badly as soon as someone add something
> to the end of SSDT.
>

Yes, it is, so don't do that, :) and this is why we made the comment here:
  +    /*
  +     * leave it at the end of ssdt so that we can conveniently get the
  +     * offset of int64 object returned by the function which will be
  +     * patched with the real address of the dsm memory by BIOS.
  +     */