[Qemu-devel] [PATCH for-2.10 0/5] tests: acpi: make sure FADT is compared to reference table

[Qemu-devel] [PATCH for-2.10 0/5] tests: acpi: make sure FADT is compared to reference table

[Igor Mammedov]

While refactoring i386/FADT generation to build_append_int_noprefix()            
and testing it, It turned out that FADT is only tested for valid                 
checksum but actual test for unintended changes isn't applied to it              
even though we have reference tables in tree.                                    
So here goes a couple of cleanups to reflect what fuctions do +                  
some comments and actual fix.                                                    
                                                                                 
Note to maintainer:                                                              
  FADT reference table is out of sync and should be updated along with           
  series applied.                                                                
                                                                                 
CC: "Michael S. Tsirkin" <mst@redhat.com>                                        
CC: Marcel Apfelbaum <marcel@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 for-2.10 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 63da978..b72fa80 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -229,12 +229,11 @@ static void test_acpi_dsdt_table(test_data *data)
     uint32_t addr = 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);
 }
 
@@ -634,6 +633,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 for-2.10 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 b72fa80..adbcc55 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -210,6 +210,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 = sdt_table->header.length - sizeof(AcpiTableHeader);
@@ -228,8 +229,6 @@ static void test_acpi_dsdt_table(test_data *data)
     AcpiSdtTable dsdt_table;
     uint32_t addr = 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");
 
@@ -245,7 +244,6 @@ static void test_acpi_tables(test_data *data)
     for (i = 0; i < tables_nr; i++) {
         AcpiSdtTable ssdt_table;
 
-        memset(&ssdt_table, 0, sizeof(ssdt_table));
         uint32_t addr = 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 for-2.10 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 adbcc55..ed32e9a 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -206,7 +206,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;
 
@@ -229,14 +233,15 @@ static void test_acpi_dsdt_table(test_data *data)
     AcpiSdtTable dsdt_table;
     uint32_t addr = 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;
@@ -245,7 +250,7 @@ static void test_acpi_tables(test_data *data)
         AcpiSdtTable ssdt_table;
 
         uint32_t addr = 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);
     }
 }
@@ -638,7 +643,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 for-2.10 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 ed32e9a..a2a90d7 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -251,6 +251,8 @@ static void fetch_rsdt_referenced_tables(test_data *data)
 
         uint32_t addr = 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);
     }
 }
@@ -425,6 +427,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 for-2.10 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 a2a90d7..129ef46 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -243,13 +243,13 @@ 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 = data->rsdt_tables_addr[i + 1]; /* fadt is first */
+        uint32_t addr = data->rsdt_tables_addr[i];
         fetch_table(&ssdt_table, addr);
 
         /* Add table to ASL test tables list */
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.10 0/5] tests: acpi: make sure FADT is compared to reference table

[Michael S. Tsirkin]

On Mon, Jul 31, 2017 at 05:40:47PM +0200, Igor Mammedov wrote:
> While refactoring i386/FADT generation to build_append_int_noprefix()            
> and testing it, It turned out that FADT is only tested for valid                 
> checksum but actual test for unintended changes isn't applied to it              
> even though we have reference tables in tree.                                    
> So here goes a couple of cleanups to reflect what fuctions do +                  
> some comments and actual fix.                                                    
>                                                                                  
> Note to maintainer:                                                              
>   FADT reference table is out of sync and should be updated along with           
>   series applied.                                                                
>                                                                                  
> CC: "Michael S. Tsirkin" <mst@redhat.com>                                        
> CC: Marcel Apfelbaum <marcel@redhat.com> 

Absolutely good stuff, but not a bugfix as such (it's not that the
test is wrong, it's that we skip FADT for now)
so I don't think this is 2.10 material.

> 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

Re: [Qemu-devel] [PATCH for-2.10 0/5] tests: acpi: make sure FADT is compared to reference table

[Igor Mammedov]

On Wed, 2 Aug 2017 00:14:18 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 31, 2017 at 05:40:47PM +0200, Igor Mammedov wrote:
> > While refactoring i386/FADT generation to build_append_int_noprefix()            
> > and testing it, It turned out that FADT is only tested for valid                 
> > checksum but actual test for unintended changes isn't applied to it              
> > even though we have reference tables in tree.                                    
> > So here goes a couple of cleanups to reflect what fuctions do +                  
> > some comments and actual fix.                                                    
> >                                                                                  
> > Note to maintainer:                                                              
> >   FADT reference table is out of sync and should be updated along with           
> >   series applied.                                                                
> >                                                                                  
> > CC: "Michael S. Tsirkin" <mst@redhat.com>                                        
> > CC: Marcel Apfelbaum <marcel@redhat.com>   
> 
> Absolutely good stuff, but not a bugfix as such (it's not that the
> test is wrong, it's that we skip FADT for now)
> so I don't think this is 2.10 material.
Agreed, it could go in when 2.11 merge window is open.

> 
> > 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  

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

[Marcel Apfelbaum]

On 31/07/2017 18:40, Igor Mammedov wrote:
> 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 63da978..b72fa80 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -229,12 +229,11 @@ static void test_acpi_dsdt_table(test_data *data)
>       uint32_t addr = 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);
>   }
>   
> @@ -634,6 +633,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);
> 

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

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

[Marcel Apfelbaum]

On 31/07/2017 18:40, Igor Mammedov wrote:
> 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 b72fa80..adbcc55 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -210,6 +210,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 = sdt_table->header.length - sizeof(AcpiTableHeader);
> @@ -228,8 +229,6 @@ static void test_acpi_dsdt_table(test_data *data)
>       AcpiSdtTable dsdt_table;
>       uint32_t addr = 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");
>   
> @@ -245,7 +244,6 @@ static void test_acpi_tables(test_data *data)
>       for (i = 0; i < tables_nr; i++) {
>           AcpiSdtTable ssdt_table;
>   
> -        memset(&ssdt_table, 0, sizeof(ssdt_table));
>           uint32_t addr = data->rsdt_tables_addr[i + 1]; /* fadt is first */
>           test_dst_table(&ssdt_table, addr);
>           g_array_append_val(data->tables, ssdt_table);
> 

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

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

[Marcel Apfelbaum]

On 31/07/2017 18:40, Igor Mammedov wrote:
> 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 adbcc55..ed32e9a 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -206,7 +206,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;
>   
> @@ -229,14 +233,15 @@ static void test_acpi_dsdt_table(test_data *data)
>       AcpiSdtTable dsdt_table;
>       uint32_t addr = 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;
> @@ -245,7 +250,7 @@ static void test_acpi_tables(test_data *data)
>           AcpiSdtTable ssdt_table;
>   
>           uint32_t addr = 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);
>       }
>   }
> @@ -638,7 +643,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)) {
> 


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

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

[Marcel Apfelbaum]

On 31/07/2017 18:40, Igor Mammedov wrote:
> 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 ed32e9a..a2a90d7 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -251,6 +251,8 @@ static void fetch_rsdt_referenced_tables(test_data *data)
>   
>           uint32_t addr = 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);
>       }
>   }
> @@ -425,6 +427,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;
> 


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

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

[Marcel Apfelbaum]

On 31/07/2017 18:40, 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.
> 

Hi Igor,

> 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 a2a90d7..129ef46 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -243,13 +243,13 @@ 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 = data->rsdt_tables_addr[i + 1]; /* fadt is first */
> +        uint32_t addr = data->rsdt_tables_addr[i];
>           fetch_table(&ssdt_table, addr);
>   
>           /* Add table to ASL test tables list */
> 

For some reason I decided not to test it... strange.
Anyway, we should also add the expected file, right?
(the rebuild script works only for existent tables, I think)

Thanks,
Marcel

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

[Igor Mammedov]

On Wed, 2 Aug 2017 16:15:10 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> On 31/07/2017 18:40, 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.
> >   
> 
> Hi Igor,
> 
> > 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 a2a90d7..129ef46 100644
> > --- a/tests/bios-tables-test.c
> > +++ b/tests/bios-tables-test.c
> > @@ -243,13 +243,13 @@ 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 = data->rsdt_tables_addr[i + 1]; /* fadt is first */
> > +        uint32_t addr = data->rsdt_tables_addr[i];
> >           fetch_table(&ssdt_table, addr);
> >   
> >           /* Add table to ASL test tables list */
> >   
> 
> For some reason I decided not to test it... strange.
> Anyway, we should also add the expected file, right?
> (the rebuild script works only for existent tables, I think)
they are there see: tests/acpi-test-data/*/FACP
files obviously stale as they haven't been actually used.
Michael should update them when he applies this series.

> 
> Thanks,
> Marcel
> 
> 
> 

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

[Marcel Apfelbaum]

On 02/08/2017 17:10, Igor Mammedov wrote:
> On Wed, 2 Aug 2017 16:15:10 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
> 
>> On 31/07/2017 18:40, 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.
>>>    
>>
>> Hi Igor,
>>
>>> 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 a2a90d7..129ef46 100644
>>> --- a/tests/bios-tables-test.c
>>> +++ b/tests/bios-tables-test.c
>>> @@ -243,13 +243,13 @@ 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 = data->rsdt_tables_addr[i + 1]; /* fadt is first */
>>> +        uint32_t addr = data->rsdt_tables_addr[i];
>>>            fetch_table(&ssdt_table, addr);
>>>    
>>>            /* Add table to ASL test tables list */
>>>    
>>
>> For some reason I decided not to test it... strange.
>> Anyway, we should also add the expected file, right?
>> (the rebuild script works only for existent tables, I think)
> they are there see: tests/acpi-test-data/*/FACP
> files obviously stale as they haven't been actually used.
> Michael should update them when he applies this series.
> 

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

>>
>> Thanks,
>> Marcel
>>
>>
>>
> 

Re: [Qemu-devel] [PATCH for-2.10 0/5] tests: acpi: make sure FADT is compared to reference table

[Michael S. Tsirkin]

On Wed, Aug 02, 2017 at 09:51:22AM +0200, Igor Mammedov wrote:
> On Wed, 2 Aug 2017 00:14:18 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Jul 31, 2017 at 05:40:47PM +0200, Igor Mammedov wrote:
> > > While refactoring i386/FADT generation to build_append_int_noprefix()            
> > > and testing it, It turned out that FADT is only tested for valid                 
> > > checksum but actual test for unintended changes isn't applied to it              
> > > even though we have reference tables in tree.                                    
> > > So here goes a couple of cleanups to reflect what fuctions do +                  
> > > some comments and actual fix.                                                    
> > >                                                                                  
> > > Note to maintainer:                                                              
> > >   FADT reference table is out of sync and should be updated along with           
> > >   series applied.                                                                
> > >                                                                                  
> > > CC: "Michael S. Tsirkin" <mst@redhat.com>                                        
> > > CC: Marcel Apfelbaum <marcel@redhat.com>   
> > 
> > Absolutely good stuff, but not a bugfix as such (it's not that the
> > test is wrong, it's that we skip FADT for now)
> > so I don't think this is 2.10 material.
> Agreed, it could go in when 2.11 merge window is open.

thanks,pls remember to repost or ping then.

> > 
> > > 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