[Qemu-devel] [PATCH v2 0/8] tests: apci: consolidate and cleanup ACPI test code

[Qemu-devel] [PATCH v2 0/8] tests: apci: consolidate and cleanup ACPI test code

[Igor Mammedov]

Changes since v1:
  * rebase on top of current master due to a lots of conflicts with
    'qtest global' removal being merged first
  * drop explicit cast to uint8_t* as sdt->aml is uint8_t* now
  * drop not comment explaining strange offsets as offsets are
    now follow ACPI spec

While working on adding tests for virt/arm board (uefi/XSDT/64-bit table pointers),
I found it's rather difficult to deal with mixed ACPI testing code that we've
collected so far. So instead of just adding a pile of XSDT hacks on top, here
goes small refactoring series:
   * that removes dead code
   * replaces reading tables with a fetch per table everywhere instead of
     mix of field by field and whole table
   * consolidates the way tables are read (reduces code duplication)
   * test no longer depends on ACPI structures from QEMU (i.e. doesn't affected
     by mistakes there) 
   * fixes FACS not being compared against reference tables
Overall test is reduced on ~160LOC and hopefully it makes easier to follow and
add more stuff on top.

PS:
arm/virt test patches fill follow up a separate series on top of this one
for not to mix things up

Git tree for testing:
  https://github.com/imammedo/qemu acpi_tests_cleanup_v2

CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Thomas Huth <thuth@redhat.com>
CC: Laurent Vivier <lvivier@redhat.com>
CC: Samuel Ortiz <sameo@linux.intel.com>
CC: wainersm@redhat.com


Igor Mammedov (8):
  tests: acpi: use AcpiSdtTable::aml in consistent way
  tests: acpi: make sure FADT is fetched only once
  tests: acpi: simplify rsdt handling
  tests: acpi: reuse fetch_table() for fetching FACS and DSDT
  tests: acpi: reuse fetch_table() in vmgenid-test
  tests: smbios: fetch whole table in one step instead of reading it
    step by step
  tests: acpi: squash sanitize_fadt_ptrs() into test_acpi_fadt_table()
  tests: acpi: use AcpiSdtTable::aml instead of
    AcpiSdtTable::header::signature

 tests/acpi-utils.h       |  44 ++------
 tests/acpi-utils.c       |  35 +++++--
 tests/bios-tables-test.c | 259 ++++++++++++-----------------------------------
 tests/vmgenid-test.c     |  64 ++++--------
 4 files changed, 121 insertions(+), 281 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/8] tests: acpi: use AcpiSdtTable::aml in consistent way

[Igor Mammedov]

Currently in the 1st case we store table body fetched from QEMU in
AcpiSdtTable::aml minus it's header but in the 2nd case when we
load reference aml from disk, it holds whole blob including header.
More over in the 1st case, we read header in separate AcpiSdtTable::header
structure and then jump over hoops to fixup tables and combine both.

Treat AcpiSdtTable::aml as whole table blob approach in both cases
and when fetching tables from QEMU, first get table length and then
fetch whole table into AcpiSdtTable::aml instead if doing it field
by field.

As result
 * AcpiSdtTable::aml is used in consistent manner
 * FADT fixups use offsets from spec instead of being shifted by
   header length
 * calculating checksums and dumping blobs becomes simpler

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
 * rebase: s/memread/qtest_memread/
 * drop explicit cast to uint8_t* as sdt->aml is uint8_t* now
     (Wainer dos Santos Moschetta <wainersm@redhat.com>)
 * drop not comment explaining starnge offsets as offsets are
   now follow APCI spec
---
 tests/acpi-utils.h       |  6 +++--
 tests/bios-tables-test.c | 64 ++++++++++++++++++------------------------------
 2 files changed, 28 insertions(+), 42 deletions(-)

diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index c5b0e12..1b0e80d 100644
--- a/tests/acpi-utils.h
+++ b/tests/acpi-utils.h
@@ -18,8 +18,10 @@
 
 /* DSDT and SSDTs format */
 typedef struct {
-    AcpiTableHeader header;
-    gchar *aml;            /* aml bytecode from guest */
+    union {
+        AcpiTableHeader *header;
+        uint8_t *aml;            /* aml bytecode from guest */
+    };
     gsize aml_len;
     gchar *aml_file;
     gchar *asl;            /* asl code generated from aml */
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index d455b2a..3f20bbd 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -163,29 +163,23 @@ static void sanitize_fadt_ptrs(test_data *data)
     for (i = 0; i < data->tables->len; i++) {
         AcpiSdtTable *sdt = &g_array_index(data->tables, AcpiSdtTable, i);
 
-        if (memcmp(&sdt->header.signature, "FACP", 4)) {
+        if (memcmp(&sdt->header->signature, "FACP", 4)) {
             continue;
         }
 
         /* check original FADT checksum before sanitizing table */
-        g_assert(!(uint8_t)(
-            acpi_calc_checksum((uint8_t *)sdt, sizeof(AcpiTableHeader)) +
-            acpi_calc_checksum((uint8_t *)sdt->aml, sdt->aml_len)
-        ));
-
-        /* 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 */
+        g_assert(!acpi_calc_checksum(sdt->aml, sdt->aml_len));
+
+        memset(sdt->aml + 36, 0, 4); /* sanitize FIRMWARE_CTRL ptr */
+        memset(sdt->aml + 40, 0, 4); /* sanitize DSDT ptr */
+        if (sdt->header->revision >= 3) {
+            memset(sdt->aml + 132, 0, 8); /* sanitize X_FIRMWARE_CTRL ptr */
+            memset(sdt->aml + 140, 0, 8); /* sanitize X_DSDT ptr */
         }
 
         /* update checksum */
-        sdt->header.checksum = 0;
-        sdt->header.checksum -=
-            acpi_calc_checksum((uint8_t *)sdt, sizeof(AcpiTableHeader)) +
-            acpi_calc_checksum((uint8_t *)sdt->aml, sdt->aml_len);
+        sdt->header->checksum = 0;
+        sdt->header->checksum -= acpi_calc_checksum(sdt->aml, sdt->aml_len);
         break;
     }
 }
@@ -212,30 +206,23 @@ static void test_acpi_facs_table(test_data *data)
  */
 static void fetch_table(QTestState *qts, AcpiSdtTable *sdt_table, uint32_t addr)
 {
-    uint8_t checksum;
-
-    memset(sdt_table, 0, sizeof(*sdt_table));
-    ACPI_READ_TABLE_HEADER(qts, &sdt_table->header, addr);
-
-    sdt_table->aml_len = le32_to_cpu(sdt_table->header.length)
-                         - sizeof(AcpiTableHeader);
+    qtest_memread(qts, addr + 4 /* Length of ACPI table */,
+                  &sdt_table->aml_len, 4);
+    sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
     sdt_table->aml = g_malloc0(sdt_table->aml_len);
-    ACPI_READ_ARRAY_PTR(qts, sdt_table->aml, sdt_table->aml_len, addr);
+    /* get whole table */
+    qtest_memread(qts, addr, sdt_table->aml, sdt_table->aml_len);
 
-    checksum = acpi_calc_checksum((uint8_t *)sdt_table,
-                                  sizeof(AcpiTableHeader)) +
-               acpi_calc_checksum((uint8_t *)sdt_table->aml,
-                                  sdt_table->aml_len);
-    g_assert(!checksum);
+    g_assert(!acpi_calc_checksum(sdt_table->aml, sdt_table->aml_len));
 }
 
 static void test_acpi_dsdt_table(test_data *data)
 {
-    AcpiSdtTable dsdt_table;
+    AcpiSdtTable dsdt_table = {};
     uint32_t addr = le32_to_cpu(data->dsdt_addr);
 
     fetch_table(data->qts, &dsdt_table, addr);
-    ACPI_ASSERT_CMP(dsdt_table.header.signature, "DSDT");
+    ACPI_ASSERT_CMP(dsdt_table.header->signature, "DSDT");
 
     /* Since DSDT isn't in RSDT, add DSDT to ASL test tables list manually */
     g_array_append_val(data->tables, dsdt_table);
@@ -248,7 +235,7 @@ static void fetch_rsdt_referenced_tables(test_data *data)
     int i;
 
     for (i = 0; i < tables_nr; i++) {
-        AcpiSdtTable ssdt_table;
+        AcpiSdtTable ssdt_table = {};
         uint32_t addr;
 
         addr = le32_to_cpu(data->rsdt_tables_addr[i]);
@@ -275,7 +262,7 @@ static void dump_aml_files(test_data *data, bool rebuild)
 
         if (rebuild) {
             aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
-                                       (gchar *)&sdt->header.signature, ext);
+                                       (gchar *)&sdt->header->signature, ext);
             fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
                         S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
         } else {
@@ -284,8 +271,6 @@ static void dump_aml_files(test_data *data, bool rebuild)
         }
         g_assert(fd >= 0);
 
-        ret = qemu_write_full(fd, sdt, sizeof(AcpiTableHeader));
-        g_assert(ret == sizeof(AcpiTableHeader));
         ret = qemu_write_full(fd, sdt->aml, sdt->aml_len);
         g_assert(ret == sdt->aml_len);
 
@@ -297,7 +282,7 @@ static void dump_aml_files(test_data *data, bool rebuild)
 
 static bool compare_signature(AcpiSdtTable *sdt, const char *signature)
 {
-   return !memcmp(&sdt->header.signature, signature, 4);
+   return !memcmp(&sdt->header->signature, signature, 4);
 }
 
 static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
@@ -395,11 +380,10 @@ static GArray *load_expected_aml(test_data *data)
         sdt = &g_array_index(data->tables, AcpiSdtTable, i);
 
         memset(&exp_sdt, 0, sizeof(exp_sdt));
-        exp_sdt.header.signature = sdt->header.signature;
 
 try_again:
         aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
-                                   (gchar *)&sdt->header.signature, ext);
+                                   (gchar *)&sdt->header->signature, ext);
         if (getenv("V")) {
             fprintf(stderr, "Looking for expected file '%s'\n", aml_file);
         }
@@ -415,7 +399,7 @@ try_again:
         if (getenv("V")) {
             fprintf(stderr, "Using expected file '%s'\n", aml_file);
         }
-        ret = g_file_get_contents(aml_file, &exp_sdt.aml,
+        ret = g_file_get_contents(aml_file, (gchar **)&exp_sdt.aml,
                                   &exp_sdt.aml_len, &error);
         g_assert(ret);
         g_assert_no_error(error);
@@ -459,7 +443,7 @@ static void test_acpi_asl(test_data *data)
                 fprintf(stderr,
                         "Warning! iasl couldn't parse the expected aml\n");
             } else {
-                uint32_t signature = cpu_to_le32(exp_sdt->header.signature);
+                uint32_t signature = cpu_to_le32(exp_sdt->header->signature);
                 sdt->tmp_files_retain = true;
                 exp_sdt->tmp_files_retain = true;
                 fprintf(stderr,
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/8] tests: acpi: make sure FADT is fetched only once

[Igor Mammedov]

Whole FADT is fetched as part of RSDT referenced tables in
fetch_rsdt_referenced_tables() albeit a bit later than when FADT
is partially parsed in fadt_fetch_facs_and_dsdt_ptrs().
However there is no reason for calling fetch_rsdt_referenced_tables()
so late, just move it right after we fetched RSDT and before
fadt_fetch_facs_and_dsdt_ptrs(). That way we can reuse whole FADT
fetched by fetch_rsdt_referenced_tables() and avoid duplicate
custom fields fetching in fadt_fetch_facs_and_dsdt_ptrs().

While at it rename fadt_fetch_facs_and_dsdt_ptrs() to
test_acpi_fadt_table(). The follow up patch will merge
fadt_fetch_facs_and_dsdt_ptrs() into test_acpi_rsdt_table(),
so that we would end up calling only test_acpi_FOO_table()
for consistency for tables that require special processing.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/bios-tables-test.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 3f20bbd..b2a40bb 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -141,18 +141,15 @@ static void test_acpi_rsdt_table(test_data *data)
     data->rsdt_tables_nr = tables_nr;
 }
 
-static void fadt_fetch_facs_and_dsdt_ptrs(test_data *data)
+static void test_acpi_fadt_table(test_data *data)
 {
-    uint32_t addr;
-    AcpiTableHeader hdr;
+    /* FADT table is 1st */
+    AcpiSdtTable *fadt = &g_array_index(data->tables, typeof(*fadt), 0);
 
-    /* FADT table comes first */
-    addr = le32_to_cpu(data->rsdt_tables_addr[0]);
-    ACPI_READ_TABLE_HEADER(data->qts, &hdr, addr);
-    ACPI_ASSERT_CMP(hdr.signature, "FACP");
+    ACPI_ASSERT_CMP(fadt->header->signature, "FACP");
 
-    ACPI_READ_FIELD(data->qts, data->facs_addr, addr);
-    ACPI_READ_FIELD(data->qts, data->dsdt_addr, addr);
+    memcpy(&data->facs_addr, fadt->aml + 36 /* FIRMWARE_CTRL */, 4);
+    memcpy(&data->dsdt_addr, fadt->aml + 40 /* DSDT */, 4);
 }
 
 static void sanitize_fadt_ptrs(test_data *data)
@@ -628,10 +625,10 @@ static void test_acpi_one(const char *params, test_data *data)
     test_acpi_rsdp_address(data);
     test_acpi_rsdp_table(data);
     test_acpi_rsdt_table(data);
-    fadt_fetch_facs_and_dsdt_ptrs(data);
+    fetch_rsdt_referenced_tables(data);
+    test_acpi_fadt_table(data);
     test_acpi_facs_table(data);
     test_acpi_dsdt_table(data);
-    fetch_rsdt_referenced_tables(data);
 
     sanitize_fadt_ptrs(data);
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/8] tests: acpi: simplify rsdt handling

[Igor Mammedov]

RSDT referenced tables always have length at offset 4 and checksum at
offset 9, that's enough for reusing fetch_table() and replacing custom
RSDT fetching code with it.
While at it
 * merge fetch_rsdt_referenced_tables() into test_acpi_rsdt_table()
 * drop test_data::rsdt_table/rsdt_tables_addr/rsdt_tables_nr since
   we need this data only for duration of test_acpi_rsdt_table() to
   fetch other tables and use locals instead.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  - rebase: s/memread/qtest_memread/
---
 tests/bios-tables-test.c | 137 +++++++++++++++++++----------------------------
 1 file changed, 55 insertions(+), 82 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index b2a40bb..8082adc 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -28,12 +28,9 @@ typedef struct {
     const char *variant;
     uint32_t rsdp_addr;
     uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
-    AcpiRsdtDescriptorRev1 rsdt_table;
     uint32_t dsdt_addr;
     uint32_t facs_addr;
     AcpiFacsDescriptorRev1 facs_table;
-    uint32_t *rsdt_tables_addr;
-    int rsdt_tables_nr;
     GArray *tables;
     uint32_t smbios_ep_addr;
     struct smbios_21_entry_point smbios_ep_table;
@@ -50,33 +47,50 @@ static const char *iasl = stringify(CONFIG_IASL);
 static const char *iasl;
 #endif
 
+static void cleanup_table_descriptor(AcpiSdtTable *table)
+{
+    g_free(table->aml);
+    if (table->aml_file &&
+        !table->tmp_files_retain &&
+        g_strstr_len(table->aml_file, -1, "aml-")) {
+        unlink(table->aml_file);
+    }
+    g_free(table->aml_file);
+    g_free(table->asl);
+    if (table->asl_file &&
+        !table->tmp_files_retain) {
+        unlink(table->asl_file);
+    }
+    g_free(table->asl_file);
+}
+
 static void free_test_data(test_data *data)
 {
-    AcpiSdtTable *temp;
     int i;
 
-    g_free(data->rsdt_tables_addr);
-
     for (i = 0; i < data->tables->len; ++i) {
-        temp = &g_array_index(data->tables, AcpiSdtTable, i);
-        g_free(temp->aml);
-        if (temp->aml_file &&
-            !temp->tmp_files_retain &&
-            g_strstr_len(temp->aml_file, -1, "aml-")) {
-            unlink(temp->aml_file);
-        }
-        g_free(temp->aml_file);
-        g_free(temp->asl);
-        if (temp->asl_file &&
-            !temp->tmp_files_retain) {
-            unlink(temp->asl_file);
-        }
-        g_free(temp->asl_file);
+        cleanup_table_descriptor(&g_array_index(data->tables, AcpiSdtTable, i));
     }
 
     g_array_free(data->tables, true);
 }
 
+/** fetch_table
+ *   load ACPI table at @addr into table descriptor @sdt_table
+ *   and check that header checksum matches actual one.
+ */
+static void fetch_table(QTestState *qts, AcpiSdtTable *sdt_table, uint32_t addr)
+{
+    qtest_memread(qts, addr + 4 /* Length of ACPI table */,
+                  &sdt_table->aml_len, 4);
+    sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
+    sdt_table->aml = g_malloc0(sdt_table->aml_len);
+    /* get whole table */
+    qtest_memread(qts, addr, sdt_table->aml, sdt_table->aml_len);
+
+    g_assert(!acpi_calc_checksum(sdt_table->aml, sdt_table->aml_len));
+}
+
 static void test_acpi_rsdp_address(test_data *data)
 {
     uint32_t off = acpi_find_rsdp_address(data->qts);
@@ -109,36 +123,30 @@ static void test_acpi_rsdp_table(test_data *data)
 
 static void test_acpi_rsdt_table(test_data *data)
 {
-    AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
     uint32_t addr = acpi_get_rsdt_address(data->rsdp_table);
-    uint32_t *tables;
-    int tables_nr;
-    uint8_t checksum;
-    uint32_t rsdt_table_length;
-
-    /* read the header */
-    ACPI_READ_TABLE_HEADER(data->qts, rsdt_table, addr);
-    ACPI_ASSERT_CMP(rsdt_table->signature, "RSDT");
-
-    rsdt_table_length = le32_to_cpu(rsdt_table->length);
-
-    /* compute the table entries in rsdt */
-    tables_nr = (rsdt_table_length - sizeof(AcpiRsdtDescriptorRev1)) /
-                sizeof(uint32_t);
-    g_assert(tables_nr > 0);
-
-    /* get the addresses of the tables pointed by rsdt */
-    tables = g_new0(uint32_t, tables_nr);
-    ACPI_READ_ARRAY_PTR(data->qts, tables, tables_nr, addr);
+    const int entry_size = 4 /* 32-bit Entry size */;
+    const int tables_off = 36 /* 1st Entry */;
+    AcpiSdtTable rsdt = {};
+    int i, table_len, table_nr;
+    uint32_t *entry;
+
+    fetch_table(data->qts, &rsdt, addr);
+    ACPI_ASSERT_CMP(rsdt.header->signature, "RSDT");
+
+    /* Load all tables and add to test list directly RSDT referenced tables */
+    table_len = le32_to_cpu(rsdt.header->length);
+    table_nr = (table_len - tables_off) / entry_size;
+    for (i = 0; i < table_nr; i++) {
+        AcpiSdtTable ssdt_table = {};
 
-    checksum = acpi_calc_checksum((uint8_t *)rsdt_table, rsdt_table_length) +
-               acpi_calc_checksum((uint8_t *)tables,
-                                  tables_nr * sizeof(uint32_t));
-    g_assert(!checksum);
+        entry = (uint32_t *)(rsdt.aml + tables_off + i * entry_size);
+        addr = le32_to_cpu(*entry);
+        fetch_table(data->qts, &ssdt_table, addr);
 
-   /* SSDT tables after FADT */
-    data->rsdt_tables_addr = tables;
-    data->rsdt_tables_nr = tables_nr;
+        /* Add table to ASL test tables list */
+        g_array_append_val(data->tables, ssdt_table);
+    }
+    cleanup_table_descriptor(&rsdt);
 }
 
 static void test_acpi_fadt_table(test_data *data)
@@ -197,22 +205,6 @@ static void test_acpi_facs_table(test_data *data)
     ACPI_ASSERT_CMP(facs_table->signature, "FACS");
 }
 
-/** fetch_table
- *   load ACPI table at @addr into table descriptor @sdt_table
- *   and check that header checksum matches actual one.
- */
-static void fetch_table(QTestState *qts, AcpiSdtTable *sdt_table, uint32_t addr)
-{
-    qtest_memread(qts, addr + 4 /* Length of ACPI table */,
-                  &sdt_table->aml_len, 4);
-    sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
-    sdt_table->aml = g_malloc0(sdt_table->aml_len);
-    /* get whole table */
-    qtest_memread(qts, addr, sdt_table->aml, sdt_table->aml_len);
-
-    g_assert(!acpi_calc_checksum(sdt_table->aml, sdt_table->aml_len));
-}
-
 static void test_acpi_dsdt_table(test_data *data)
 {
     AcpiSdtTable dsdt_table = {};
@@ -225,24 +217,6 @@ static void test_acpi_dsdt_table(test_data *data)
     g_array_append_val(data->tables, dsdt_table);
 }
 
-/* Load all tables and add to test list directly RSDT referenced tables */
-static void fetch_rsdt_referenced_tables(test_data *data)
-{
-    int tables_nr = data->rsdt_tables_nr;
-    int i;
-
-    for (i = 0; i < tables_nr; i++) {
-        AcpiSdtTable ssdt_table = {};
-        uint32_t addr;
-
-        addr = le32_to_cpu(data->rsdt_tables_addr[i]);
-        fetch_table(data->qts, &ssdt_table, addr);
-
-        /* Add table to ASL test tables list */
-        g_array_append_val(data->tables, ssdt_table);
-    }
-}
-
 static void dump_aml_files(test_data *data, bool rebuild)
 {
     AcpiSdtTable *sdt;
@@ -625,7 +599,6 @@ static void test_acpi_one(const char *params, test_data *data)
     test_acpi_rsdp_address(data);
     test_acpi_rsdp_table(data);
     test_acpi_rsdt_table(data);
-    fetch_rsdt_referenced_tables(data);
     test_acpi_fadt_table(data);
     test_acpi_facs_table(data);
     test_acpi_dsdt_table(data);
-- 
2.7.4

[Qemu-devel] [PATCH v2 4/8] tests: acpi: reuse fetch_table() for fetching FACS and DSDT

[Igor Mammedov]

It allows to remove a bit more of code duplication and
reuse common utility to get ACPI tables from guest (modulo RSDP).

While at it, consolidate signature checking into fetch_table() instead
of open-codding it.

Considering FACS is special and doesn't have checksum, make checksum
validation optin, the same goes for signature verification.

PS:
By pure accident, patch also fixes FACS not being tested against
reference table since it wasn't added to data::tables list.
But we managed not to regress it since reference file was added
by commit
   (d25979380 acpi unit-test: add test files)
back in 2013

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  v2:
    - rebase: s/memread/qtest_memread/
---
 tests/bios-tables-test.c | 78 +++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 48 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 8082adc..0f6dd84 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -28,9 +28,6 @@ typedef struct {
     const char *variant;
     uint32_t rsdp_addr;
     uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
-    uint32_t dsdt_addr;
-    uint32_t facs_addr;
-    AcpiFacsDescriptorRev1 facs_table;
     GArray *tables;
     uint32_t smbios_ep_addr;
     struct smbios_21_entry_point smbios_ep_table;
@@ -76,11 +73,18 @@ static void free_test_data(test_data *data)
 }
 
 /** fetch_table
- *   load ACPI table at @addr into table descriptor @sdt_table
- *   and check that header checksum matches actual one.
+ *   load ACPI table at @addr_ptr offset pointer into table descriptor
+ *   @sdt_table and check that signature/checksum matches actual one.
  */
-static void fetch_table(QTestState *qts, AcpiSdtTable *sdt_table, uint32_t addr)
+static void fetch_table(QTestState *qts, AcpiSdtTable *sdt_table,
+                        uint8_t *addr_ptr, const char *sig,
+                        bool verify_checksum)
 {
+    uint32_t addr;
+
+    memcpy(&addr, addr_ptr , sizeof(addr));
+    addr = le32_to_cpu(addr);
+
     qtest_memread(qts, addr + 4 /* Length of ACPI table */,
                   &sdt_table->aml_len, 4);
     sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
@@ -88,7 +92,12 @@ static void fetch_table(QTestState *qts, AcpiSdtTable *sdt_table, uint32_t addr)
     /* get whole table */
     qtest_memread(qts, addr, sdt_table->aml, sdt_table->aml_len);
 
-    g_assert(!acpi_calc_checksum(sdt_table->aml, sdt_table->aml_len));
+    if (sig) {
+        ACPI_ASSERT_CMP(sdt_table->header->signature, sig);
+    }
+    if (verify_checksum) {
+        g_assert(!acpi_calc_checksum(sdt_table->aml, sdt_table->aml_len));
+    }
 }
 
 static void test_acpi_rsdp_address(test_data *data)
@@ -123,15 +132,13 @@ static void test_acpi_rsdp_table(test_data *data)
 
 static void test_acpi_rsdt_table(test_data *data)
 {
-    uint32_t addr = acpi_get_rsdt_address(data->rsdp_table);
     const int entry_size = 4 /* 32-bit Entry size */;
     const int tables_off = 36 /* 1st Entry */;
     AcpiSdtTable rsdt = {};
     int i, table_len, table_nr;
-    uint32_t *entry;
 
-    fetch_table(data->qts, &rsdt, addr);
-    ACPI_ASSERT_CMP(rsdt.header->signature, "RSDT");
+    fetch_table(data->qts, &rsdt, &data->rsdp_table[16 /* RsdtAddress */],
+                "RSDT", true);
 
     /* Load all tables and add to test list directly RSDT referenced tables */
     table_len = le32_to_cpu(rsdt.header->length);
@@ -139,9 +146,8 @@ static void test_acpi_rsdt_table(test_data *data)
     for (i = 0; i < table_nr; i++) {
         AcpiSdtTable ssdt_table = {};
 
-        entry = (uint32_t *)(rsdt.aml + tables_off + i * entry_size);
-        addr = le32_to_cpu(*entry);
-        fetch_table(data->qts, &ssdt_table, addr);
+        fetch_table(data->qts, &ssdt_table,
+                    rsdt.aml + tables_off + i * entry_size, NULL, true);
 
         /* Add table to ASL test tables list */
         g_array_append_val(data->tables, ssdt_table);
@@ -152,12 +158,18 @@ static void test_acpi_rsdt_table(test_data *data)
 static void test_acpi_fadt_table(test_data *data)
 {
     /* FADT table is 1st */
-    AcpiSdtTable *fadt = &g_array_index(data->tables, typeof(*fadt), 0);
+    AcpiSdtTable table = g_array_index(data->tables, typeof(table), 0);
+    uint8_t *fadt_aml = table.aml;
 
-    ACPI_ASSERT_CMP(fadt->header->signature, "FACP");
+    ACPI_ASSERT_CMP(table.header->signature, "FACP");
 
-    memcpy(&data->facs_addr, fadt->aml + 36 /* FIRMWARE_CTRL */, 4);
-    memcpy(&data->dsdt_addr, fadt->aml + 40 /* DSDT */, 4);
+    /* Since DSDT/FACS isn't in RSDT, add them to ASL test list manually */
+    fetch_table(data->qts, &table, fadt_aml + 36 /* FIRMWARE_CTRL */,
+                "FACS", false);
+    g_array_append_val(data->tables, table);
+
+    fetch_table(data->qts, &table, fadt_aml + 40 /* DSDT */, "DSDT", true);
+    g_array_append_val(data->tables, table);
 }
 
 static void sanitize_fadt_ptrs(test_data *data)
@@ -189,34 +201,6 @@ static void sanitize_fadt_ptrs(test_data *data)
     }
 }
 
-static void test_acpi_facs_table(test_data *data)
-{
-    AcpiFacsDescriptorRev1 *facs_table = &data->facs_table;
-    uint32_t addr = le32_to_cpu(data->facs_addr);
-
-    ACPI_READ_FIELD(data->qts, facs_table->signature, addr);
-    ACPI_READ_FIELD(data->qts, facs_table->length, addr);
-    ACPI_READ_FIELD(data->qts, facs_table->hardware_signature, addr);
-    ACPI_READ_FIELD(data->qts, facs_table->firmware_waking_vector, addr);
-    ACPI_READ_FIELD(data->qts, facs_table->global_lock, addr);
-    ACPI_READ_FIELD(data->qts, facs_table->flags, addr);
-    ACPI_READ_ARRAY(data->qts, facs_table->resverved3, addr);
-
-    ACPI_ASSERT_CMP(facs_table->signature, "FACS");
-}
-
-static void test_acpi_dsdt_table(test_data *data)
-{
-    AcpiSdtTable dsdt_table = {};
-    uint32_t addr = le32_to_cpu(data->dsdt_addr);
-
-    fetch_table(data->qts, &dsdt_table, addr);
-    ACPI_ASSERT_CMP(dsdt_table.header->signature, "DSDT");
-
-    /* Since DSDT isn't in RSDT, add DSDT to ASL test tables list manually */
-    g_array_append_val(data->tables, dsdt_table);
-}
-
 static void dump_aml_files(test_data *data, bool rebuild)
 {
     AcpiSdtTable *sdt;
@@ -600,8 +584,6 @@ static void test_acpi_one(const char *params, test_data *data)
     test_acpi_rsdp_table(data);
     test_acpi_rsdt_table(data);
     test_acpi_fadt_table(data);
-    test_acpi_facs_table(data);
-    test_acpi_dsdt_table(data);
 
     sanitize_fadt_ptrs(data);
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 5/8] tests: acpi: reuse fetch_table() in vmgenid-test

[Igor Mammedov]

Move fetch_table() into acpi-utils.c renaming it to acpi_fetch_table()
and reuse it in vmgenid-test that reads RSDT and then tables it references,
to find and parse VMGNEID SSDT.
While at it wrap RSDT referenced tables enumeration into FOREACH macro
(similar to what we do with QLIST_FOREACH & co) to reuse it with bios and
vmgenid tests.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  - rebase: s/memread/qtest_memread/
---
 tests/acpi-utils.h       | 23 ++++++-----------
 tests/acpi-utils.c       | 35 ++++++++++++++++++++------
 tests/bios-tables-test.c | 55 +++++++++--------------------------------
 tests/vmgenid-test.c     | 64 +++++++++++++++---------------------------------
 4 files changed, 67 insertions(+), 110 deletions(-)

diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index 1b0e80d..1aa00db 100644
--- a/tests/acpi-utils.h
+++ b/tests/acpi-utils.h
@@ -22,7 +22,7 @@ typedef struct {
         AcpiTableHeader *header;
         uint8_t *aml;            /* aml bytecode from guest */
     };
-    gsize aml_len;
+    uint32_t aml_len;
     gchar *aml_file;
     gchar *asl;            /* asl code generated from aml */
     gsize asl_len;
@@ -47,19 +47,6 @@ typedef struct {
 #define ACPI_READ_ARRAY(qts, arr, addr)                                 \
     ACPI_READ_ARRAY_PTR(qts, arr, sizeof(arr) / sizeof(arr[0]), addr)
 
-#define ACPI_READ_TABLE_HEADER(qts, table, addr)                 \
-    do {                                                         \
-        ACPI_READ_FIELD(qts, (table)->signature, addr);          \
-        ACPI_READ_FIELD(qts, (table)->length, addr);             \
-        ACPI_READ_FIELD(qts, (table)->revision, addr);           \
-        ACPI_READ_FIELD(qts, (table)->checksum, addr);           \
-        ACPI_READ_ARRAY(qts, (table)->oem_id, addr);             \
-        ACPI_READ_ARRAY(qts, (table)->oem_table_id, addr);       \
-        ACPI_READ_FIELD(qts, (table)->oem_revision, addr);       \
-        ACPI_READ_ARRAY(qts, (table)->asl_compiler_id, addr);    \
-        ACPI_READ_FIELD(qts, (table)->asl_compiler_revision, addr);     \
-    } while (0)
-
 #define ACPI_ASSERT_CMP(actual, expected) do { \
     char ACPI_ASSERT_CMP_str[5] = {}; \
     memcpy(ACPI_ASSERT_CMP_str, &actual, 4); \
@@ -73,11 +60,17 @@ typedef struct {
 } while (0)
 
 
+#define ACPI_FOREACH_RSDT_ENTRY(table, table_len, entry_ptr, entry_size) \
+    for (entry_ptr = table + 36 /* 1st Entry */;                         \
+         entry_ptr < table + table_len;                                  \
+         entry_ptr += entry_size)
 
 uint8_t acpi_calc_checksum(const uint8_t *data, int len);
 uint32_t acpi_find_rsdp_address(QTestState *qts);
-uint32_t acpi_get_rsdt_address(uint8_t *rsdp_table);
 uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table);
 void acpi_parse_rsdp_table(QTestState *qts, uint32_t addr, uint8_t *rsdp_table);
+void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
+                      const uint8_t *addr_ptr, const char *sig,
+                      bool verify_checksum);
 
 #endif  /* TEST_ACPI_UTILS_H */
diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
index 17abcc4..cc33b46 100644
--- a/tests/acpi-utils.c
+++ b/tests/acpi-utils.c
@@ -51,14 +51,6 @@ uint32_t acpi_find_rsdp_address(QTestState *qts)
     return off;
 }
 
-uint32_t acpi_get_rsdt_address(uint8_t *rsdp_table)
-{
-    uint32_t rsdt_physical_address;
-
-    memcpy(&rsdt_physical_address, &rsdp_table[16 /* RsdtAddress offset */], 4);
-    return le32_to_cpu(rsdt_physical_address);
-}
-
 uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table)
 {
     uint64_t xsdt_physical_address;
@@ -92,3 +84,30 @@ void acpi_parse_rsdp_table(QTestState *qts, uint32_t addr, uint8_t *rsdp_table)
 
     ACPI_ASSERT_CMP64(*((uint64_t *)(rsdp_table)), "RSD PTR ");
 }
+
+/** acpi_fetch_table
+ *  load ACPI table at @addr_ptr offset pointer into buffer and return it in
+ *  @aml, its length in @aml_len and check that signature/checksum matches
+ *  actual one.
+ */
+void acpi_fetch_table(QTestState *qts, uint8_t **aml, uint32_t *aml_len,
+                      const uint8_t *addr_ptr, const char *sig,
+                      bool verify_checksum)
+{
+    uint32_t addr, len;
+
+    memcpy(&addr, addr_ptr , sizeof(addr));
+    addr = le32_to_cpu(addr);
+    qtest_memread(qts, addr + 4, &len, 4); /* Length of ACPI table */
+    *aml_len = le32_to_cpu(len);
+    *aml = g_malloc0(*aml_len);
+    /* get whole table */
+    qtest_memread(qts, addr, *aml, *aml_len);
+
+    if (sig) {
+        ACPI_ASSERT_CMP(**aml, sig);
+    }
+    if (verify_checksum) {
+        g_assert(!acpi_calc_checksum(*aml, *aml_len));
+    }
+}
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 0f6dd84..8fdd1c1 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -72,34 +72,6 @@ static void free_test_data(test_data *data)
     g_array_free(data->tables, true);
 }
 
-/** fetch_table
- *   load ACPI table at @addr_ptr offset pointer into table descriptor
- *   @sdt_table and check that signature/checksum matches actual one.
- */
-static void fetch_table(QTestState *qts, AcpiSdtTable *sdt_table,
-                        uint8_t *addr_ptr, const char *sig,
-                        bool verify_checksum)
-{
-    uint32_t addr;
-
-    memcpy(&addr, addr_ptr , sizeof(addr));
-    addr = le32_to_cpu(addr);
-
-    qtest_memread(qts, addr + 4 /* Length of ACPI table */,
-                  &sdt_table->aml_len, 4);
-    sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
-    sdt_table->aml = g_malloc0(sdt_table->aml_len);
-    /* get whole table */
-    qtest_memread(qts, addr, sdt_table->aml, sdt_table->aml_len);
-
-    if (sig) {
-        ACPI_ASSERT_CMP(sdt_table->header->signature, sig);
-    }
-    if (verify_checksum) {
-        g_assert(!acpi_calc_checksum(sdt_table->aml, sdt_table->aml_len));
-    }
-}
-
 static void test_acpi_rsdp_address(test_data *data)
 {
     uint32_t off = acpi_find_rsdp_address(data->qts);
@@ -132,23 +104,19 @@ static void test_acpi_rsdp_table(test_data *data)
 
 static void test_acpi_rsdt_table(test_data *data)
 {
-    const int entry_size = 4 /* 32-bit Entry size */;
-    const int tables_off = 36 /* 1st Entry */;
     AcpiSdtTable rsdt = {};
-    int i, table_len, table_nr;
+    uint8_t *ent;
 
-    fetch_table(data->qts, &rsdt, &data->rsdp_table[16 /* RsdtAddress */],
-                "RSDT", true);
+    /* read RSDT table */
+    acpi_fetch_table(data->qts, &rsdt.aml, &rsdt.aml_len,
+                     &data->rsdp_table[16 /* RsdtAddress */], "RSDT", true);
 
     /* Load all tables and add to test list directly RSDT referenced tables */
-    table_len = le32_to_cpu(rsdt.header->length);
-    table_nr = (table_len - tables_off) / entry_size;
-    for (i = 0; i < table_nr; i++) {
+    ACPI_FOREACH_RSDT_ENTRY(rsdt.aml, rsdt.aml_len, ent, 4 /* Entry size */) {
         AcpiSdtTable ssdt_table = {};
 
-        fetch_table(data->qts, &ssdt_table,
-                    rsdt.aml + tables_off + i * entry_size, NULL, true);
-
+        acpi_fetch_table(data->qts, &ssdt_table.aml, &ssdt_table.aml_len, ent,
+                         NULL, true);
         /* Add table to ASL test tables list */
         g_array_append_val(data->tables, ssdt_table);
     }
@@ -164,11 +132,12 @@ static void test_acpi_fadt_table(test_data *data)
     ACPI_ASSERT_CMP(table.header->signature, "FACP");
 
     /* Since DSDT/FACS isn't in RSDT, add them to ASL test list manually */
-    fetch_table(data->qts, &table, fadt_aml + 36 /* FIRMWARE_CTRL */,
-                "FACS", false);
+    acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
+                     fadt_aml + 36 /* FIRMWARE_CTRL */, "FACS", false);
     g_array_append_val(data->tables, table);
 
-    fetch_table(data->qts, &table, fadt_aml + 40 /* DSDT */, "DSDT", true);
+    acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
+                     fadt_aml + 40 /* DSDT */, "DSDT", true);
     g_array_append_val(data->tables, table);
 }
 
@@ -355,7 +324,7 @@ try_again:
             fprintf(stderr, "Using expected file '%s'\n", aml_file);
         }
         ret = g_file_get_contents(aml_file, (gchar **)&exp_sdt.aml,
-                                  &exp_sdt.aml_len, &error);
+                                  (gsize *)&exp_sdt.aml_len, &error);
         g_assert(ret);
         g_assert_no_error(error);
         g_assert(exp_sdt.aml);
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
index 1c1d435..52cdd83 100644
--- a/tests/vmgenid-test.c
+++ b/tests/vmgenid-test.c
@@ -23,26 +23,13 @@
                                   */
 #define RSDP_ADDR_INVALID 0x100000 /* RSDP must be below this address */
 
-typedef struct {
-    AcpiTableHeader header;
-    gchar name_op;
-    gchar vgia[4];
-    gchar val_op;
-    uint32_t vgia_val;
-} QEMU_PACKED VgidTable;
-
 static uint32_t acpi_find_vgia(QTestState *qts)
 {
     uint32_t rsdp_offset;
     uint32_t guid_offset = 0;
     uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
-    uint32_t rsdt, rsdt_table_length;
-    AcpiRsdtDescriptorRev1 rsdt_table;
-    size_t tables_nr;
-    uint32_t *tables;
-    AcpiTableHeader ssdt_table;
-    VgidTable vgid_table;
-    int i;
+    uint32_t rsdt_len, table_length;
+    uint8_t *rsdt, *ent;
 
     /* Wait for guest firmware to finish and start the payload. */
     boot_sector_test(qts);
@@ -52,48 +39,37 @@ static uint32_t acpi_find_vgia(QTestState *qts)
 
     g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
 
-    acpi_parse_rsdp_table(qts, rsdp_offset, rsdp_table);
-
-    rsdt = acpi_get_rsdt_address(rsdp_table);
-    g_assert(rsdt);
 
-    /* read the header */
-    ACPI_READ_TABLE_HEADER(qts, &rsdt_table, rsdt);
-    ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
-    rsdt_table_length = le32_to_cpu(rsdt_table.length);
-
-    /* compute the table entries in rsdt */
-    g_assert_cmpint(rsdt_table_length, >, sizeof(AcpiRsdtDescriptorRev1));
-    tables_nr = (rsdt_table_length - sizeof(AcpiRsdtDescriptorRev1)) /
-                sizeof(uint32_t);
+    acpi_parse_rsdp_table(qts, rsdp_offset, rsdp_table);
+    acpi_fetch_table(qts, &rsdt, &rsdt_len, &rsdp_table[16 /* RsdtAddress */],
+                     "RSDT", true);
 
-    /* get the addresses of the tables pointed by rsdt */
-    tables = g_new0(uint32_t, tables_nr);
-    ACPI_READ_ARRAY_PTR(qts, tables, tables_nr, rsdt);
+    ACPI_FOREACH_RSDT_ENTRY(rsdt, rsdt_len, ent, 4 /* Entry size */) {
+        uint8_t *table_aml;
 
-    for (i = 0; i < tables_nr; i++) {
-        uint32_t addr = le32_to_cpu(tables[i]);
-        ACPI_READ_TABLE_HEADER(qts, &ssdt_table, addr);
-        if (!strncmp((char *)ssdt_table.oem_table_id, "VMGENID", 7)) {
+        acpi_fetch_table(qts, &table_aml, &table_length, ent, NULL, true);
+        if (!memcmp(table_aml + 16 /* OEM Table ID */, "VMGENID", 7)) {
+            uint32_t vgia_val;
+            uint8_t *aml = &table_aml[36 /* AML byte-code start */];
             /* the first entry in the table should be VGIA
              * That's all we need
              */
-            ACPI_READ_FIELD(qts, vgid_table.name_op, addr);
-            g_assert(vgid_table.name_op == 0x08);  /* name */
-            ACPI_READ_ARRAY(qts, vgid_table.vgia, addr);
-            g_assert(memcmp(vgid_table.vgia, "VGIA", 4) == 0);
-            ACPI_READ_FIELD(qts, vgid_table.val_op, addr);
-            g_assert(vgid_table.val_op == 0x0C);  /* dword */
-            ACPI_READ_FIELD(qts, vgid_table.vgia_val, addr);
+            g_assert(aml[0 /* name_op*/] == 0x08);
+            g_assert(memcmp(&aml[1 /* name */], "VGIA", 4) == 0);
+            g_assert(aml[5 /* value op */] == 0x0C /* dword */);
+            memcpy(&vgia_val, &aml[6 /* value */], 4);
+
             /* The GUID is written at a fixed offset into the fw_cfg file
              * in order to implement the "OVMF SDT Header probe suppressor"
              * see docs/specs/vmgenid.txt for more details
              */
-            guid_offset = le32_to_cpu(vgid_table.vgia_val) + VMGENID_GUID_OFFSET;
+            guid_offset = le32_to_cpu(vgia_val) + VMGENID_GUID_OFFSET;
+            g_free(table_aml);
             break;
         }
+        g_free(table_aml);
     }
-    g_free(tables);
+    g_free(rsdt);
     return guid_offset;
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 6/8] tests: smbios: fetch whole table in one step instead of reading it step by step

[Igor Mammedov]

replace a bunch of ACPI_READ_ARRAY/ACPI_READ_FIELD macro, that read
SMBIOS table field by field with one memread() to fetch whole table
at once and drop no longer used ACPI_READ_ARRAY/ACPI_READ_FIELD macro.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
V2:
  rebase: s/memread/qtest_memread/
---
 tests/acpi-utils.h       | 17 -----------------
 tests/bios-tables-test.c | 15 +--------------
 2 files changed, 1 insertion(+), 31 deletions(-)

diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index 1aa00db..cb7183e 100644
--- a/tests/acpi-utils.h
+++ b/tests/acpi-utils.h
@@ -30,23 +30,6 @@ typedef struct {
     bool tmp_files_retain;   /* do not delete the temp asl/aml */
 } AcpiSdtTable;
 
-#define ACPI_READ_FIELD(qts, field, addr)                \
-    do {                                                 \
-        qtest_memread(qts, addr, &field, sizeof(field)); \
-        addr += sizeof(field);                           \
-    } while (0)
-
-#define ACPI_READ_ARRAY_PTR(qts, arr, length, addr)      \
-    do {                                                 \
-        int idx;                                         \
-        for (idx = 0; idx < length; ++idx) {             \
-            ACPI_READ_FIELD(qts, arr[idx], addr);        \
-        }                                                \
-    } while (0)
-
-#define ACPI_READ_ARRAY(qts, arr, addr)                                 \
-    ACPI_READ_ARRAY_PTR(qts, arr, sizeof(arr) / sizeof(arr[0]), addr)
-
 #define ACPI_ASSERT_CMP(actual, expected) do { \
     char ACPI_ASSERT_CMP_str[5] = {}; \
     memcpy(ACPI_ASSERT_CMP_str, &actual, 4); \
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 8fdd1c1..dcd6be8 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -406,32 +406,19 @@ static bool smbios_ep_table_ok(test_data *data)
     struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
     uint32_t addr = data->smbios_ep_addr;
 
-    ACPI_READ_ARRAY(data->qts, ep_table->anchor_string, addr);
+    qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
     if (memcmp(ep_table->anchor_string, "_SM_", 4)) {
         return false;
     }
-    ACPI_READ_FIELD(data->qts, ep_table->checksum, addr);
-    ACPI_READ_FIELD(data->qts, ep_table->length, addr);
-    ACPI_READ_FIELD(data->qts, ep_table->smbios_major_version, addr);
-    ACPI_READ_FIELD(data->qts, ep_table->smbios_minor_version, addr);
-    ACPI_READ_FIELD(data->qts, ep_table->max_structure_size, addr);
-    ACPI_READ_FIELD(data->qts, ep_table->entry_point_revision, addr);
-    ACPI_READ_ARRAY(data->qts, ep_table->formatted_area, addr);
-    ACPI_READ_ARRAY(data->qts, ep_table->intermediate_anchor_string, addr);
     if (memcmp(ep_table->intermediate_anchor_string, "_DMI_", 5)) {
         return false;
     }
-    ACPI_READ_FIELD(data->qts, ep_table->intermediate_checksum, addr);
-    ACPI_READ_FIELD(data->qts, ep_table->structure_table_length, addr);
     if (ep_table->structure_table_length == 0) {
         return false;
     }
-    ACPI_READ_FIELD(data->qts, ep_table->structure_table_address, addr);
-    ACPI_READ_FIELD(data->qts, ep_table->number_of_structures, addr);
     if (ep_table->number_of_structures == 0) {
         return false;
     }
-    ACPI_READ_FIELD(data->qts, ep_table->smbios_bcd_revision, addr);
     if (acpi_calc_checksum((uint8_t *)ep_table, sizeof *ep_table) ||
         acpi_calc_checksum((uint8_t *)ep_table + 0x10,
                            sizeof *ep_table - 0x10)) {
-- 
2.7.4

[Qemu-devel] [PATCH v2 7/8] tests: acpi: squash sanitize_fadt_ptrs() into test_acpi_fadt_table()

[Igor Mammedov]

some parts of sanitize_fadt_ptrs() do redundant job
  - locating FADT
  - checking original checksum

There is no need to do it as test_acpi_fadt_table() already does that,
so drop duplicate code and move remaining fixup code into
test_acpi_fadt_table().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/bios-tables-test.c | 39 ++++++++++-----------------------------
 1 file changed, 10 insertions(+), 29 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index dcd6be8..9139dec 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -128,6 +128,7 @@ static void test_acpi_fadt_table(test_data *data)
     /* FADT table is 1st */
     AcpiSdtTable table = g_array_index(data->tables, typeof(table), 0);
     uint8_t *fadt_aml = table.aml;
+    uint32_t fadt_len = table.aml_len;
 
     ACPI_ASSERT_CMP(table.header->signature, "FACP");
 
@@ -139,35 +140,17 @@ static void test_acpi_fadt_table(test_data *data)
     acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
                      fadt_aml + 40 /* DSDT */, "DSDT", true);
     g_array_append_val(data->tables, table);
-}
-
-static void sanitize_fadt_ptrs(test_data *data)
-{
-    /* fixup pointers in FADT */
-    int i;
-
-    for (i = 0; i < data->tables->len; i++) {
-        AcpiSdtTable *sdt = &g_array_index(data->tables, AcpiSdtTable, i);
-
-        if (memcmp(&sdt->header->signature, "FACP", 4)) {
-            continue;
-        }
 
-        /* check original FADT checksum before sanitizing table */
-        g_assert(!acpi_calc_checksum(sdt->aml, sdt->aml_len));
-
-        memset(sdt->aml + 36, 0, 4); /* sanitize FIRMWARE_CTRL ptr */
-        memset(sdt->aml + 40, 0, 4); /* sanitize DSDT ptr */
-        if (sdt->header->revision >= 3) {
-            memset(sdt->aml + 132, 0, 8); /* sanitize X_FIRMWARE_CTRL ptr */
-            memset(sdt->aml + 140, 0, 8); /* sanitize X_DSDT ptr */
-        }
-
-        /* update checksum */
-        sdt->header->checksum = 0;
-        sdt->header->checksum -= acpi_calc_checksum(sdt->aml, sdt->aml_len);
-        break;
+    memset(fadt_aml + 36, 0, 4); /* sanitize FIRMWARE_CTRL ptr */
+    memset(fadt_aml + 40, 0, 4); /* sanitize DSDT ptr */
+    if (fadt_aml[8 /* FADT Major Version */] >= 3) {
+        memset(fadt_aml + 132, 0, 8); /* sanitize X_FIRMWARE_CTRL ptr */
+        memset(fadt_aml + 140, 0, 8); /* sanitize X_DSDT ptr */
     }
+
+    /* update checksum */
+    fadt_aml[9 /* Checksum */] = 0;
+    fadt_aml[9 /* Checksum */] -= acpi_calc_checksum(fadt_aml, fadt_len);
 }
 
 static void dump_aml_files(test_data *data, bool rebuild)
@@ -541,8 +524,6 @@ static void test_acpi_one(const char *params, test_data *data)
     test_acpi_rsdt_table(data);
     test_acpi_fadt_table(data);
 
-    sanitize_fadt_ptrs(data);
-
     if (iasl) {
         if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
             dump_aml_files(data, true);
-- 
2.7.4

[Qemu-devel] [PATCH v2 8/8] tests: acpi: use AcpiSdtTable::aml instead of AcpiSdtTable::header::signature

[Igor Mammedov]

AcpiSdtTable::header::signature is the only remained field from
AcpiTableHeader structure used by tests. Instead of using packed
structure to access signature, just read it directly from table
blob and remove no longer used AcpiSdtTable::header / union and
keep only AcpiSdtTable::aml byte array.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/acpi-utils.h       |  6 +-----
 tests/bios-tables-test.c | 20 +++++++++-----------
 2 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index cb7183e..ef388bb 100644
--- a/tests/acpi-utils.h
+++ b/tests/acpi-utils.h
@@ -13,15 +13,11 @@
 #ifndef TEST_ACPI_UTILS_H
 #define TEST_ACPI_UTILS_H
 
-#include "hw/acpi/acpi-defs.h"
 #include "libqtest.h"
 
 /* DSDT and SSDTs format */
 typedef struct {
-    union {
-        AcpiTableHeader *header;
-        uint8_t *aml;            /* aml bytecode from guest */
-    };
+    uint8_t *aml;            /* aml bytecode from guest */
     uint32_t aml_len;
     gchar *aml_file;
     gchar *asl;            /* asl code generated from aml */
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 9139dec..0bf7164 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -44,6 +44,11 @@ static const char *iasl = stringify(CONFIG_IASL);
 static const char *iasl;
 #endif
 
+static bool compare_signature(const AcpiSdtTable *sdt, const char *signature)
+{
+   return !memcmp(sdt->aml, signature, 4);
+}
+
 static void cleanup_table_descriptor(AcpiSdtTable *table)
 {
     g_free(table->aml);
@@ -130,7 +135,7 @@ static void test_acpi_fadt_table(test_data *data)
     uint8_t *fadt_aml = table.aml;
     uint32_t fadt_len = table.aml_len;
 
-    ACPI_ASSERT_CMP(table.header->signature, "FACP");
+    g_assert(compare_signature(&table, "FACP"));
 
     /* Since DSDT/FACS isn't in RSDT, add them to ASL test list manually */
     acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
@@ -169,7 +174,7 @@ static void dump_aml_files(test_data *data, bool rebuild)
 
         if (rebuild) {
             aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
-                                       (gchar *)&sdt->header->signature, ext);
+                                       sdt->aml, ext);
             fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
                         S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
         } else {
@@ -187,11 +192,6 @@ static void dump_aml_files(test_data *data, bool rebuild)
     }
 }
 
-static bool compare_signature(AcpiSdtTable *sdt, const char *signature)
-{
-   return !memcmp(&sdt->header->signature, signature, 4);
-}
-
 static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
 {
     AcpiSdtTable *temp;
@@ -290,7 +290,7 @@ static GArray *load_expected_aml(test_data *data)
 
 try_again:
         aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
-                                   (gchar *)&sdt->header->signature, ext);
+                                   sdt->aml, ext);
         if (getenv("V")) {
             fprintf(stderr, "Looking for expected file '%s'\n", aml_file);
         }
@@ -350,14 +350,12 @@ static void test_acpi_asl(test_data *data)
                 fprintf(stderr,
                         "Warning! iasl couldn't parse the expected aml\n");
             } else {
-                uint32_t signature = cpu_to_le32(exp_sdt->header->signature);
                 sdt->tmp_files_retain = true;
                 exp_sdt->tmp_files_retain = true;
                 fprintf(stderr,
                         "acpi-test: Warning! %.4s mismatch. "
                         "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
-                        (gchar *)&signature,
-                        sdt->asl_file, sdt->aml_file,
+                        exp_sdt->aml, sdt->asl_file, sdt->aml_file,
                         exp_sdt->asl_file, exp_sdt->aml_file);
                 if (getenv("V")) {
                     const char *diff_cmd = getenv("DIFF");
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 6/8] tests: smbios: fetch whole table in one step instead of reading it step by step

[Philippe Mathieu-Daudé]

On 12/27/18 3:13 PM, Igor Mammedov wrote:
> replace a bunch of ACPI_READ_ARRAY/ACPI_READ_FIELD macro, that read
> SMBIOS table field by field with one memread() to fetch whole table
> at once and drop no longer used ACPI_READ_ARRAY/ACPI_READ_FIELD macro.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

> ---
> V2:
>   rebase: s/memread/qtest_memread/
> ---
>  tests/acpi-utils.h       | 17 -----------------
>  tests/bios-tables-test.c | 15 +--------------
>  2 files changed, 1 insertion(+), 31 deletions(-)
> 
> diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> index 1aa00db..cb7183e 100644
> --- a/tests/acpi-utils.h
> +++ b/tests/acpi-utils.h
> @@ -30,23 +30,6 @@ typedef struct {
>      bool tmp_files_retain;   /* do not delete the temp asl/aml */
>  } AcpiSdtTable;
>  
> -#define ACPI_READ_FIELD(qts, field, addr)                \
> -    do {                                                 \
> -        qtest_memread(qts, addr, &field, sizeof(field)); \
> -        addr += sizeof(field);                           \
> -    } while (0)
> -
> -#define ACPI_READ_ARRAY_PTR(qts, arr, length, addr)      \
> -    do {                                                 \
> -        int idx;                                         \
> -        for (idx = 0; idx < length; ++idx) {             \
> -            ACPI_READ_FIELD(qts, arr[idx], addr);        \
> -        }                                                \
> -    } while (0)
> -
> -#define ACPI_READ_ARRAY(qts, arr, addr)                                 \
> -    ACPI_READ_ARRAY_PTR(qts, arr, sizeof(arr) / sizeof(arr[0]), addr)
> -
>  #define ACPI_ASSERT_CMP(actual, expected) do { \
>      char ACPI_ASSERT_CMP_str[5] = {}; \
>      memcpy(ACPI_ASSERT_CMP_str, &actual, 4); \
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 8fdd1c1..dcd6be8 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -406,32 +406,19 @@ static bool smbios_ep_table_ok(test_data *data)
>      struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
>      uint32_t addr = data->smbios_ep_addr;
>  
> -    ACPI_READ_ARRAY(data->qts, ep_table->anchor_string, addr);
> +    qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
>      if (memcmp(ep_table->anchor_string, "_SM_", 4)) {
>          return false;
>      }
> -    ACPI_READ_FIELD(data->qts, ep_table->checksum, addr);
> -    ACPI_READ_FIELD(data->qts, ep_table->length, addr);
> -    ACPI_READ_FIELD(data->qts, ep_table->smbios_major_version, addr);
> -    ACPI_READ_FIELD(data->qts, ep_table->smbios_minor_version, addr);
> -    ACPI_READ_FIELD(data->qts, ep_table->max_structure_size, addr);
> -    ACPI_READ_FIELD(data->qts, ep_table->entry_point_revision, addr);
> -    ACPI_READ_ARRAY(data->qts, ep_table->formatted_area, addr);
> -    ACPI_READ_ARRAY(data->qts, ep_table->intermediate_anchor_string, addr);
>      if (memcmp(ep_table->intermediate_anchor_string, "_DMI_", 5)) {
>          return false;
>      }
> -    ACPI_READ_FIELD(data->qts, ep_table->intermediate_checksum, addr);
> -    ACPI_READ_FIELD(data->qts, ep_table->structure_table_length, addr);
>      if (ep_table->structure_table_length == 0) {
>          return false;
>      }
> -    ACPI_READ_FIELD(data->qts, ep_table->structure_table_address, addr);
> -    ACPI_READ_FIELD(data->qts, ep_table->number_of_structures, addr);
>      if (ep_table->number_of_structures == 0) {
>          return false;
>      }
> -    ACPI_READ_FIELD(data->qts, ep_table->smbios_bcd_revision, addr);
>      if (acpi_calc_checksum((uint8_t *)ep_table, sizeof *ep_table) ||
>          acpi_calc_checksum((uint8_t *)ep_table + 0x10,
>                             sizeof *ep_table - 0x10)) {
> 

Re: [Qemu-devel] [PATCH v2 0/8] tests: apci: consolidate and cleanup ACPI test code

[Thomas Huth]

On 2018-12-27 15:13, Igor Mammedov wrote:
> Changes since v1:
>   * rebase on top of current master due to a lots of conflicts with
>     'qtest global' removal being merged first

Sorry for that ... I hope it was not too much trouble!

>   * drop explicit cast to uint8_t* as sdt->aml is uint8_t* now
>   * drop not comment explaining strange offsets as offsets are
>     now follow ACPI spec
> 
> While working on adding tests for virt/arm board (uefi/XSDT/64-bit table pointers),
> I found it's rather difficult to deal with mixed ACPI testing code that we've
> collected so far. So instead of just adding a pile of XSDT hacks on top, here
> goes small refactoring series:
>    * that removes dead code
>    * replaces reading tables with a fetch per table everywhere instead of
>      mix of field by field and whole table
>    * consolidates the way tables are read (reduces code duplication)
>    * test no longer depends on ACPI structures from QEMU (i.e. doesn't affected
>      by mistakes there) 
>    * fixes FACS not being compared against reference tables
> Overall test is reduced on ~160LOC and hopefully it makes easier to follow and
> add more stuff on top.
> 
> PS:
> arm/virt test patches fill follow up a separate series on top of this one
> for not to mix things up
> 
> Git tree for testing:
>   https://github.com/imammedo/qemu acpi_tests_cleanup_v2
> 
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> CC: Thomas Huth <thuth@redhat.com>

Do you want me to take this through the qtests tree, or will you take it
through your ACPI tree?

In the latter case, FWIW:
Acked-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/8] tests: apci: consolidate and cleanup ACPI test code

[Michael S. Tsirkin]

On Wed, Jan 02, 2019 at 02:11:23PM +0100, Thomas Huth wrote:
> On 2018-12-27 15:13, Igor Mammedov wrote:
> > Changes since v1:
> >   * rebase on top of current master due to a lots of conflicts with
> >     'qtest global' removal being merged first
> 
> Sorry for that ... I hope it was not too much trouble!
> 
> >   * drop explicit cast to uint8_t* as sdt->aml is uint8_t* now
> >   * drop not comment explaining strange offsets as offsets are
> >     now follow ACPI spec
> > 
> > While working on adding tests for virt/arm board (uefi/XSDT/64-bit table pointers),
> > I found it's rather difficult to deal with mixed ACPI testing code that we've
> > collected so far. So instead of just adding a pile of XSDT hacks on top, here
> > goes small refactoring series:
> >    * that removes dead code
> >    * replaces reading tables with a fetch per table everywhere instead of
> >      mix of field by field and whole table
> >    * consolidates the way tables are read (reduces code duplication)
> >    * test no longer depends on ACPI structures from QEMU (i.e. doesn't affected
> >      by mistakes there) 
> >    * fixes FACS not being compared against reference tables
> > Overall test is reduced on ~160LOC and hopefully it makes easier to follow and
> > add more stuff on top.
> > 
> > PS:
> > arm/virt test patches fill follow up a separate series on top of this one
> > for not to mix things up
> > 
> > Git tree for testing:
> >   https://github.com/imammedo/qemu acpi_tests_cleanup_v2
> > 
> > CC: "Michael S. Tsirkin" <mst@redhat.com>
> > CC: Thomas Huth <thuth@redhat.com>
> 
> Do you want me to take this through the qtests tree, or will you take it
> through your ACPI tree?
> 
> In the latter case, FWIW:
> Acked-by: Thomas Huth <thuth@redhat.com>

I'll merge it, thanks for the ack!