[Qemu-devel] [PATCH v2 0/8] hw: acpi: RSDP fixes and refactoring

[Qemu-devel] [PATCH v2 0/8] hw: acpi: RSDP fixes and refactoring

[Samuel Ortiz]

This patch serie fixes a couple of RSDP checksum related issues:

- On RSDP rev2, we are not adding the extended checksum and computing
  the checksum on the full table instead of the first 20 bytes.
- On RSDP rev1, we are computing the checksum on 36 bytes instead of 20.
  We're lucky enough that this is only adding zeroes to the checksum.

A guest Linux kernel does not seem to care about RSDP checksums, so
those 2 fixes are mostly for correctness sake.
Any machine type that generates rev2 RSDP will see its RSDP table
modified but since x86 builds RSDP v1 and all ACPI tests only run on
either pc or q35, the ACPI tests tables are not affected by this fix.

The serie also extends the ARM virt ACPI RSDP build routine to support
both RSDP v1 and v2, in order to share this code between x86 and
aarch64. While extending, we also convert the routine to the latest
build_append_foo() API. The new implementation is a closer reflection of
the ACPI spec itself, is endian agnostic and allows for getting rid of the
AcpiRsdpDescriptor structure.

v1 -> v2:
  * Add missing SOBs.
  * Rephrase some commit messages according to the maintainer's input.
  * Replace ACPI spec definitions with hard coded constants.
  * Replace init_rsdp_data() helper with open coded rsdp variable init.
  * Rename acpi_find_[rx]sdt_address to acpi_get_[rx]sdt_address.

Igor Mammedov (2):
  hw: arm: acpi: Fix incorrect checksums in RSDP
  hw: i386: Use correct RSDT length for checksum

Samuel Ortiz (6):
  hw: acpi: The RSDP build API can return void
  hw: arm: Carry RSDP specific data through AcpiRsdpData
  hw: arm: Convert the RSDP build to the buid_append_foo() API
  hw: arm: Support both legacy and current RSDP build
  hw: acpi: Export and share the ARM RSDP build
  hw: acpi: Remove AcpiRsdpDescriptor and fix tests

 include/hw/acpi/acpi-defs.h | 19 ++++++--------
 include/hw/acpi/aml-build.h |  2 ++
 tests/acpi-utils.h          |  5 +++-
 hw/acpi/aml-build.c         | 50 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 40 +++++++----------------------
 hw/i386/acpi-build.c        | 36 +++++++-------------------
 tests/acpi-utils.c          | 46 ++++++++++++++++++++++++++++------
 tests/bios-tables-test.c    | 27 ++++++++++++++------
 tests/vmgenid-test.c        |  8 +++---
 9 files changed, 143 insertions(+), 90 deletions(-)

-- 
2.19.2

[Qemu-devel] [PATCH v2 1/8] hw: acpi: The RSDP build API can return void

[Samuel Ortiz]

For both x86 and ARM architectures, the internal RSDP build API can
return void as the current return value is unused.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/virt-acpi-build.c | 4 +---
 hw/i386/acpi-build.c     | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 5785fb697c..fcaa350892 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -367,7 +367,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
 }
 
 /* RSDP */
-static GArray *
+static void
 build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
 {
     AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
@@ -392,8 +392,6 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
         (char *)rsdp - rsdp_table->data, sizeof *rsdp,
         (char *)&rsdp->checksum - rsdp_table->data);
-
-    return rsdp_table;
 }
 
 static void
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 236a20eaa8..35f17d0d91 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2547,7 +2547,7 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
                  "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
 }
 
-static GArray *
+static void
 build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
 {
     AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
@@ -2569,8 +2569,6 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
         (char *)rsdp - rsdp_table->data, sizeof *rsdp,
         (char *)&rsdp->checksum - rsdp_table->data);
-
-    return rsdp_table;
 }
 
 typedef
-- 
2.19.2

[Qemu-devel] [PATCH v2 2/8] hw: arm: acpi: Fix incorrect checksums in RSDP

[Samuel Ortiz]

From: Igor Mammedov <imammedo@redhat.com>

When RSDP table was introduced (d4bec5d87), we calculated only legacy
checksum, and that was incorrect as it
 - specified rev=2 and forgot about extended checksum.
 - legacy checksum calculated on full table instead of the 1st 20 bytes

Fix it by adding extended checksum calculation and using correct
size for legacy checksum.

While at it use explicit constants to specify sub/full tables
sizes instead of relying on AcpiRsdpDescriptor size and fields offsets.
The follow up commits will convert this table to build_append_int_noprefix() API,
will use constants anyway and remove unused AcpiRsdpDescriptor structure.

Based on "[PATCH v5 05/24] hw: acpi: Implement XSDT support for  RSDP"
by Samuel Ortiz, who did it right in his impl.

Fixes: d4bec5d87 (hw/arm/virt-acpi-build: Generate RSDP table)
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
CC: Ard Biesheuvel <ard.biesheuvel@linaro.org>
CC: Shannon Zhao <shannon.zhaosl@gmail.com>
Reviewed-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
---
 hw/arm/virt-acpi-build.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index fcaa350892..0835900052 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -390,8 +390,13 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
 
     /* Checksum to be filled by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-        (char *)rsdp - rsdp_table->data, sizeof *rsdp,
+        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
         (char *)&rsdp->checksum - rsdp_table->data);
+
+    /* Extended checksum to be filled by Guest linker */
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
+        (char *)rsdp - rsdp_table->data, 36 /* ACPI rev 2.0 RSDP size */,
+        (char *)&rsdp->extended_checksum - rsdp_table->data);
 }
 
 static void
-- 
2.19.2

[Qemu-devel] [PATCH v2 3/8] hw: i386: Use correct RSDT length for checksum

[Samuel Ortiz]

From: Igor Mammedov <imammedo@redhat.com>

AcpiRsdpDescriptor describes revision 2 RSDP table so using sizeof(*rsdp)
for checksum calculation isn't correct since we are adding extra 16 bytes.
But acpi_data_push() zeroes out table, so just by luck we are summing up
exta zeros which still yelds correct checksum.

Fix it up by explicitly stating table size instead of using
pointer arithmetics on stucture.

PS:
Extra 16 bytes are still wasted, but droping them will break migration
for machines older than 2.3 due to size mismatch, for 2.3 and older it's
not an issue since they are using resizable memory regions (a1666142d)
for ACPI blobs. So keep wasting memory to avoid breaking old machines.

Fixes: 72c194f7e (i386: ACPI table generation code from seabios)
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
---
 hw/i386/acpi-build.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 35f17d0d91..fb877648ac 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2550,6 +2550,11 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
 static void
 build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
 {
+    /* AcpiRsdpDescriptor describes revision 2 RSDP table and as result we
+     * allocate extra 16 bytes for pc/q35 RSDP rev1 as well. Keep extra 16 bytes
+     * wasted to make sure we won't breake migration for machine types older
+     * than 2.3 due to size mismatch.
+     */
     AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
     unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
     unsigned rsdt_pa_offset =
@@ -2567,7 +2572,7 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
 
     /* Checksum to be filled by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-        (char *)rsdp - rsdp_table->data, sizeof *rsdp,
+        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
         (char *)&rsdp->checksum - rsdp_table->data);
 }
 
-- 
2.19.2

[Qemu-devel] [PATCH v2 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData

[Samuel Ortiz]

That will allow us to generalize the ARM build_rsdp() routine to support
both legacy RSDP (The current i386 implementation) and extended RSDP
(The ARM implementation).

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
---
 include/hw/acpi/acpi-defs.h |  8 ++++++++
 hw/arm/virt-acpi-build.c    | 18 +++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index af8e023968..8425ecb8c6 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
 } QEMU_PACKED;
 typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
 
+typedef struct AcpiRsdpData {
+    uint8_t oem_id[6]; /* OEM identification */
+    uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
+
+    unsigned *rsdt_tbl_offset;
+    unsigned *xsdt_tbl_offset;
+} AcpiRsdpData;
+
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */
 
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 0835900052..ce8bfa5a37 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
 
 /* RSDP */
 static void
-build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
+build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
 {
     AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
     unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
@@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
                              true /* fseg memory */);
 
     memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
-    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
+    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
     rsdp->length = cpu_to_le32(sizeof(*rsdp));
-    rsdp->revision = 0x02;
+    rsdp->revision = rsdp_data->revision;
 
     /* Address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
-        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
+        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
 
     /* Checksum to be filled by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
@@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     build_xsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
 
     /* RSDP is in FSEG memory, so allocate it separately */
-    build_rsdp(tables->rsdp, tables->linker, xsdt);
+    {
+        AcpiRsdpData rsdp_data = {
+            .revision = 2,
+            .oem_id = ACPI_BUILD_APPNAME6,
+            .xsdt_tbl_offset = &xsdt,
+            .rsdt_tbl_offset = NULL,
+        };
+        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
+    }
 
     /* Cleanup memory that's no longer used. */
     g_array_free(table_offsets, true);
-- 
2.19.2

[Qemu-devel] [PATCH v2 5/8] hw: arm: Convert the RSDP build to the buid_append_foo() API

[Samuel Ortiz]

Instead of filling a mapped and packed C structure field in random order
and being careful about endianness and sizes, build_rsdp() now uses
build_append_int_noprefix() to compose RSDP table.

This makes for an easier to review and maintain code that is almost
matching 1:1 the ACPI spec itself.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
---
 hw/arm/virt-acpi-build.c | 42 +++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index ce8bfa5a37..4782aea4fe 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -368,35 +368,37 @@ static void acpi_dsdt_add_power_button(Aml *scope)
 
 /* RSDP */
 static void
-build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
+build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
 {
-    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
-    unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
-    unsigned xsdt_pa_offset =
-        (char *)&rsdp->xsdt_physical_address - rsdp_table->data;
-
-    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
+    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, tbl, 16,
                              true /* fseg memory */);
 
-    memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
-    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
-    rsdp->length = cpu_to_le32(sizeof(*rsdp));
-    rsdp->revision = rsdp_data->revision;
+    g_array_append_vals(tbl, "RSD PTR ", 8); /* Signature */
+    build_append_int_noprefix(tbl, 0, 1); /* Checksum */
+    g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
+    build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
+    build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
+    build_append_int_noprefix(tbl, 36, 4); /* Length */
 
-    /* Address to be filled by Guest linker */
-    bios_linker_loader_add_pointer(linker,
-        ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
-        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
+    /* XSDT address to be filled by guest linker */
+    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
+    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
+                                   24, 8,
+                                   ACPI_BUILD_TABLE_FILE,
+                                   *rsdp_data->xsdt_tbl_offset);
 
-    /* Checksum to be filled by Guest linker */
+    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
+    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
+
+    /* Checksum to be filled by guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
-        (char *)&rsdp->checksum - rsdp_table->data);
+                                    0, 20, /* ACPI rev 1.0 RSDP size */
+                                    8);
 
     /* Extended checksum to be filled by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-        (char *)rsdp - rsdp_table->data, 36 /* ACPI rev 2.0 RSDP size */,
-        (char *)&rsdp->extended_checksum - rsdp_table->data);
+                                    0, 36, /* ACPI rev 2.0 RSDP size */
+                                    32);
 }
 
 static void
-- 
2.19.2

[Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build

[Samuel Ortiz]

We add the ability to build legacy or current RSDP tables, based on the
AcpiRsdpData revision field passed to build_rsdp().
Although arm/virt only uses RSDP v2, adding that capability to
build_rsdp will allow us to share the RSDP build code between ARM and x86.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
---
 hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 4782aea4fe..e1338b6f5a 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
     g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
     build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
     build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
-    build_append_int_noprefix(tbl, 36, 4); /* Length */
-
-    /* XSDT address to be filled by guest linker */
-    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
-    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
-                                   24, 8,
-                                   ACPI_BUILD_TABLE_FILE,
-                                   *rsdp_data->xsdt_tbl_offset);
-
-    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
-    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
+    if (rsdp_data->rsdt_tbl_offset) {
+        /* RSDT address to be filled by guest linker */
+        bios_linker_loader_add_pointer(linker,
+                                       ACPI_BUILD_RSDP_FILE, 16, 4,
+                                       ACPI_BUILD_TABLE_FILE,
+                                       *rsdp_data->rsdt_tbl_offset);
+    }
 
     /* Checksum to be filled by guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
                                     0, 20, /* ACPI rev 1.0 RSDP size */
                                     8);
 
+    if (rsdp_data->revision == 0) {
+        /* ACPI 1.0 RSDP, we're done */
+        return;
+    }
+
+    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
+    g_assert(rsdp_data->xsdt_tbl_offset != NULL);
+
+    build_append_int_noprefix(tbl, 36, 4); /* Length */
+
+    /* XSDT address to be filled by guest linker */
+    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
+    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE, 24, 8,
+                                   ACPI_BUILD_TABLE_FILE,
+                                   *rsdp_data->xsdt_tbl_offset);
+
+    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
+    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
+
     /* Extended checksum to be filled by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
                                     0, 36, /* ACPI rev 2.0 RSDP size */
-- 
2.19.2

[Qemu-devel] [PATCH v2 7/8] hw: acpi: Export and share the ARM RSDP build

[Samuel Ortiz]

Now that build_rsdp() supports building both legacy and current RSDP
tables, we can move it to a generic folder (hw/acpi) and have the i386
ACPI code reuse it in order to reduce code duplication.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/acpi/aml-build.h |  2 ++
 hw/acpi/aml-build.c         | 50 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 50 -------------------------------------
 hw/i386/acpi-build.c        | 39 +++++++----------------------
 4 files changed, 61 insertions(+), 80 deletions(-)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 6c36903c0a..1a563ad756 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -388,6 +388,8 @@ void acpi_add_table(GArray *table_offsets, GArray *table_data);
 void acpi_build_tables_init(AcpiBuildTables *tables);
 void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre);
 void
+build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data);
+void
 build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
            const char *oem_id, const char *oem_table_id);
 void
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 1e43cd736d..3d5f77e02b 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1589,6 +1589,56 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
     g_array_free(tables->vmgenid, mfre);
 }
 
+/* RSDP */
+void
+build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
+{
+    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, tbl, 16,
+                             true /* fseg memory */);
+
+    g_array_append_vals(tbl, "RSD PTR ", 8); /* Signature */
+    build_append_int_noprefix(tbl, 0, 1); /* Checksum */
+    g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
+    build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
+    build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
+    if (rsdp_data->rsdt_tbl_offset) {
+        /* RSDT address to be filled by guest linker */
+        bios_linker_loader_add_pointer(linker,
+                                       ACPI_BUILD_RSDP_FILE, 16, 4,
+                                       ACPI_BUILD_TABLE_FILE,
+                                       *rsdp_data->rsdt_tbl_offset);
+    }
+
+    /* Checksum to be filled by guest linker */
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
+                                    0, 20, /* ACPI rev 1.0 RSDP size */
+                                    8);
+
+    if (rsdp_data->revision == 0) {
+        /* ACPI 1.0 RSDP, we're done */
+        return;
+    }
+
+    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
+    g_assert(rsdp_data->xsdt_tbl_offset != NULL);
+
+    build_append_int_noprefix(tbl, 36, 4); /* Length */
+
+    /* XSDT address to be filled by guest linker */
+    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
+    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE, 24, 8,
+                                   ACPI_BUILD_TABLE_FILE,
+                                   *rsdp_data->xsdt_tbl_offset);
+
+    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
+    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
+
+    /* Extended checksum to be filled by Guest linker */
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
+                                    0, 36, /* ACPI rev 2.0 RSDP size */
+                                    32);
+}
+
 /* Build rsdt table */
 void
 build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index e1338b6f5a..95fad6f0ce 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -366,56 +366,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
     aml_append(scope, dev);
 }
 
-/* RSDP */
-static void
-build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
-{
-    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, tbl, 16,
-                             true /* fseg memory */);
-
-    g_array_append_vals(tbl, "RSD PTR ", 8); /* Signature */
-    build_append_int_noprefix(tbl, 0, 1); /* Checksum */
-    g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
-    build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
-    build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
-    if (rsdp_data->rsdt_tbl_offset) {
-        /* RSDT address to be filled by guest linker */
-        bios_linker_loader_add_pointer(linker,
-                                       ACPI_BUILD_RSDP_FILE, 16, 4,
-                                       ACPI_BUILD_TABLE_FILE,
-                                       *rsdp_data->rsdt_tbl_offset);
-    }
-
-    /* Checksum to be filled by guest linker */
-    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-                                    0, 20, /* ACPI rev 1.0 RSDP size */
-                                    8);
-
-    if (rsdp_data->revision == 0) {
-        /* ACPI 1.0 RSDP, we're done */
-        return;
-    }
-
-    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
-    g_assert(rsdp_data->xsdt_tbl_offset != NULL);
-
-    build_append_int_noprefix(tbl, 36, 4); /* Length */
-
-    /* XSDT address to be filled by guest linker */
-    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
-    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE, 24, 8,
-                                   ACPI_BUILD_TABLE_FILE,
-                                   *rsdp_data->xsdt_tbl_offset);
-
-    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
-    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
-
-    /* Extended checksum to be filled by Guest linker */
-    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-                                    0, 36, /* ACPI rev 2.0 RSDP size */
-                                    32);
-}
-
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index fb877648ac..528a5fd4ed 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2547,35 +2547,6 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
                  "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
 }
 
-static void
-build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
-{
-    /* AcpiRsdpDescriptor describes revision 2 RSDP table and as result we
-     * allocate extra 16 bytes for pc/q35 RSDP rev1 as well. Keep extra 16 bytes
-     * wasted to make sure we won't breake migration for machine types older
-     * than 2.3 due to size mismatch.
-     */
-    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
-    unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
-    unsigned rsdt_pa_offset =
-        (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
-
-    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
-                             true /* fseg memory */);
-
-    memcpy(&rsdp->signature, "RSD PTR ", 8);
-    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
-    /* Address to be filled by Guest linker */
-    bios_linker_loader_add_pointer(linker,
-        ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
-        ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
-
-    /* Checksum to be filled by Guest linker */
-    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
-        (char *)&rsdp->checksum - rsdp_table->data);
-}
-
 typedef
 struct AcpiBuildState {
     /* Copy of table in RAM (for patching). */
@@ -2732,7 +2703,15 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
                slic_oem.id, slic_oem.table_id);
 
     /* RSDP is in FSEG memory, so allocate it separately */
-    build_rsdp(tables->rsdp, tables->linker, rsdt);
+    {
+        AcpiRsdpData rsdp_data = {
+            .revision = 0,
+            .oem_id = ACPI_BUILD_APPNAME6,
+            .xsdt_tbl_offset = NULL,
+            .rsdt_tbl_offset = &rsdt,
+        };
+        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
+    }
 
     /* We'll expose it all to Guest so we want to reduce
      * chance of size changes.
-- 
2.19.2

[Qemu-devel] [PATCH v2 8/8] hw: acpi: Remove AcpiRsdpDescriptor and fix tests

[Samuel Ortiz]

The only remaining AcpiRsdpDescriptor users are the ACPI utils for the
BIOS table tests.
We remove that dependency and can thus remove the structure itself.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
---
 include/hw/acpi/acpi-defs.h | 13 -----------
 tests/acpi-utils.h          |  5 +++-
 tests/acpi-utils.c          | 46 ++++++++++++++++++++++++++++++-------
 tests/bios-tables-test.c    | 27 +++++++++++++++-------
 tests/vmgenid-test.c        |  8 ++++---
 5 files changed, 66 insertions(+), 33 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 8425ecb8c6..5021cb9e79 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -40,19 +40,6 @@ enum {
     ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE,
 };
 
-struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
-    uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
-    uint8_t  checksum;               /* To make sum of struct == 0 */
-    uint8_t  oem_id [6];             /* OEM identification */
-    uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
-    uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT */
-    uint32_t length;                 /* XSDT Length in bytes including hdr */
-    uint64_t xsdt_physical_address;  /* 64-bit physical address of XSDT */
-    uint8_t  extended_checksum;      /* Checksum of entire table */
-    uint8_t  reserved [3];           /* Reserved field must be 0 */
-} QEMU_PACKED;
-typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
-
 typedef struct AcpiRsdpData {
     uint8_t oem_id[6]; /* OEM identification */
     uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index ac52abd0dd..c26c8976a6 100644
--- a/tests/acpi-utils.h
+++ b/tests/acpi-utils.h
@@ -82,6 +82,9 @@ typedef struct {
 
 uint8_t acpi_calc_checksum(const uint8_t *data, int len);
 uint32_t acpi_find_rsdp_address(void);
-void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table);
+uint32_t acpi_get_rsdt_address(uint8_t *rsdp_table);
+uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table);
+void acpi_parse_rsdp_table(uint32_t addr, uint8_t *rsdp_table,
+                           uint8_t revision);
 
 #endif  /* TEST_ACPI_UTILS_H */
diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
index 41dc1ea9b4..b1113bda8b 100644
--- a/tests/acpi-utils.c
+++ b/tests/acpi-utils.c
@@ -52,14 +52,44 @@ uint32_t acpi_find_rsdp_address(void)
     return off;
 }
 
-void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table)
+uint32_t acpi_get_rsdt_address(uint8_t *rsdp_table)
 {
-    ACPI_READ_FIELD(rsdp_table->signature, addr);
-    ACPI_ASSERT_CMP64(rsdp_table->signature, "RSD PTR ");
+    uint32_t rsdt_physical_address;
+    uint8_t revision = rsdp_table[15 /* Revision offset */];
 
-    ACPI_READ_FIELD(rsdp_table->checksum, addr);
-    ACPI_READ_ARRAY(rsdp_table->oem_id, addr);
-    ACPI_READ_FIELD(rsdp_table->revision, addr);
-    ACPI_READ_FIELD(rsdp_table->rsdt_physical_address, addr);
-    ACPI_READ_FIELD(rsdp_table->length, addr);
+    if (revision != 0) { /* ACPI 1.0 RSDP */
+        return 0;
+    }
+
+    memcpy(&rsdt_physical_address, &rsdp_table[16 /* RsdtAddress offset */], 4);
+    return le32_to_cpu(rsdt_physical_address);
+}
+
+uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table)
+{
+    uint64_t xsdt_physical_address;
+    uint8_t revision = rsdp_table[15 /* Revision offset */];
+
+    if (revision != 2) { /* ACPI 2.0+ RSDP */
+        return 0;
+    }
+
+    memcpy(&xsdt_physical_address, &rsdp_table[24 /* XsdtAddress offset */], 8);
+    return le64_to_cpu(xsdt_physical_address);
+}
+
+void acpi_parse_rsdp_table(uint32_t addr, uint8_t *rsdp_table, uint8_t revision)
+{
+    switch (revision) {
+    case 0: /* ACPI 1.0 RSDP */
+        memread(addr, rsdp_table, 20);
+        break;
+    case 2: /* ACPI 2.0+ RSDP */
+        memread(addr, rsdp_table, 36);
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    ACPI_ASSERT_CMP64(*((uint64_t *)(rsdp_table)), "RSD PTR ");
 }
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index d661d9be62..84f1500920 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -27,7 +27,8 @@ typedef struct {
     const char *machine;
     const char *variant;
     uint32_t rsdp_addr;
-    AcpiRsdpDescriptor rsdp_table;
+    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
+    uint32_t rsdt_physical_address;
     AcpiRsdtDescriptorRev1 rsdt_table;
     uint32_t dsdt_addr;
     uint32_t facs_addr;
@@ -83,21 +84,31 @@ static void test_acpi_rsdp_address(test_data *data)
     data->rsdp_addr = off;
 }
 
-static void test_acpi_rsdp_table(test_data *data)
+static void test_acpi_rsdp_table(test_data *data, uint8_t revision)
 {
-    AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table;
+    uint8_t *rsdp_table = data->rsdp_table;
     uint32_t addr = data->rsdp_addr;
 
-    acpi_parse_rsdp_table(addr, rsdp_table);
+    acpi_parse_rsdp_table(addr, rsdp_table, revision);
 
-    /* rsdp checksum is not for the whole table, but for the first 20 bytes */
-    g_assert(!acpi_calc_checksum((uint8_t *)rsdp_table, 20));
+    switch (revision) {
+    case 0: /* ACPI 1.0 RSDP */
+        /* With rev 1, checksum is only for the first 20 bytes */
+        g_assert(!acpi_calc_checksum(rsdp_table,  20));
+        break;
+    case 2: /* ACPI 2.0+ RSDP */
+        /* With revision 2, we have 2 checksums */
+        g_assert(!acpi_calc_checksum(rsdp_table, 20));
+        g_assert(!acpi_calc_checksum(rsdp_table, 36));
+    default:
+        g_assert_not_reached();
+    }
 }
 
 static void test_acpi_rsdt_table(test_data *data)
 {
     AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
-    uint32_t addr = le32_to_cpu(data->rsdp_table.rsdt_physical_address);
+    uint32_t addr = acpi_get_rsdt_address(data->rsdp_table);
     uint32_t *tables;
     int tables_nr;
     uint8_t checksum;
@@ -626,7 +637,7 @@ static void test_acpi_one(const char *params, test_data *data)
 
     data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
     test_acpi_rsdp_address(data);
-    test_acpi_rsdp_table(data);
+    test_acpi_rsdp_table(data, 0); /* ACPI 1.0 RSDP */
     test_acpi_rsdt_table(data);
     fadt_fetch_facs_and_dsdt_ptrs(data);
     test_acpi_facs_table(data);
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
index 0a6fb55f2e..5f30dee059 100644
--- a/tests/vmgenid-test.c
+++ b/tests/vmgenid-test.c
@@ -35,7 +35,7 @@ static uint32_t acpi_find_vgia(void)
 {
     uint32_t rsdp_offset;
     uint32_t guid_offset = 0;
-    AcpiRsdpDescriptor rsdp_table;
+    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
     uint32_t rsdt, rsdt_table_length;
     AcpiRsdtDescriptorRev1 rsdt_table;
     size_t tables_nr;
@@ -52,9 +52,11 @@ static uint32_t acpi_find_vgia(void)
 
     g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
 
-    acpi_parse_rsdp_table(rsdp_offset, &rsdp_table);
+    acpi_parse_rsdp_table(rsdp_offset, rsdp_table, 0);
+
+    rsdt = acpi_get_rsdt_address(rsdp_table);
+    g_assert(rsdt);
 
-    rsdt = le32_to_cpu(rsdp_table.rsdt_physical_address);
     /* read the header */
     ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
     ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
-- 
2.19.2

Re: [Qemu-devel] [PATCH v2 1/8] hw: acpi: The RSDP build API can return void

[Andrew Jones]

On Thu, Nov 29, 2018 at 02:24:21PM +0100, Samuel Ortiz wrote:
> For both x86 and ARM architectures, the internal RSDP build API can
> return void as the current return value is unused.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 4 +---
>  hw/i386/acpi-build.c     | 4 +---
>  2 files changed, 2 insertions(+), 6 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/8] hw: arm: acpi: Fix incorrect checksums in RSDP

[Andrew Jones]

On Thu, Nov 29, 2018 at 02:24:22PM +0100, Samuel Ortiz wrote:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> When RSDP table was introduced (d4bec5d87), we calculated only legacy
> checksum, and that was incorrect as it
>  - specified rev=2 and forgot about extended checksum.
>  - legacy checksum calculated on full table instead of the 1st 20 bytes
> 
> Fix it by adding extended checksum calculation and using correct
> size for legacy checksum.
> 
> While at it use explicit constants to specify sub/full tables
> sizes instead of relying on AcpiRsdpDescriptor size and fields offsets.
> The follow up commits will convert this table to build_append_int_noprefix() API,
> will use constants anyway and remove unused AcpiRsdpDescriptor structure.
> 
> Based on "[PATCH v5 05/24] hw: acpi: Implement XSDT support for  RSDP"
> by Samuel Ortiz, who did it right in his impl.
> 
> Fixes: d4bec5d87 (hw/arm/virt-acpi-build: Generate RSDP table)
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> CC: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> CC: Shannon Zhao <shannon.zhaosl@gmail.com>
> Reviewed-by: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

I don't see my R-b here, but this patch doesn't appear to have changed
since I reviewed it. Here it is again

Reviewed-by: Andrew Jones <drjones@redhat.com>

> ---
>  hw/arm/virt-acpi-build.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index fcaa350892..0835900052 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -390,8 +390,13 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
>  
>      /* Checksum to be filled by Guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> -        (char *)rsdp - rsdp_table->data, sizeof *rsdp,
> +        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
>          (char *)&rsdp->checksum - rsdp_table->data);
> +
> +    /* Extended checksum to be filled by Guest linker */
> +    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> +        (char *)rsdp - rsdp_table->data, 36 /* ACPI rev 2.0 RSDP size */,
> +        (char *)&rsdp->extended_checksum - rsdp_table->data);
>  }
>  
>  static void
> -- 
> 2.19.2
> 
>

Re: [Qemu-devel] [PATCH v2 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData

[Andrew Jones]

On Thu, Nov 29, 2018 at 02:24:24PM +0100, Samuel Ortiz wrote:
> That will allow us to generalize the ARM build_rsdp() routine to support
> both legacy RSDP (The current i386 implementation) and extended RSDP
> (The ARM implementation).
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  include/hw/acpi/acpi-defs.h |  8 ++++++++
>  hw/arm/virt-acpi-build.c    | 18 +++++++++++++-----
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index af8e023968..8425ecb8c6 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
>  } QEMU_PACKED;
>  typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
>  
> +typedef struct AcpiRsdpData {
> +    uint8_t oem_id[6]; /* OEM identification */
> +    uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
> +
> +    unsigned *rsdt_tbl_offset;
> +    unsigned *xsdt_tbl_offset;

Why use pointers?

> +} AcpiRsdpData;
> +
>  /* Table structure from Linux kernel (the ACPI tables are under the
>     BSD license) */
>  
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 0835900052..ce8bfa5a37 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>  
>  /* RSDP */
>  static void
> -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
>  {
>      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
>      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
>                               true /* fseg memory */);
>  
>      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));

sizeof(rsdp_data->oem_id)

>      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> -    rsdp->revision = 0x02;
> +    rsdp->revision = rsdp_data->revision;
>  
>      /* Address to be filled by Guest linker */
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
>  
>      /* Checksum to be filled by Guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> @@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      build_xsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
>  
>      /* RSDP is in FSEG memory, so allocate it separately */
> -    build_rsdp(tables->rsdp, tables->linker, xsdt);
> +    {
> +        AcpiRsdpData rsdp_data = {
> +            .revision = 2,
> +            .oem_id = ACPI_BUILD_APPNAME6,
> +            .xsdt_tbl_offset = &xsdt,
> +            .rsdt_tbl_offset = NULL,

nit: no need for this explicit NULLing

> +        };
> +        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> +    }
>  
>      /* Cleanup memory that's no longer used. */
>      g_array_free(table_offsets, true);
> -- 
> 2.19.2
> 
>

Thanks,
drew 

Re: [Qemu-devel] [PATCH v2 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData

[Samuel Ortiz]

On Thu, Nov 29, 2018 at 03:09:32PM +0100, Andrew Jones wrote:
> On Thu, Nov 29, 2018 at 02:24:24PM +0100, Samuel Ortiz wrote:
> > That will allow us to generalize the ARM build_rsdp() routine to support
> > both legacy RSDP (The current i386 implementation) and extended RSDP
> > (The ARM implementation).
> > 
> > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > ---
> >  include/hw/acpi/acpi-defs.h |  8 ++++++++
> >  hw/arm/virt-acpi-build.c    | 18 +++++++++++++-----
> >  2 files changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index af8e023968..8425ecb8c6 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
> >  } QEMU_PACKED;
> >  typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
> >  
> > +typedef struct AcpiRsdpData {
> > +    uint8_t oem_id[6]; /* OEM identification */
> > +    uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
> > +
> > +    unsigned *rsdt_tbl_offset;
> > +    unsigned *xsdt_tbl_offset;
> 
> Why use pointers?
Mostly to be consistent with the FADT data structure (AcpiFadtData).


> > +} AcpiRsdpData;
> > +
> >  /* Table structure from Linux kernel (the ACPI tables are under the
> >     BSD license) */
> >  
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 0835900052..ce8bfa5a37 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> >  
> >  /* RSDP */
> >  static void
> > -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> >  {
> >      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> >      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> > @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> >                               true /* fseg memory */);
> >  
> >      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> > -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> > +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
> 
> sizeof(rsdp_data->oem_id)
Which is the same thing. Note that this piece of code eventually gets
removed later in the serie.



> >      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> > -    rsdp->revision = 0x02;
> > +    rsdp->revision = rsdp_data->revision;
> >  
> >      /* Address to be filled by Guest linker */
> >      bios_linker_loader_add_pointer(linker,
> >          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> > -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> > +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
> >  
> >      /* Checksum to be filled by Guest linker */
> >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > @@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> >      build_xsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> >  
> >      /* RSDP is in FSEG memory, so allocate it separately */
> > -    build_rsdp(tables->rsdp, tables->linker, xsdt);
> > +    {
> > +        AcpiRsdpData rsdp_data = {
> > +            .revision = 2,
> > +            .oem_id = ACPI_BUILD_APPNAME6,
> > +            .xsdt_tbl_offset = &xsdt,
> > +            .rsdt_tbl_offset = NULL,
> 
> nit: no need for this explicit NULLing
Unfortunately, yes.

Cheers,
Samuel.

Re: [Qemu-devel] [PATCH v2 5/8] hw: arm: Convert the RSDP build to the buid_append_foo() API

[Andrew Jones]

On Thu, Nov 29, 2018 at 02:24:25PM +0100, Samuel Ortiz wrote:
> Instead of filling a mapped and packed C structure field in random order
> and being careful about endianness and sizes, build_rsdp() now uses
> build_append_int_noprefix() to compose RSDP table.
> 
> This makes for an easier to review and maintain code that is almost
> matching 1:1 the ACPI spec itself.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  hw/arm/virt-acpi-build.c | 42 +++++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 20 deletions(-)


Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build

[Andrew Jones]

On Thu, Nov 29, 2018 at 02:24:26PM +0100, Samuel Ortiz wrote:
> We add the ability to build legacy or current RSDP tables, based on the
> AcpiRsdpData revision field passed to build_rsdp().
> Although arm/virt only uses RSDP v2, adding that capability to
> build_rsdp will allow us to share the RSDP build code between ARM and x86.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 4782aea4fe..e1338b6f5a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
>      g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
>      build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
>      build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
> -    build_append_int_noprefix(tbl, 36, 4); /* Length */
> -
> -    /* XSDT address to be filled by guest linker */
> -    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> -    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
> -                                   24, 8,
> -                                   ACPI_BUILD_TABLE_FILE,
> -                                   *rsdp_data->xsdt_tbl_offset);
> -
> -    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> -    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> +    if (rsdp_data->rsdt_tbl_offset) {

I see why a pointer was used now. Using a pointer ensures a zero
offset won't fail this test. However the test could be replaced with
rsdp_data->revision == 0.

> +        /* RSDT address to be filled by guest linker */
> +        bios_linker_loader_add_pointer(linker,
> +                                       ACPI_BUILD_RSDP_FILE, 16, 4,
> +                                       ACPI_BUILD_TABLE_FILE,
> +                                       *rsdp_data->rsdt_tbl_offset);
> +    }
>  
>      /* Checksum to be filled by guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>                                      0, 20, /* ACPI rev 1.0 RSDP size */
>                                      8);
>  
> +    if (rsdp_data->revision == 0) {
> +        /* ACPI 1.0 RSDP, we're done */
> +        return;
> +    }
> +
> +    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
> +    g_assert(rsdp_data->xsdt_tbl_offset != NULL);

So here's the justification for the pointers. We sanity check the callers.
We're missing the (rsdp_data->revision == 0 && rsdp_data->rsdt_tbl_offset)
sanity check though.

> +
> +    build_append_int_noprefix(tbl, 36, 4); /* Length */
> +
> +    /* XSDT address to be filled by guest linker */
> +    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE, 24, 8,
> +                                   ACPI_BUILD_TABLE_FILE,
> +                                   *rsdp_data->xsdt_tbl_offset);
> +
> +    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> +    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> +
>      /* Extended checksum to be filled by Guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>                                      0, 36, /* ACPI rev 2.0 RSDP size */
> -- 
> 2.19.2
> 
>

Thanks,
drew

Re: [Qemu-devel] [PATCH v2 7/8] hw: acpi: Export and share the ARM RSDP build

[Andrew Jones]

On Thu, Nov 29, 2018 at 02:24:27PM +0100, Samuel Ortiz wrote:
> Now that build_rsdp() supports building both legacy and current RSDP
> tables, we can move it to a generic folder (hw/acpi) and have the i386
> ACPI code reuse it in order to reduce code duplication.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/acpi/aml-build.h |  2 ++
>  hw/acpi/aml-build.c         | 50 +++++++++++++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c    | 50 -------------------------------------
>  hw/i386/acpi-build.c        | 39 +++++++----------------------
>  4 files changed, 61 insertions(+), 80 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build

[Samuel Ortiz]

On Thu, Nov 29, 2018 at 03:42:43PM +0100, Andrew Jones wrote:
> On Thu, Nov 29, 2018 at 02:24:26PM +0100, Samuel Ortiz wrote:
> > We add the ability to build legacy or current RSDP tables, based on the
> > AcpiRsdpData revision field passed to build_rsdp().
> > Although arm/virt only uses RSDP v2, adding that capability to
> > build_rsdp will allow us to share the RSDP build code between ARM and x86.
> > 
> > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > ---
> >  hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++-----------
> >  1 file changed, 26 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 4782aea4fe..e1338b6f5a 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> >      g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
> >      build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
> >      build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
> > -    build_append_int_noprefix(tbl, 36, 4); /* Length */
> > -
> > -    /* XSDT address to be filled by guest linker */
> > -    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> > -    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
> > -                                   24, 8,
> > -                                   ACPI_BUILD_TABLE_FILE,
> > -                                   *rsdp_data->xsdt_tbl_offset);
> > -
> > -    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> > -    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> > +    if (rsdp_data->rsdt_tbl_offset) {
> 
> I see why a pointer was used now. Using a pointer ensures a zero
> offset won't fail this test. However the test could be replaced with
> rsdp_data->revision == 0.
> 
> > +        /* RSDT address to be filled by guest linker */
> > +        bios_linker_loader_add_pointer(linker,
> > +                                       ACPI_BUILD_RSDP_FILE, 16, 4,
> > +                                       ACPI_BUILD_TABLE_FILE,
> > +                                       *rsdp_data->rsdt_tbl_offset);
> > +    }
> >  
> >      /* Checksum to be filled by guest linker */
> >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> >                                      0, 20, /* ACPI rev 1.0 RSDP size */
> >                                      8);
> >  
> > +    if (rsdp_data->revision == 0) {
> > +        /* ACPI 1.0 RSDP, we're done */
> > +        return;
> > +    }
> > +
> > +    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
> > +    g_assert(rsdp_data->xsdt_tbl_offset != NULL);
> 
> So here's the justification for the pointers. We sanity check the callers.
We could sanity check the callers without pointers as well, I don't
think there's a strong advantage for pointers here, except consistence.


> We're missing the (rsdp_data->revision == 0 && rsdp_data->rsdt_tbl_offset)
> sanity check though.
I think there's nothing preventing a caller to include both rsdt and
xsdt if it wants to be able to run on both < 2.0 and 2.0+ platforms with
the same table. So if rsdt is set we should add it, regardless of the revision.

Cheers,
Samuel.

Re: [Qemu-devel] [PATCH v2 8/8] hw: acpi: Remove AcpiRsdpDescriptor and fix tests

[Andrew Jones]

On Thu, Nov 29, 2018 at 02:24:28PM +0100, Samuel Ortiz wrote:
> The only remaining AcpiRsdpDescriptor users are the ACPI utils for the
> BIOS table tests.
> We remove that dependency and can thus remove the structure itself.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  include/hw/acpi/acpi-defs.h | 13 -----------
>  tests/acpi-utils.h          |  5 +++-
>  tests/acpi-utils.c          | 46 ++++++++++++++++++++++++++++++-------
>  tests/bios-tables-test.c    | 27 +++++++++++++++-------
>  tests/vmgenid-test.c        |  8 ++++---
>  5 files changed, 66 insertions(+), 33 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 8425ecb8c6..5021cb9e79 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -40,19 +40,6 @@ enum {
>      ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE,
>  };
>  
> -struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
> -    uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
> -    uint8_t  checksum;               /* To make sum of struct == 0 */
> -    uint8_t  oem_id [6];             /* OEM identification */
> -    uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
> -    uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT */
> -    uint32_t length;                 /* XSDT Length in bytes including hdr */
> -    uint64_t xsdt_physical_address;  /* 64-bit physical address of XSDT */
> -    uint8_t  extended_checksum;      /* Checksum of entire table */
> -    uint8_t  reserved [3];           /* Reserved field must be 0 */
> -} QEMU_PACKED;
> -typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
> -
>  typedef struct AcpiRsdpData {
>      uint8_t oem_id[6]; /* OEM identification */
>      uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
> diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> index ac52abd0dd..c26c8976a6 100644
> --- a/tests/acpi-utils.h
> +++ b/tests/acpi-utils.h
> @@ -82,6 +82,9 @@ typedef struct {
>  
>  uint8_t acpi_calc_checksum(const uint8_t *data, int len);
>  uint32_t acpi_find_rsdp_address(void);
> -void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table);
> +uint32_t acpi_get_rsdt_address(uint8_t *rsdp_table);
> +uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table);
> +void acpi_parse_rsdp_table(uint32_t addr, uint8_t *rsdp_table,
> +                           uint8_t revision);
>  
>  #endif  /* TEST_ACPI_UTILS_H */
> diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
> index 41dc1ea9b4..b1113bda8b 100644
> --- a/tests/acpi-utils.c
> +++ b/tests/acpi-utils.c
> @@ -52,14 +52,44 @@ uint32_t acpi_find_rsdp_address(void)
>      return off;
>  }
>  
> -void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table)
> +uint32_t acpi_get_rsdt_address(uint8_t *rsdp_table)
>  {
> -    ACPI_READ_FIELD(rsdp_table->signature, addr);
> -    ACPI_ASSERT_CMP64(rsdp_table->signature, "RSD PTR ");
> +    uint32_t rsdt_physical_address;
> +    uint8_t revision = rsdp_table[15 /* Revision offset */];
>  
> -    ACPI_READ_FIELD(rsdp_table->checksum, addr);
> -    ACPI_READ_ARRAY(rsdp_table->oem_id, addr);
> -    ACPI_READ_FIELD(rsdp_table->revision, addr);
> -    ACPI_READ_FIELD(rsdp_table->rsdt_physical_address, addr);
> -    ACPI_READ_FIELD(rsdp_table->length, addr);
> +    if (revision != 0) { /* ACPI 1.0 RSDP */
> +        return 0;
> +    }

So now we assume a zero offset can't be valid, which means we don't need
pointers in the struct. Your answer that pointers are used for consistency
is good enough for me though.

> +
> +    memcpy(&rsdt_physical_address, &rsdp_table[16 /* RsdtAddress offset */], 4);
> +    return le32_to_cpu(rsdt_physical_address);
> +}
> +
> +uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table)
> +{
> +    uint64_t xsdt_physical_address;
> +    uint8_t revision = rsdp_table[15 /* Revision offset */];
> +
> +    if (revision != 2) { /* ACPI 2.0+ RSDP */

revision < 2

> +        return 0;
> +    }
> +
> +    memcpy(&xsdt_physical_address, &rsdp_table[24 /* XsdtAddress offset */], 8);
> +    return le64_to_cpu(xsdt_physical_address);
> +}
> +
> +void acpi_parse_rsdp_table(uint32_t addr, uint8_t *rsdp_table, uint8_t revision)
> +{
> +    switch (revision) {
> +    case 0: /* ACPI 1.0 RSDP */
> +        memread(addr, rsdp_table, 20);
> +        break;
> +    case 2: /* ACPI 2.0+ RSDP */
> +        memread(addr, rsdp_table, 36);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    ACPI_ASSERT_CMP64(*((uint64_t *)(rsdp_table)), "RSD PTR ");
>  }
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index d661d9be62..84f1500920 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -27,7 +27,8 @@ typedef struct {
>      const char *machine;
>      const char *variant;
>      uint32_t rsdp_addr;
> -    AcpiRsdpDescriptor rsdp_table;
> +    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
> +    uint32_t rsdt_physical_address;

I don't see rsdt_physical_address getting used anyway.

>      AcpiRsdtDescriptorRev1 rsdt_table;
>      uint32_t dsdt_addr;
>      uint32_t facs_addr;
> @@ -83,21 +84,31 @@ static void test_acpi_rsdp_address(test_data *data)
>      data->rsdp_addr = off;
>  }
>  
> -static void test_acpi_rsdp_table(test_data *data)
> +static void test_acpi_rsdp_table(test_data *data, uint8_t revision)
>  {
> -    AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table;
> +    uint8_t *rsdp_table = data->rsdp_table;
>      uint32_t addr = data->rsdp_addr;
>  
> -    acpi_parse_rsdp_table(addr, rsdp_table);
> +    acpi_parse_rsdp_table(addr, rsdp_table, revision);
>  
> -    /* rsdp checksum is not for the whole table, but for the first 20 bytes */
> -    g_assert(!acpi_calc_checksum((uint8_t *)rsdp_table, 20));
> +    switch (revision) {
> +    case 0: /* ACPI 1.0 RSDP */
> +        /* With rev 1, checksum is only for the first 20 bytes */
> +        g_assert(!acpi_calc_checksum(rsdp_table,  20));
> +        break;
> +    case 2: /* ACPI 2.0+ RSDP */
> +        /* With revision 2, we have 2 checksums */
> +        g_assert(!acpi_calc_checksum(rsdp_table, 20));
> +        g_assert(!acpi_calc_checksum(rsdp_table, 36));
> +    default:
> +        g_assert_not_reached();
> +    }
>  }
>  
>  static void test_acpi_rsdt_table(test_data *data)
>  {
>      AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
> -    uint32_t addr = le32_to_cpu(data->rsdp_table.rsdt_physical_address);
> +    uint32_t addr = acpi_get_rsdt_address(data->rsdp_table);
>      uint32_t *tables;
>      int tables_nr;
>      uint8_t checksum;
> @@ -626,7 +637,7 @@ static void test_acpi_one(const char *params, test_data *data)
>  
>      data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
>      test_acpi_rsdp_address(data);
> -    test_acpi_rsdp_table(data);
> +    test_acpi_rsdp_table(data, 0); /* ACPI 1.0 RSDP */
>      test_acpi_rsdt_table(data);
>      fadt_fetch_facs_and_dsdt_ptrs(data);
>      test_acpi_facs_table(data);
> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
> index 0a6fb55f2e..5f30dee059 100644
> --- a/tests/vmgenid-test.c
> +++ b/tests/vmgenid-test.c
> @@ -35,7 +35,7 @@ static uint32_t acpi_find_vgia(void)
>  {
>      uint32_t rsdp_offset;
>      uint32_t guid_offset = 0;
> -    AcpiRsdpDescriptor rsdp_table;
> +    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
>      uint32_t rsdt, rsdt_table_length;
>      AcpiRsdtDescriptorRev1 rsdt_table;
>      size_t tables_nr;
> @@ -52,9 +52,11 @@ static uint32_t acpi_find_vgia(void)
>  
>      g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
>  
> -    acpi_parse_rsdp_table(rsdp_offset, &rsdp_table);
> +    acpi_parse_rsdp_table(rsdp_offset, rsdp_table, 0);
> +
> +    rsdt = acpi_get_rsdt_address(rsdp_table);
> +    g_assert(rsdt);
>  
> -    rsdt = le32_to_cpu(rsdp_table.rsdt_physical_address);
>      /* read the header */
>      ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
>      ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
> -- 
> 2.19.2
> 
>

Thanks,
drew 

Re: [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build

[Andrew Jones]

On Thu, Nov 29, 2018 at 03:59:42PM +0100, Samuel Ortiz wrote:
> On Thu, Nov 29, 2018 at 03:42:43PM +0100, Andrew Jones wrote:
> > On Thu, Nov 29, 2018 at 02:24:26PM +0100, Samuel Ortiz wrote:
> > > We add the ability to build legacy or current RSDP tables, based on the
> > > AcpiRsdpData revision field passed to build_rsdp().
> > > Although arm/virt only uses RSDP v2, adding that capability to
> > > build_rsdp will allow us to share the RSDP build code between ARM and x86.
> > > 
> > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > ---
> > >  hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++-----------
> > >  1 file changed, 26 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 4782aea4fe..e1338b6f5a 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> > >      g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
> > >      build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
> > >      build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
> > > -    build_append_int_noprefix(tbl, 36, 4); /* Length */
> > > -
> > > -    /* XSDT address to be filled by guest linker */
> > > -    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> > > -    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
> > > -                                   24, 8,
> > > -                                   ACPI_BUILD_TABLE_FILE,
> > > -                                   *rsdp_data->xsdt_tbl_offset);
> > > -
> > > -    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> > > -    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> > > +    if (rsdp_data->rsdt_tbl_offset) {
> > 
> > I see why a pointer was used now. Using a pointer ensures a zero
> > offset won't fail this test. However the test could be replaced with
> > rsdp_data->revision == 0.
> > 
> > > +        /* RSDT address to be filled by guest linker */
> > > +        bios_linker_loader_add_pointer(linker,
> > > +                                       ACPI_BUILD_RSDP_FILE, 16, 4,
> > > +                                       ACPI_BUILD_TABLE_FILE,
> > > +                                       *rsdp_data->rsdt_tbl_offset);
> > > +    }
> > >  
> > >      /* Checksum to be filled by guest linker */
> > >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > >                                      0, 20, /* ACPI rev 1.0 RSDP size */
> > >                                      8);
> > >  
> > > +    if (rsdp_data->revision == 0) {
> > > +        /* ACPI 1.0 RSDP, we're done */
> > > +        return;
> > > +    }
> > > +
> > > +    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
> > > +    g_assert(rsdp_data->xsdt_tbl_offset != NULL);
> > 
> > So here's the justification for the pointers. We sanity check the callers.
> We could sanity check the callers without pointers as well, I don't
> think there's a strong advantage for pointers here, except consistence.
> 
> 
> > We're missing the (rsdp_data->revision == 0 && rsdp_data->rsdt_tbl_offset)
> > sanity check though.
> I think there's nothing preventing a caller to include both rsdt and
> xsdt if it wants to be able to run on both < 2.0 and 2.0+ platforms with
> the same table. So if rsdt is set we should add it, regardless of the revision.

True, but if revision is zero, then we *must* have it. Otherwise we'll
silently return from this function with neither an rsdt nor xsdt linked.

Thanks,
drew

> 
> Cheers,
> Samuel.

Re: [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build

[Samuel Ortiz]

On Thu, Nov 29, 2018 at 04:09:47PM +0100, Andrew Jones wrote:
> On Thu, Nov 29, 2018 at 03:59:42PM +0100, Samuel Ortiz wrote:
> > On Thu, Nov 29, 2018 at 03:42:43PM +0100, Andrew Jones wrote:
> > > On Thu, Nov 29, 2018 at 02:24:26PM +0100, Samuel Ortiz wrote:
> > > > We add the ability to build legacy or current RSDP tables, based on the
> > > > AcpiRsdpData revision field passed to build_rsdp().
> > > > Although arm/virt only uses RSDP v2, adding that capability to
> > > > build_rsdp will allow us to share the RSDP build code between ARM and x86.
> > > > 
> > > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > > ---
> > > >  hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++-----------
> > > >  1 file changed, 26 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > index 4782aea4fe..e1338b6f5a 100644
> > > > --- a/hw/arm/virt-acpi-build.c
> > > > +++ b/hw/arm/virt-acpi-build.c
> > > > @@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> > > >      g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
> > > >      build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
> > > >      build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
> > > > -    build_append_int_noprefix(tbl, 36, 4); /* Length */
> > > > -
> > > > -    /* XSDT address to be filled by guest linker */
> > > > -    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> > > > -    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
> > > > -                                   24, 8,
> > > > -                                   ACPI_BUILD_TABLE_FILE,
> > > > -                                   *rsdp_data->xsdt_tbl_offset);
> > > > -
> > > > -    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> > > > -    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> > > > +    if (rsdp_data->rsdt_tbl_offset) {
> > > 
> > > I see why a pointer was used now. Using a pointer ensures a zero
> > > offset won't fail this test. However the test could be replaced with
> > > rsdp_data->revision == 0.
> > > 
> > > > +        /* RSDT address to be filled by guest linker */
> > > > +        bios_linker_loader_add_pointer(linker,
> > > > +                                       ACPI_BUILD_RSDP_FILE, 16, 4,
> > > > +                                       ACPI_BUILD_TABLE_FILE,
> > > > +                                       *rsdp_data->rsdt_tbl_offset);
> > > > +    }
> > > >  
> > > >      /* Checksum to be filled by guest linker */
> > > >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > > >                                      0, 20, /* ACPI rev 1.0 RSDP size */
> > > >                                      8);
> > > >  
> > > > +    if (rsdp_data->revision == 0) {
> > > > +        /* ACPI 1.0 RSDP, we're done */
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
> > > > +    g_assert(rsdp_data->xsdt_tbl_offset != NULL);
> > > 
> > > So here's the justification for the pointers. We sanity check the callers.
> > We could sanity check the callers without pointers as well, I don't
> > think there's a strong advantage for pointers here, except consistence.
> > 
> > 
> > > We're missing the (rsdp_data->revision == 0 && rsdp_data->rsdt_tbl_offset)
> > > sanity check though.
> > I think there's nothing preventing a caller to include both rsdt and
> > xsdt if it wants to be able to run on both < 2.0 and 2.0+ platforms with
> > the same table. So if rsdt is set we should add it, regardless of the revision.
> 
> True, but if revision is zero, then we *must* have it. Otherwise we'll
> silently return from this function with neither an rsdt nor xsdt linked.
Correct, thanks. With v3 I'm adding a small piece of code at the start
of build_rsdp() that checks for valid rsdt and xsdt pointers (for rev 0
and 2, respectively).

Cheers,
Samuel.

Re: [Qemu-devel] [PATCH v2 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData

[Igor Mammedov]

On Thu, 29 Nov 2018 15:26:27 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> On Thu, Nov 29, 2018 at 03:09:32PM +0100, Andrew Jones wrote:
> > On Thu, Nov 29, 2018 at 02:24:24PM +0100, Samuel Ortiz wrote:  
> > > That will allow us to generalize the ARM build_rsdp() routine to support
> > > both legacy RSDP (The current i386 implementation) and extended RSDP
> > > (The ARM implementation).
> > > 
> > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > ---
> > >  include/hw/acpi/acpi-defs.h |  8 ++++++++
> > >  hw/arm/virt-acpi-build.c    | 18 +++++++++++++-----
> > >  2 files changed, 21 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > index af8e023968..8425ecb8c6 100644
> > > --- a/include/hw/acpi/acpi-defs.h
> > > +++ b/include/hw/acpi/acpi-defs.h
> > > @@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
> > >  } QEMU_PACKED;
> > >  typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
> > >  
> > > +typedef struct AcpiRsdpData {
> > > +    uint8_t oem_id[6]; /* OEM identification */
> > > +    uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
> > > +
> > > +    unsigned *rsdt_tbl_offset;
> > > +    unsigned *xsdt_tbl_offset;  
> > 
> > Why use pointers?  
> Mostly to be consistent with the FADT data structure (AcpiFadtData).
My guess would be that's for detecting an optional field and
skipping setting it in build_rsdp(). But it's mostly for
consistence with current code.

> 
> > > +} AcpiRsdpData;
> > > +
> > >  /* Table structure from Linux kernel (the ACPI tables are under the
> > >     BSD license) */
> > >  
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 0835900052..ce8bfa5a37 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > >  
> > >  /* RSDP */
> > >  static void
> > > -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> > >  {
> > >      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> > >      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> > > @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > >                               true /* fseg memory */);
> > >  
> > >      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> > > -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> > > +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));  
> > 
> > sizeof(rsdp_data->oem_id)  
> Which is the same thing. Note that this piece of code eventually gets
> removed later in the serie.
> 
> 
> 
> > >      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> > > -    rsdp->revision = 0x02;
> > > +    rsdp->revision = rsdp_data->revision;
> > >  
> > >      /* Address to be filled by Guest linker */
> > >      bios_linker_loader_add_pointer(linker,
> > >          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> > > -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> > > +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
> > >  
> > >      /* Checksum to be filled by Guest linker */
> > >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > > @@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> > >      build_xsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> > >  
> > >      /* RSDP is in FSEG memory, so allocate it separately */
> > > -    build_rsdp(tables->rsdp, tables->linker, xsdt);
> > > +    {
> > > +        AcpiRsdpData rsdp_data = {
> > > +            .revision = 2,
> > > +            .oem_id = ACPI_BUILD_APPNAME6,
> > > +            .xsdt_tbl_offset = &xsdt,
> > > +            .rsdt_tbl_offset = NULL,  
> > 
> > nit: no need for this explicit NULLing  
then one would get random junk from the stack this variable is allocate from
in this field.

> Unfortunately, yes.
> 
> Cheers,
> Samuel.

Re: [Qemu-devel] [PATCH v2 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData

[Igor Mammedov]

On Thu, 29 Nov 2018 14:24:24 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> That will allow us to generalize the ARM build_rsdp() routine to support
> both legacy RSDP (The current i386 implementation) and extended RSDP
> (The ARM implementation).
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  include/hw/acpi/acpi-defs.h |  8 ++++++++
>  hw/arm/virt-acpi-build.c    | 18 +++++++++++++-----
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index af8e023968..8425ecb8c6 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
>  } QEMU_PACKED;
>  typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
>  
> +typedef struct AcpiRsdpData {
> +    uint8_t oem_id[6]; /* OEM identification */
> +    uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
> +
> +    unsigned *rsdt_tbl_offset;
> +    unsigned *xsdt_tbl_offset;
> +} AcpiRsdpData;
> +
>  /* Table structure from Linux kernel (the ACPI tables are under the
>     BSD license) */
>  
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 0835900052..ce8bfa5a37 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>  
>  /* RSDP */
>  static void
> -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
>  {
>      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
>      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
>                               true /* fseg memory */);
>  
>      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
>      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> -    rsdp->revision = 0x02;
> +    rsdp->revision = rsdp_data->revision;
>  
>      /* Address to be filled by Guest linker */
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
>  
>      /* Checksum to be filled by Guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> @@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      build_xsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
>  
>      /* RSDP is in FSEG memory, so allocate it separately */
> -    build_rsdp(tables->rsdp, tables->linker, xsdt);
> +    {
> +        AcpiRsdpData rsdp_data = {
> +            .revision = 2,
> +            .oem_id = ACPI_BUILD_APPNAME6,
> +            .xsdt_tbl_offset = &xsdt,
> +            .rsdt_tbl_offset = NULL,
> +        };
> +        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> +    }
>  
>      /* Cleanup memory that's no longer used. */
>      g_array_free(table_offsets, true);

Re: [Qemu-devel] [PATCH v2 5/8] hw: arm: Convert the RSDP build to the buid_append_foo() API

[Igor Mammedov]

On Thu, 29 Nov 2018 14:24:25 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> Instead of filling a mapped and packed C structure field in random order
> and being careful about endianness and sizes, build_rsdp() now uses
> build_append_int_noprefix() to compose RSDP table.
> 
> This makes for an easier to review and maintain code that is almost
> matching 1:1 the ACPI spec itself.
nit if you happen to respin:
 This makes review and maintaining code easier and ..

> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  hw/arm/virt-acpi-build.c | 42 +++++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index ce8bfa5a37..4782aea4fe 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -368,35 +368,37 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>  
>  /* RSDP */
>  static void
> -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> +build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
>  {
> -    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> -    unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> -    unsigned xsdt_pa_offset =
> -        (char *)&rsdp->xsdt_physical_address - rsdp_table->data;
> -
> -    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
> +    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, tbl, 16,
>                               true /* fseg memory */);
>  
> -    memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> -    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
> -    rsdp->length = cpu_to_le32(sizeof(*rsdp));
> -    rsdp->revision = rsdp_data->revision;
> +    g_array_append_vals(tbl, "RSD PTR ", 8); /* Signature */
> +    build_append_int_noprefix(tbl, 0, 1); /* Checksum */
> +    g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
> +    build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
> +    build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
> +    build_append_int_noprefix(tbl, 36, 4); /* Length */
>  
> -    /* Address to be filled by Guest linker */
> -    bios_linker_loader_add_pointer(linker,
> -        ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> -        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
> +    /* XSDT address to be filled by guest linker */
> +    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
> +                                   24, 8,
> +                                   ACPI_BUILD_TABLE_FILE,
> +                                   *rsdp_data->xsdt_tbl_offset);
>  
> -    /* Checksum to be filled by Guest linker */
> +    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> +    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> +
> +    /* Checksum to be filled by guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> -        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
this is always 0 with all current users and you a replacing it with 0
constant. That's fine but that assumes that above condition stays always true.
I'd add assert at the beginning of function to make sure that
tbl length is 0
or as an alternative
 build_rsdp(...)
 {
   start_off = tbl->len;
   ...
   bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, start_off, 20 ...

so it won't explode even if RSDP isn't at the beginning of file.
If you go for the later than bios_linker_loader_add_pointer() will also need similar treatment

> -        (char *)&rsdp->checksum - rsdp_table->data);
> +                                    0, 20, /* ACPI rev 1.0 RSDP size */
> +                                    8);
>  
>      /* Extended checksum to be filled by Guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> -        (char *)rsdp - rsdp_table->data, 36 /* ACPI rev 2.0 RSDP size */,
> -        (char *)&rsdp->extended_checksum - rsdp_table->data);
> +                                    0, 36, /* ACPI rev 2.0 RSDP size */
> +                                    32);
>  }
>  
>  static void

Re: [Qemu-devel] [PATCH v2 8/8] hw: acpi: Remove AcpiRsdpDescriptor and fix tests

[Samuel Ortiz]

On Thu, Nov 29, 2018 at 04:07:43PM +0100, Andrew Jones wrote:
> On Thu, Nov 29, 2018 at 02:24:28PM +0100, Samuel Ortiz wrote:
> > The only remaining AcpiRsdpDescriptor users are the ACPI utils for the
> > BIOS table tests.
> > We remove that dependency and can thus remove the structure itself.
> > 
> > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > ---
> >  include/hw/acpi/acpi-defs.h | 13 -----------
> >  tests/acpi-utils.h          |  5 +++-
> >  tests/acpi-utils.c          | 46 ++++++++++++++++++++++++++++++-------
> >  tests/bios-tables-test.c    | 27 +++++++++++++++-------
> >  tests/vmgenid-test.c        |  8 ++++---
> >  5 files changed, 66 insertions(+), 33 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 8425ecb8c6..5021cb9e79 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -40,19 +40,6 @@ enum {
> >      ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE,
> >  };
> >  
> > -struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
> > -    uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
> > -    uint8_t  checksum;               /* To make sum of struct == 0 */
> > -    uint8_t  oem_id [6];             /* OEM identification */
> > -    uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
> > -    uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT */
> > -    uint32_t length;                 /* XSDT Length in bytes including hdr */
> > -    uint64_t xsdt_physical_address;  /* 64-bit physical address of XSDT */
> > -    uint8_t  extended_checksum;      /* Checksum of entire table */
> > -    uint8_t  reserved [3];           /* Reserved field must be 0 */
> > -} QEMU_PACKED;
> > -typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
> > -
> >  typedef struct AcpiRsdpData {
> >      uint8_t oem_id[6]; /* OEM identification */
> >      uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
> > diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> > index ac52abd0dd..c26c8976a6 100644
> > --- a/tests/acpi-utils.h
> > +++ b/tests/acpi-utils.h
> > @@ -82,6 +82,9 @@ typedef struct {
> >  
> >  uint8_t acpi_calc_checksum(const uint8_t *data, int len);
> >  uint32_t acpi_find_rsdp_address(void);
> > -void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table);
> > +uint32_t acpi_get_rsdt_address(uint8_t *rsdp_table);
> > +uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table);
> > +void acpi_parse_rsdp_table(uint32_t addr, uint8_t *rsdp_table,
> > +                           uint8_t revision);
> >  
> >  #endif  /* TEST_ACPI_UTILS_H */
> > diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
> > index 41dc1ea9b4..b1113bda8b 100644
> > --- a/tests/acpi-utils.c
> > +++ b/tests/acpi-utils.c
> > @@ -52,14 +52,44 @@ uint32_t acpi_find_rsdp_address(void)
> >      return off;
> >  }
> >  
> > -void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table)
> > +uint32_t acpi_get_rsdt_address(uint8_t *rsdp_table)
> >  {
> > -    ACPI_READ_FIELD(rsdp_table->signature, addr);
> > -    ACPI_ASSERT_CMP64(rsdp_table->signature, "RSD PTR ");
> > +    uint32_t rsdt_physical_address;
> > +    uint8_t revision = rsdp_table[15 /* Revision offset */];
> >  
> > -    ACPI_READ_FIELD(rsdp_table->checksum, addr);
> > -    ACPI_READ_ARRAY(rsdp_table->oem_id, addr);
> > -    ACPI_READ_FIELD(rsdp_table->revision, addr);
> > -    ACPI_READ_FIELD(rsdp_table->rsdt_physical_address, addr);
> > -    ACPI_READ_FIELD(rsdp_table->length, addr);
> > +    if (revision != 0) { /* ACPI 1.0 RSDP */
> > +        return 0;
> > +    }
> 
> So now we assume a zero offset can't be valid, which means we don't need
> pointers in the struct. Your answer that pointers are used for consistency
> is good enough for me though.
> 
> > +
> > +    memcpy(&rsdt_physical_address, &rsdp_table[16 /* RsdtAddress offset */], 4);
> > +    return le32_to_cpu(rsdt_physical_address);
> > +}
> > +
> > +uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table)
> > +{
> > +    uint64_t xsdt_physical_address;
> > +    uint8_t revision = rsdp_table[15 /* Revision offset */];
> > +
> > +    if (revision != 2) { /* ACPI 2.0+ RSDP */
> 
> revision < 2
> 
> > +        return 0;
> > +    }
> > +
> > +    memcpy(&xsdt_physical_address, &rsdp_table[24 /* XsdtAddress offset */], 8);
> > +    return le64_to_cpu(xsdt_physical_address);
> > +}
> > +
> > +void acpi_parse_rsdp_table(uint32_t addr, uint8_t *rsdp_table, uint8_t revision)
> > +{
> > +    switch (revision) {
> > +    case 0: /* ACPI 1.0 RSDP */
> > +        memread(addr, rsdp_table, 20);
> > +        break;
> > +    case 2: /* ACPI 2.0+ RSDP */
> > +        memread(addr, rsdp_table, 36);
> > +        break;
> > +    default:
> > +        g_assert_not_reached();
> > +    }
> > +
> > +    ACPI_ASSERT_CMP64(*((uint64_t *)(rsdp_table)), "RSD PTR ");
> >  }
> > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > index d661d9be62..84f1500920 100644
> > --- a/tests/bios-tables-test.c
> > +++ b/tests/bios-tables-test.c
> > @@ -27,7 +27,8 @@ typedef struct {
> >      const char *machine;
> >      const char *variant;
> >      uint32_t rsdp_addr;
> > -    AcpiRsdpDescriptor rsdp_table;
> > +    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
> > +    uint32_t rsdt_physical_address;
> 
> I don't see rsdt_physical_address getting used anyway.
Good catch, thanks. Removed in v3.

Cheers,
Samuel.

Re: [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build

[Igor Mammedov]

On Thu, 29 Nov 2018 14:24:26 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> We add the ability to build legacy or current RSDP tables, based on the
> AcpiRsdpData revision field passed to build_rsdp().
> Although arm/virt only uses RSDP v2, adding that capability to
> build_rsdp will allow us to share the RSDP build code between ARM and x86.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 4782aea4fe..e1338b6f5a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
>      g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
>      build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
>      build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
> -    build_append_int_noprefix(tbl, 36, 4); /* Length */
> -
> -    /* XSDT address to be filled by guest linker */
> -    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> -    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
> -                                   24, 8,
> -                                   ACPI_BUILD_TABLE_FILE,
> -                                   *rsdp_data->xsdt_tbl_offset);
> -
> -    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> -    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> +    if (rsdp_data->rsdt_tbl_offset) {
> +        /* RSDT address to be filled by guest linker */
> +        bios_linker_loader_add_pointer(linker,
> +                                       ACPI_BUILD_RSDP_FILE, 16, 4,
> +                                       ACPI_BUILD_TABLE_FILE,
> +                                       *rsdp_data->rsdt_tbl_offset);
> +    }
>  
>      /* Checksum to be filled by guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>                                      0, 20, /* ACPI rev 1.0 RSDP size */
>                                      8);
>  
> +    if (rsdp_data->revision == 0) {
> +        /* ACPI 1.0 RSDP, we're done */
> +        return;
> +    }
> +
> +    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
> +    g_assert(rsdp_data->xsdt_tbl_offset != NULL);
> +
> +    build_append_int_noprefix(tbl, 36, 4); /* Length */
> +
> +    /* XSDT address to be filled by guest linker */
> +    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE, 24, 8,
> +                                   ACPI_BUILD_TABLE_FILE,
> +                                   *rsdp_data->xsdt_tbl_offset);
> +
> +    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> +    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> +
>      /* Extended checksum to be filled by Guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>                                      0, 36, /* ACPI rev 2.0 RSDP size */

Re: [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build

[Igor Mammedov]

On Thu, 29 Nov 2018 15:59:42 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> On Thu, Nov 29, 2018 at 03:42:43PM +0100, Andrew Jones wrote:
> > On Thu, Nov 29, 2018 at 02:24:26PM +0100, Samuel Ortiz wrote:  
> > > We add the ability to build legacy or current RSDP tables, based on the
> > > AcpiRsdpData revision field passed to build_rsdp().
> > > Although arm/virt only uses RSDP v2, adding that capability to
> > > build_rsdp will allow us to share the RSDP build code between ARM and x86.
> > > 
> > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > ---
> > >  hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++-----------
> > >  1 file changed, 26 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 4782aea4fe..e1338b6f5a 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> > >      g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
> > >      build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
> > >      build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
> > > -    build_append_int_noprefix(tbl, 36, 4); /* Length */
> > > -
> > > -    /* XSDT address to be filled by guest linker */
> > > -    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> > > -    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
> > > -                                   24, 8,
> > > -                                   ACPI_BUILD_TABLE_FILE,
> > > -                                   *rsdp_data->xsdt_tbl_offset);
> > > -
> > > -    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> > > -    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> > > +    if (rsdp_data->rsdt_tbl_offset) {  
> > 
> > I see why a pointer was used now. Using a pointer ensures a zero
> > offset won't fail this test. However the test could be replaced with
> > rsdp_data->revision == 0.
> >   
> > > +        /* RSDT address to be filled by guest linker */
> > > +        bios_linker_loader_add_pointer(linker,
> > > +                                       ACPI_BUILD_RSDP_FILE, 16, 4,
> > > +                                       ACPI_BUILD_TABLE_FILE,
> > > +                                       *rsdp_data->rsdt_tbl_offset);
> > > +    }
> > >  
> > >      /* Checksum to be filled by guest linker */
> > >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > >                                      0, 20, /* ACPI rev 1.0 RSDP size */
> > >                                      8);
> > >  
> > > +    if (rsdp_data->revision == 0) {
> > > +        /* ACPI 1.0 RSDP, we're done */
> > > +        return;
> > > +    }
> > > +
> > > +    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
> > > +    g_assert(rsdp_data->xsdt_tbl_offset != NULL);  
> > 
> > So here's the justification for the pointers. We sanity check the callers.  
> We could sanity check the callers without pointers as well, I don't
> think there's a strong advantage for pointers here, except consistence.
I remember now,
we need pointer to RSDT offset here so that we could skip bios_linker_loader_add_pointer(rsdt)
when RSDT isn't provided (0 is also a valid offset), as for xsdt field it follows
rsdt one for consistency and we can do sanity checks on it.

assert here seems not really necessary as it guarantied to SIGSEGV
on NULL point dereference at bios_linker_loader_add_pointer(*xdst) callsite.
But I don't care here, so I'll leave it up to Samuel.

> > We're missing the (rsdp_data->revision == 0 && rsdp_data->rsdt_tbl_offset)
> > sanity check though.  
> I think there's nothing preventing a caller to include both rsdt and
> xsdt if it wants to be able to run on both < 2.0 and 2.0+ platforms with
> the same table. So if rsdt is set we should add it, regardless of the revision.
agreed,
we don't do this in QEMU but it's correct, there could be both and it's better
follow better document spec in this case.

> 
> Cheers,
> Samuel.

Re: [Qemu-devel] [PATCH v2 7/8] hw: acpi: Export and share the ARM RSDP build

[Igor Mammedov]

On Thu, 29 Nov 2018 14:24:27 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> Now that build_rsdp() supports building both legacy and current RSDP
> tables, we can move it to a generic folder (hw/acpi) and have the i386
> ACPI code reuse it in order to reduce code duplication.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
with nit below fixed

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  include/hw/acpi/aml-build.h |  2 ++
>  hw/acpi/aml-build.c         | 50 +++++++++++++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c    | 50 -------------------------------------
>  hw/i386/acpi-build.c        | 39 +++++++----------------------
>  4 files changed, 61 insertions(+), 80 deletions(-)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 6c36903c0a..1a563ad756 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -388,6 +388,8 @@ void acpi_add_table(GArray *table_offsets, GArray *table_data);
>  void acpi_build_tables_init(AcpiBuildTables *tables);
>  void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre);
>  void
> +build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data);
> +void
>  build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
>             const char *oem_id, const char *oem_table_id);
>  void
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 1e43cd736d..3d5f77e02b 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1589,6 +1589,56 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
>      g_array_free(tables->vmgenid, mfre);
>  }
>  
> +/* RSDP */
> +void
> +build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> +{
> +    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, tbl, 16,
> +                             true /* fseg memory */);
> +
> +    g_array_append_vals(tbl, "RSD PTR ", 8); /* Signature */
> +    build_append_int_noprefix(tbl, 0, 1); /* Checksum */
> +    g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
> +    build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
> +    build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
> +    if (rsdp_data->rsdt_tbl_offset) {
> +        /* RSDT address to be filled by guest linker */
> +        bios_linker_loader_add_pointer(linker,
> +                                       ACPI_BUILD_RSDP_FILE, 16, 4,
> +                                       ACPI_BUILD_TABLE_FILE,
> +                                       *rsdp_data->rsdt_tbl_offset);
> +    }
> +
> +    /* Checksum to be filled by guest linker */
> +    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> +                                    0, 20, /* ACPI rev 1.0 RSDP size */
> +                                    8);
> +
> +    if (rsdp_data->revision == 0) {
> +        /* ACPI 1.0 RSDP, we're done */
> +        return;
> +    }
> +
> +    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
> +    g_assert(rsdp_data->xsdt_tbl_offset != NULL);
> +
> +    build_append_int_noprefix(tbl, 36, 4); /* Length */
> +
> +    /* XSDT address to be filled by guest linker */
> +    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE, 24, 8,
> +                                   ACPI_BUILD_TABLE_FILE,
> +                                   *rsdp_data->xsdt_tbl_offset);
> +
> +    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> +    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> +
> +    /* Extended checksum to be filled by Guest linker */
> +    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> +                                    0, 36, /* ACPI rev 2.0 RSDP size */
> +                                    32);
> +}
> +
>  /* Build rsdt table */
>  void
>  build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index e1338b6f5a..95fad6f0ce 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -366,56 +366,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>      aml_append(scope, dev);
>  }
>  
> -/* RSDP */
I'd make comment more descriptive, see build_slit() for example

> -static void
> -build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> -{
> -    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, tbl, 16,
> -                             true /* fseg memory */);
> -
> -    g_array_append_vals(tbl, "RSD PTR ", 8); /* Signature */
> -    build_append_int_noprefix(tbl, 0, 1); /* Checksum */
> -    g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
> -    build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
> -    build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
> -    if (rsdp_data->rsdt_tbl_offset) {
> -        /* RSDT address to be filled by guest linker */
> -        bios_linker_loader_add_pointer(linker,
> -                                       ACPI_BUILD_RSDP_FILE, 16, 4,
> -                                       ACPI_BUILD_TABLE_FILE,
> -                                       *rsdp_data->rsdt_tbl_offset);
> -    }
> -
> -    /* Checksum to be filled by guest linker */
> -    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> -                                    0, 20, /* ACPI rev 1.0 RSDP size */
> -                                    8);
> -
> -    if (rsdp_data->revision == 0) {
> -        /* ACPI 1.0 RSDP, we're done */
> -        return;
> -    }
> -
> -    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
> -    g_assert(rsdp_data->xsdt_tbl_offset != NULL);
> -
> -    build_append_int_noprefix(tbl, 36, 4); /* Length */
> -
> -    /* XSDT address to be filled by guest linker */
> -    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> -    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE, 24, 8,
> -                                   ACPI_BUILD_TABLE_FILE,
> -                                   *rsdp_data->xsdt_tbl_offset);
> -
> -    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> -    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> -
> -    /* Extended checksum to be filled by Guest linker */
> -    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> -                                    0, 36, /* ACPI rev 2.0 RSDP size */
> -                                    32);
> -}
> -
>  static void
>  build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index fb877648ac..528a5fd4ed 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2547,35 +2547,6 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
>                   "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
>  }
>  
> -static void
> -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
> -{
> -    /* AcpiRsdpDescriptor describes revision 2 RSDP table and as result we
> -     * allocate extra 16 bytes for pc/q35 RSDP rev1 as well. Keep extra 16 bytes
> -     * wasted to make sure we won't breake migration for machine types older
> -     * than 2.3 due to size mismatch.
> -     */
> -    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> -    unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
> -    unsigned rsdt_pa_offset =
> -        (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
> -
> -    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
> -                             true /* fseg memory */);
> -
> -    memcpy(&rsdp->signature, "RSD PTR ", 8);
> -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
> -    /* Address to be filled by Guest linker */
> -    bios_linker_loader_add_pointer(linker,
> -        ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
> -        ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
> -
> -    /* Checksum to be filled by Guest linker */
> -    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> -        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
> -        (char *)&rsdp->checksum - rsdp_table->data);
> -}
> -
>  typedef
>  struct AcpiBuildState {
>      /* Copy of table in RAM (for patching). */
> @@ -2732,7 +2703,15 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>                 slic_oem.id, slic_oem.table_id);
>  
>      /* RSDP is in FSEG memory, so allocate it separately */
> -    build_rsdp(tables->rsdp, tables->linker, rsdt);
> +    {
> +        AcpiRsdpData rsdp_data = {
> +            .revision = 0,
> +            .oem_id = ACPI_BUILD_APPNAME6,
> +            .xsdt_tbl_offset = NULL,
> +            .rsdt_tbl_offset = &rsdt,
> +        };
> +        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> +    }
>  
>      /* We'll expose it all to Guest so we want to reduce
>       * chance of size changes.

Re: [Qemu-devel] [PATCH v2 8/8] hw: acpi: Remove AcpiRsdpDescriptor and fix tests

[Igor Mammedov]

On Thu, 29 Nov 2018 14:24:28 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> The only remaining AcpiRsdpDescriptor users are the ACPI utils for the
> BIOS table tests.
> We remove that dependency and can thus remove the structure itself.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  include/hw/acpi/acpi-defs.h | 13 -----------
>  tests/acpi-utils.h          |  5 +++-
>  tests/acpi-utils.c          | 46 ++++++++++++++++++++++++++++++-------
>  tests/bios-tables-test.c    | 27 +++++++++++++++-------
>  tests/vmgenid-test.c        |  8 ++++---
>  5 files changed, 66 insertions(+), 33 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 8425ecb8c6..5021cb9e79 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -40,19 +40,6 @@ enum {
>      ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE,
>  };
>  
> -struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
> -    uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
> -    uint8_t  checksum;               /* To make sum of struct == 0 */
> -    uint8_t  oem_id [6];             /* OEM identification */
> -    uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
> -    uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT */
> -    uint32_t length;                 /* XSDT Length in bytes including hdr */
> -    uint64_t xsdt_physical_address;  /* 64-bit physical address of XSDT */
> -    uint8_t  extended_checksum;      /* Checksum of entire table */
> -    uint8_t  reserved [3];           /* Reserved field must be 0 */
> -} QEMU_PACKED;
> -typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
> -
>  typedef struct AcpiRsdpData {
>      uint8_t oem_id[6]; /* OEM identification */
>      uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
> diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> index ac52abd0dd..c26c8976a6 100644
> --- a/tests/acpi-utils.h
> +++ b/tests/acpi-utils.h
> @@ -82,6 +82,9 @@ typedef struct {
>  
>  uint8_t acpi_calc_checksum(const uint8_t *data, int len);
>  uint32_t acpi_find_rsdp_address(void);
> -void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table);
> +uint32_t acpi_get_rsdt_address(uint8_t *rsdp_table);
> +uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table);
> +void acpi_parse_rsdp_table(uint32_t addr, uint8_t *rsdp_table,
> +                           uint8_t revision);
>  
>  #endif  /* TEST_ACPI_UTILS_H */
> diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
> index 41dc1ea9b4..b1113bda8b 100644
> --- a/tests/acpi-utils.c
> +++ b/tests/acpi-utils.c
> @@ -52,14 +52,44 @@ uint32_t acpi_find_rsdp_address(void)
>      return off;
>  }
>  
> -void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table)
> +uint32_t acpi_get_rsdt_address(uint8_t *rsdp_table)
>  {
> -    ACPI_READ_FIELD(rsdp_table->signature, addr);
> -    ACPI_ASSERT_CMP64(rsdp_table->signature, "RSD PTR ");
> +    uint32_t rsdt_physical_address;
> +    uint8_t revision = rsdp_table[15 /* Revision offset */];
>  
> -    ACPI_READ_FIELD(rsdp_table->checksum, addr);
> -    ACPI_READ_ARRAY(rsdp_table->oem_id, addr);
> -    ACPI_READ_FIELD(rsdp_table->revision, addr);
> -    ACPI_READ_FIELD(rsdp_table->rsdt_physical_address, addr);
> -    ACPI_READ_FIELD(rsdp_table->length, addr);
> +    if (revision != 0) { /* ACPI 1.0 RSDP */
> +        return 0;
> +    }
It seems not needed,
Why not return whatever is in table?

> +
> +    memcpy(&rsdt_physical_address, &rsdp_table[16 /* RsdtAddress offset */], 4);
> +    return le32_to_cpu(rsdt_physical_address);
> +}
> +
> +uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table)
> +{
> +    uint64_t xsdt_physical_address;
> +    uint8_t revision = rsdp_table[15 /* Revision offset */];
> +
> +    if (revision != 2) { /* ACPI 2.0+ RSDP */
maybe replace this with assert, if we are getting XSDT, revision must be 2 or more

> +        return 0;
> +    }
> +
> +    memcpy(&xsdt_physical_address, &rsdp_table[24 /* XsdtAddress offset */], 8);
> +    return le64_to_cpu(xsdt_physical_address);
> +}
> +
> +void acpi_parse_rsdp_table(uint32_t addr, uint8_t *rsdp_table, uint8_t revision)
> +{
why do we need pass in revision?
It would be better if we read sub-table first, then parse revision
and then read the rest if necessary.

Then it's up to a caller what do with the table that  we've fetched here

> +    switch (revision) {
> +    case 0: /* ACPI 1.0 RSDP */
> +        memread(addr, rsdp_table, 20);
> +        break;
> +    case 2: /* ACPI 2.0+ RSDP */
> +        memread(addr, rsdp_table, 36);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    ACPI_ASSERT_CMP64(*((uint64_t *)(rsdp_table)), "RSD PTR ");
>  }
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index d661d9be62..84f1500920 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -27,7 +27,8 @@ typedef struct {
>      const char *machine;
>      const char *variant;
>      uint32_t rsdp_addr;
> -    AcpiRsdpDescriptor rsdp_table;
> +    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
> +    uint32_t rsdt_physical_address;
>      AcpiRsdtDescriptorRev1 rsdt_table;
>      uint32_t dsdt_addr;
>      uint32_t facs_addr;
> @@ -83,21 +84,31 @@ static void test_acpi_rsdp_address(test_data *data)
>      data->rsdp_addr = off;
>  }
>  
> -static void test_acpi_rsdp_table(test_data *data)
> +static void test_acpi_rsdp_table(test_data *data, uint8_t revision)
like wise, I'd drop revision here and use whatever is in the table
if we need to test for specific revision in the future we can add
test case for that then.

>  {
> -    AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table;
> +    uint8_t *rsdp_table = data->rsdp_table;
>      uint32_t addr = data->rsdp_addr;
>  
> -    acpi_parse_rsdp_table(addr, rsdp_table);
> +    acpi_parse_rsdp_table(addr, rsdp_table, revision);
>  
> -    /* rsdp checksum is not for the whole table, but for the first 20 bytes */
> -    g_assert(!acpi_calc_checksum((uint8_t *)rsdp_table, 20));
> +    switch (revision) {
> +    case 0: /* ACPI 1.0 RSDP */
> +        /* With rev 1, checksum is only for the first 20 bytes */
> +        g_assert(!acpi_calc_checksum(rsdp_table,  20));
> +        break;
> +    case 2: /* ACPI 2.0+ RSDP */
> +        /* With revision 2, we have 2 checksums */
> +        g_assert(!acpi_calc_checksum(rsdp_table, 20));
> +        g_assert(!acpi_calc_checksum(rsdp_table, 36));
> +    default:
> +        g_assert_not_reached();
> +    }
>  }
>  
>  static void test_acpi_rsdt_table(test_data *data)
>  {
>      AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
> -    uint32_t addr = le32_to_cpu(data->rsdp_table.rsdt_physical_address);
> +    uint32_t addr = acpi_get_rsdt_address(data->rsdp_table);
>      uint32_t *tables;
>      int tables_nr;
>      uint8_t checksum;
> @@ -626,7 +637,7 @@ static void test_acpi_one(const char *params, test_data *data)
>  
>      data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
>      test_acpi_rsdp_address(data);
> -    test_acpi_rsdp_table(data);
> +    test_acpi_rsdp_table(data, 0); /* ACPI 1.0 RSDP */
>      test_acpi_rsdt_table(data);
>      fadt_fetch_facs_and_dsdt_ptrs(data);
>      test_acpi_facs_table(data);
> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
> index 0a6fb55f2e..5f30dee059 100644
> --- a/tests/vmgenid-test.c
> +++ b/tests/vmgenid-test.c
> @@ -35,7 +35,7 @@ static uint32_t acpi_find_vgia(void)
>  {
>      uint32_t rsdp_offset;
>      uint32_t guid_offset = 0;
> -    AcpiRsdpDescriptor rsdp_table;
> +    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
>      uint32_t rsdt, rsdt_table_length;
>      AcpiRsdtDescriptorRev1 rsdt_table;
>      size_t tables_nr;
> @@ -52,9 +52,11 @@ static uint32_t acpi_find_vgia(void)
>  
>      g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
>  
> -    acpi_parse_rsdp_table(rsdp_offset, &rsdp_table);
> +    acpi_parse_rsdp_table(rsdp_offset, rsdp_table, 0);
> +
> +    rsdt = acpi_get_rsdt_address(rsdp_table);
> +    g_assert(rsdt);
>  
> -    rsdt = le32_to_cpu(rsdp_table.rsdt_physical_address);
>      /* read the header */
>      ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
>      ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");

Re: [Qemu-devel] [PATCH v2 0/8] hw: acpi: RSDP fixes and refactoring

[Igor Mammedov]

On Thu, 29 Nov 2018 14:24:20 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> This patch serie fixes a couple of RSDP checksum related issues:
> 
> - On RSDP rev2, we are not adding the extended checksum and computing
>   the checksum on the full table instead of the first 20 bytes.
> - On RSDP rev1, we are computing the checksum on 36 bytes instead of 20.
>   We're lucky enough that this is only adding zeroes to the checksum.
> 
> A guest Linux kernel does not seem to care about RSDP checksums, so
> those 2 fixes are mostly for correctness sake.
> Any machine type that generates rev2 RSDP will see its RSDP table
> modified but since x86 builds RSDP v1 and all ACPI tests only run on
> either pc or q35, the ACPI tests tables are not affected by this fix.
> 
> The serie also extends the ARM virt ACPI RSDP build routine to support
> both RSDP v1 and v2, in order to share this code between x86 and
> aarch64. While extending, we also convert the routine to the latest
> build_append_foo() API. The new implementation is a closer reflection of
> the ACPI spec itself, is endian agnostic and allows for getting rid of the
> AcpiRsdpDescriptor structure.

Series needs to fix up minor issues but otherwise it looks good.

> v1 -> v2:
>   * Add missing SOBs.
>   * Rephrase some commit messages according to the maintainer's input.
>   * Replace ACPI spec definitions with hard coded constants.
>   * Replace init_rsdp_data() helper with open coded rsdp variable init.
>   * Rename acpi_find_[rx]sdt_address to acpi_get_[rx]sdt_address.
> 
> Igor Mammedov (2):
>   hw: arm: acpi: Fix incorrect checksums in RSDP
>   hw: i386: Use correct RSDT length for checksum
> 
> Samuel Ortiz (6):
>   hw: acpi: The RSDP build API can return void
>   hw: arm: Carry RSDP specific data through AcpiRsdpData
>   hw: arm: Convert the RSDP build to the buid_append_foo() API
>   hw: arm: Support both legacy and current RSDP build
>   hw: acpi: Export and share the ARM RSDP build
>   hw: acpi: Remove AcpiRsdpDescriptor and fix tests
> 
>  include/hw/acpi/acpi-defs.h | 19 ++++++--------
>  include/hw/acpi/aml-build.h |  2 ++
>  tests/acpi-utils.h          |  5 +++-
>  hw/acpi/aml-build.c         | 50 +++++++++++++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c    | 40 +++++++----------------------
>  hw/i386/acpi-build.c        | 36 +++++++-------------------
>  tests/acpi-utils.c          | 46 ++++++++++++++++++++++++++++------
>  tests/bios-tables-test.c    | 27 ++++++++++++++------
>  tests/vmgenid-test.c        |  8 +++---
>  9 files changed, 143 insertions(+), 90 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData

[Andrew Jones]

On Fri, Nov 30, 2018 at 08:26:21AM +0100, Igor Mammedov wrote:
> On Thu, 29 Nov 2018 15:26:27 +0100
> Samuel Ortiz <sameo@linux.intel.com> wrote:
> 
> > On Thu, Nov 29, 2018 at 03:09:32PM +0100, Andrew Jones wrote:
> > > On Thu, Nov 29, 2018 at 02:24:24PM +0100, Samuel Ortiz wrote:  
> > > > That will allow us to generalize the ARM build_rsdp() routine to support
> > > > both legacy RSDP (The current i386 implementation) and extended RSDP
> > > > (The ARM implementation).
> > > > 
> > > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > > ---
> > > >  include/hw/acpi/acpi-defs.h |  8 ++++++++
> > > >  hw/arm/virt-acpi-build.c    | 18 +++++++++++++-----
> > > >  2 files changed, 21 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > > index af8e023968..8425ecb8c6 100644
> > > > --- a/include/hw/acpi/acpi-defs.h
> > > > +++ b/include/hw/acpi/acpi-defs.h
> > > > @@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
> > > >  } QEMU_PACKED;
> > > >  typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
> > > >  
> > > > +typedef struct AcpiRsdpData {
> > > > +    uint8_t oem_id[6]; /* OEM identification */
> > > > +    uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
> > > > +
> > > > +    unsigned *rsdt_tbl_offset;
> > > > +    unsigned *xsdt_tbl_offset;  
> > > 
> > > Why use pointers?  
> > Mostly to be consistent with the FADT data structure (AcpiFadtData).
> My guess would be that's for detecting an optional field and
> skipping setting it in build_rsdp(). But it's mostly for
> consistence with current code.
> 
> > 
> > > > +} AcpiRsdpData;
> > > > +
> > > >  /* Table structure from Linux kernel (the ACPI tables are under the
> > > >     BSD license) */
> > > >  
> > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > index 0835900052..ce8bfa5a37 100644
> > > > --- a/hw/arm/virt-acpi-build.c
> > > > +++ b/hw/arm/virt-acpi-build.c
> > > > @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > > >  
> > > >  /* RSDP */
> > > >  static void
> > > > -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > > +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> > > >  {
> > > >      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> > > >      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> > > > @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > >                               true /* fseg memory */);
> > > >  
> > > >      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> > > > -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> > > > +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));  
> > > 
> > > sizeof(rsdp_data->oem_id)  
> > Which is the same thing. Note that this piece of code eventually gets
> > removed later in the serie.
> > 
> > 
> > 
> > > >      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> > > > -    rsdp->revision = 0x02;
> > > > +    rsdp->revision = rsdp_data->revision;
> > > >  
> > > >      /* Address to be filled by Guest linker */
> > > >      bios_linker_loader_add_pointer(linker,
> > > >          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> > > > -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> > > > +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
> > > >  
> > > >      /* Checksum to be filled by Guest linker */
> > > >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > > > @@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> > > >      build_xsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> > > >  
> > > >      /* RSDP is in FSEG memory, so allocate it separately */
> > > > -    build_rsdp(tables->rsdp, tables->linker, xsdt);
> > > > +    {
> > > > +        AcpiRsdpData rsdp_data = {
> > > > +            .revision = 2,
> > > > +            .oem_id = ACPI_BUILD_APPNAME6,
> > > > +            .xsdt_tbl_offset = &xsdt,
> > > > +            .rsdt_tbl_offset = NULL,  
> > > 
> > > nit: no need for this explicit NULLing  
> then one would get random junk from the stack this variable is allocate from
> in this field.

Not when you initialize the structure with = { ... }. All uninitialized
pointers will be NULL and all uninitialized arithmetic types will be zero.
Of course explicitly setting it to NULL is fine too, which is why this is
only a "nit".

> 
> > Unfortunately, yes.
> > 
> > Cheers,
> > Samuel.
> 
>