hw: acpi: RSDP fixes and refactoring

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]>

---
 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

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]>

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

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]>

---
 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

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]>

---
 include/hw/acpi/acpi-defs.h | 11 +++++++++++
 hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 5 deletions(-)

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

index af8e023968..e7fd24c6c5 100644
@@ -53,6 +53,17 @@ 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;
+
+#define ACPI_RSDP_REV_1 0
+#define ACPI_RSDP_REV_2 2
+
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */
 

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

index 0835900052..2dad465ecf 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,

@@ -399,6 +399,20 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
         (char *)&rsdp->extended_checksum - rsdp_table->data);
 }
 
+static void
+init_rsdp_data(AcpiRsdpData *data, const char *oem_id, uint8_t revision,
+               unsigned *rsdt_offset, unsigned *xsdt_offset)
+{
+    /* Caller must provide an OEM ID */
+    g_assert(oem_id);
+    g_assert(strlen(oem_id) >= 6);
+
+    memcpy(data->oem_id, oem_id, 6);
+    data->revision = revision;
+    data->rsdt_tbl_offset = rsdt_offset;
+    data->xsdt_tbl_offset = xsdt_offset;
+}
+
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {

@@ -810,6 +824,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     GArray *table_offsets;
     unsigned dsdt, xsdt;
     GArray *tables_blob = tables->table_data;
+    AcpiRsdpData rsdp;
 
     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));

@@ -857,7 +872,9 @@ 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);
+    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
+                   NULL, &xsdt);
+    build_rsdp(tables->rsdp, tables->linker, &rsdp);
 
     /* Cleanup memory that's no longer used. */
     g_array_free(table_offsets, true);

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]>

---
 include/hw/acpi/acpi-defs.h | 11 +++++++++++
 hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 5 deletions(-)

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

index af8e023968..e7fd24c6c5 100644
@@ -53,6 +53,17 @@ struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
 } QEMU_PACKED;
 typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
 
+typedef struct AcpiRsdpData {

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

+
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */

hw/arm/virt-acpi-build.c +18 -4

index 0835900052..2dad465ecf 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,

@@ -399,6 +399,20 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
         (char *)&rsdp->extended_checksum - rsdp_table->data);
 }
 
+static void
+init_rsdp_data(AcpiRsdpData *data, const char *oem_id, uint8_t revision,
+               unsigned *rsdt_offset, unsigned *xsdt_offset)
+{
+    /* Caller must provide an OEM ID */
+    g_assert(oem_id);
+    g_assert(strlen(oem_id) >= 6);
+
+    memcpy(data->oem_id, oem_id, 6);
+    data->revision = revision;
+    data->rsdt_tbl_offset = rsdt_offset;
+    data->xsdt_tbl_offset = xsdt_offset;
+}
+
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {

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

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

Quoted 26 lines · On Mon, 26 Nov 2018 17:29:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>

---
 include/hw/acpi/acpi-defs.h | 11 +++++++++++
 hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 5 deletions(-)

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

index af8e023968..e7fd24c6c5 100644
@@ -53,6 +53,17 @@ 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;
+

include/hw/acpi/acpi-defs.h struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ · line 65

Quoted 2 lines

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

+#define ACPI_RSDP_REV_1 0
+#define ACPI_RSDP_REV_2 2

it's one time used spec defined values so just use values directly in place with a comment, so reader won't have to jump around code when comparing to spec.

Quoted 57 lines

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

+
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */
 

hw/arm/virt-acpi-build.c +18 -4

index 0835900052..2dad465ecf 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,

@@ -399,6 +399,20 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
         (char *)&rsdp->extended_checksum - rsdp_table->data);
 }
 
+static void
+init_rsdp_data(AcpiRsdpData *data, const char *oem_id, uint8_t revision,
+               unsigned *rsdt_offset, unsigned *xsdt_offset)
+{
+    /* Caller must provide an OEM ID */
+    g_assert(oem_id);
+    g_assert(strlen(oem_id) >= 6);
+
+    memcpy(data->oem_id, oem_id, 6);
+    data->revision = revision;
+    data->rsdt_tbl_offset = rsdt_offset;
+    data->xsdt_tbl_offset = xsdt_offset;
+}
+
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 827

Quoted 4 lines

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

@@ -810,6 +824,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     GArray *table_offsets;
     unsigned dsdt, xsdt;
     GArray *tables_blob = tables->table_data;
+    AcpiRsdpData rsdp;

s/rsdp/rsdp_info/

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 876

Quoted 10 lines

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

     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));

@@ -857,7 +872,9 @@ 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);
+    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
+                   NULL, &xsdt);

It would be more concise to use declarative style without extra clutter:

(quoted diff) +0 -2

-    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
-                   NULL, &xsdt);

Hi Igor,

Quoted 30 lines · Igor Mammedov

Quoted 26 lines · On Mon, 26 Nov 2018 17:29:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>

---
 include/hw/acpi/acpi-defs.h | 11 +++++++++++
 hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 5 deletions(-)

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

index af8e023968..e7fd24c6c5 100644
@@ -53,6 +53,17 @@ 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;
+  

Quoted 5 lines

include/hw/acpi/acpi-defs.h struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ · line 65

Quoted 2 lines

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

+#define ACPI_RSDP_REV_1 0
+#define ACPI_RSDP_REV_2 2  

it's one time used spec defined values so just use values directly in place with a comment, so reader won't have to jump around code when comparing to spec.

It's also used in the ACPI tests fix patch.

Also the 0 for revision 1 is a little confusing, I feel the above definition is clearer.

Quoted 79 lines

Quoted 59 lines

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

+
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */
 

hw/arm/virt-acpi-build.c +18 -4

index 0835900052..2dad465ecf 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,

@@ -399,6 +399,20 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
         (char *)&rsdp->extended_checksum - rsdp_table->data);
 }
 
+static void
+init_rsdp_data(AcpiRsdpData *data, const char *oem_id, uint8_t revision,
+               unsigned *rsdt_offset, unsigned *xsdt_offset)
+{
+    /* Caller must provide an OEM ID */
+    g_assert(oem_id);
+    g_assert(strlen(oem_id) >= 6);
+
+    memcpy(data->oem_id, oem_id, 6);
+    data->revision = revision;
+    data->rsdt_tbl_offset = rsdt_offset;
+    data->xsdt_tbl_offset = xsdt_offset;
+}
+
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 827

Quoted 4 lines

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

@@ -810,6 +824,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     GArray *table_offsets;
     unsigned dsdt, xsdt;
     GArray *tables_blob = tables->table_data;
+    AcpiRsdpData rsdp;  

s/rsdp/rsdp_info/

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 876

Quoted 10 lines

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

     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));

@@ -857,7 +872,9 @@ 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);
+    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
+                   NULL, &xsdt);  

It would be more concise to use declarative style without extra clutter:

(quoted diff) +0 -2

-    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
-                   NULL, &xsdt);

Review

Quoted 10 lines

(quoted diff) +9 -1

-    build_rsdp(tables->rsdp, tables->linker, &rsdp);
+    {
+       AcpiRsdpData rsdp = {
+           .revision = 2,
+           .oem_id = ACPI_BUILD_APPNAME6,
+           .xsdt_tbl_offset = &xsdt,
+           .rsdt_tbl_offset = NULL,
+       };
+       build_rsdp(tables->rsdp, tables->linker, &rsdp);
+    }  

2 things here, imo:

- This is not more concise.

- It's code duplication as almost the same snippet is going to be used for i386/acpi-build.c

Cheers,
Samuel.

Quoted 33 lines · On Tue, 27 Nov 2018 16:42:18 +0100 Samuel Ortiz <[email protected]> wrote:

Hi Igor,

Quoted 30 lines · Igor Mammedov

Quoted 26 lines · On Mon, 26 Nov 2018 17:29:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>

---
 include/hw/acpi/acpi-defs.h | 11 +++++++++++
 hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 5 deletions(-)

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

index af8e023968..e7fd24c6c5 100644
@@ -53,6 +53,17 @@ 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;
+  

Quoted 6 lines

Quoted 5 lines

include/hw/acpi/acpi-defs.h struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ · line 65

Quoted 2 lines

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

+#define ACPI_RSDP_REV_1 0
+#define ACPI_RSDP_REV_2 2  

it's one time used spec defined values so just use values directly in place with a comment, so reader won't have to jump around code when comparing to spec.

It's also used in the ACPI tests fix patch.

it's better to use in test it's own version (we just opencode them there) see fadt_fetch_facs_and_dsdt_ptrs()/sanitize_fadt_ptrs()
same applies for length.
that way if we break it in qemu's code test would catch the thing

Quoted 2 lines

Also the 0 for revision 1 is a little confusing, I feel the above definition is clearer.

that's confusion is in the spec, so we just mimic it, no need to add more on top

Quoted 89 lines

Quoted 79 lines

Quoted 59 lines

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

+
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */
 

hw/arm/virt-acpi-build.c +18 -4

index 0835900052..2dad465ecf 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,

@@ -399,6 +399,20 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
         (char *)&rsdp->extended_checksum - rsdp_table->data);
 }
 
+static void
+init_rsdp_data(AcpiRsdpData *data, const char *oem_id, uint8_t revision,
+               unsigned *rsdt_offset, unsigned *xsdt_offset)
+{
+    /* Caller must provide an OEM ID */
+    g_assert(oem_id);
+    g_assert(strlen(oem_id) >= 6);
+
+    memcpy(data->oem_id, oem_id, 6);
+    data->revision = revision;
+    data->rsdt_tbl_offset = rsdt_offset;
+    data->xsdt_tbl_offset = xsdt_offset;
+}
+
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 827

Quoted 4 lines

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

@@ -810,6 +824,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     GArray *table_offsets;
     unsigned dsdt, xsdt;
     GArray *tables_blob = tables->table_data;
+    AcpiRsdpData rsdp;  

s/rsdp/rsdp_info/

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 876

Quoted 10 lines

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

     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));

@@ -857,7 +872,9 @@ 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);
+    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
+                   NULL, &xsdt);  

It would be more concise to use declarative style without extra clutter:

(quoted diff) +0 -2

-    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
-                   NULL, &xsdt);

Review

Quoted 10 lines

(quoted diff) +9 -1

-    build_rsdp(tables->rsdp, tables->linker, &rsdp);
+    {
+       AcpiRsdpData rsdp = {
+           .revision = 2,
+           .oem_id = ACPI_BUILD_APPNAME6,
+           .xsdt_tbl_offset = &xsdt,
+           .rsdt_tbl_offset = NULL,
+       };
+       build_rsdp(tables->rsdp, tables->linker, &rsdp);
+    }  

Quoted 3 lines

2 things here, imo:

- This is not more concise.

with function, one have to jump to it's definition/body to find out what each argument is, with declaration + initialization inplace it's clear what values mean as you see fields right there as well.

If it's simple structure it is clearer to use initializer, instead of wrapper helper. With complex structure it could be other way around.

Quoted 2 lines

- It's code duplication as almost the same snippet is going to be used for i386/acpi-build.c

the same goes for init_rsdp_data(...), the only difference the declaration isn't folded in 2 lines to be more readable and there is boiler plate helper function adds.

Quoted 3 lines

Cheers,
Samuel.

Quoted 47 lines · Igor Mammedov

Quoted 33 lines · On Tue, 27 Nov 2018 16:42:18 +0100 Samuel Ortiz <[email protected]> wrote:

Hi Igor,

Quoted 30 lines · Igor Mammedov

Quoted 26 lines · On Mon, 26 Nov 2018 17:29:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>

---
 include/hw/acpi/acpi-defs.h | 11 +++++++++++
 hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 5 deletions(-)

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

index af8e023968..e7fd24c6c5 100644
@@ -53,6 +53,17 @@ 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;
+  

Quoted 6 lines

Quoted 5 lines

include/hw/acpi/acpi-defs.h struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ · line 65

Quoted 2 lines

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

+#define ACPI_RSDP_REV_1 0
+#define ACPI_RSDP_REV_2 2  

it's one time used spec defined values so just use values directly in place with a comment, so reader won't have to jump around code when comparing to spec.

It's also used in the ACPI tests fix patch.

it's better to use in test it's own version (we just opencode them there) see fadt_fetch_facs_and_dsdt_ptrs()/sanitize_fadt_ptrs()
same applies for length.
that way if we break it in qemu's code test would catch the thing

Quoted 3 lines

Quoted 2 lines

Also the 0 for revision 1 is a little confusing, I feel the above definition is clearer.

that's confusion is in the spec, so we just mimic it, no need to add more on top

To be more precise, there is a huge number of constants in ACPI such that adding defines for them all would be a huge burden,

and will not make it easy to check values against the spec at all (case in point ACPI_RSDP_REV_2 is actually wrong, 2 is version 3 and up).

Thus the preferred style is to add a comment near the value matching spec name verbatim, so one can copy it and look it up in the spec. Sometimes one needs to reference specific spec version.

0 /* Revision: ACPI version 1.0 */

and

1 /* Revision: ACPI 2.0 */

and

2 /* Revision: ACPI 3.0a */

For style consistency, if the value is used multiple times, we avoid duplication by using an inline function and not a macro.

Quoted 38 lines · On Tue, 27 Nov 2018 16:42:18 +0100 Samuel Ortiz <[email protected]> wrote:

Hi Igor,

Quoted 35 lines · Igor Mammedov

Quoted 19 lines · On Mon, 26 Nov 2018 17:29:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>

---
 include/hw/acpi/acpi-defs.h | 11 +++++++++++
 hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 5 deletions(-)

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

index af8e023968..e7fd24c6c5 100644
@@ -53,6 +53,17 @@ struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
 } QEMU_PACKED;
 typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
 
+typedef struct AcpiRsdpData {

include/hw/acpi/acpi-defs.h struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ · line 66

Quoted 10 lines

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

+    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;
+  
 
+#define ACPI_RSDP_REV_1 0
+#define ACPI_RSDP_REV_2 2  

it's one time used spec defined values so just use values directly in place with a comment, so reader won't have to jump around code when comparing to spec.

Quoted 1 line

It's also used in the ACPI tests fix patch.

it's better to use in test it's own version (we just opencode them there) see fadt_fetch_facs_and_dsdt_ptrs()/sanitize_fadt_ptrs()
same applies for length.

that way if we break it in qemu's code test would catch the thing

Quoted 2 lines

Also the 0 for revision 1 is a little confusing, I feel the above definition is clearer.

that's confusion is in the spec, so we just mimic it, no need to add more on top

Quoted 89 lines

Quoted 89 lines

Quoted 59 lines

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

+
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */

hw/arm/virt-acpi-build.c +18 -4

index 0835900052..2dad465ecf 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,

@@ -399,6 +399,20 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
         (char *)&rsdp->extended_checksum - rsdp_table->data);
 }
 
+static void
+init_rsdp_data(AcpiRsdpData *data, const char *oem_id, uint8_t revision,
+               unsigned *rsdt_offset, unsigned *xsdt_offset)
+{
+    /* Caller must provide an OEM ID */
+    g_assert(oem_id);
+    g_assert(strlen(oem_id) >= 6);
+
+    memcpy(data->oem_id, oem_id, 6);
+    data->revision = revision;
+    data->rsdt_tbl_offset = rsdt_offset;
+    data->xsdt_tbl_offset = xsdt_offset;
+}
+
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 827

Quoted 4 lines

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

@@ -810,6 +824,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     GArray *table_offsets;
     unsigned dsdt, xsdt;
     GArray *tables_blob = tables->table_data;
+    AcpiRsdpData rsdp;  

s/rsdp/rsdp_info/

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 876

Quoted 10 lines

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

     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));

@@ -857,7 +872,9 @@ 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);
+    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
+                   NULL, &xsdt);  

It would be more concise to use declarative style without extra clutter:

(quoted diff) +9 -3

-    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
-                   NULL, &xsdt);
-    build_rsdp(tables->rsdp, tables->linker, &rsdp);
+    {
+       AcpiRsdpData rsdp = {
+           .revision = 2,
+           .oem_id = ACPI_BUILD_APPNAME6,
+           .xsdt_tbl_offset = &xsdt,
+           .rsdt_tbl_offset = NULL,
+       };
+       build_rsdp(tables->rsdp, tables->linker, &rsdp);
+    }  

Quoted 3 lines

2 things here, imo:

- This is not more concise.

with function, one have to jump to it's definition/body to find out what each argument is, with declaration + initialization inplace it's clear what values mean as you see fields right there as well.

If it's simple structure it is clearer to use initializer, instead of wrapper helper. With complex structure it could be other way around.

Quoted 2 lines

- It's code duplication as almost the same snippet is going to be used for i386/acpi-build.c

the same goes for init_rsdp_data(...)

Quoted 41 lines · Igor Mammedov

Quoted 38 lines · On Tue, 27 Nov 2018 16:42:18 +0100 Samuel Ortiz <[email protected]> wrote:

Hi Igor,

Quoted 35 lines · Igor Mammedov

Quoted 19 lines · On Mon, 26 Nov 2018 17:29:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>

---
 include/hw/acpi/acpi-defs.h | 11 +++++++++++
 hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 5 deletions(-)

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

index af8e023968..e7fd24c6c5 100644
@@ -53,6 +53,17 @@ struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
 } QEMU_PACKED;
 typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
 
+typedef struct AcpiRsdpData {

include/hw/acpi/acpi-defs.h struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ · line 66

Quoted 10 lines

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

+    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;
+  
 
+#define ACPI_RSDP_REV_1 0
+#define ACPI_RSDP_REV_2 2  

it's one time used spec defined values so just use values directly in place with a comment, so reader won't have to jump around code when comparing to spec.

Quoted 4 lines

Quoted 1 line

It's also used in the ACPI tests fix patch.

it's better to use in test it's own version (we just opencode them there) see fadt_fetch_facs_and_dsdt_ptrs()/sanitize_fadt_ptrs()
same applies for length.

I think you're trying to explain to me that this:

	/* sdt->aml field offset := spec offset - header size */
        memset(sdt->aml + 0, 0, 4); /* sanitize FIRMWARE_CTRL(36) ptr */
        memset(sdt->aml + 4, 0, 4); /* sanitize DSDT(40) ptr */
        if (sdt->header.revision >= 3) {
            memset(sdt->aml + 96, 0, 8); /* sanitize X_FIRMWARE_CTRL(132) ptr */
            memset(sdt->aml + 104, 0, 8); /* sanitize X_DSDT(140) ptr */
        }

is good coding practice. I'm having a hard time internalizing that hard coded constants and comments not directly mapping the code (How do I map "sanitize X_FIRMWARE_CTRL(132) ptr" to "sdt->aml + 96, 0, 8"?) is indeed good practice. But I'll take the pragmatic route and follow what you guys advice for.

Quoted 95 lines

that way if we break it in qemu's code test would catch the thing

Quoted 2 lines

Also the 0 for revision 1 is a little confusing, I feel the above definition is clearer.

that's confusion is in the spec, so we just mimic it, no need to add more on top

Quoted 89 lines

Quoted 89 lines

Quoted 59 lines

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

+
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */

hw/arm/virt-acpi-build.c +18 -4

index 0835900052..2dad465ecf 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,

@@ -399,6 +399,20 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
         (char *)&rsdp->extended_checksum - rsdp_table->data);
 }
 
+static void
+init_rsdp_data(AcpiRsdpData *data, const char *oem_id, uint8_t revision,
+               unsigned *rsdt_offset, unsigned *xsdt_offset)
+{
+    /* Caller must provide an OEM ID */
+    g_assert(oem_id);
+    g_assert(strlen(oem_id) >= 6);
+
+    memcpy(data->oem_id, oem_id, 6);
+    data->revision = revision;
+    data->rsdt_tbl_offset = rsdt_offset;
+    data->xsdt_tbl_offset = xsdt_offset;
+}
+
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 827

Quoted 4 lines

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

@@ -810,6 +824,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     GArray *table_offsets;
     unsigned dsdt, xsdt;
     GArray *tables_blob = tables->table_data;
+    AcpiRsdpData rsdp;  

s/rsdp/rsdp_info/

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 876

Quoted 10 lines

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

     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));

@@ -857,7 +872,9 @@ 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);
+    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
+                   NULL, &xsdt);  

It would be more concise to use declarative style without extra clutter:

(quoted diff) +9 -3

-    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
-                   NULL, &xsdt);
-    build_rsdp(tables->rsdp, tables->linker, &rsdp);
+    {
+       AcpiRsdpData rsdp = {
+           .revision = 2,
+           .oem_id = ACPI_BUILD_APPNAME6,
+           .xsdt_tbl_offset = &xsdt,
+           .rsdt_tbl_offset = NULL,
+       };
+       build_rsdp(tables->rsdp, tables->linker, &rsdp);
+    }  

Quoted 6 lines

Quoted 3 lines

2 things here, imo:

- This is not more concise.

with function, one have to jump to it's definition/body to find out what each argument is, with declaration + initialization inplace it's clear what values mean as you see fields right there as well.

With a structure you need to go and look at the structure definition to know which fields you need to initialize and what their names are. And no, you can't safely copy paste the above snippet and rest assured your code is safe, because C allows you to leave some structure fields uninitialized.

Quoted 3 lines

If it's simple structure it is clearer to use initializer, instead of wrapper helper. With complex structure it could be other way around.

Quoted 3 lines

Quoted 2 lines

- It's code duplication as almost the same snippet is going to be used for i386/acpi-build.c

the same goes for init_rsdp_data(...)

I disagree here as well. But I'd like to see this code being merged, I'll comply. Do you have any comments on the tests part of that serie, besides the fact that it's using defined constants as opposed to hard coded ones?

Quoted 41 lines · Igor Mammedov

Quoted 38 lines · On Tue, 27 Nov 2018 16:42:18 +0100 Samuel Ortiz <[email protected]> wrote:

Hi Igor,

Quoted 35 lines · Igor Mammedov

Quoted 19 lines · On Mon, 26 Nov 2018 17:29:37 +0100 Samuel Ortiz <[email protected]> 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 <[email protected]>

---
 include/hw/acpi/acpi-defs.h | 11 +++++++++++
 hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 5 deletions(-)

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

index af8e023968..e7fd24c6c5 100644
@@ -53,6 +53,17 @@ struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
 } QEMU_PACKED;
 typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
 
+typedef struct AcpiRsdpData {

include/hw/acpi/acpi-defs.h struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ · line 66

Quoted 10 lines

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

+    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;
+    
 
+#define ACPI_RSDP_REV_1 0
+#define ACPI_RSDP_REV_2 2    

it's one time used spec defined values so just use values directly in place with a comment, so reader won't have to jump around code when comparing to spec.

Quoted 4 lines

Quoted 1 line

It's also used in the ACPI tests fix patch.

it's better to use in test it's own version (we just opencode them there) see fadt_fetch_facs_and_dsdt_ptrs()/sanitize_fadt_ptrs()
same applies for length.

I think you're trying to explain to me that this:

	/* sdt->aml field offset := spec offset - header size */
        memset(sdt->aml + 0, 0, 4); /* sanitize FIRMWARE_CTRL(36) ptr */
        memset(sdt->aml + 4, 0, 4); /* sanitize DSDT(40) ptr */
        if (sdt->header.revision >= 3) {
            memset(sdt->aml + 96, 0, 8); /* sanitize X_FIRMWARE_CTRL(132) ptr */
            memset(sdt->aml + 104, 0, 8); /* sanitize X_DSDT(140) ptr */
        }

is good coding practice. I'm having a hard time internalizing that hard coded constants and comments not directly mapping the code (How do I map "sanitize X_FIRMWARE_CTRL(132) ptr" to "sdt->aml + 96, 0, 8"?) is indeed good practice. But I'll take the pragmatic route and follow what you guys advice for.

Quoted 98 lines

that way if we break it in qemu's code test would catch the thing

Quoted 2 lines

Also the 0 for revision 1 is a little confusing, I feel the above definition is clearer.

that's confusion is in the spec, so we just mimic it, no need to add more on top

Quoted 92 lines

Quoted 79 lines

Quoted 59 lines

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

+
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */

hw/arm/virt-acpi-build.c +18 -4

index 0835900052..2dad465ecf 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,

@@ -399,6 +399,20 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
         (char *)&rsdp->extended_checksum - rsdp_table->data);
 }
 
+static void
+init_rsdp_data(AcpiRsdpData *data, const char *oem_id, uint8_t revision,
+               unsigned *rsdt_offset, unsigned *xsdt_offset)
+{
+    /* Caller must provide an OEM ID */
+    g_assert(oem_id);
+    g_assert(strlen(oem_id) >= 6);
+
+    memcpy(data->oem_id, oem_id, 6);
+    data->revision = revision;
+    data->rsdt_tbl_offset = rsdt_offset;
+    data->xsdt_tbl_offset = xsdt_offset;
+}
+
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 827

Quoted 4 lines

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

@@ -810,6 +824,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     GArray *table_offsets;
     unsigned dsdt, xsdt;
     GArray *tables_blob = tables->table_data;
+    AcpiRsdpData rsdp;    

s/rsdp/rsdp_info/

hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 876

Quoted 10 lines

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

     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));

@@ -857,7 +872,9 @@ 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);
+    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
+                   NULL, &xsdt);    

It would be more concise to use declarative style without extra clutter:

(quoted diff) +0 -2

-    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_2,
-                   NULL, &xsdt);

Review

Quoted 10 lines

(quoted diff) +9 -1

-    build_rsdp(tables->rsdp, tables->linker, &rsdp);
+    {
+       AcpiRsdpData rsdp = {
+           .revision = 2,
+           .oem_id = ACPI_BUILD_APPNAME6,
+           .xsdt_tbl_offset = &xsdt,
+           .rsdt_tbl_offset = NULL,
+       };
+       build_rsdp(tables->rsdp, tables->linker, &rsdp);
+    }    

2 things here, imo:

- This is not more concise.

Quoted 3 lines

with function, one have to jump to it's definition/body to find out what each argument is, with declaration + initialization inplace it's clear what values mean as you see fields right there as well.

With a structure you need to go and look at the structure definition to know which fields you need to initialize and what their names are. And no, you can't safely copy paste the above snippet and rest assured your code is safe, because C allows you to leave some structure fields uninitialized.

Quoted 6 lines

If it's simple structure it is clearer to use initializer, instead of wrapper helper. With complex structure it could be other way around.

Quoted 2 lines

- It's code duplication as almost the same snippet is going to be used for i386/acpi-build.c

the same goes for init_rsdp_data(...)

I disagree here as well. But I'd like to see this code being merged, I'll comply. Do you have any comments on the tests part of that serie, besides the fact that it's using defined constants as opposed to hard coded ones?

Cheers,
Samuel.

build_rsdp() is now closely following the ACPI spec instead of filling a mapped and packed C structure.

With this change we will eventually be able to get rid of the AcpiRsdpDescriptor structure as we're just appending values to the RSDP file directly and not mapping this structure in memory any more.

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

---
 include/hw/acpi/acpi-defs.h | 22 ++++++++++++++
 hw/arm/virt-acpi-build.c    | 60 ++++++++++++++++++++++++-------------
 2 files changed, 61 insertions(+), 21 deletions(-)

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

index e7fd24c6c5..9b2f6b8043 100644
@@ -61,9 +61,31 @@ typedef struct AcpiRsdpData {
     unsigned *xsdt_tbl_offset;
 } AcpiRsdpData;
 
+/* RSDP signature */
+#define ACPI_RSDP_SIGNATURE "RSD PTR "
+
+/* RSDP Revisions */
 #define ACPI_RSDP_REV_1 0
 #define ACPI_RSDP_REV_2 2
 
 
+/* RSDP lengths */
+#define ACPI_RSDP_REV_1_LEN    20
+#define ACPI_RSDP_REV_2_LEN    36
+#define ACPI_RSDP_SIG_LEN      8
+#define ACPI_RSDP_OEMID_LEN    6
+#define ACPI_RSDP_REVISION_LEN 1

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

+
+
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */

hw/arm/virt-acpi-build.c +39 -21

index 2dad465ecf..d47978a55e 100644
@@ -368,35 +368,53 @@ 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;
+    /* RSDP signature */
+    g_array_append_vals(tbl, ACPI_RSDP_SIGNATURE, ACPI_RSDP_SIG_LEN);
+
+    /* Space for the checksum */
+    build_append_int_noprefix(tbl, 0, ACPI_RSDP_CHECKSUM_LEN);
+
+    /* OEM ID */
+    g_array_append_vals(tbl, rsdp_data->oem_id, ACPI_RSDP_OEMID_LEN);
+
+    /* Revision */
+    build_append_int_noprefix(tbl, rsdp_data->revision,
+                              ACPI_RSDP_REVISION_LEN);
 
-    /* Address to be filled by Guest linker */
+    /* Space for the RSDT address (32 bit) */
+    build_append_int_noprefix(tbl, 0, 4);
+
+    /* Length */
+    build_append_int_noprefix(tbl, ACPI_RSDP_REV_2_LEN, ACPI_RSDP_LEN_LEN);
+
+    /* XSDT address to be filled by guest linker */
+    build_append_int_noprefix(tbl, 0, 8); /* XSDT address (64 bit) */
     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);
+                                   ACPI_BUILD_RSDP_FILE,
+                                   ACPI_RSDP_XSDT_OFFSET, 8,
+                                   ACPI_BUILD_TABLE_FILE,
+                                   *rsdp_data->xsdt_tbl_offset);
+
+    /* Space for the extended checksum */
+    build_append_int_noprefix(tbl, 0, ACPI_RSDP_CHECKSUM_LEN);
+
+    /* Space for the reserved bytes */
+    build_append_int_noprefix(tbl, 0, ACPI_RSDP_RESERVED_LEN);
 
-    /* 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);
+    /* Checksum to be filled by guest linker */
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, 0,
+                                    ACPI_RSDP_REV_1_LEN,
+                                    ACPI_RSDP_CHECKSUM_OFFSET);
 
     /* 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);
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, 0,
+                                    ACPI_RSDP_REV_2_LEN,
+                                    ACPI_RSDP_EXT_CHECKSUM_OFFSET);
 }
 
 static void

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]>

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

hw/arm/virt-acpi-build.c +21 -0

index d47978a55e..10f8388b63 100644
@@ -389,6 +389,27 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
     /* Space for the RSDT address (32 bit) */
     build_append_int_noprefix(tbl, 0, 4);
 
+    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,
+                                    ACPI_RSDP_REV_1_LEN,
+                                    ACPI_RSDP_CHECKSUM_OFFSET);
+
+    if (rsdp_data->revision == ACPI_RSDP_REV_1) {
+        /* Legacy 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);
+
     /* Length */
     build_append_int_noprefix(tbl, ACPI_RSDP_REV_2_LEN, ACPI_RSDP_LEN_LEN);
 

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

     /* Extended checksum to be filled by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, 0,
                                     ACPI_RSDP_REV_2_LEN,

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]>

---
 include/hw/acpi/aml-build.h |  5 +++
 hw/acpi/aml-build.c         | 81 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 81 -------------------------------------
 hw/i386/acpi-build.c        | 34 ++--------------
 4 files changed, 90 insertions(+), 111 deletions(-)

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

index 6c36903c0a..1bbe496e32 100644
@@ -388,6 +388,11 @@ 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
+init_rsdp_data(AcpiRsdpData *data, const char *oem_id, uint8_t revision,
+               unsigned *rsdt_offset, unsigned *xsdt_offset);
+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 +81 -0

index 1e43cd736d..d4e15980f1 100644
@@ -1589,6 +1589,87 @@ 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 */);
+
+    /* RSDP signature */
+    g_array_append_vals(tbl, ACPI_RSDP_SIGNATURE, ACPI_RSDP_SIG_LEN);
+
+    /* Space for the checksum */
+    build_append_int_noprefix(tbl, 0, ACPI_RSDP_CHECKSUM_LEN);
+
+    /* OEM ID */
+    g_array_append_vals(tbl, rsdp_data->oem_id, ACPI_RSDP_OEMID_LEN);
+
+    /* Revision */
+    build_append_int_noprefix(tbl, rsdp_data->revision,
+                              ACPI_RSDP_REVISION_LEN);
+
+    /* Space for the RSDT address (32 bit) */
+    build_append_int_noprefix(tbl, 0, 4);
+
+    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,
+                                    ACPI_RSDP_REV_1_LEN,
+                                    ACPI_RSDP_CHECKSUM_OFFSET);
+
+    if (rsdp_data->revision == ACPI_RSDP_REV_1) {
+        /* Legacy 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);
+
+    /* Length */
+    build_append_int_noprefix(tbl, ACPI_RSDP_REV_2_LEN, ACPI_RSDP_LEN_LEN);
+
+    /* XSDT address to be filled by guest linker */
+    build_append_int_noprefix(tbl, 0, 8); /* XSDT address (64 bit) */
+    bios_linker_loader_add_pointer(linker,
+                                   ACPI_BUILD_RSDP_FILE,
+                                   ACPI_RSDP_XSDT_OFFSET, 8,
+                                   ACPI_BUILD_TABLE_FILE,
+                                   *rsdp_data->xsdt_tbl_offset);
+
+    /* Space for the extended checksum */
+    build_append_int_noprefix(tbl, 0, ACPI_RSDP_CHECKSUM_LEN);
+
+    /* Space for the reserved bytes */
+    build_append_int_noprefix(tbl, 0, ACPI_RSDP_RESERVED_LEN);
+
+    /* Extended checksum to be filled by Guest linker */
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, 0,
+                                    ACPI_RSDP_REV_2_LEN,
+                                    ACPI_RSDP_EXT_CHECKSUM_OFFSET);
+}
+
+void
+init_rsdp_data(AcpiRsdpData *data, const char *oem_id, uint8_t revision,
+               unsigned *rsdt_offset, unsigned *xsdt_offset)
+{
+    /* Caller must provide an OEM ID */
+    g_assert(oem_id);
+    g_assert(strlen(oem_id) >= 6);
+
+    memcpy(data->oem_id, oem_id, 6);
+    data->revision = revision;
+    data->rsdt_tbl_offset = rsdt_offset;
+    data->xsdt_tbl_offset = xsdt_offset;
+}
+
 /* Build rsdt table */
 void
 build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,

hw/arm/virt-acpi-build.c +0 -81

index 10f8388b63..3fe86fd33e 100644
@@ -366,87 +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 */);
-
-    /* RSDP signature */
-    g_array_append_vals(tbl, ACPI_RSDP_SIGNATURE, ACPI_RSDP_SIG_LEN);
-
-    /* Space for the checksum */
-    build_append_int_noprefix(tbl, 0, ACPI_RSDP_CHECKSUM_LEN);
-
-    /* OEM ID */
-    g_array_append_vals(tbl, rsdp_data->oem_id, ACPI_RSDP_OEMID_LEN);
-
-    /* Revision */
-    build_append_int_noprefix(tbl, rsdp_data->revision,
-                              ACPI_RSDP_REVISION_LEN);
-
-    /* Space for the RSDT address (32 bit) */
-    build_append_int_noprefix(tbl, 0, 4);
-
-    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,
-                                    ACPI_RSDP_REV_1_LEN,
-                                    ACPI_RSDP_CHECKSUM_OFFSET);
-
-    if (rsdp_data->revision == ACPI_RSDP_REV_1) {
-        /* Legacy 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);
-
-    /* Length */
-    build_append_int_noprefix(tbl, ACPI_RSDP_REV_2_LEN, ACPI_RSDP_LEN_LEN);
-
-    /* XSDT address to be filled by guest linker */
-    build_append_int_noprefix(tbl, 0, 8); /* XSDT address (64 bit) */
-    bios_linker_loader_add_pointer(linker,
-                                   ACPI_BUILD_RSDP_FILE,
-                                   ACPI_RSDP_XSDT_OFFSET, 8,
-                                   ACPI_BUILD_TABLE_FILE,
-                                   *rsdp_data->xsdt_tbl_offset);
-
-    /* Space for the extended checksum */
-    build_append_int_noprefix(tbl, 0, ACPI_RSDP_CHECKSUM_LEN);
-
-    /* Space for the reserved bytes */
-    build_append_int_noprefix(tbl, 0, ACPI_RSDP_RESERVED_LEN);
-
-    /* Extended checksum to be filled by Guest linker */
-    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, 0,
-                                    ACPI_RSDP_REV_2_LEN,
-                                    ACPI_RSDP_EXT_CHECKSUM_OFFSET);
-}
-
-static void
-init_rsdp_data(AcpiRsdpData *data, const char *oem_id, uint8_t revision,
-               unsigned *rsdt_offset, unsigned *xsdt_offset)
-{
-    /* Caller must provide an OEM ID */
-    g_assert(oem_id);
-    g_assert(strlen(oem_id) >= 6);
-
-    memcpy(data->oem_id, oem_id, 6);
-    data->revision = revision;
-    data->rsdt_tbl_offset = rsdt_offset;
-    data->xsdt_tbl_offset = xsdt_offset;
-}
-
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {

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

index fb877648ac..f7ab112599 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). */

@@ -2625,6 +2596,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     GArray *tables_blob = tables->table_data;
     AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
     Object *vmgenid_dev;
+    AcpiRsdpData rsdp;
 
     acpi_get_pm_info(&pm);
     acpi_get_misc_info(&misc);

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

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

We switched to the build_append_*() API for all RSDP callers, and this patch converts all existing tests depending on this structure. It is no longer needed and we can thus remove it.

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

---
 include/hw/acpi/acpi-defs.h | 13 ----------
 tests/acpi-utils.h          |  5 +++-
 tests/acpi-utils.c          | 48 ++++++++++++++++++++++++++++++-------
 tests/bios-tables-test.c    | 27 ++++++++++++++-------
 tests/vmgenid-test.c        |  8 ++++---
 5 files changed, 67 insertions(+), 34 deletions(-)

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

index 9b2f6b8043..abef1d57ae 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 +4 -1

index ac52abd0dd..fb1a67a21f 100644
@@ -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_find_rsdt_address(uint8_t *rsdp_table);
+uint64_t acpi_find_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 */

tests/acpi-utils.c +0 -0

index 41dc1ea9b4..c8fe2d1342 100644

tests/acpi-utils.c +23 -0

+    uint8_t revision = rsdp_table[ACPI_RSDP_REVISION_OFFSET];
+
+    if (revision != ACPI_RSDP_REV_2) {
+        return 0;
+    }
+
+    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 ACPI_RSDP_REV_1:
+        memread(addr, rsdp_table, ACPI_RSDP_REV_1_LEN);
+        break;
+    case ACPI_RSDP_REV_2:
+        memread(addr, rsdp_table, ACPI_RSDP_REV_2_LEN);
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    ACPI_ASSERT_CMP64(*((uint64_t *)(rsdp_table)), ACPI_RSDP_SIGNATURE);
 }

tests/bios-tables-test.c +19 -8

index d661d9be62..8cde404bda 100644
@@ -27,7 +27,8 @@ typedef struct {
     const char *machine;
     const char *variant;
     uint32_t rsdp_addr;
-    AcpiRsdpDescriptor rsdp_table;
+    uint8_t rsdp_table[ACPI_RSDP_REV_2_LEN];
+    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 ACPI_RSDP_REV_1:
+        /* With rev 1, checksum is only for the first 20 bytes */
+        g_assert(!acpi_calc_checksum(rsdp_table,  ACPI_RSDP_REV_1_LEN));
+        break;
+    case ACPI_RSDP_REV_2:
+        /* With revision 2, we have 2 checksums */
+        g_assert(!acpi_calc_checksum(rsdp_table, ACPI_RSDP_REV_1_LEN));
+        g_assert(!acpi_calc_checksum(rsdp_table, ACPI_RSDP_REV_2_LEN));
+    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_find_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, ACPI_RSDP_REV_1);
     test_acpi_rsdt_table(data);
     fadt_fetch_facs_and_dsdt_ptrs(data);
     test_acpi_facs_table(data);

tests/vmgenid-test.c +5 -3

index 0a6fb55f2e..db5b084c18 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[ACPI_RSDP_REV_2_LEN];
     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, ACPI_RSDP_REV_1);
+
+    rsdt = acpi_find_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");