| Message ID | 1451933528-133684-4-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > + 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. > build_header(linker, table_data, > (void *)(table_data->data + table_data->len - ssdt->buf->len), > "SSDT", ssdt->buf->len, revision, "NVDIMM"); > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > index ef44d02..b4726a4 100644 > --- a/include/hw/acpi/aml-build.h > +++ b/include/hw/acpi/aml-build.h > @@ -246,6 +246,7 @@ Aml *aml_name(const char *name_format, ...) GCC_FMT_ATTR(1, 2); > Aml *aml_name_decl(const char *name, Aml *val); > Aml *aml_return(Aml *val); > Aml *aml_int(const uint64_t val); > +Aml *aml_int64(const uint64_t val); > Aml *aml_arg(int pos); > Aml *aml_to_integer(Aml *arg); > Aml *aml_to_hexstring(Aml *src, Aml *dst); -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. + */ -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > >> + 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. > > 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. > + */ > > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); pointer += (unsigned long)src_file->data; 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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))); + 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)); build_header(linker, table_data, (void *)(table_data->data + table_data->len - ssdt->buf->len), "SSDT", ssdt->buf->len, revision, "NVDIMM"); diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h index ef44d02..b4726a4 100644 --- a/include/hw/acpi/aml-build.h +++ b/include/hw/acpi/aml-build.h @@ -246,6 +246,7 @@ Aml *aml_name(const char *name_format, ...) GCC_FMT_ATTR(1, 2); Aml *aml_name_decl(const char *name, Aml *val); Aml *aml_return(Aml *val); Aml *aml_int(const uint64_t val); +Aml *aml_int64(const uint64_t val); Aml *aml_arg(int pos); Aml *aml_to_integer(Aml *arg); Aml *aml_to_hexstring(Aml *src, Aml *dst);
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(-)