On Fri, 8 Jan 2016 11:40:53 +0800
Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> wrote:
> On 01/07/2016 07:04 PM, Igor Mammedov wrote:
> > On Wed, 6 Jan 2016 23:39:04 +0800
> > Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> wrote:
> >
> >> On 01/06/2016 11:23 PM, Igor Mammedov wrote:
> >>> On Tue, 5 Jan 2016 02:52:05 +0800
> >>> Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >>>> ---
> >>>> 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
