Re: [Qemu-devel] [RFC PATCH 04/11] hw/arm/virt-acpi-build: Generate XSDT table and add a build_header function

[Laszlo Ersek <lersek@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]

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 <zhaoshenglong@huawei.com>
> ---
>  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

[-- Attachment #2: 0001-virt-acpi-build-internal-representation-for-XSDT-ent.patch --]
[-- Type: text/x-patch, Size: 3453 bytes --]

>From 899ed2c6329c33520f5bad143dd50508012f1f27 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
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 <lersek@redhat.com>
---
 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(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 */
+        /* xsdt->table_offset_entry to be filled by Guest linker */
         bios_linker_loader_add_pointer(linker,
                                        ACPI_BUILD_TABLE_FILE,
                                        ACPI_BUILD_TABLE_FILE,
@@ -425,7 +425,7 @@ void acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
     unsigned facs, dsdt, xsdt;
 
     table_offsets = g_array_new(false, true /* clear */,
-                                        sizeof(uint32_t));
+                                        sizeof(uint64_t));
     ACPI_BUILD_DPRINTF("init ACPI tables\n");
 
     bios_linker_loader_alloc(tables->linker, ACPI_BUILD_TABLE_FILE,
-- 
1.8.3.1