hw: acpi: RSDP fixes and refactoring

PATCH In review Samuel Ortiz 4 participants 20 messages Dec 17, 2018 – Dec 18, 2018

lore

Share & tools

lore

Samuel Ortiz

author

2018-12-17 10:59 UTC #0 lore raw

v4 cover hw: acpi: RSDP fixes and refactoring

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.

V3 -> V4:
* Add missing Reviewed-Bys from Igor and Andrew.
* Removed unneeded white space addition.
* Added 16 bytes padding to revision 0 RSDP structures for legacy (2.2 and older) machine types.
* Fixed a missing break in an ACPI RSDP test switch.

V2 -> V3:
* Add Reviewed-Bys from Igor and Andrew.
* Check for valid RSDP and XSDT pointers depending on the revision.
* Use RSDP initial table len as offset for checksum and pointer insertion.
* Do not explictly pass RSDP revision as the RSDP table parsing test utility.

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          |  4 ++-
 hw/acpi/aml-build.c         | 68 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 40 +++++-----------------
 hw/i386/acpi-build.c        | 46 +++++++++++--------------
 tests/acpi-utils.c          | 46 ++++++++++++++++++++-----
 tests/bios-tables-test.c    | 22 +++++++++---
 tests/vmgenid-test.c        |  8 +++--
 9 files changed, 168 insertions(+), 87 deletions(-)

Signature · 1 line

2.19.2

Samuel Ortiz

author

2018-12-17 10:59 UTC #1 lore raw

PATCH v4 1/8 hw: acpi: The RSDP build API can return void

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 <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Tested-by Philippe Mathieu-Daudé <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

---
 hw/arm/virt-acpi-build.c | 4 +---
 hw/i386/acpi-build.c     | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

hw/arm/virt-acpi-build.c +1 -3

index 5785fb697c..fcaa350892 100644
@@ -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

hw/i386/acpi-build.c +1 -3

index 236a20eaa8..35f17d0d91 100644
@@ -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

Signature · 1 line

2.19.2

Samuel Ortiz

author

2018-12-17 10:59 UTC #2 lore raw

PATCH v4 2/8 hw: arm: acpi: Fix incorrect checksums in RSDP

In reply to Samuel Ortiz

From: Igor Mammedov <[email protected]>

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 <[email protected]>

CC Ard Biesheuvel <[email protected]>

CC Shannon Zhao <[email protected]>

Reviewed-by Samuel Ortiz <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Samuel Ortiz <[email protected]>

---
 hw/arm/virt-acpi-build.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

hw/arm/virt-acpi-build.c +6 -1

index fcaa350892..0835900052 100644
@@ -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

Signature · 1 line

2.19.2

Samuel Ortiz

author

2018-12-17 10:59 UTC #3 lore raw

PATCH v4 3/8 hw: i386: Use correct RSDT length for checksum

In reply to Samuel Ortiz

From: Igor Mammedov <[email protected]>

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 <[email protected]>

Reviewed-by Samuel Ortiz <[email protected]>

Signed-off-by Samuel Ortiz <[email protected]>

---
 hw/i386/acpi-build.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

hw/i386/acpi-build.c +6 -1

index 35f17d0d91..fb877648ac 100644
@@ -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);
 }

Signature · 1 line

2.19.2

Samuel Ortiz

author

2018-12-17 10:59 UTC #4 lore raw

PATCH v4 4/8 hw: arm: Carry RSDP specific data through AcpiRsdpData

In reply to 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 <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

---
 include/hw/acpi/acpi-defs.h |  8 ++++++++
 hw/arm/virt-acpi-build.c    | 18 +++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

include/hw/acpi/acpi-defs.h +8 -0

index af8e023968..8425ecb8c6 100644
@@ -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) */

hw/arm/virt-acpi-build.c +13 -5

index 0835900052..ce8bfa5a37 100644
@@ -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);

Signature · 1 line

2.19.2

Samuel Ortiz

author

2018-12-17 10:59 UTC #5 lore raw

PATCH v4 5/8 hw: arm: Convert the RSDP build to the buid_append_foo() API

In reply to 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 reviewing and maintaining code easier as this is almost matching 1:1 the ACPI spec itself.

Signed-off-by Samuel Ortiz <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

---
 hw/arm/virt-acpi-build.c | 42 ++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

hw/arm/virt-acpi-build.c +23 ~~-19~~

index ce8bfa5a37..4a6b53fbfc 100644
@@ -368,35 +368,39 @@ 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;
+    int tbl_off = tbl->len; /* Table offset in the RSDP file */
 
-    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,
+                                   tbl_off + 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);
+                                    tbl_off, 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);
+                                    tbl_off, 36, /* ACPI rev 2.0 RSDP size */
+                                    32);
 }
 
 static void

Signature · 1 line

2.19.2

Samuel Ortiz

author

2018-12-17 10:59 UTC #6 lore raw

PATCH v4 6/8 hw: arm: Support both legacy and current RSDP build

In reply to 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 <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

---
 hw/arm/virt-acpi-build.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

hw/arm/virt-acpi-build.c +33 -5

index 4a6b53fbfc..05f6654371 100644
@@ -372,6 +372,20 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
 {
     int tbl_off = tbl->len; /* Table offset in the RSDP file */
 
+    switch (rsdp_data->revision) {
+    case 0:
+        /* With ACPI 1.0, we must have an RSDT pointer */
+        g_assert(rsdp_data->rsdt_tbl_offset);
+        break;
+    case 2:
+        /* With ACPI 2.0+, we must have an XSDT pointer */
+        g_assert(rsdp_data->xsdt_tbl_offset);
+        break;
+    default:
+        /* Only revisions 0 (ACPI 1.0) and 2 (ACPI 2.0+) are valid for RSDP */
+        g_assert_not_reached();
+    }
+
     bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, tbl, 16,
                              true /* fseg memory */);

@@ -380,10 +394,29 @@ 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 */
+    if (rsdp_data->rsdt_tbl_offset) {
+        /* RSDT address to be filled by guest linker */
+        bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
+                                       tbl_off + 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,
+                                    tbl_off, 20, /* ACPI rev 1.0 RSDP size */
+                                    8);
+
+    if (rsdp_data->revision == 0) {
+        /* ACPI 1.0 RSDP, we're done */
+        return;
+    }
+
     build_append_int_noprefix(tbl, 36, 4); /* Length */
 
     /* XSDT address to be filled by guest linker */
     build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
+    /* We already validated our xsdt pointer */
     bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
                                    tbl_off + 24, 8,
                                    ACPI_BUILD_TABLE_FILE,

@@ -392,11 +425,6 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
     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,
-                                    tbl_off, 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,
                                     tbl_off, 36, /* ACPI rev 2.0 RSDP size */

Signature · 1 line

2.19.2

Samuel Ortiz

author

2018-12-17 10:59 UTC #7 lore raw

PATCH v4 7/8 hw: acpi: Export and share the ARM RSDP build

In reply to 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 <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Tested-by Philippe Mathieu-Daudé <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 include/hw/acpi/aml-build.h |  2 ++
 hw/acpi/aml-build.c         | 68 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 65 -----------------------------------
 hw/i386/acpi-build.c        | 49 +++++++++++---------------
 4 files changed, 89 insertions(+), 95 deletions(-)

include/hw/acpi/aml-build.h +2 -0

index 6c36903c0a..1a563ad756 100644
@@ -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

hw/acpi/aml-build.c +68 -0

74 lines not on this page · raw message

hw/arm/virt-acpi-build.c +0 ~~-65~~

71 lines not on this page · raw message

hw/i386/acpi-build.c +19 ~~-30~~

61 lines not on this page · raw message

Signature · 1 line

2.19.2

Igor Mammedov

Reviewed-by on v4 7/8

2018-12-17 12:20 UTC #8 lore raw

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

Quoted 9 lines · On Mon, 17 Dec 2018 11:48:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>
Reviewed-by Philippe Mathieu-Daudé <[email protected]>
Tested-by Philippe Mathieu-Daudé <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Reviewed-by Igor Mammedov <[email protected]>

for future reference, if one changes patch in a significant way, one is supposed to drop Tested/Reviewed-by tags so that reviewers would look at it again and we by mistake won't merge not actually reviewed changes.

[...]

Quoted 47 lines

hw/i386/acpi-build.c +2 ~~-30~~

index fb877648ac..846cb6d755 100644
@@ -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,25 @@ 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 = {

hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2717

Quoted 10 lines

hw/i386/acpi-build.c +10 -0

+            .revision = 0,
+            .oem_id = ACPI_BUILD_APPNAME6,
+            .xsdt_tbl_offset = NULL,
+            .rsdt_tbl_offset = &rsdt,
+        };
+        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
+        if (!pcmc->rsdp_in_ram) {
+            /*
+             * Legacy machine types (2.2 and older) expect to get a complete
+             * revision 2 RSDP table, even though they only look at the

not true, rev is set to 0 for pc machines, the point of the original comment was that we allocate extra 16 bytes but not actually using them and why it's bad to drop it suddenly.

hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2720

Quoted 3 lines

hw/i386/acpi-build.c +3 -0

+             * revision 0 fields (xsdt pointer is not set). So in order to
+             * not break migration to those machine types we waste 16 bytes
+             * that we amend to the RSDP revision 0 structure.

                          ^^^ added

hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2721

Quoted 1 line

hw/i386/acpi-build.c +1 -0
+             */

Perhaps amended original comment would be clearer:

/* We used to allocate extra space for RSDP rev 2 but used only space for
* legacy RSDP and extra bytes were zeroed out. Keep wasting extra 16 bytes
* to make sure we won't breake migration for machine types 2.2 and older
* due to RSDP blob size mismatch.
*/

hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2727

Quoted 6 lines

hw/i386/acpi-build.c +3 -0

+            build_append_int_noprefix(tables->rsdp, 0, 16);
+        }
+    }
 
     /* We'll expose it all to Guest so we want to reduce
      * chance of size changes.

With comment fixed:

Reviewed-by Igor Mammedov <[email protected]>

since it's minor fixup and doesn't affect applying remaining patches you can post v5 7/8 as reply to v4 7/8

Samuel Ortiz

author

2018-12-17 13:50 UTC #9 lore raw

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

Quoted 62 lines · Igor Mammedov

Quoted 9 lines · On Mon, 17 Dec 2018 11:48:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>
Reviewed-by Philippe Mathieu-Daudé <[email protected]>
Tested-by Philippe Mathieu-Daudé <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Reviewed-by Igor Mammedov <[email protected]>

[...]

Quoted 43 lines

hw/i386/acpi-build.c +0 ~~-29~~

index fb877648ac..846cb6d755 100644
@@ -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,25 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
                slic_oem.id, slic_oem.table_id);

Quoted 15 lines

Quoted 4 lines

hw/i386/acpi-build.c +2 -1

     /* RSDP is in FSEG memory, so allocate it separately */
-    build_rsdp(tables->rsdp, tables->linker, rsdt);
+    {
+        AcpiRsdpData rsdp_data = {

hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2717

Quoted 10 lines

hw/i386/acpi-build.c +10 -0

+            .revision = 0,
+            .oem_id = ACPI_BUILD_APPNAME6,
+            .xsdt_tbl_offset = NULL,
+            .rsdt_tbl_offset = &rsdt,
+        };
+        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
+        if (!pcmc->rsdp_in_ram) {
+            /*
+             * Legacy machine types (2.2 and older) expect to get a complete
+             * revision 2 RSDP table, even though they only look at the

not true, rev is set to 0 for pc machines,

Rev is set to 0 but they effectively expect to get a structure which size is the rev 2 one. That's what I meant.

Quoted 21 lines

the point of the original comment was
that we allocate extra 16 bytes but not actually using them and why it's bad to drop it suddenly.
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2720
Quoted 3 lines
hw/i386/acpi-build.c +3 -0
+             * revision 0 fields (xsdt pointer is not set). So in order to
+             * not break migration to those machine types we waste 16 bytes
+             * that we amend to the RSDP revision 0 structure.
                          ^^^ added
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2721
Quoted 1 line
hw/i386/acpi-build.c +1 -0
+             */
Perhaps amended original comment would be clearer:
/* We used to allocate extra space for RSDP rev 2 but used only space for
* legacy RSDP and extra bytes were zeroed out. Keep wasting extra 16 bytes
* to make sure we won't breake migration for machine types 2.2 and older
* due to RSDP blob size mismatch.
*/
Quoted 4 lines
hw/i386/acpi-build.c +3 -0
+            build_append_int_noprefix(tables->rsdp, 0, 16);
+        }
+    }
 

Quoted 8 lines

hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2727
Quoted 2 lines
hw/i386/acpi-build.c +0 -0
     /* We'll expose it all to Guest so we want to reduce
      * chance of size changes.
With comment fixed:
Reviewed-by Igor Mammedov <[email protected]>
since it's minor fixup and doesn't affect applying remaining patches you can post v5 7/8 as reply to v4 7/8

I'll do that.
Since I assume the 16 bytes addition is a significant change, I'll also remove all reviewed-by/tested-by tags from this patch, except yours.

Cheers,
Samuel.

Igor Mammedov

2018-12-17 15:35 UTC #10 lore raw

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

Quoted 78 lines · On Mon, 17 Dec 2018 14:49:59 +0100 Samuel Ortiz <[email protected]> wrote:

Quoted 62 lines · Igor Mammedov

Quoted 9 lines · On Mon, 17 Dec 2018 11:48:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>
Reviewed-by Philippe Mathieu-Daudé <[email protected]>
Tested-by Philippe Mathieu-Daudé <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Reviewed-by Igor Mammedov <[email protected]>

[...]

Quoted 43 lines

hw/i386/acpi-build.c +0 ~~-29~~

index fb877648ac..846cb6d755 100644
@@ -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,25 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
                slic_oem.id, slic_oem.table_id);

Quoted 15 lines

Quoted 4 lines

hw/i386/acpi-build.c +2 -1

     /* RSDP is in FSEG memory, so allocate it separately */
-    build_rsdp(tables->rsdp, tables->linker, rsdt);
+    {
+        AcpiRsdpData rsdp_data = {

hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2717

Quoted 10 lines

hw/i386/acpi-build.c +10 -0

+            .revision = 0,
+            .oem_id = ACPI_BUILD_APPNAME6,
+            .xsdt_tbl_offset = NULL,
+            .rsdt_tbl_offset = &rsdt,
+        };
+        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
+        if (!pcmc->rsdp_in_ram) {
+            /*
+             * Legacy machine types (2.2 and older) expect to get a complete
+             * revision 2 RSDP table, even though they only look at the

not true, rev is set to 0 for pc machines,

Quoted 21 lines

Rev is set to 0 but they effectively expect to get a structure which size is the rev 2 one. That's what I meant.
Quoted 18 lines
the point of the original comment was
that we allocate extra 16 bytes but not actually using them and why it's bad to drop it suddenly.
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2720
Quoted 3 lines
hw/i386/acpi-build.c +3 -0
+             * revision 0 fields (xsdt pointer is not set). So in order to
+             * not break migration to those machine types we waste 16 bytes
+             * that we amend to the RSDP revision 0 structure.
                          ^^^ added
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2721
Quoted 1 line
hw/i386/acpi-build.c +1 -0
+             */
Perhaps amended original comment would be clearer:
/* We used to allocate extra space for RSDP rev 2 but used only space for
* legacy RSDP and extra bytes were zeroed out. Keep wasting extra 16 bytes
* to make sure we won't breake migration for machine types 2.2 and older
* due to RSDP blob size mismatch.
*/
Quoted 1 line
hw/i386/acpi-build.c +1 -0
+            build_append_int_noprefix(tables->rsdp, 0, 16);

Looks like I've haven't noticed, it might work but are you sure?
0 here is uint64_t and then it's shifted 16 times which is sort of gray area (undefined behavior)

using g_array_append_val[s]() here might be better

Quoted 18 lines

Quoted 11 lines
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2727
Quoted 5 lines
hw/i386/acpi-build.c +2 -0
+        }
+    }
 
     /* We'll expose it all to Guest so we want to reduce
      * chance of size changes.
With comment fixed:
Reviewed-by Igor Mammedov <[email protected]>
since it's minor fixup and doesn't affect applying remaining patches you can post v5 7/8 as reply to v4 7/8
I'll do that.
Since I assume the 16 bytes addition is a significant change, I'll also remove all reviewed-by/tested-by tags from this patch, except yours.
Cheers,
Samuel.

Samuel Ortiz

author

2018-12-17 16:47 UTC #11 lore raw

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

Quoted 101 lines · Igor Mammedov

Quoted 98 lines · On Mon, 17 Dec 2018 14:49:59 +0100 Samuel Ortiz <[email protected]> wrote:
Quoted 62 lines · Igor Mammedov
Quoted 9 lines · On Mon, 17 Dec 2018 11:48:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>
Reviewed-by Philippe Mathieu-Daudé <[email protected]>
Tested-by Philippe Mathieu-Daudé <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Reviewed-by Igor Mammedov <[email protected]>
for future reference, if one changes patch in a significant way, one is supposed to drop Tested/Reviewed-by tags so that reviewers would look at it again and we by mistake won't merge not actually reviewed changes.
[...]
Quoted 43 lines
hw/i386/acpi-build.c +0 ~~-29~~
index fb877648ac..846cb6d755 100644
@@ -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,25 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
                slic_oem.id, slic_oem.table_id);
 
Quoted 15 lines
Quoted 4 lines
hw/i386/acpi-build.c +2 -1
     /* RSDP is in FSEG memory, so allocate it separately */
-    build_rsdp(tables->rsdp, tables->linker, rsdt);
+    {
+        AcpiRsdpData rsdp_data = {
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2717
Quoted 10 lines
hw/i386/acpi-build.c +10 -0
+            .revision = 0,
+            .oem_id = ACPI_BUILD_APPNAME6,
+            .xsdt_tbl_offset = NULL,
+            .rsdt_tbl_offset = &rsdt,
+        };
+        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
+        if (!pcmc->rsdp_in_ram) {
+            /*
+             * Legacy machine types (2.2 and older) expect to get a complete
+             * revision 2 RSDP table, even though they only look at the
not true, rev is set to 0 for pc machines,
Rev is set to 0 but they effectively expect to get a structure which size is the rev 2 one. That's what I meant.
Quoted 17 lines
the point of the original comment was
that we allocate extra 16 bytes but not actually using them and why it's bad to drop it suddenly.
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2720
Quoted 3 lines
hw/i386/acpi-build.c +3 -0
+             * revision 0 fields (xsdt pointer is not set). So in order to
+             * not break migration to those machine types we waste 16 bytes
+             * that we amend to the RSDP revision 0 structure.
                          ^^^ added
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2721
Quoted 1 line
hw/i386/acpi-build.c +1 -0
+             */
Perhaps amended original comment would be clearer:
/* We used to allocate extra space for RSDP rev 2 but used only space for
* legacy RSDP and extra bytes were zeroed out. Keep wasting extra 16 bytes
* to make sure we won't breake migration for machine types 2.2 and older
* due to RSDP blob size mismatch.
*/

Quoted 3 lines

Quoted 1 line
Quoted 1 line
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2722
Quoted 1 line
hw/i386/acpi-build.c +1 -0
+            build_append_int_noprefix(tables->rsdp, 0, 16);
Looks like I've haven't noticed, it might work but are you sure?
0 here is uint64_t and then it's shifted 16 times which is sort of gray area (undefined behavior)

AFAIK shifting by N bits when N > width(left operand) is undefined but that's not the case here. build_append_int_noprefix(foo, 0, 16) shift a 64 bits wide unsigned int by 8 bits, for each iterations of a 16 iterations loop. So we're doing 16 iterations of properly defined behaviour.

Cheers,
Samuel.

Michael S. Tsirkin

2018-12-17 23:04 UTC #12 lore raw

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

Quoted 105 lines · Samuel Ortiz

Quoted 101 lines · Igor Mammedov
Quoted 98 lines · On Mon, 17 Dec 2018 14:49:59 +0100 Samuel Ortiz <[email protected]> wrote:
Quoted 62 lines · Igor Mammedov
Quoted 9 lines · On Mon, 17 Dec 2018 11:48:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>
Reviewed-by Philippe Mathieu-Daudé <[email protected]>
Tested-by Philippe Mathieu-Daudé <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Reviewed-by Igor Mammedov <[email protected]>
for future reference, if one changes patch in a significant way, one is supposed to drop Tested/Reviewed-by tags so that reviewers would look at it again and we by mistake won't merge not actually reviewed changes.
[...]
Quoted 43 lines
hw/i386/acpi-build.c +0 ~~-29~~
index fb877648ac..846cb6d755 100644
@@ -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,25 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
                slic_oem.id, slic_oem.table_id);
 
Quoted 15 lines
Quoted 4 lines
hw/i386/acpi-build.c +2 -1
     /* RSDP is in FSEG memory, so allocate it separately */
-    build_rsdp(tables->rsdp, tables->linker, rsdt);
+    {
+        AcpiRsdpData rsdp_data = {
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2717
Quoted 10 lines
hw/i386/acpi-build.c +10 -0
+            .revision = 0,
+            .oem_id = ACPI_BUILD_APPNAME6,
+            .xsdt_tbl_offset = NULL,
+            .rsdt_tbl_offset = &rsdt,
+        };
+        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
+        if (!pcmc->rsdp_in_ram) {
+            /*
+             * Legacy machine types (2.2 and older) expect to get a complete
+             * revision 2 RSDP table, even though they only look at the
not true, rev is set to 0 for pc machines,
Rev is set to 0 but they effectively expect to get a structure which size is the rev 2 one. That's what I meant.
Quoted 17 lines
the point of the original comment was
that we allocate extra 16 bytes but not actually using them and why it's bad to drop it suddenly.
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2720
Quoted 3 lines
hw/i386/acpi-build.c +3 -0
+             * revision 0 fields (xsdt pointer is not set). So in order to
+             * not break migration to those machine types we waste 16 bytes
+             * that we amend to the RSDP revision 0 structure.
                          ^^^ added
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2721
Quoted 1 line
hw/i386/acpi-build.c +1 -0
+             */
Perhaps amended original comment would be clearer:
/* We used to allocate extra space for RSDP rev 2 but used only space for
* legacy RSDP and extra bytes were zeroed out. Keep wasting extra 16 bytes
* to make sure we won't breake migration for machine types 2.2 and older
* due to RSDP blob size mismatch.
*/
Quoted 3 lines
Quoted 1 line
Quoted 1 line
hw/i386/acpi-build.c void acpi_build(AcpiBuildTables *tables, MachineState *machine) · line 2722
Quoted 1 line
hw/i386/acpi-build.c +1 -0
+            build_append_int_noprefix(tables->rsdp, 0, 16);
Looks like I've haven't noticed, it might work but are you sure?
0 here is uint64_t and then it's shifted 16 times which is sort of gray area (undefined behavior)

Quoted 7 lines

AFAIK shifting by N bits when N > width(left operand) is undefined but that's not the case here. build_append_int_noprefix(foo, 0, 16) shift a 64 bits wide unsigned int by 8 bits, for each iterations of a 16 iterations loop. So we're doing 16 iterations of properly defined behaviour.
Cheers,
Samuel.

It works fine but is IMHO unnecessarily tricky. Simple

	for (i = 0; i < 16; ++i)
		build_append_byte(tables->rsdp, 0)

is imho clearer.

Samuel Ortiz

author Reviewed-by Igor Mammedov on v4 7/8

2018-12-17 14:06 UTC #13 lore raw

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

In reply to Samuel Ortiz

From: Samuel Ortiz <[email protected]>

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 <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---

V4 -> V5:
* Fixed the 16 bytes padding comment.

---
 include/hw/acpi/aml-build.h |  2 ++
 hw/acpi/aml-build.c         | 68 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 65 -----------------------------------
 hw/i386/acpi-build.c        | 49 +++++++++++---------------
 4 files changed, 89 insertions(+), 95 deletions(-)

include/hw/acpi/aml-build.h +2 -0

index 6c36903c0a..1a563ad756 100644
@@ -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

hw/acpi/aml-build.c +68 -0

74 lines not on this page · raw message

hw/arm/virt-acpi-build.c +0 ~~-65~~

71 lines not on this page · raw message

hw/i386/acpi-build.c +19 ~~-30~~

61 lines not on this page · raw message

Signature · 1 line

2.19.2

Igor Mammedov

2018-12-17 15:26 UTC #14 lore raw

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

Quoted 2 lines · On Mon, 17 Dec 2018 15:06:11 +0100 Samuel Ortiz <[email protected]> wrote:

From: Samuel Ortiz <[email protected]>

Subject should be
[Qemu-devel] [PATCH v5 7/8] hw: acpi: Export and share the ARM RSDP build

^^^

and without "Re: "

Quoted 13 lines

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 <[email protected]>
Reviewed-by Igor Mammedov <[email protected]>
---
V4 -> V5:
* Fixed the 16 bytes padding comment.
---

[...]

Samuel Ortiz

author

2018-12-17 15:33 UTC #15 lore raw

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

Quoted 11 lines · Igor Mammedov

Quoted 2 lines · On Mon, 17 Dec 2018 15:06:11 +0100 Samuel Ortiz <[email protected]> wrote:
From: Samuel Ortiz <[email protected]>
Subject should be
[Qemu-devel] [PATCH v5 7/8] hw: acpi: Export and share the ARM RSDP build
                      ^^^
and without "Re: "

I did not realize it's ok to break a thread's topic/subject. Will resend.

Quoted 15 lines

Quoted 13 lines
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 <[email protected]>
Reviewed-by Igor Mammedov <[email protected]>
---
V4 -> V5:
* Fixed the 16 bytes padding comment.
---
[...]

Samuel Ortiz

author

2018-12-17 15:35 UTC #16 lore raw

PATCH v5 7/8 hw: acpi: Export and share the ARM RSDP build

In reply to Samuel Ortiz

From: Samuel Ortiz <[email protected]>

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 <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 include/hw/acpi/aml-build.h |  2 ++
 hw/acpi/aml-build.c         | 68 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 65 -----------------------------------
 hw/i386/acpi-build.c        | 49 +++++++++++---------------
 4 files changed, 89 insertions(+), 95 deletions(-)

include/hw/acpi/aml-build.h +2 -0

index 6c36903c0a..1a563ad756 100644
@@ -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

hw/acpi/aml-build.c +68 -0

74 lines not on this page · raw message

hw/arm/virt-acpi-build.c +0 ~~-65~~

71 lines not on this page · raw message

hw/i386/acpi-build.c +19 ~~-30~~

61 lines not on this page · raw message

Signature · 1 line

2.19.2

Andrew Jones

Reviewed-by on v5 7/8

2018-12-17 15:46 UTC #17 lore raw

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

Quoted 7 lines · Samuel Ortiz

From: Samuel Ortiz <[email protected]>
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.

Quoted 9 lines

Signed-off-by Samuel Ortiz <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

---
 include/hw/acpi/aml-build.h |  2 ++
 hw/acpi/aml-build.c         | 68 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 65 -----------------------------------
 hw/i386/acpi-build.c        | 49 +++++++++++---------------
 4 files changed, 89 insertions(+), 95 deletions(-)

Reviewed-by Andrew Jones <[email protected]>

Samuel Ortiz

author

2018-12-17 10:59 UTC #18 lore raw

PATCH v4 8/8 hw: acpi: Remove AcpiRsdpDescriptor and fix tests

In reply to 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 <[email protected]>

Reviewed-by Igor Mammedov <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

---
 include/hw/acpi/acpi-defs.h | 13 -----------
 tests/acpi-utils.h          |  4 +++-
 tests/acpi-utils.c          | 46 ++++++++++++++++++++++++++++++-------
 tests/bios-tables-test.c    | 22 ++++++++++++++----
 tests/vmgenid-test.c        |  8 ++++---
 5 files changed, 63 insertions(+), 30 deletions(-)

include/hw/acpi/acpi-defs.h +0 ~~-13~~

index 8425ecb8c6..5021cb9e79 100644
@@ -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 */

tests/acpi-utils.h +3 -1

index ac52abd0dd..4f4899deb5 100644
@@ -82,6 +82,8 @@ 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);
 
 #endif  /* TEST_ACPI_UTILS_H */

tests/acpi-utils.c +38 -8

52 lines not on this page · raw message

tests/bios-tables-test.c +17 -5

index d661d9be62..544a87b55c 100644
@@ -27,7 +27,7 @@ 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 */];
     AcpiRsdtDescriptorRev1 rsdt_table;
     uint32_t dsdt_addr;
     uint32_t facs_addr;

@@ -85,19 +85,31 @@ static void test_acpi_rsdp_address(test_data *data)
 
 static void test_acpi_rsdp_table(test_data *data)
 {
-    AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table;
+    uint8_t *rsdp_table = data->rsdp_table, revision;
     uint32_t addr = data->rsdp_addr;
 
     acpi_parse_rsdp_table(addr, rsdp_table);
+    revision = rsdp_table[15 /* Revision offset */];
 
-    /* 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));
+        break;
+    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;

tests/vmgenid-test.c +5 -3

index 0a6fb55f2e..97219ae86c 100644
@@ -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);
+
+    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");

Signature · 1 line

2.19.2

Michael S. Tsirkin

2018-12-18 01:11 UTC #19 lore raw

In reply to Samuel Ortiz

Quoted 13 lines · 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.

Quoted 6 lines

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.

OK I queued this, so we can proceed to do incremental patches on top.

Quoted 47 lines

V3 -> V4:
* Add missing Reviewed-Bys from Igor and Andrew.
* Removed unneeded white space addition.
* Added 16 bytes padding to revision 0 RSDP structures for legacy (2.2 and older) machine types.
* Fixed a missing break in an ACPI RSDP test switch.
V2 -> V3:
* Add Reviewed-Bys from Igor and Andrew.
* Check for valid RSDP and XSDT pointers depending on the revision.
* Use RSDP initial table len as offset for checksum and pointer insertion.
* Do not explictly pass RSDP revision as the RSDP table parsing test utility.
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          |  4 ++-
 hw/acpi/aml-build.c         | 68 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 40 +++++-----------------
 hw/i386/acpi-build.c        | 46 +++++++++++--------------
 tests/acpi-utils.c          | 46 ++++++++++++++++++++-----
 tests/bios-tables-test.c    | 22 +++++++++---
 tests/vmgenid-test.c        |  8 +++--
 9 files changed, 168 insertions(+), 87 deletions(-)
Signature · 1 line
2.19.2