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

[Wainer dos Santos Moschetta <wainersm@redhat.com>]

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,