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 <address@hidden> 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 <address@hidden>
> > ---
> >   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,  
>