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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 8 Jan 2016 11:40:53 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> On 01/07/2016 07:04 PM, Igor Mammedov wrote:
> > On Wed, 6 Jan 2016 23:39:04 +0800
> > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:
> >  
> >> 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.  
> > perhaps I'm blind but I don't see bios_linker_loader_add_pointer() using
> > value stored in aml_int() in any way, see below.
> >  
> >>  
> >>>  
> >>>> +    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,  
> > here table_data->data + offset is a pointer to the first byte of aml_int() value.
> >
> > then bios_linker_loader_add_pointer(GArray *linker,
> >                                      const char *dest_file,
> >                                      const char *src_file,
> >                                      GArray *table, void *pointer,
> >                                      uint8_t pointer_size)
> > {
> >      ...
> >      size_t offset = (gchar *)pointer - table->data;
> >      ...
> >      entry.pointer.offset = cpu_to_le32(offset);
> >      ...
> > }
> >
> > 'pointer' argument that is passed to bios_linker_loader_add_pointer()
> > isn't dereferenced to access aml_int() value.  
> 
> The story is in BIOS handling， please refer the function, romfile_loader_add_pointer()
> in src/fw/romfile_loader.c of seabios:
> 
>      memcpy(&pointer, dest_file->data + offset, entry->pointer_size);
>      pointer = le64_to_cpu(pointer);
so 'pointer' holds offset from scr_file start,
that looks quite non-intuitive for user of bios_linker_loader_add_pointer() API,
I guess it came from the fact that initially linker API
was intended for usage with fixed tables.

I'd rather make bios_linker_loader_add_pointer() fixed and
make it not rely on contents of binary blobs it's supposed
to patch and pass the offset from scr_file start as part of
COMMAND_ADD_POINTER operation.
Or if it's hard to do so from compat POV with current impl,
write it in blob inside of bios_linker_loader_add_pointer()
and do not require creators of patched blobs to know
how linker works internally.

  bios_linker_loader_add_pointer(GArray *linker,
                                      const char *dest_file,
                                      const char *src_file,
                                      GArray *table,
+                                     uint64_t offset_in_src_file,
                                      void *pointer,
                                      uint8_t pointer_size)

>      pointer += (unsigned long)src_file->data;
that looks wrong src_file->data are going to truncated here if
addr is 64-bit.

>      pointer = cpu_to_le64(pointer);
>      memcpy(dest_file->data + offset, &pointer, entry->pointer_size);
> 
> >  
> >>>> +                                   sizeof(uint64_t));  
> > also it looks like there is bug somewhere in linker as it patches
> > only lower 32 bits of pointed value even though it has been told that
> > pointer size is 64bit.  
> 
> Do you mean that the BIOS allocated memory is always below 4G?
> Yes, it is true in current QEMU as it is using u32 to represent memory
> address, however i did not check the implementation in OVMF.
> 
> Can we assume that BIOS allocatedc address is always 32 bits in QEMU? I
> see this is no spec/comment guarantees this and this is completely
> depended on BIOS's implementation, so i made the QEMU be 64bit address
> aware.