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

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

[Samuel Ortiz]

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

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

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

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

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 | 44 ++++++++++++++------
 include/hw/acpi/aml-build.h |  5 +++
 tests/acpi-utils.h          |  5 ++-
 hw/acpi/aml-build.c         | 81 +++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 35 ++--------------
 hw/i386/acpi-build.c        | 31 ++------------
 tests/acpi-utils.c          | 48 +++++++++++++++++-----
 tests/bios-tables-test.c    | 27 +++++++++----
 tests/vmgenid-test.c        |  8 ++--
 9 files changed, 193 insertions(+), 91 deletions(-)

-- 
2.19.1

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

[Samuel Ortiz]

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

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

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

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

[Samuel Ortiz]

From: Igor Mammedov <imammedo@redhat.com>

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

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

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

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

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

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

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

[Samuel Ortiz]

From: Igor Mammedov <imammedo@redhat.com>

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

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

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

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

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

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

[Samuel Ortiz]

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

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

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index af8e023968..e7fd24c6c5 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -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) */
 
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 0835900052..2dad465ecf 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
 
 /* RSDP */
 static void
-build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
+build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
 {
     AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
     unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
@@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
                              true /* fseg memory */);
 
     memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
-    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
+    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
     rsdp->length = cpu_to_le32(sizeof(*rsdp));
-    rsdp->revision = 0x02;
+    rsdp->revision = rsdp_data->revision;
 
     /* Address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
-        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
+        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
 
     /* Checksum to be filled by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
@@ -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);
-- 
2.19.1

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

[Samuel Ortiz]

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 <sameo@linux.intel.com>
---
 include/hw/acpi/acpi-defs.h | 22 ++++++++++++++
 hw/arm/virt-acpi-build.c    | 60 ++++++++++++++++++++++++-------------
 2 files changed, 61 insertions(+), 21 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index e7fd24c6c5..9b2f6b8043 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -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
+#define ACPI_RSDP_LEN_LEN      4
+#define ACPI_RSDP_CHECKSUM_LEN 1
+#define ACPI_RSDP_RESERVED_LEN 3
+
+/* RSDP offsets */
+#define ACPI_RSDP_CHECKSUM_OFFSET     8
+#define ACPI_RSDP_REVISION_OFFSET     15
+#define ACPI_RSDP_RSDT_OFFSET         16
+#define ACPI_RSDP_XSDT_OFFSET         24
+#define ACPI_RSDP_EXT_CHECKSUM_OFFSET 32
+
+
 /* Table structure from Linux kernel (the ACPI tables are under the
    BSD license) */
 
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 2dad465ecf..d47978a55e 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -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
-- 
2.19.1

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

[Samuel Ortiz]

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

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

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index d47978a55e..10f8388b63 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -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);
 
@@ -406,11 +427,6 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
     /* 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, 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, 0,
                                     ACPI_RSDP_REV_2_LEN,
-- 
2.19.1

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

[Samuel Ortiz]

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

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
---
 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(-)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 6c36903c0a..1bbe496e32 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -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
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 1e43cd736d..d4e15980f1 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -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,
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 10f8388b63..3fe86fd33e 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -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)
 {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index fb877648ac..f7ab112599 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2547,35 +2547,6 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
                  "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
 }
 
-static void
-build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
-{
-    /* AcpiRsdpDescriptor describes revision 2 RSDP table and as result we
-     * allocate extra 16 bytes for pc/q35 RSDP rev1 as well. Keep extra 16 bytes
-     * wasted to make sure we won't breake migration for machine types older
-     * than 2.3 due to size mismatch.
-     */
-    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
-    unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
-    unsigned rsdt_pa_offset =
-        (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
-
-    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
-                             true /* fseg memory */);
-
-    memcpy(&rsdp->signature, "RSD PTR ", 8);
-    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
-    /* Address to be filled by Guest linker */
-    bios_linker_loader_add_pointer(linker,
-        ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
-        ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
-
-    /* Checksum to be filled by Guest linker */
-    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
-        (char *)&rsdp->checksum - rsdp_table->data);
-}
-
 typedef
 struct AcpiBuildState {
     /* Copy of table in RAM (for patching). */
@@ -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);
@@ -2732,7 +2704,9 @@ 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);
+    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_1,
+                   &rsdt, NULL);
+    build_rsdp(tables->rsdp, tables->linker, &rsdp);
 
     /* We'll expose it all to Guest so we want to reduce
      * chance of size changes.
-- 
2.19.1

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

[Samuel Ortiz]

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 <sameo@linux.intel.com>
---
 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(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 9b2f6b8043..abef1d57ae 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -40,19 +40,6 @@ enum {
     ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE,
 };
 
-struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
-    uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
-    uint8_t  checksum;               /* To make sum of struct == 0 */
-    uint8_t  oem_id [6];             /* OEM identification */
-    uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
-    uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT */
-    uint32_t length;                 /* XSDT Length in bytes including hdr */
-    uint64_t xsdt_physical_address;  /* 64-bit physical address of XSDT */
-    uint8_t  extended_checksum;      /* Checksum of entire table */
-    uint8_t  reserved [3];           /* Reserved field must be 0 */
-} QEMU_PACKED;
-typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
-
 typedef struct AcpiRsdpData {
     uint8_t oem_id[6]; /* OEM identification */
     uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index ac52abd0dd..fb1a67a21f 100644
--- a/tests/acpi-utils.h
+++ b/tests/acpi-utils.h
@@ -82,6 +82,9 @@ typedef struct {
 
 uint8_t acpi_calc_checksum(const uint8_t *data, int len);
 uint32_t acpi_find_rsdp_address(void);
-void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table);
+uint32_t acpi_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 */
diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
index 41dc1ea9b4..c8fe2d1342 100644
--- a/tests/acpi-utils.c
+++ b/tests/acpi-utils.c
@@ -52,14 +52,44 @@ uint32_t acpi_find_rsdp_address(void)
     return off;
 }
 
-void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table)
+uint32_t acpi_find_rsdt_address(uint8_t *rsdp_table)
 {
-    ACPI_READ_FIELD(rsdp_table->signature, addr);
-    ACPI_ASSERT_CMP64(rsdp_table->signature, "RSD PTR ");
-
-    ACPI_READ_FIELD(rsdp_table->checksum, addr);
-    ACPI_READ_ARRAY(rsdp_table->oem_id, addr);
-    ACPI_READ_FIELD(rsdp_table->revision, addr);
-    ACPI_READ_FIELD(rsdp_table->rsdt_physical_address, addr);
-    ACPI_READ_FIELD(rsdp_table->length, addr);
+    uint32_t rsdt_physical_address = *((uint32_t *)(rsdp_table +
+                                                    ACPI_RSDP_RSDT_OFFSET));
+    uint8_t revision = rsdp_table[ACPI_RSDP_REVISION_OFFSET];
+
+    if (revision != ACPI_RSDP_REV_1) {
+        return 0;
+    }
+
+    return le32_to_cpu(rsdt_physical_address);
+}
+
+uint64_t acpi_find_xsdt_address(uint8_t *rsdp_table)
+{
+    uint64_t xsdt_physical_address = *((uint64_t *)(rsdp_table +
+                                                    ACPI_RSDP_XSDT_OFFSET));
+    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);
 }
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index d661d9be62..8cde404bda 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -27,7 +27,8 @@ typedef struct {
     const char *machine;
     const char *variant;
     uint32_t rsdp_addr;
-    AcpiRsdpDescriptor rsdp_table;
+    uint8_t rsdp_table[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);
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
index 0a6fb55f2e..db5b084c18 100644
--- a/tests/vmgenid-test.c
+++ b/tests/vmgenid-test.c
@@ -35,7 +35,7 @@ static uint32_t acpi_find_vgia(void)
 {
     uint32_t rsdp_offset;
     uint32_t guid_offset = 0;
-    AcpiRsdpDescriptor rsdp_table;
+    uint8_t rsdp_table[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");
-- 
2.19.1

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

[Philippe Mathieu-Daudé]

On 26/11/18 17:29, Samuel Ortiz wrote:
> For both x86 and ARM architectures, the internal RSDP build API can
> return void as the current return value is unused.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

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

[Philippe Mathieu-Daudé]

Hi Samuel,

On 26/11/18 17:29, Samuel Ortiz wrote:
> Now that build_rsdp() supports building both legacy and current RSDP
> tables, we can move it to a generic folder (hw/acpi) and have the i386
> ACPI code reuse it in order to reduce code duplication.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  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(-)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 6c36903c0a..1bbe496e32 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -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
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 1e43cd736d..d4e15980f1 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -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,
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 10f8388b63..3fe86fd33e 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -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)
>  {
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index fb877648ac..f7ab112599 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2547,35 +2547,6 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
>                   "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
>  }
>  
> -static void
> -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
> -{
> -    /* AcpiRsdpDescriptor describes revision 2 RSDP table and as result we
> -     * allocate extra 16 bytes for pc/q35 RSDP rev1 as well. Keep extra 16 bytes
> -     * wasted to make sure we won't breake migration for machine types older
> -     * than 2.3 due to size mismatch.
> -     */
> -    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> -    unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
> -    unsigned rsdt_pa_offset =
> -        (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
> -
> -    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
> -                             true /* fseg memory */);
> -
> -    memcpy(&rsdp->signature, "RSD PTR ", 8);
> -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
> -    /* Address to be filled by Guest linker */
> -    bios_linker_loader_add_pointer(linker,
> -        ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
> -        ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
> -
> -    /* Checksum to be filled by Guest linker */
> -    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> -        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
> -        (char *)&rsdp->checksum - rsdp_table->data);
> -}
> -
>  typedef
>  struct AcpiBuildState {
>      /* Copy of table in RAM (for patching). */
> @@ -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);
> @@ -2732,7 +2704,9 @@ 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);
> +    init_rsdp_data(&rsdp, ACPI_BUILD_APPNAME6, ACPI_RSDP_REV_1,
> +                   &rsdt, NULL);

If you ever respin this series, please consider split this patch in 2:
- Export/use init_rsdp_data()
- current patch
This will ease review. The patch is fine regardless :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

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

[Philippe Mathieu-Daudé]

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

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  include/hw/acpi/acpi-defs.h | 11 +++++++++++
>  hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
>  2 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index af8e023968..e7fd24c6c5 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -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) */
>  
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 0835900052..2dad465ecf 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>  
>  /* RSDP */
>  static void
> -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
>  {
>      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
>      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
>                               true /* fseg memory */);
>  
>      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
>      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> -    rsdp->revision = 0x02;
> +    rsdp->revision = rsdp_data->revision;
>  
>      /* Address to be filled by Guest linker */
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
>  
>      /* Checksum to be filled by Guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> @@ -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);
> 

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

[Thomas Huth]

On 2018-11-26 17:29, Samuel Ortiz wrote:
> For both x86 and ARM architectures, the internal RSDP build API can
> return void as the current return value is unused.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 4 +---
>  hw/i386/acpi-build.c     | 4 +---
>  2 files changed, 2 insertions(+), 6 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Igor Mammedov]

On Mon, 26 Nov 2018 17:29:35 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

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

For future reference, if one re-sends someone else patch, one is supposed to
add his/her own SoB as the last one.

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

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

[Igor Mammedov]

On Mon, 26 Nov 2018 17:29:37 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> That will allow us to generalize the ARM build_rsdp() routine to support
> both legacy RSDP (The current i386 implementation) and extended RSDP
> (The ARM implementation).
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  include/hw/acpi/acpi-defs.h | 11 +++++++++++
>  hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
>  2 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index af8e023968..e7fd24c6c5 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -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
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.

> +
>  /* Table structure from Linux kernel (the ACPI tables are under the
>     BSD license) */
>  
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 0835900052..2dad465ecf 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>  
>  /* RSDP */
>  static void
> -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
>  {
>      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
>      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
>                               true /* fseg memory */);
>  
>      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
>      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> -    rsdp->revision = 0x02;
> +    rsdp->revision = rsdp_data->revision;
>  
>      /* Address to be filled by Guest linker */
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
>  
>      /* Checksum to be filled by Guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> @@ -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;
s/rsdp/rsdp_info/

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

-    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);
+    }

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

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

[Samuel Ortiz]

Hi Igor,

On Tue, Nov 27, 2018 at 04:25:51PM +0100, Igor Mammedov wrote:
> On Mon, 26 Nov 2018 17:29:37 +0100
> Samuel Ortiz <sameo@linux.intel.com> wrote:
> 
> > That will allow us to generalize the ARM build_rsdp() routine to support
> > both legacy RSDP (The current i386 implementation) and extended RSDP
> > (The ARM implementation).
> > 
> > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > ---
> >  include/hw/acpi/acpi-defs.h | 11 +++++++++++
> >  hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
> >  2 files changed, 33 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index af8e023968..e7fd24c6c5 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -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
> 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.


> > +
> >  /* Table structure from Linux kernel (the ACPI tables are under the
> >     BSD license) */
> >  
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 0835900052..2dad465ecf 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> >  
> >  /* RSDP */
> >  static void
> > -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> >  {
> >      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> >      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> > @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> >                               true /* fseg memory */);
> >  
> >      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> > -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> > +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
> >      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> > -    rsdp->revision = 0x02;
> > +    rsdp->revision = rsdp_data->revision;
> >  
> >      /* Address to be filled by Guest linker */
> >      bios_linker_loader_add_pointer(linker,
> >          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> > -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> > +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
> >  
> >      /* Checksum to be filled by Guest linker */
> >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > @@ -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;
> s/rsdp/rsdp_info/
> 
> >  
> >      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:
> 
> -    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);
> +    }
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.

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

[Igor Mammedov]

On Mon, 26 Nov 2018 17:29:38 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> 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.
this is not a goal, it is just byproduct when conversion is complete.
So I'd mention it in the patch that switches pc variant to a generic one
and removes structure that it's no longer in use.

maybe slightly rephrase message:
"
Instead of filling a mapped and packed C structure field in random
order and being careful about endianness and sizes, just use
build_append_int_noprefix() to compose RSDP table.
As result we will have almost 1:1 code/spec match, that's easy
to review and maintain (endian-agnostic, no pointer/sizeof math
only explicit values from spec).
"

> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  include/hw/acpi/acpi-defs.h | 22 ++++++++++++++
>  hw/arm/virt-acpi-build.c    | 60 ++++++++++++++++++++++++-------------
>  2 files changed, 61 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index e7fd24c6c5..9b2f6b8043 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -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
> +#define ACPI_RSDP_LEN_LEN      4
> +#define ACPI_RSDP_CHECKSUM_LEN 1
> +#define ACPI_RSDP_RESERVED_LEN 3
> +
> +/* RSDP offsets */
> +#define ACPI_RSDP_CHECKSUM_OFFSET     8
> +#define ACPI_RSDP_REVISION_OFFSET     15
> +#define ACPI_RSDP_RSDT_OFFSET         16
> +#define ACPI_RSDP_XSDT_OFFSET         24
> +#define ACPI_RSDP_EXT_CHECKSUM_OFFSET 32
likewise in previous patch, these defines aren't reused elsewhere
so it's just an extra jumps for reader to verify spec compliance.
Use constants directly with a comment beside it on the same line
if space allows or above (comment must match spec's field name
so it would be easy to find field in the spec),
see build_fadt() as an example

Beside above notes patch looks good.

> +
> +
>  /* Table structure from Linux kernel (the ACPI tables are under the
>     BSD license) */
>  
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 2dad465ecf..d47978a55e 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -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

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

[Igor Mammedov]

On Tue, 27 Nov 2018 16:42:18 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> Hi Igor,
> 
> On Tue, Nov 27, 2018 at 04:25:51PM +0100, Igor Mammedov wrote:
> > On Mon, 26 Nov 2018 17:29:37 +0100
> > Samuel Ortiz <sameo@linux.intel.com> wrote:
> >   
> > > That will allow us to generalize the ARM build_rsdp() routine to support
> > > both legacy RSDP (The current i386 implementation) and extended RSDP
> > > (The ARM implementation).
> > > 
> > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > ---
> > >  include/hw/acpi/acpi-defs.h | 11 +++++++++++
> > >  hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
> > >  2 files changed, 33 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > index af8e023968..e7fd24c6c5 100644
> > > --- a/include/hw/acpi/acpi-defs.h
> > > +++ b/include/hw/acpi/acpi-defs.h
> > > @@ -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  
> > 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

> 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

> 
> 
> > > +
> > >  /* Table structure from Linux kernel (the ACPI tables are under the
> > >     BSD license) */
> > >  
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 0835900052..2dad465ecf 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > >  
> > >  /* RSDP */
> > >  static void
> > > -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> > >  {
> > >      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> > >      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> > > @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > >                               true /* fseg memory */);
> > >  
> > >      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> > > -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> > > +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
> > >      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> > > -    rsdp->revision = 0x02;
> > > +    rsdp->revision = rsdp_data->revision;
> > >  
> > >      /* Address to be filled by Guest linker */
> > >      bios_linker_loader_add_pointer(linker,
> > >          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> > > -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> > > +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
> > >  
> > >      /* Checksum to be filled by Guest linker */
> > >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > > @@ -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;  
> > s/rsdp/rsdp_info/
> >   
> > >  
> > >      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:
> > 
> > -    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);
> > +    }  
> 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.

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

> 
> Cheers,
> Samuel.

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

[Igor Mammedov]

On Mon, 26 Nov 2018 17:29:39 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> We add the ability to build legacy or current RSDP tables, based on the
> AcpiRsdpData revision field passed to build_rsdp().
> Although arm/virt only uses RSDP v2, adding that capability to
> build_rsdp will allow us to share the RSDP build code between ARM and x86.
> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  hw/arm/virt-acpi-build.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index d47978a55e..10f8388b63 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -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);
>  
> @@ -406,11 +427,6 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
>      /* 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, 0,
> -                                    ACPI_RSDP_REV_1_LEN,
> -                                    ACPI_RSDP_CHECKSUM_OFFSET);
> -
maybe put this hunk in 5/8 at the place where are you moving it here, so you don't have to touch it twice
(I'll leave it up to you)
otherwise looks good 


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

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

[Michael S. Tsirkin]

On Tue, Nov 27, 2018 at 05:27:49PM +0100, Igor Mammedov wrote:
> On Tue, 27 Nov 2018 16:42:18 +0100
> Samuel Ortiz <sameo@linux.intel.com> wrote:
> 
> > Hi Igor,
> > 
> > On Tue, Nov 27, 2018 at 04:25:51PM +0100, Igor Mammedov wrote:
> > > On Mon, 26 Nov 2018 17:29:37 +0100
> > > Samuel Ortiz <sameo@linux.intel.com> wrote:
> > >   
> > > > That will allow us to generalize the ARM build_rsdp() routine to support
> > > > both legacy RSDP (The current i386 implementation) and extended RSDP
> > > > (The ARM implementation).
> > > > 
> > > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > > ---
> > > >  include/hw/acpi/acpi-defs.h | 11 +++++++++++
> > > >  hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
> > > >  2 files changed, 33 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > > index af8e023968..e7fd24c6c5 100644
> > > > --- a/include/hw/acpi/acpi-defs.h
> > > > +++ b/include/hw/acpi/acpi-defs.h
> > > > @@ -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  
> > > 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
> 
> > 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.

> > 
> > 
> > > > +
> > > >  /* Table structure from Linux kernel (the ACPI tables are under the
> > > >     BSD license) */
> > > >  
> > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > index 0835900052..2dad465ecf 100644
> > > > --- a/hw/arm/virt-acpi-build.c
> > > > +++ b/hw/arm/virt-acpi-build.c
> > > > @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > > >  
> > > >  /* RSDP */
> > > >  static void
> > > > -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > > +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> > > >  {
> > > >      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> > > >      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> > > > @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > >                               true /* fseg memory */);
> > > >  
> > > >      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> > > > -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> > > > +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
> > > >      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> > > > -    rsdp->revision = 0x02;
> > > > +    rsdp->revision = rsdp_data->revision;
> > > >  
> > > >      /* Address to be filled by Guest linker */
> > > >      bios_linker_loader_add_pointer(linker,
> > > >          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> > > > -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> > > > +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
> > > >  
> > > >      /* Checksum to be filled by Guest linker */
> > > >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > > > @@ -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;  
> > > s/rsdp/rsdp_info/
> > >   
> > > >  
> > > >      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:
> > > 
> > > -    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);
> > > +    }  
> > 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.
> 
> > - 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.
> 
> > 
> > Cheers,
> > Samuel.

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

[Samuel Ortiz]

On Tue, Nov 27, 2018 at 05:27:49PM +0100, Igor Mammedov wrote:
> On Tue, 27 Nov 2018 16:42:18 +0100
> Samuel Ortiz <sameo@linux.intel.com> wrote:
> 
> > Hi Igor,
> > 
> > On Tue, Nov 27, 2018 at 04:25:51PM +0100, Igor Mammedov wrote:
> > > On Mon, 26 Nov 2018 17:29:37 +0100
> > > Samuel Ortiz <sameo@linux.intel.com> wrote:
> > >   
> > > > That will allow us to generalize the ARM build_rsdp() routine to support
> > > > both legacy RSDP (The current i386 implementation) and extended RSDP
> > > > (The ARM implementation).
> > > > 
> > > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > > ---
> > > >  include/hw/acpi/acpi-defs.h | 11 +++++++++++
> > > >  hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
> > > >  2 files changed, 33 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > > index af8e023968..e7fd24c6c5 100644
> > > > --- a/include/hw/acpi/acpi-defs.h
> > > > +++ b/include/hw/acpi/acpi-defs.h
> > > > @@ -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  
> > > 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.

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.


> that way if we break it in qemu's code test would catch the thing
> 
> > 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
> 
> > 
> > 
> > > > +
> > > >  /* Table structure from Linux kernel (the ACPI tables are under the
> > > >     BSD license) */
> > > >  
> > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > index 0835900052..2dad465ecf 100644
> > > > --- a/hw/arm/virt-acpi-build.c
> > > > +++ b/hw/arm/virt-acpi-build.c
> > > > @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > > >  
> > > >  /* RSDP */
> > > >  static void
> > > > -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > > +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> > > >  {
> > > >      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> > > >      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> > > > @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > >                               true /* fseg memory */);
> > > >  
> > > >      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> > > > -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> > > > +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
> > > >      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> > > > -    rsdp->revision = 0x02;
> > > > +    rsdp->revision = rsdp_data->revision;
> > > >  
> > > >      /* Address to be filled by Guest linker */
> > > >      bios_linker_loader_add_pointer(linker,
> > > >          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> > > > -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> > > > +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
> > > >  
> > > >      /* Checksum to be filled by Guest linker */
> > > >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > > > @@ -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;  
> > > s/rsdp/rsdp_info/
> > >   
> > > >  
> > > >      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:
> > > 
> > > -    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);
> > > +    }  
> > 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.

> If it's simple structure it is clearer to use initializer, instead of
> wrapper helper. With complex structure it could be other way around.
>
> > - 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.

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

[Igor Mammedov]

On Mon, 26 Nov 2018 17:29:41 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> 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.
I'd rephrase it as following:

The sole user of AcpiRsdpDescriptor structure is test,
drop test dependency on it and remove no longer used structure.


> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  include/hw/acpi/acpi-defs.h | 13 ----------
>  tests/acpi-utils.h          |  5 +++-
>  tests/acpi-utils.c          | 48 ++++++++++++++++++++++++++++++-------
>  tests/bios-tables-test.c    | 27 ++++++++++++++-------
>  tests/vmgenid-test.c        |  8 ++++---
>  5 files changed, 67 insertions(+), 34 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 9b2f6b8043..abef1d57ae 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -40,19 +40,6 @@ enum {
>      ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE,
>  };
>  
> -struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
> -    uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
> -    uint8_t  checksum;               /* To make sum of struct == 0 */
> -    uint8_t  oem_id [6];             /* OEM identification */
> -    uint8_t  revision;               /* Must be 0 for 1.0, 2 for 2.0 */
> -    uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT */
> -    uint32_t length;                 /* XSDT Length in bytes including hdr */
> -    uint64_t xsdt_physical_address;  /* 64-bit physical address of XSDT */
> -    uint8_t  extended_checksum;      /* Checksum of entire table */
> -    uint8_t  reserved [3];           /* Reserved field must be 0 */
> -} QEMU_PACKED;
> -typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
> -
>  typedef struct AcpiRsdpData {
>      uint8_t oem_id[6]; /* OEM identification */
>      uint8_t revision;  /* Must be 0 for 1.0, 2 for 2.0 */
> diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> index ac52abd0dd..fb1a67a21f 100644
> --- a/tests/acpi-utils.h
> +++ b/tests/acpi-utils.h
> @@ -82,6 +82,9 @@ typedef struct {
>  
>  uint8_t acpi_calc_checksum(const uint8_t *data, int len);
>  uint32_t acpi_find_rsdp_address(void);
> -void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table);
> +uint32_t acpi_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 */
> diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
> index 41dc1ea9b4..c8fe2d1342 100644
> --- a/tests/acpi-utils.c
> +++ b/tests/acpi-utils.c
> @@ -52,14 +52,44 @@ uint32_t acpi_find_rsdp_address(void)
>      return off;
>  }
>  
> -void acpi_parse_rsdp_table(uint32_t addr, AcpiRsdpDescriptor *rsdp_table)
> +uint32_t acpi_find_rsdt_address(uint8_t *rsdp_table)
s/acpi_find_rsdt_address/acpi_get_rsdt_address/
likewise for xsdt

>  {
> -    ACPI_READ_FIELD(rsdp_table->signature, addr);
> -    ACPI_ASSERT_CMP64(rsdp_table->signature, "RSD PTR ");
> -
> -    ACPI_READ_FIELD(rsdp_table->checksum, addr);
> -    ACPI_READ_ARRAY(rsdp_table->oem_id, addr);
> -    ACPI_READ_FIELD(rsdp_table->revision, addr);
> -    ACPI_READ_FIELD(rsdp_table->rsdt_physical_address, addr);
> -    ACPI_READ_FIELD(rsdp_table->length, addr);
> +    uint32_t rsdt_physical_address = *((uint32_t *)(rsdp_table +
> +                                                    ACPI_RSDP_RSDT_OFFSET));
above cast looks pretty ugly and on some hw it might trigger SIGBUS
maybe:

       memcpy(addr, &rsdp_table[16 /* RsdtAddress offset */], 4);

> +    uint8_t revision = rsdp_table[ACPI_RSDP_REVISION_OFFSET];
> +
> +    if (revision != ACPI_RSDP_REV_1) {
theoretically not rev1 might have rsdt pointer as well for legacy OS
but we don't do this in QEMU, so it doesn't really matter

> +        return 0;
> +    }
> +
> +    return le32_to_cpu(rsdt_physical_address);
> +}
> +
> +uint64_t acpi_find_xsdt_address(uint8_t *rsdp_table)
> +{
> +    uint64_t xsdt_physical_address = *((uint64_t *)(rsdp_table +
> +                                                    ACPI_RSDP_XSDT_OFFSET));
access to uninitialized data if not rev2,
here it's not harmful but it's better not to confuse tools and fetch value after revision check

> +    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);
>  }
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index d661d9be62..8cde404bda 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -27,7 +27,8 @@ typedef struct {
>      const char *machine;
>      const char *variant;
>      uint32_t rsdp_addr;
> -    AcpiRsdpDescriptor rsdp_table;
> +    uint8_t rsdp_table[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);
> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
> index 0a6fb55f2e..db5b084c18 100644
> --- a/tests/vmgenid-test.c
> +++ b/tests/vmgenid-test.c
> @@ -35,7 +35,7 @@ static uint32_t acpi_find_vgia(void)
>  {
>      uint32_t rsdp_offset;
>      uint32_t guid_offset = 0;
> -    AcpiRsdpDescriptor rsdp_table;
> +    uint8_t rsdp_table[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");

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

[Samuel Ortiz]

Hi Michael,

On Tue, Nov 27, 2018 at 10:26:30PM -0500, Michael S. Tsirkin wrote:
> On Tue, Nov 27, 2018 at 05:27:49PM +0100, Igor Mammedov wrote:
> > On Tue, 27 Nov 2018 16:42:18 +0100
> > Samuel Ortiz <sameo@linux.intel.com> wrote:
> > 
> > > Hi Igor,
> > > 
> > > On Tue, Nov 27, 2018 at 04:25:51PM +0100, Igor Mammedov wrote:
> > > > On Mon, 26 Nov 2018 17:29:37 +0100
> > > > Samuel Ortiz <sameo@linux.intel.com> wrote:
> > > >   
> > > > > That will allow us to generalize the ARM build_rsdp() routine to support
> > > > > both legacy RSDP (The current i386 implementation) and extended RSDP
> > > > > (The ARM implementation).
> > > > > 
> > > > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > > > ---
> > > > >  include/hw/acpi/acpi-defs.h | 11 +++++++++++
> > > > >  hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
> > > > >  2 files changed, 33 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > > > index af8e023968..e7fd24c6c5 100644
> > > > > --- a/include/hw/acpi/acpi-defs.h
> > > > > +++ b/include/hw/acpi/acpi-defs.h
> > > > > @@ -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  
> > > > 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
> > 
> > > 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,
I find that defining a set of well named constants is a lot less painful
than maintaining code with at least the same amount of hard coded
constants. That's a personal opinion, for sure.

> 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).
I may be misreading the spec, but I understand 0 is for ACPI 1.0 and 2
is for ACPI 2.0+. The latest spec is a little confusing with regard to
this field, but when looking at the 2.0a ACPI spec for RSDP:

"The ACPI version 1.0 revision number of this table is zero. The ACPI 2.0
value for this field is 2."

> 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.
Not entirely sure how this materializes. Do you mean that e.g. if I want
to check for an RSDP revision I'd have to define inline functions of
that kind:

bool is_rsdp_revision_0(uint8_t *rsdp_table);
bool is_rsdp_revision_2(uint8_t *rsdp_table);

or do you have something else in mind?

Cheers,
Samuel.

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

[Samuel Ortiz]

On Wed, Nov 28, 2018 at 10:46:41AM +0100, Samuel Ortiz wrote:
> On Tue, Nov 27, 2018 at 05:27:49PM +0100, Igor Mammedov wrote:
> > On Tue, 27 Nov 2018 16:42:18 +0100
> > Samuel Ortiz <sameo@linux.intel.com> wrote:
> > 
> > > Hi Igor,
> > > 
> > > On Tue, Nov 27, 2018 at 04:25:51PM +0100, Igor Mammedov wrote:
> > > > On Mon, 26 Nov 2018 17:29:37 +0100
> > > > Samuel Ortiz <sameo@linux.intel.com> wrote:
> > > >   
> > > > > That will allow us to generalize the ARM build_rsdp() routine to support
> > > > > both legacy RSDP (The current i386 implementation) and extended RSDP
> > > > > (The ARM implementation).
> > > > > 
> > > > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > > > ---
> > > > >  include/hw/acpi/acpi-defs.h | 11 +++++++++++
> > > > >  hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
> > > > >  2 files changed, 33 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > > > index af8e023968..e7fd24c6c5 100644
> > > > > --- a/include/hw/acpi/acpi-defs.h
> > > > > +++ b/include/hw/acpi/acpi-defs.h
> > > > > @@ -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  
> > > > 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.
> 
> 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.
> 
> 
> > that way if we break it in qemu's code test would catch the thing
> > 
> > > 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
> > 
> > > 
> > > 
> > > > > +
> > > > >  /* Table structure from Linux kernel (the ACPI tables are under the
> > > > >     BSD license) */
> > > > >  
> > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > > index 0835900052..2dad465ecf 100644
> > > > > --- a/hw/arm/virt-acpi-build.c
> > > > > +++ b/hw/arm/virt-acpi-build.c
> > > > > @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > > > >  
> > > > >  /* RSDP */
> > > > >  static void
> > > > > -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > > > +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> > > > >  {
> > > > >      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> > > > >      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> > > > > @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > > >                               true /* fseg memory */);
> > > > >  
> > > > >      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> > > > > -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> > > > > +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
> > > > >      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> > > > > -    rsdp->revision = 0x02;
> > > > > +    rsdp->revision = rsdp_data->revision;
> > > > >  
> > > > >      /* Address to be filled by Guest linker */
> > > > >      bios_linker_loader_add_pointer(linker,
> > > > >          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> > > > > -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> > > > > +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
> > > > >  
> > > > >      /* Checksum to be filled by Guest linker */
> > > > >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > > > > @@ -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;  
> > > > s/rsdp/rsdp_info/
> > > >   
> > > > >  
> > > > >      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:
> > > > 
> > > > -    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);
> > > > +    }  
> > > 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.
> 
> > If it's simple structure it is clearer to use initializer, instead of
> > wrapper helper. With complex structure it could be other way around.
> >
> > > - 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?
I just saw your comments on patch #8, thanks.

Cheers,
Samuel.

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

[Igor Mammedov]

On Wed, 28 Nov 2018 10:46:41 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> On Tue, Nov 27, 2018 at 05:27:49PM +0100, Igor Mammedov wrote:
> > On Tue, 27 Nov 2018 16:42:18 +0100
> > Samuel Ortiz <sameo@linux.intel.com> wrote:
> >   
> > > Hi Igor,
> > > 
> > > On Tue, Nov 27, 2018 at 04:25:51PM +0100, Igor Mammedov wrote:  
> > > > On Mon, 26 Nov 2018 17:29:37 +0100
> > > > Samuel Ortiz <sameo@linux.intel.com> wrote:
> > > >     
> > > > > That will allow us to generalize the ARM build_rsdp() routine to support
> > > > > both legacy RSDP (The current i386 implementation) and extended RSDP
> > > > > (The ARM implementation).
> > > > > 
> > > > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > > > ---
> > > > >  include/hw/acpi/acpi-defs.h | 11 +++++++++++
> > > > >  hw/arm/virt-acpi-build.c    | 27 ++++++++++++++++++++++-----
> > > > >  2 files changed, 33 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > > > index af8e023968..e7fd24c6c5 100644
> > > > > --- a/include/hw/acpi/acpi-defs.h
> > > > > +++ b/include/hw/acpi/acpi-defs.h
> > > > > @@ -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    
> > > > 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.  
> 
> 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.
Even though it's ugly, constants directly comparable to spec table
definitions and comments are supposed to hep locate relevant field/table,
macro isn't making it better in this case (just a bit more obscure).

At least for ACPI spec bound code, it's consensus that we reached
after discussing how to better to handle this usecase (from maintainability pov).

> > that way if we break it in qemu's code test would catch the thing
> >   
> > > 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
> >   
> > > 
> > >   
> > > > > +
> > > > >  /* Table structure from Linux kernel (the ACPI tables are under the
> > > > >     BSD license) */
> > > > >  
> > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > > index 0835900052..2dad465ecf 100644
> > > > > --- a/hw/arm/virt-acpi-build.c
> > > > > +++ b/hw/arm/virt-acpi-build.c
> > > > > @@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > > > >  
> > > > >  /* RSDP */
> > > > >  static void
> > > > > -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > > > +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> > > > >  {
> > > > >      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> > > > >      unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> > > > > @@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > > >                               true /* fseg memory */);
> > > > >  
> > > > >      memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> > > > > -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> > > > > +    memcpy(rsdp->oem_id, rsdp_data->oem_id, sizeof(rsdp->oem_id));
> > > > >      rsdp->length = cpu_to_le32(sizeof(*rsdp));
> > > > > -    rsdp->revision = 0x02;
> > > > > +    rsdp->revision = rsdp_data->revision;
> > > > >  
> > > > >      /* Address to be filled by Guest linker */
> > > > >      bios_linker_loader_add_pointer(linker,
> > > > >          ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> > > > > -        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> > > > > +        ACPI_BUILD_TABLE_FILE, *rsdp_data->xsdt_tbl_offset);
> > > > >  
> > > > >      /* Checksum to be filled by Guest linker */
> > > > >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > > > > @@ -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;    
> > > > s/rsdp/rsdp_info/
> > > >     
> > > > >  
> > > > >      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:
> > > > 
> > > > -    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);
> > > > +    }    
> > > 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.
true, one has to be more careful with initializing.
I don't have a strong opinion here, I'll leave it up to you.

> > If it's simple structure it is clearer to use initializer, instead of
> > wrapper helper. With complex structure it could be other way around.
> >  
> > > - 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.
> 
>