On Wed, 19 Dec 2018 17:02:36 -0200
Wainer dos Santos Moschetta <waine...@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 <imamm...@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,
>
