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

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

[Igor Mammedov]

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) 
   * fixiex FACS not beint compared against reference tables
Overall test is reduced on ~170LOC and hopefully it makes easier to add more
stuff on top.

PS:
Series depends on '[PATCH v3 0/8] hw: acpi: RSDP fixes and refactoring'
to avoid nontrivial conflicts

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


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>


Igor Mammedov (9):
  tests: acpi: remove not used ACPI_READ_GENERIC_ADDRESS macro
  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       |  51 ++--------
 tests/acpi-utils.c       |  33 ++++--
 tests/bios-tables-test.c | 257 ++++++++++++-----------------------------------
 tests/vmgenid-test.c     |  63 ++++--------
 4 files changed, 116 insertions(+), 288 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/9] tests: acpi: remove not used ACPI_READ_GENERIC_ADDRESS macro

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/acpi-utils.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index 4f4899d..c8844a2 100644
--- a/tests/acpi-utils.h
+++ b/tests/acpi-utils.h
@@ -70,14 +70,6 @@ typedef struct {
     g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
 } while (0)
 
-#define ACPI_READ_GENERIC_ADDRESS(field, addr)       \
-    do {                                             \
-        ACPI_READ_FIELD((field).space_id, addr);     \
-        ACPI_READ_FIELD((field).bit_width, addr);    \
-        ACPI_READ_FIELD((field).bit_offset, addr);   \
-        ACPI_READ_FIELD((field).access_width, addr); \
-        ACPI_READ_FIELD((field).address, addr);      \
-    } while (0)
 
 
 uint8_t acpi_calc_checksum(const uint8_t *data, int len);
-- 
2.7.4

[Qemu-devel] [PATCH 2/9] 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>
---
 tests/acpi-utils.h       |  6 +++--
 tests/bios-tables-test.c | 59 +++++++++++++++++-------------------------------
 2 files changed, 25 insertions(+), 40 deletions(-)

diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index c8844a2..2244e8e 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 8749b77..1666cf7 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -161,29 +161,24 @@ 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)
-        ));
+        g_assert(!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 */
+        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;
     }
 }
@@ -210,30 +205,21 @@ static void test_acpi_facs_table(test_data *data)
  */
 static void fetch_table(AcpiSdtTable *sdt_table, uint32_t addr)
 {
-    uint8_t checksum;
-
-    memset(sdt_table, 0, sizeof(*sdt_table));
-    ACPI_READ_TABLE_HEADER(&sdt_table->header, addr);
-
-    sdt_table->aml_len = le32_to_cpu(sdt_table->header.length)
-                         - sizeof(AcpiTableHeader);
+    memread(addr + 4, &sdt_table->aml_len, 4); /* Length of ACPI table */
+    sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
     sdt_table->aml = g_malloc0(sdt_table->aml_len);
-    ACPI_READ_ARRAY_PTR(sdt_table->aml, sdt_table->aml_len, addr);
+    memread(addr, sdt_table->aml, sdt_table->aml_len); /* get whole table */
 
-    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(&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);
@@ -246,7 +232,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]);
@@ -273,7 +259,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 {
@@ -282,8 +268,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);
 
@@ -295,7 +279,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)
@@ -390,11 +374,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, "\nLooking for expected file '%s'\n", aml_file);
         }
@@ -410,7 +393,7 @@ try_again:
         if (getenv("V")) {
             fprintf(stderr, "\nUsing 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);
@@ -454,7 +437,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 3/9] 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 1666cf7..5faf75f 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -139,18 +139,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(&hdr, addr);
-    ACPI_ASSERT_CMP(hdr.signature, "FACP");
+    ACPI_ASSERT_CMP(fadt->header->signature, "FACP");
 
-    ACPI_READ_FIELD(data->facs_addr, addr);
-    ACPI_READ_FIELD(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)
@@ -622,10 +619,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 4/9] 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>
---
 tests/bios-tables-test.c | 128 +++++++++++++++++++----------------------------
 1 file changed, 51 insertions(+), 77 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 5faf75f..bea33a6 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;
@@ -49,33 +46,48 @@ 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(AcpiSdtTable *sdt_table, uint32_t addr)
+{
+    memread(addr + 4, &sdt_table->aml_len, 4); /* Length of ACPI table */
+    sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
+    sdt_table->aml = g_malloc0(sdt_table->aml_len);
+    memread(addr, sdt_table->aml, sdt_table->aml_len); /* get whole table */
+
+    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();
@@ -107,36 +119,31 @@ 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;
+    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;
 
     /* read the header */
-    ACPI_READ_TABLE_HEADER(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);
+    fetch_table(&rsdt, addr);
+    ACPI_ASSERT_CMP(rsdt.header->signature, "RSDT");
 
-    /* get the addresses of the tables pointed by rsdt */
-    tables = g_new0(uint32_t, tables_nr);
-    ACPI_READ_ARRAY_PTR(tables, tables_nr, addr);
+    /* 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(&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)
@@ -196,20 +203,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(AcpiSdtTable *sdt_table, uint32_t addr)
-{
-    memread(addr + 4, &sdt_table->aml_len, 4); /* Length of ACPI table */
-    sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
-    sdt_table->aml = g_malloc0(sdt_table->aml_len);
-    memread(addr, sdt_table->aml, sdt_table->aml_len); /* get whole table */
-
-    g_assert(!acpi_calc_checksum(sdt_table->aml, sdt_table->aml_len));
-}
-
 static void test_acpi_dsdt_table(test_data *data)
 {
     AcpiSdtTable dsdt_table = {};
@@ -222,24 +215,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(&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;
@@ -619,7 +594,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 5/9] 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>
---
 tests/bios-tables-test.c | 74 +++++++++++++++++-------------------------------
 1 file changed, 26 insertions(+), 48 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index bea33a6..df61fc2 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;
@@ -75,17 +72,27 @@ 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(AcpiSdtTable *sdt_table, uint32_t addr)
+static void fetch_table(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);
     memread(addr + 4, &sdt_table->aml_len, 4); /* Length of ACPI table */
     sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
     sdt_table->aml = g_malloc0(sdt_table->aml_len);
     memread(addr, sdt_table->aml, sdt_table->aml_len); /* get whole table */
 
-    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)
@@ -119,16 +126,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;
 
     /* read the header */
-    fetch_table(&rsdt, addr);
-    ACPI_ASSERT_CMP(rsdt.header->signature, "RSDT");
+    fetch_table(&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);
@@ -136,9 +140,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(&ssdt_table, addr);
+        fetch_table(&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);
@@ -149,12 +152,17 @@ 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(table.header->signature, "FACP");
 
-    ACPI_ASSERT_CMP(fadt->header->signature, "FACP");
+    /* Since DSDT/FACS isn't in RSDT, add them to ASL test list manually */
+    fetch_table(&table, fadt_aml + 36 /* FIRMWARE_CTRL */, "FACS", false);
+    g_array_append_val(data->tables, table);
 
-    memcpy(&data->facs_addr, fadt->aml + 36 /* FIRMWARE_CTRL */, 4);
-    memcpy(&data->dsdt_addr, fadt->aml + 40 /* DSDT */, 4);
+    fetch_table(&table, fadt_aml + 40 /* DSDT */, "DSDT", true);
+    g_array_append_val(data->tables, table);
 }
 
 static void sanitize_fadt_ptrs(test_data *data)
@@ -187,34 +195,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(facs_table->signature, addr);
-    ACPI_READ_FIELD(facs_table->length, addr);
-    ACPI_READ_FIELD(facs_table->hardware_signature, addr);
-    ACPI_READ_FIELD(facs_table->firmware_waking_vector, addr);
-    ACPI_READ_FIELD(facs_table->global_lock, addr);
-    ACPI_READ_FIELD(facs_table->flags, addr);
-    ACPI_READ_ARRAY(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(&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;
@@ -595,8 +575,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 6/9] 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>
---
 tests/acpi-utils.h       | 22 ++++++-----------
 tests/acpi-utils.c       | 33 +++++++++++++++++++------
 tests/bios-tables-test.c | 48 +++++++++---------------------------
 tests/vmgenid-test.c     | 63 +++++++++++++++---------------------------------
 4 files changed, 62 insertions(+), 104 deletions(-)

diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
index 2244e8e..1b584d3 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(arr, addr)                               \
     ACPI_READ_ARRAY_PTR(arr, sizeof(arr) / sizeof(arr[0]), addr)
 
-#define ACPI_READ_TABLE_HEADER(table, addr)                      \
-    do {                                                         \
-        ACPI_READ_FIELD((table)->signature, addr);               \
-        ACPI_READ_FIELD((table)->length, addr);                  \
-        ACPI_READ_FIELD((table)->revision, addr);                \
-        ACPI_READ_FIELD((table)->checksum, addr);                \
-        ACPI_READ_ARRAY((table)->oem_id, addr);                  \
-        ACPI_READ_ARRAY((table)->oem_table_id, addr);            \
-        ACPI_READ_FIELD((table)->oem_revision, addr);            \
-        ACPI_READ_ARRAY((table)->asl_compiler_id, addr);         \
-        ACPI_READ_FIELD((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,16 @@ 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(void);
-uint32_t acpi_get_rsdt_address(uint8_t *rsdp_table);
 uint64_t acpi_get_xsdt_address(uint8_t *rsdp_table);
 void acpi_parse_rsdp_table(uint32_t addr, uint8_t *rsdp_table);
+void acpi_fetch_table(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 d07f748..de1492c 100644
--- a/tests/acpi-utils.c
+++ b/tests/acpi-utils.c
@@ -52,14 +52,6 @@ uint32_t acpi_find_rsdp_address(void)
     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;
@@ -93,3 +85,28 @@ void acpi_parse_rsdp_table(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(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);
+    memread(addr + 4, &len, 4); /* Length of ACPI table */
+    *aml_len = le32_to_cpu(len);
+    *aml = g_malloc0(*aml_len);
+    memread(addr, *aml, *aml_len); /* get whole table */
+
+    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 df61fc2..8160fc0 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -71,30 +71,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(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);
-    memread(addr + 4, &sdt_table->aml_len, 4); /* Length of ACPI table */
-    sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
-    sdt_table->aml = g_malloc0(sdt_table->aml_len);
-    memread(addr, sdt_table->aml, sdt_table->aml_len); /* get whole table */
-
-    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();
@@ -126,22 +102,18 @@ 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;
 
-    /* read the header */
-    fetch_table(&rsdt, &data->rsdp_table[16 /* RsdtAddress */], "RSDT", true);
+    /* read RSDT table */
+    acpi_fetch_table(&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(&ssdt_table, rsdt.aml + tables_off + i * entry_size,
-                    NULL, true);
+        acpi_fetch_table(&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);
@@ -158,10 +130,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(&table, fadt_aml + 36 /* FIRMWARE_CTRL */, "FACS", false);
+    acpi_fetch_table(&table.aml, &table.aml_len,
+                     fadt_aml + 36 /* FIRMWARE_CTRL */, "FACS", false);
     g_array_append_val(data->tables, table);
 
-    fetch_table(&table, fadt_aml + 40 /* DSDT */, "DSDT", true);
+    acpi_fetch_table(&table.aml, &table.aml_len,
+                     fadt_aml + 40 /* DSDT */, "DSDT", true);
     g_array_append_val(data->tables, table);
 }
 
@@ -346,7 +320,7 @@ try_again:
             fprintf(stderr, "\nUsing 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 97219ae..316dcf5 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(void)
 {
     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(global_qtest);
@@ -53,47 +40,35 @@ static uint32_t acpi_find_vgia(void)
     g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
 
     acpi_parse_rsdp_table(rsdp_offset, rsdp_table);
+    acpi_fetch_table(&rsdt, &rsdt_len, &rsdp_table[16 /* RsdtAddress */],
+                     "RSDT", true);
 
-    rsdt = acpi_get_rsdt_address(rsdp_table);
-    g_assert(rsdt);
-
-    /* read the header */
-    ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
-    ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
-    rsdt_table_length = le32_to_cpu(rsdt_table.length);
+    ACPI_FOREACH_RSDT_ENTRY(rsdt, rsdt_len, ent, 4 /* Entry size */) {
+        uint8_t *table_aml;
 
-    /* compute the table entries in rsdt */
-    g_assert_cmpint(rsdt_table_length, >, sizeof(AcpiRsdtDescriptorRev1));
-    tables_nr = (rsdt_table_length - sizeof(AcpiRsdtDescriptorRev1)) /
-                sizeof(uint32_t);
-
-    /* get the addresses of the tables pointed by rsdt */
-    tables = g_new0(uint32_t, tables_nr);
-    ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
-
-    for (i = 0; i < tables_nr; i++) {
-        uint32_t addr = le32_to_cpu(tables[i]);
-        ACPI_READ_TABLE_HEADER(&ssdt_table, addr);
-        if (!strncmp((char *)ssdt_table.oem_table_id, "VMGENID", 7)) {
+        acpi_fetch_table(&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(vgid_table.name_op, addr);
-            g_assert(vgid_table.name_op == 0x08);  /* name */
-            ACPI_READ_ARRAY(vgid_table.vgia, addr);
-            g_assert(memcmp(vgid_table.vgia, "VGIA", 4) == 0);
-            ACPI_READ_FIELD(vgid_table.val_op, addr);
-            g_assert(vgid_table.val_op == 0x0C);  /* dword */
-            ACPI_READ_FIELD(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 7/9] 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>
---
 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 1b584d3..bc0cd19 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(field, addr)            \
-    do {                                        \
-        memread(addr, &field, sizeof(field));   \
-        addr += sizeof(field);                  \
-    } while (0)
-
-#define ACPI_READ_ARRAY_PTR(arr, length, addr)  \
-    do {                                        \
-        int idx;                                \
-        for (idx = 0; idx < length; ++idx) {    \
-            ACPI_READ_FIELD(arr[idx], addr);    \
-        }                                       \
-    } while (0)
-
-#define ACPI_READ_ARRAY(arr, addr)                               \
-    ACPI_READ_ARRAY_PTR(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 8160fc0..9aa7c86 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -402,32 +402,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(ep_table->anchor_string, addr);
+    memread(addr, ep_table, sizeof(*ep_table));
     if (memcmp(ep_table->anchor_string, "_SM_", 4)) {
         return false;
     }
-    ACPI_READ_FIELD(ep_table->checksum, addr);
-    ACPI_READ_FIELD(ep_table->length, addr);
-    ACPI_READ_FIELD(ep_table->smbios_major_version, addr);
-    ACPI_READ_FIELD(ep_table->smbios_minor_version, addr);
-    ACPI_READ_FIELD(ep_table->max_structure_size, addr);
-    ACPI_READ_FIELD(ep_table->entry_point_revision, addr);
-    ACPI_READ_ARRAY(ep_table->formatted_area, addr);
-    ACPI_READ_ARRAY(ep_table->intermediate_anchor_string, addr);
     if (memcmp(ep_table->intermediate_anchor_string, "_DMI_", 5)) {
         return false;
     }
-    ACPI_READ_FIELD(ep_table->intermediate_checksum, addr);
-    ACPI_READ_FIELD(ep_table->structure_table_length, addr);
     if (ep_table->structure_table_length == 0) {
         return false;
     }
-    ACPI_READ_FIELD(ep_table->structure_table_address, addr);
-    ACPI_READ_FIELD(ep_table->number_of_structures, addr);
     if (ep_table->number_of_structures == 0) {
         return false;
     }
-    ACPI_READ_FIELD(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 8/9] 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 | 40 ++++++++++------------------------------
 1 file changed, 10 insertions(+), 30 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 9aa7c86..40b224f 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -126,6 +126,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");
 
@@ -137,36 +138,17 @@ static void test_acpi_fadt_table(test_data *data)
     acpi_fetch_table(&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((uint8_t *)sdt->aml, sdt->aml_len));
-
-        /* sdt->aml field offset := spec offset - header size */
-        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)
@@ -537,8 +519,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 9/9] 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 bc0cd19..8d04f76 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 40b224f..1a4e3b3 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -43,6 +43,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);
@@ -128,7 +133,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(&table.aml, &table.aml_len,
@@ -167,7 +172,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 {
@@ -185,11 +190,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;
@@ -285,7 +285,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, "\nLooking for expected file '%s'\n", aml_file);
         }
@@ -345,14 +345,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 1/9] tests: acpi: remove not used ACPI_READ_GENERIC_ADDRESS macro

[Philippe Mathieu-Daudé]

On 12/10/18 7:10 PM, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

> ---
>  tests/acpi-utils.h | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> index 4f4899d..c8844a2 100644
> --- a/tests/acpi-utils.h
> +++ b/tests/acpi-utils.h
> @@ -70,14 +70,6 @@ typedef struct {
>      g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
>  } while (0)
>  
> -#define ACPI_READ_GENERIC_ADDRESS(field, addr)       \
> -    do {                                             \
> -        ACPI_READ_FIELD((field).space_id, addr);     \
> -        ACPI_READ_FIELD((field).bit_width, addr);    \
> -        ACPI_READ_FIELD((field).bit_offset, addr);   \
> -        ACPI_READ_FIELD((field).access_width, addr); \
> -        ACPI_READ_FIELD((field).address, addr);      \
> -    } while (0)
>  
>  
>  uint8_t acpi_calc_checksum(const uint8_t *data, int len);
> 

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

[Michael S. Tsirkin]

On Mon, Dec 10, 2018 at 07:10:06PM +0100, Igor Mammedov wrote:
> 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) 
>    * fixiex FACS not beint compared against reference tables
> Overall test is reduced on ~170LOC and hopefully it makes easier to add more
> stuff on top.

So this was posted outside the merge window - do you still want
it merged? If yes pls repost.

Another idea I had for a while is to compare against expected
files before iasl. Only disassemble on comparison failure.

This will
- speed the good path up
- allow testing on systems without iasl (though debugging will be
  harder)

> PS:
> Series depends on '[PATCH v3 0/8] hw: acpi: RSDP fixes and refactoring'
> to avoid nontrivial conflicts
> 
> PS2:
> arm/virt test patches fill follow up a separate series on top of this one
> for not to mix things up
> 
> 
> 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>
> 
> 
> Igor Mammedov (9):
>   tests: acpi: remove not used ACPI_READ_GENERIC_ADDRESS macro
>   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       |  51 ++--------
>  tests/acpi-utils.c       |  33 ++++--
>  tests/bios-tables-test.c | 257 ++++++++++++-----------------------------------
>  tests/vmgenid-test.c     |  63 ++++--------
>  4 files changed, 116 insertions(+), 288 deletions(-)
> 
> -- 
> 2.7.4

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

[Daniel P. Berrangé]

On Wed, Dec 19, 2018 at 11:38:36AM -0500, Michael S. Tsirkin wrote:
> On Mon, Dec 10, 2018 at 07:10:06PM +0100, Igor Mammedov wrote:
> > 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) 
> >    * fixiex FACS not beint compared against reference tables
> > Overall test is reduced on ~170LOC and hopefully it makes easier to add more
> > stuff on top.
> 
> So this was posted outside the merge window - do you still want
> it merged? If yes pls repost.

Huh, QEMU development does not have merge windows. Subsystem maintainers
should review patch series at any time & queue it if it is acceptable.
The freeze process only applies to maintainers sending pull requests for
merge to git mater.  Contributors shouldn't be expected to resubmit
patches in this case.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Michael S. Tsirkin]

On Wed, Dec 19, 2018 at 04:45:17PM +0000, Daniel P. Berrangé wrote:
> On Wed, Dec 19, 2018 at 11:38:36AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Dec 10, 2018 at 07:10:06PM +0100, Igor Mammedov wrote:
> > > 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) 
> > >    * fixiex FACS not beint compared against reference tables
> > > Overall test is reduced on ~170LOC and hopefully it makes easier to add more
> > > stuff on top.
> > 
> > So this was posted outside the merge window - do you still want
> > it merged? If yes pls repost.
> 
> Huh, QEMU development does not have merge windows. Subsystem maintainers
> should review patch series at any time & queue it if it is acceptable.
> The freeze process only applies to maintainers sending pull requests for
> merge to git mater.  Contributors shouldn't be expected to resubmit
> patches in this case.
> 
> Regards,
> Daniel

Yea, I try to help drive-by contributors by tracking their patches even
during the freeze. Merged some of these in the 1st pull req.  But Igor
is a co-maintainer of the ACPI subsystem. Shouldn't be a problem.

> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 2/9] tests: acpi: use AcpiSdtTable::aml in consistent way

[Wainer dos Santos Moschetta]

On 12/10/2018 04:10 PM, Igor Mammedov wrote:
> 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>
> ---
>   tests/acpi-utils.h       |  6 +++--
>   tests/bios-tables-test.c | 59 +++++++++++++++++-------------------------------
>   2 files changed, 25 insertions(+), 40 deletions(-)
>
> diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> index c8844a2..2244e8e 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 8749b77..1666cf7 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -161,29 +161,24 @@ 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)
> -        ));
> +        g_assert(!acpi_calc_checksum((uint8_t *)sdt->aml, sdt->aml_len));

Being uint8_t * aml, does it still need that cast to uint8_t?

>   
>           /* sdt->aml field offset := spec offset - header size */

Need to change ^ comment too since offset is not relative to header 
length anymore.


- Wainer

> -        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 */
> +        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;
>       }
>   }
> @@ -210,30 +205,21 @@ static void test_acpi_facs_table(test_data *data)
>    */
>   static void fetch_table(AcpiSdtTable *sdt_table, uint32_t addr)
>   {
> -    uint8_t checksum;
> -
> -    memset(sdt_table, 0, sizeof(*sdt_table));
> -    ACPI_READ_TABLE_HEADER(&sdt_table->header, addr);
> -
> -    sdt_table->aml_len = le32_to_cpu(sdt_table->header.length)
> -                         - sizeof(AcpiTableHeader);
> +    memread(addr + 4, &sdt_table->aml_len, 4); /* Length of ACPI table */
> +    sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
>       sdt_table->aml = g_malloc0(sdt_table->aml_len);
> -    ACPI_READ_ARRAY_PTR(sdt_table->aml, sdt_table->aml_len, addr);
> +    memread(addr, sdt_table->aml, sdt_table->aml_len); /* get whole table */
>   
> -    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(&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);
> @@ -246,7 +232,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]);
> @@ -273,7 +259,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 {
> @@ -282,8 +268,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);
>   
> @@ -295,7 +279,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)
> @@ -390,11 +374,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, "\nLooking for expected file '%s'\n", aml_file);
>           }
> @@ -410,7 +393,7 @@ try_again:
>           if (getenv("V")) {
>               fprintf(stderr, "\nUsing 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);
> @@ -454,7 +437,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,

Re: [Qemu-devel] [PATCH 2/9] tests: acpi: use AcpiSdtTable::aml in consistent way

[Igor Mammedov]

On Wed, 19 Dec 2018 17:02:36 -0200
Wainer dos Santos Moschetta <wainersm@redhat.com> wrote:

> On 12/10/2018 04:10 PM, Igor Mammedov wrote:
> > 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>
> > ---
> >   tests/acpi-utils.h       |  6 +++--
> >   tests/bios-tables-test.c | 59 +++++++++++++++++-------------------------------
> >   2 files changed, 25 insertions(+), 40 deletions(-)
> >
> > diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> > index c8844a2..2244e8e 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 8749b77..1666cf7 100644
> > --- a/tests/bios-tables-test.c
> > +++ b/tests/bios-tables-test.c
> > @@ -161,29 +161,24 @@ 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)
> > -        ));
> > +        g_assert(!acpi_calc_checksum((uint8_t *)sdt->aml, sdt->aml_len));  
> 
> Being uint8_t * aml, does it still need that cast to uint8_t?
> 
> >   
> >           /* sdt->aml field offset := spec offset - header size */  
> 
> Need to change ^ comment too since offset is not relative to header 
> length anymore.
Thanks,
 I'll fix 2 above notes and check for other similar places

> 
> - Wainer
> 
> > -        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 */
> > +        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;
> >       }
> >   }
> > @@ -210,30 +205,21 @@ static void test_acpi_facs_table(test_data *data)
> >    */
> >   static void fetch_table(AcpiSdtTable *sdt_table, uint32_t addr)
> >   {
> > -    uint8_t checksum;
> > -
> > -    memset(sdt_table, 0, sizeof(*sdt_table));
> > -    ACPI_READ_TABLE_HEADER(&sdt_table->header, addr);
> > -
> > -    sdt_table->aml_len = le32_to_cpu(sdt_table->header.length)
> > -                         - sizeof(AcpiTableHeader);
> > +    memread(addr + 4, &sdt_table->aml_len, 4); /* Length of ACPI table */
> > +    sdt_table->aml_len = le32_to_cpu(sdt_table->aml_len);
> >       sdt_table->aml = g_malloc0(sdt_table->aml_len);
> > -    ACPI_READ_ARRAY_PTR(sdt_table->aml, sdt_table->aml_len, addr);
> > +    memread(addr, sdt_table->aml, sdt_table->aml_len); /* get whole table */
> >   
> > -    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(&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);
> > @@ -246,7 +232,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]);
> > @@ -273,7 +259,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 {
> > @@ -282,8 +268,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);
> >   
> > @@ -295,7 +279,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)
> > @@ -390,11 +374,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, "\nLooking for expected file '%s'\n", aml_file);
> >           }
> > @@ -410,7 +393,7 @@ try_again:
> >           if (getenv("V")) {
> >               fprintf(stderr, "\nUsing 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);
> > @@ -454,7 +437,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,  
> 

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

[Igor Mammedov]

On Wed, 19 Dec 2018 11:38:36 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 10, 2018 at 07:10:06PM +0100, Igor Mammedov wrote:
> > 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) 
> >    * fixiex FACS not beint compared against reference tables
> > Overall test is reduced on ~170LOC and hopefully it makes easier to add more
> > stuff on top.  
> 
> So this was posted outside the merge window - do you still want
> it merged? If yes pls repost.
no problem I'll wait for a couple of days for more comments on patches itself
and repost.

> Another idea I had for a while is to compare against expected
> files before iasl. Only disassemble on comparison failure.
I'll add that to my TODO list (it seems easy enough to implement quickly),
but so far starting VM is the largest time consumer and
with UEFI firmware it becomes even worse.

> 
> This will
> - speed the good path up
> - allow testing on systems without iasl (though debugging will be
>   harder)
> 
> > PS:
> > Series depends on '[PATCH v3 0/8] hw: acpi: RSDP fixes and refactoring'
> > to avoid nontrivial conflicts
> > 
> > PS2:
> > arm/virt test patches fill follow up a separate series on top of this one
> > for not to mix things up
> > 
> > 
> > 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>
> > 
> > 
> > Igor Mammedov (9):
> >   tests: acpi: remove not used ACPI_READ_GENERIC_ADDRESS macro
> >   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       |  51 ++--------
> >  tests/acpi-utils.c       |  33 ++++--
> >  tests/bios-tables-test.c | 257 ++++++++++++-----------------------------------
> >  tests/vmgenid-test.c     |  63 ++++--------
> >  4 files changed, 116 insertions(+), 288 deletions(-)
> > 
> > -- 
> > 2.7.4