From mboxrd@z Thu Jan 1 00:00:00 1970 From: Igor Mammedov Subject: Re: [PATCH 3/6] nvdimm acpi: introduce patched dsm memory Date: Fri, 8 Jan 2016 18:06:37 +0100 Message-ID: <20160108180637.2e7bf64c@nial.brq.redhat.com> References: <1451933528-133684-1-git-send-email-guangrong.xiao@linux.intel.com> <1451933528-133684-4-git-send-email-guangrong.xiao@linux.intel.com> <20160106162301.735e6517@nial.brq.redhat.com> <568D3518.2000402@linux.intel.com> <20160107120437.6a231688@nial.brq.redhat.com> <568F2FC5.5010700@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: pbonzini@redhat.com, gleb@kernel.org, mtosatti@redhat.com, stefanha@redhat.com, mst@redhat.com, rth@twiddle.net, ehabkost@redhat.com, dan.j.williams@intel.com, kvm@vger.kernel.org, qemu-devel@nongnu.org To: Xiao Guangrong Return-path: Received: from mx1.redhat.com ([209.132.183.28]:54744 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754368AbcAHRGm convert rfc822-to-8bit (ORCPT ); Fri, 8 Jan 2016 12:06:42 -0500 In-Reply-To: <568F2FC5.5010700@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, 8 Jan 2016 11:40:53 +0800 Xiao Guangrong wrote: > On 01/07/2016 07:04 PM, Igor Mammedov wrote: > > On Wed, 6 Jan 2016 23:39:04 +0800 > > Xiao Guangrong wrote: > > =20 > >> On 01/06/2016 11:23 PM, Igor Mammedov wrote: =20 > >>> On Tue, 5 Jan 2016 02:52:05 +0800 > >>> Xiao Guangrong wrote: > >>> =20 > >>>> 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 > >>>> --- > >>>> 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 =3D 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 *lin= ker, > >>>> 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 *devic= e_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 wi= ll be > >>>> + * patched with the real address of the dsm memory by BIOS. > >>>> + */ > >>>> + method =3D aml_method(NVDIMM_GET_DSM_MEM, 0, AML_NOTSERIALI= ZED); > >>>> + aml_append(method, aml_return(aml_int64(0x0))); =20 > >>> there is no need in dedicated aml_int64(), you can use aml_int(0x= 6400000000) trick =20 > >> > >> We can not do this due to the trick in bios_linker_loader_add_poi= nter() which will > >> issue a COMMAND_ADD_POINTER to BIOS, however, this request does: > >> /* > >> * COMMAND_ADD_POINTER - patch the table (originating fr= om > >> * @dest_file) at @pointer.offset, by adding a pointer t= o 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 =3D old-offset + the address of the new = table allocated by BIOS. > >> > >> So we expect 0 offset here. =20 > > 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. > > =20 > >> =20 > >>> =20 > >>>> + 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->bu= f->len); > >>>> + > >>>> + offset =3D table_data->len - 8; > >>>> + > >>>> + bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGE= T_PAGE_SIZE, > >>>> + false /* high memory */); > >>>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FIL= E, > >>>> + NVDIMM_DSM_MEM_FILE, table_d= ata, > >>>> + table_data->data + offset, =20 > > here table_data->data + offset is a pointer to the first byte of am= l_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 =3D (gchar *)pointer - table->data; > > ... > > entry.pointer.offset =3D cpu_to_le32(offset); > > ... > > } > > > > 'pointer' argument that is passed to bios_linker_loader_add_pointer= () > > isn't dereferenced to access aml_int() value. =20 >=20 > The story is in BIOS handling=EF=BC=8C please refer the function, rom= file_loader_add_pointer() > in src/fw/romfile_loader.c of seabios: >=20 > memcpy(&pointer, dest_file->data + offset, entry->pointer_size); > pointer =3D le64_to_cpu(pointer); so 'pointer' holds offset from scr_file start, that looks quite non-intuitive for user of bios_linker_loader_add_point= er() 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 +=3D (unsigned long)src_file->data; that looks wrong src_file->data are going to truncated here if addr is 64-bit. > pointer =3D cpu_to_le64(pointer); > memcpy(dest_file->data + offset, &pointer, entry->pointer_size); >=20 > > =20 > >>>> + sizeof(uint64_t)); =20 > > 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 th= at > > pointer size is 64bit. =20 >=20 > 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 memor= y > address, however i did not check the implementation in OVMF. >=20 > 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 addres= s > aware. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47196) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHaUx-0002Lp-Az for qemu-devel@nongnu.org; Fri, 08 Jan 2016 12:06:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHaUt-0004TK-17 for qemu-devel@nongnu.org; Fri, 08 Jan 2016 12:06:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47507) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHaUs-0004T7-PP for qemu-devel@nongnu.org; Fri, 08 Jan 2016 12:06:42 -0500 Date: Fri, 8 Jan 2016 18:06:37 +0100 From: Igor Mammedov Message-ID: <20160108180637.2e7bf64c@nial.brq.redhat.com> In-Reply-To: <568F2FC5.5010700@linux.intel.com> References: <1451933528-133684-1-git-send-email-guangrong.xiao@linux.intel.com> <1451933528-133684-4-git-send-email-guangrong.xiao@linux.intel.com> <20160106162301.735e6517@nial.brq.redhat.com> <568D3518.2000402@linux.intel.com> <20160107120437.6a231688@nial.brq.redhat.com> <568F2FC5.5010700@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 3/6] nvdimm acpi: introduce patched dsm memory List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong Cc: ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On Fri, 8 Jan 2016 11:40:53 +0800 Xiao Guangrong wrote: > On 01/07/2016 07:04 PM, Igor Mammedov wrote: > > On Wed, 6 Jan 2016 23:39:04 +0800 > > Xiao Guangrong wrote: > > =20 > >> On 01/06/2016 11:23 PM, Igor Mammedov wrote: =20 > >>> On Tue, 5 Jan 2016 02:52:05 +0800 > >>> Xiao Guangrong wrote: > >>> =20 > >>>> 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 > >>>> --- > >>>> 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 =3D 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 *sta= te, 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_lis= t, 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_li= st, 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 =3D aml_method(NVDIMM_GET_DSM_MEM, 0, AML_NOTSERIALIZED); > >>>> + aml_append(method, aml_return(aml_int64(0x0))); =20 > >>> there is no need in dedicated aml_int64(), you can use aml_int(0x6400= 000000) trick =20 > >> > >> 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 th= e table > >> * originating from @src_file. 1,2,4 or 8 byte unsigned > >> * addition is used depending on @pointer.size. > >> */ > >> > >> that means the new-offset =3D old-offset + the address of the new tabl= e allocated by BIOS. > >> > >> So we expect 0 offset here. =20 > > 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. > > =20 > >> =20 > >>> =20 > >>>> + aml_append(sb_scope, method); > >>>> aml_append(ssdt, sb_scope); > >>>> /* copy AML table into ACPI tables blob and patch header ther= e */ > >>>> g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->l= en); > >>>> + > >>>> + offset =3D table_data->len - 8; > >>>> + > >>>> + bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PA= GE_SIZE, > >>>> + false /* high memory */); > >>>> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE, > >>>> + NVDIMM_DSM_MEM_FILE, table_data, > >>>> + table_data->data + offset, =20 > > here table_data->data + offset is a pointer to the first byte of aml_in= t() 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 =3D (gchar *)pointer - table->data; > > ... > > entry.pointer.offset =3D cpu_to_le32(offset); > > ... > > } > > > > 'pointer' argument that is passed to bios_linker_loader_add_pointer() > > isn't dereferenced to access aml_int() value. =20 >=20 > The story is in BIOS handling=EF=BC=8C please refer the function, romfile= _loader_add_pointer() > in src/fw/romfile_loader.c of seabios: >=20 > memcpy(&pointer, dest_file->data + offset, entry->pointer_size); > pointer =3D 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 +=3D (unsigned long)src_file->data; that looks wrong src_file->data are going to truncated here if addr is 64-bit. > pointer =3D cpu_to_le64(pointer); > memcpy(dest_file->data + offset, &pointer, entry->pointer_size); >=20 > > =20 > >>>> + sizeof(uint64_t)); =20 > > 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. =20 >=20 > 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. >=20 > 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.