[Qemu-devel] [PATCH 0/5] tests: acpi: fix FADT not being tested and cleanups

[Qemu-devel] [PATCH 0/5] tests: acpi: fix FADT not being tested and cleanups

[Igor Mammedov]

Even though we have tests/acpi-test-data/[q35|pc]/FACP reference table
in source tree, it hasn't been actually tested. As result it went stale
and we did not notice any changes that were happening to it.

Fix it buy making sure it's not ignored and before fixing it,
do some cleanups in testcase that hopefully will make more clear what
functions actually do. 

NOTE to maintaner:
   after fix in 5/5 update following reference tables, to match reality
   tests/acpi-test-data/pc/FACP
   tests/acpi-test-data/q35/FACP


CC: Michael S. Tsirkin" <mst@redhat.com>
CC: Thomas Huth <thuth@redhat.com>

Igor Mammedov (5):
  tests: acpi: move tested tables array allocation outside of
    test_acpi_dsdt_table()
  tests: acpi: init table descriptor in test_dst_table()
  tests: acpi: rename test_acpi_tables()/test_dst_table() to reflect its
    usage
  tests: acpi: add comments to fetch_rsdt_referenced_tables/data->tables
    usage
  tests: acpi: fix FADT not being compared to reference table

 tests/bios-tables-test.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/5] tests: acpi: move tested tables array allocation outside of test_acpi_dsdt_table()

[Igor Mammedov]

at best it's confusing that array for list of tables to be tested
against reference tables is allocated within test_acpi_dsdt_table()
and at worst it would just overwrite list of tables if they were
added before test_acpi_dsdt_table().
Move array initialization to test_acpi_one() before we start
processing tables.

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

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index e28e0c9..a5639d1 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -234,12 +234,11 @@ static void test_acpi_dsdt_table(test_data *data)
     uint32_t addr = le32_to_cpu(data->fadt_table.dsdt);
 
     memset(&dsdt_table, 0, sizeof(dsdt_table));
-    data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
 
     test_dst_table(&dsdt_table, addr);
     ACPI_ASSERT_CMP(dsdt_table.header.signature, "DSDT");
 
-    /* Place DSDT first */
+    /* Since DSDT isn't in RSDT, add DSDT to ASL test tables list manually */
     g_array_append_val(data->tables, dsdt_table);
 }
 
@@ -636,6 +635,7 @@ static void test_acpi_one(const char *params, test_data *data)
 
     boot_sector_test();
 
+    data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
     test_acpi_rsdp_address(data);
     test_acpi_rsdp_table(data);
     test_acpi_rsdt_table(data);
-- 
2.7.4

[Qemu-devel] [PATCH 2/5] tests: acpi: init table descriptor in test_dst_table()

[Igor Mammedov]

remove code duplication and make sure that table descriptor
passed in for initialization is in expected state.

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

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index a5639d1..7825483 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -214,6 +214,7 @@ static void test_dst_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)
@@ -233,8 +234,6 @@ static void test_acpi_dsdt_table(test_data *data)
     AcpiSdtTable dsdt_table;
     uint32_t addr = le32_to_cpu(data->fadt_table.dsdt);
 
-    memset(&dsdt_table, 0, sizeof(dsdt_table));
-
     test_dst_table(&dsdt_table, addr);
     ACPI_ASSERT_CMP(dsdt_table.header.signature, "DSDT");
 
@@ -251,7 +250,6 @@ static void test_acpi_tables(test_data *data)
         AcpiSdtTable ssdt_table;
         uint32_t addr;
 
-        memset(&ssdt_table, 0, sizeof(ssdt_table));
         addr = le32_to_cpu(data->rsdt_tables_addr[i + 1]); /* fadt is first */
         test_dst_table(&ssdt_table, addr);
         g_array_append_val(data->tables, ssdt_table);
-- 
2.7.4

[Qemu-devel] [PATCH 3/5] tests: acpi: rename test_acpi_tables()/test_dst_table() to reflect its usage

[Igor Mammedov]

Main purpose of test_dst_table() is loading a table from QEMU
with checking that checksum in header matches actual one,
rename it reflect main action it performs.

Likewise test_acpi_tables() name is to broad, while the function
only loads tables referenced by RSDT, rename it to reflect it.

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

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 7825483..af22555 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -210,7 +210,11 @@ static void test_acpi_facs_table(test_data *data)
     ACPI_ASSERT_CMP(facs_table->signature, "FACS");
 }
 
-static void test_dst_table(AcpiSdtTable *sdt_table, uint32_t addr)
+/** 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)
 {
     uint8_t checksum;
 
@@ -234,14 +238,15 @@ static void test_acpi_dsdt_table(test_data *data)
     AcpiSdtTable dsdt_table;
     uint32_t addr = le32_to_cpu(data->fadt_table.dsdt);
 
-    test_dst_table(&dsdt_table, 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 test_acpi_tables(test_data *data)
+/* 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 - 1; /* fadt is first */
     int i;
@@ -251,7 +256,7 @@ static void test_acpi_tables(test_data *data)
         uint32_t addr;
 
         addr = le32_to_cpu(data->rsdt_tables_addr[i + 1]); /* fadt is first */
-        test_dst_table(&ssdt_table, addr);
+        fetch_table(&ssdt_table, addr);
         g_array_append_val(data->tables, ssdt_table);
     }
 }
@@ -640,7 +645,7 @@ static void test_acpi_one(const char *params, test_data *data)
     test_acpi_fadt_table(data);
     test_acpi_facs_table(data);
     test_acpi_dsdt_table(data);
-    test_acpi_tables(data);
+    fetch_rsdt_referenced_tables(data);
 
     if (iasl) {
         if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
-- 
2.7.4

[Qemu-devel] [PATCH 4/5] tests: acpi: add comments to fetch_rsdt_referenced_tables/data->tables usage

[Igor Mammedov]

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

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index af22555..81c558e 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -257,6 +257,8 @@ static void fetch_rsdt_referenced_tables(test_data *data)
 
         addr = le32_to_cpu(data->rsdt_tables_addr[i + 1]); /* fadt is first */
         fetch_table(&ssdt_table, addr);
+
+        /* Add table to ASL test tables list */
         g_array_append_val(data->tables, ssdt_table);
     }
 }
@@ -427,6 +429,7 @@ try_again:
     return exp_tables;
 }
 
+/* test the list of tables in @data->tables against reference tables */
 static void test_acpi_asl(test_data *data)
 {
     int i;
-- 
2.7.4

[Qemu-devel] [PATCH 5/5] tests: acpi: fix FADT not being compared to reference table

[Igor Mammedov]

It turns out that FADT isn't actually tested for changes
against reference table, since it happens to be the 1st
table in RSDT which is currently ignored.
Fix it by making sure that all tables from RSDT are added
to test list.

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

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 81c558e..c5dccdb 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -248,14 +248,14 @@ static void test_acpi_dsdt_table(test_data *data)
 /* 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 - 1; /* fadt is first */
+    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 + 1]); /* fadt is first */
+        addr = le32_to_cpu(data->rsdt_tables_addr[i]);
         fetch_table(&ssdt_table, addr);
 
         /* Add table to ASL test tables list */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 5/5] tests: acpi: fix FADT not being compared to reference table

[Michael S. Tsirkin]

On Fri, Dec 29, 2017 at 04:16:42PM +0100, Igor Mammedov wrote:
> It turns out that FADT isn't actually tested for changes
> against reference table, since it happens to be the 1st
> table in RSDT which is currently ignored.
> Fix it by making sure that all tables from RSDT are added
> to test list.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

This was intentional, wasn't it?
The reason IIRC was that FADT includes things like the DSDT
address which can change at any time.

So I think we'll have to tweak the FADT to compare it.

E.g. replace any non-zero pointer with a known pattern,
and fix up the checksum.

What do you think?

> ---
>  tests/bios-tables-test.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 81c558e..c5dccdb 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -248,14 +248,14 @@ static void test_acpi_dsdt_table(test_data *data)
>  /* 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 - 1; /* fadt is first */
> +    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 + 1]); /* fadt is first */
> +        addr = le32_to_cpu(data->rsdt_tables_addr[i]);
>          fetch_table(&ssdt_table, addr);
>  
>          /* Add table to ASL test tables list */
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH 5/5] tests: acpi: fix FADT not being compared to reference table

[Igor Mammedov]

On Tue, 16 Jan 2018 06:16:34 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Dec 29, 2017 at 04:16:42PM +0100, Igor Mammedov wrote:
> > It turns out that FADT isn't actually tested for changes
> > against reference table, since it happens to be the 1st
> > table in RSDT which is currently ignored.
> > Fix it by making sure that all tables from RSDT are added
> > to test list.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> This was intentional, wasn't it?
> The reason IIRC was that FADT includes things like the DSDT
> address which can change at any time.
> 
> So I think we'll have to tweak the FADT to compare it.
> 
> E.g. replace any non-zero pointer with a known pattern,
> and fix up the checksum.
> 
> What do you think?
allocated pointers seem to be stable within a BIOS,
but bios update might change that, so you are right.

Perhaps we can just zero out pointers and checksum
in the table when comparing/dumping it
(if isal is fine with it).

So, pls take the rest of the series modulo this
patch which I'll fix/respin to handle pointers.

> 
> > ---
> >  tests/bios-tables-test.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > index 81c558e..c5dccdb 100644
> > --- a/tests/bios-tables-test.c
> > +++ b/tests/bios-tables-test.c
> > @@ -248,14 +248,14 @@ static void test_acpi_dsdt_table(test_data *data)
> >  /* 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 - 1; /* fadt is first */
> > +    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 + 1]); /* fadt is first */
> > +        addr = le32_to_cpu(data->rsdt_tables_addr[i]);
> >          fetch_table(&ssdt_table, addr);
> >  
> >          /* Add table to ASL test tables list */
> > -- 
> > 2.7.4  
>