Re: [Qemu-devel] [RFC PATCH 04/11] hw/arm/virt-acpi-build: Generate XSDT table and add a build_header function
On 2015/1/25 6:04, Laszlo Ersek wrote:
> comments below, fix attached
>
> On 01/24/15 10:21, Shannon Zhao wrote:
>> XDST points to other tables except FACS & DSDT.
>> Implement a common header helper functions for generating ACPI tables.
>>
>> Signed-off-by: Shannon Zhao
>> ---
>> hw/arm/virt-acpi-build.c| 34 ++
>> include/hw/acpi/acpi-defs.h |9 +
>> 2 files changed, 43 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 9c3971a..446947a 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -61,6 +61,22 @@
>> #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"
>> #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log"
>>
>> +static void
>> +build_header(GArray *linker, GArray *table_data,
>> + AcpiTableHeader *h, const char *sig, int len, uint8_t rev)
>> +{
>> +memcpy(&h->signature, sig, sizeof(h->signature));
>> +h->length = cpu_to_le32(len);
>> +h->revision = rev;
>> +memcpy(h->oem_id, ACPI_VIRT_QEMU_STR_6, sizeof(h->oem_id));
>> +memcpy(h->oem_table_id, ACPI_VIRT_MACH_STR_8, sizeof(h->oem_table_id));
>> +h->oem_revision = cpu_to_le32(1);
>> +h->checksum = 0;
>> +/* Checksum to be filled in by Guest linker */
>> +bios_linker_loader_add_checksum(linker, ACPI_BUILD_TABLE_FILE,
>> +table_data->data, h, len, &h->checksum);
>> +}
>> +
>> static inline void *acpi_data_push(GArray *table_data, uint64_t size)
>> {
>> unsigned off = table_data->len;
>> @@ -115,6 +131,24 @@ build_rsdp(GArray *rsdp_table, GArray *linker, uint64_t
>> xsdt)
>> static void
>> build_xsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
>> {
>> +AcpiXsdtDescriptor *xsdt;
>> +size_t xsdt_len;
>> +int i;
>> +
>> +xsdt_len = sizeof(*xsdt) + sizeof(uint64_t) * table_offsets->len;
>> +xsdt = acpi_data_push(table_data, xsdt_len);
>> +memcpy(xsdt->table_offset_entry, table_offsets->data,
>> + sizeof(uint64_t) * table_offsets->len);
>
> This is a bug, but it's not introduced here. The bug is introduced in:
>
> [RFC PATCH 02/11] hw/arm/virt-acpi-build: Basic framework for
> building ACPI tables
>
> I'm attaching the fix.
>
Greate, thanks. Will fix this at next version.
> Please do not squash the fix into this patch; you have to split it up.
> The code fixes go into 02/11, and the typo fix goes:
>
>> +for (i = 0; i < table_offsets->len; ++i) {
>> +/* rsdt->table_offset_entry to be filled by Guest linker */
>
> here.
>
Ok
Re: [Qemu-devel] [RFC PATCH 04/11] hw/arm/virt-acpi-build: Generate XSDT table and add a build_header function
comments below, fix attached
On 01/24/15 10:21, Shannon Zhao wrote:
> XDST points to other tables except FACS & DSDT.
> Implement a common header helper functions for generating ACPI tables.
>
> Signed-off-by: Shannon Zhao
> ---
> hw/arm/virt-acpi-build.c| 34 ++
> include/hw/acpi/acpi-defs.h |9 +
> 2 files changed, 43 insertions(+), 0 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9c3971a..446947a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -61,6 +61,22 @@
> #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"
> #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log"
>
> +static void
> +build_header(GArray *linker, GArray *table_data,
> + AcpiTableHeader *h, const char *sig, int len, uint8_t rev)
> +{
> +memcpy(&h->signature, sig, sizeof(h->signature));
> +h->length = cpu_to_le32(len);
> +h->revision = rev;
> +memcpy(h->oem_id, ACPI_VIRT_QEMU_STR_6, sizeof(h->oem_id));
> +memcpy(h->oem_table_id, ACPI_VIRT_MACH_STR_8, sizeof(h->oem_table_id));
> +h->oem_revision = cpu_to_le32(1);
> +h->checksum = 0;
> +/* Checksum to be filled in by Guest linker */
> +bios_linker_loader_add_checksum(linker, ACPI_BUILD_TABLE_FILE,
> +table_data->data, h, len, &h->checksum);
> +}
> +
> static inline void *acpi_data_push(GArray *table_data, uint64_t size)
> {
> unsigned off = table_data->len;
> @@ -115,6 +131,24 @@ build_rsdp(GArray *rsdp_table, GArray *linker, uint64_t
> xsdt)
> static void
> build_xsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
> {
> +AcpiXsdtDescriptor *xsdt;
> +size_t xsdt_len;
> +int i;
> +
> +xsdt_len = sizeof(*xsdt) + sizeof(uint64_t) * table_offsets->len;
> +xsdt = acpi_data_push(table_data, xsdt_len);
> +memcpy(xsdt->table_offset_entry, table_offsets->data,
> + sizeof(uint64_t) * table_offsets->len);
This is a bug, but it's not introduced here. The bug is introduced in:
[RFC PATCH 02/11] hw/arm/virt-acpi-build: Basic framework for
building ACPI tables
I'm attaching the fix.
Please do not squash the fix into this patch; you have to split it up.
The code fixes go into 02/11, and the typo fix goes:
> +for (i = 0; i < table_offsets->len; ++i) {
> +/* rsdt->table_offset_entry to be filled by Guest linker */
here.
Thanks,
Laszlo
>From 899ed2c6329c33520f5bad143dd50508012f1f27 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek
Date: Sat, 24 Jan 2015 20:43:48 +0100
Subject: [PATCH] virt-acpi-build: internal representation for XSDT entries
must be uint64_t
The build_xsdt() function copies the data contents of the gradually built
"table_offsets" GArray directly into the XSDT, with memcpy(), using an
element type of uint64_t.
For this to work, the element type of the source array, "table_offsets",
must also be uint64_t.
Otherwise, the XSDT entries will be filled with garbage, pointing outside
of the target blob. edk2's linker/loader catches & rejects the first
pointer:
Loading driver at 0x000BEE36000 EntryPoint=0x000BEE362B0 AcpiPlatformDxe.efi
InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF BB7BCE98
ProcessCmdAllocate: File="etc/acpi/rsdp" Alignment=0x10 Zone=2 Size=0x24 Address=0xB7048000
ProcessCmdAllocate: File="etc/acpi/tables" Alignment=0x40 Zone=1 Size=0xB4E Address=0xB7047000
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49 Start=0x40 Length=0x8CE
ProcessCmdAddPointer: PointerFile="etc/acpi/tables" PointeeFile="etc/acpi/tables" PointerOffset=0x992 PointerSize=8
ProcessCmdAddPointer: PointerFile="etc/acpi/tables" PointeeFile="etc/acpi/tables" PointerOffset=0x99A PointerSize=8
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x917 Start=0x90E Length=0x10C
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0xA23 Start=0xA1A Length=0x90
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0xAB3 Start=0xAAA Length=0x60
-> ProcessCmdAddPointer: invalid pointer value in "etc/acpi/tables"
InstallAllQemuLinkedTables: freeing "etc/acpi/rsdp"
InstallAllQemuLinkedTables: freeing "etc/acpi/tables"
Error: Image at 000BEE36000 start failed: Protocol Error
Signed-off-by: Laszlo Ersek
---
hw/arm/virt-acpi-build.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 5c76ca2..904fc33 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -94,7 +94,7 @@ static unsigned acpi_data_len(GArray *table)
static inline void acpi_add_table(GArray *table_offsets, GArray *table_data)
{
-uint32_t offset = cpu_to_le32(table_data->len);
+uint64_t offset = cpu_to_le64(table_data->len);
g_array_append_val(table_offsets, offset);
}
@@ -245,7 +245,7 @@ build_xsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
memcpy(xsd
[Qemu-devel] [RFC PATCH 04/11] hw/arm/virt-acpi-build: Generate XSDT table and add a build_header function
XDST points to other tables except FACS & DSDT.
Implement a common header helper functions for generating ACPI tables.
Signed-off-by: Shannon Zhao
---
hw/arm/virt-acpi-build.c| 34 ++
include/hw/acpi/acpi-defs.h |9 +
2 files changed, 43 insertions(+), 0 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9c3971a..446947a 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -61,6 +61,22 @@
#define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"
#define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log"
+static void
+build_header(GArray *linker, GArray *table_data,
+ AcpiTableHeader *h, const char *sig, int len, uint8_t rev)
+{
+memcpy(&h->signature, sig, sizeof(h->signature));
+h->length = cpu_to_le32(len);
+h->revision = rev;
+memcpy(h->oem_id, ACPI_VIRT_QEMU_STR_6, sizeof(h->oem_id));
+memcpy(h->oem_table_id, ACPI_VIRT_MACH_STR_8, sizeof(h->oem_table_id));
+h->oem_revision = cpu_to_le32(1);
+h->checksum = 0;
+/* Checksum to be filled in by Guest linker */
+bios_linker_loader_add_checksum(linker, ACPI_BUILD_TABLE_FILE,
+table_data->data, h, len, &h->checksum);
+}
+
static inline void *acpi_data_push(GArray *table_data, uint64_t size)
{
unsigned off = table_data->len;
@@ -115,6 +131,24 @@ build_rsdp(GArray *rsdp_table, GArray *linker, uint64_t
xsdt)
static void
build_xsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
{
+AcpiXsdtDescriptor *xsdt;
+size_t xsdt_len;
+int i;
+
+xsdt_len = sizeof(*xsdt) + sizeof(uint64_t) * table_offsets->len;
+xsdt = acpi_data_push(table_data, xsdt_len);
+memcpy(xsdt->table_offset_entry, table_offsets->data,
+ sizeof(uint64_t) * table_offsets->len);
+for (i = 0; i < table_offsets->len; ++i) {
+/* rsdt->table_offset_entry to be filled by Guest linker */
+bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE,
+ ACPI_BUILD_TABLE_FILE,
+ table_data,
&xsdt->table_offset_entry[i],
+ sizeof(uint64_t));
+}
+build_header(linker, table_data,
+ (void *)xsdt, "XSDT", xsdt_len, 1);
}
/* MADT */
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index c4468f8..779f872 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -88,6 +88,15 @@ struct AcpiTableHeader /* ACPI common table header */
typedef struct AcpiTableHeader AcpiTableHeader;
/*
+ * Extended System Description Table (XSDT)
+ */
+struct AcpiXsdtDescriptor {
+ACPI_TABLE_HEADER_DEF
+uint64_t table_offset_entry[1]; /* Array of pointers to ACPI tables */
+} QEMU_PACKED;
+typedef struct AcpiXsdtDescriptor AcpiXsdtDescriptor;
+
+/*
* ACPI 1.0 Fixed ACPI Description Table (FADT)
*/
struct AcpiFadtDescriptorRev1
--
1.7.1
