Re: [Qemu-devel] [PATCH for-1.7] acpi unit-test: ensure both 1.6 machine and default machine received the same acpi tables

[Michael S. Tsirkin]

On Thu, Nov 14, 2013 at 10:21:02AM +0200, Marcel Apfelbaum wrote:
> On Wed, 2013-11-13 at 21:43 +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 13, 2013 at 06:11:25PM +0200, Marcel Apfelbaum wrote:
> > > Machines before 1.6 used ACPI supplied by SeaBios,
> > > for 1.7+ machines the tables are build by qemu.
> > > This patch checks that both machines have the same ACPI tables.
> > > 
> > > Also checks the signature of FACS table.
> > > 
> > > Signed-off-by: Marcel Apfelbaum <address@hidden>
> > > ---
> > > Note that for the moment we check only that the
> > > machines have the same tables and not the content of the tables.
> > > 
> > > This patch assumes that the new bios.bin is used.
> > > 
> > >  tests/acpi-test.c | 106 
> > > ++++++++++++++++++++++++++++++++++++++++++------------
> > >  1 file changed, 83 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> > > index d6ff66f..ba66963 100644
> > > --- a/tests/acpi-test.c
> > > +++ b/tests/acpi-test.c
> > > @@ -30,10 +30,11 @@ typedef struct {
> > >      AcpiRsdpDescriptor rsdp_table;
> > >      AcpiRsdtDescriptorRev1 rsdt_table;
> > >      AcpiFadtDescriptorRev1 fadt_table;
> > > +    AcpiFacsDescriptorRev1 facs_table;
> > >      uint32_t *rsdt_tables_addr;
> > >      int rsdt_tables_nr;
> > >      AcpiSdtTable dsdt_table;
> > > -    AcpiSdtTable *ssdt_tables;
> > > +    GArray *ssdt_tables;
> > >  } test_data;
> > >  
> > >  #define LOW(x) ((x) & 0xff)
> > > @@ -117,6 +118,18 @@ static uint8_t boot_sector[0x200] = {
> > >  
> > >  static const char *disk = "tests/acpi-test-disk.raw";
> > >  
> > > +static void free_test_data(test_data *data)
> > > +{
> > > +    int i;
> > > +
> > > +    g_free(data->rsdt_tables_addr);
> > > +    for (i = 0; i < data->ssdt_tables->len; ++i) {
> > > +        g_free(g_array_index(data->ssdt_tables, AcpiSdtTable, i).aml);
> > > +    }
> > > +    g_array_free(data->ssdt_tables, false);
> > > +    g_free(data->dsdt_table.aml);
> > > +}
> > > +
> > >  static uint8_t acpi_checksum(const uint8_t *data, int len)
> > >  {
> > >      int i;
> > > @@ -252,6 +265,22 @@ static void test_acpi_fadt_table(test_data *data)
> > >      g_assert(!acpi_checksum((uint8_t *)fadt_table, fadt_table->length));
> > >  }
> > >  
> > > +static void test_acpi_facs_table(test_data *data)
> > > +{
> > > +    AcpiFacsDescriptorRev1 *facs_table = &data->facs_table;
> > > +    uint32_t addr = data->fadt_table.firmware_ctrl;
> > > +
> > > +    ACPI_READ_FIELD(facs_table->signature, addr);
> > > +    ACPI_READ_FIELD(facs_table->length, addr);
> > > +    ACPI_READ_FIELD(facs_table->hardware_signature, addr);
> > > +    ACPI_READ_FIELD(facs_table->firmware_waking_vector, addr);
> > > +    ACPI_READ_FIELD(facs_table->global_lock, addr);
> > > +    ACPI_READ_FIELD(facs_table->flags, addr);
> > > +    ACPI_READ_ARRAY(facs_table->resverved3, addr);
> > > +
> > > +    g_assert_cmphex(facs_table->signature, ==, ACPI_FACS_SIGNATURE);
> > > +}
> > > +
> > >  static void test_dst_table(AcpiSdtTable *sdt_table, uint32_t addr)
> > >  {
> > >      uint8_t checksum;
> > > @@ -278,30 +307,30 @@ static void test_acpi_dsdt_table(test_data *data)
> > >  
> > >  static void test_acpi_ssdt_tables(test_data *data)
> > >  {
> > > -    AcpiSdtTable *ssdt_tables;
> > > +    GArray *ssdt_tables;
> > >      int ssdt_tables_nr = data->rsdt_tables_nr - 1; /* fadt is first */
> > >      int i;
> > >  
> > > -    ssdt_tables = g_new0(AcpiSdtTable, ssdt_tables_nr);
> > > +    ssdt_tables = g_array_sized_new(false, true, sizeof(AcpiSdtTable),
> > > +                                    ssdt_tables_nr);
> > >      for (i = 0; i < ssdt_tables_nr; i++) {
> > > -        AcpiSdtTable *ssdt_table = &ssdt_tables[i];
> > > +        AcpiSdtTable ssdt_table;
> > >          uint32_t addr = data->rsdt_tables_addr[i + 1]; /* fadt is first 
> > > */
> > > -
> > > -        test_dst_table(ssdt_table, addr);
> > > +        test_dst_table(&ssdt_table, addr);
> > > +        g_array_append_val(ssdt_tables, ssdt_table);
> > >      }
> > >      data->ssdt_tables = ssdt_tables;
> > >  }
> > >  
> > > -static void test_acpi_one(const char *params)
> > > +static void test_acpi_one(const char *params, test_data *data)
> > >  {
> > >      char *args;
> > >      uint8_t signature_low;
> > >      uint8_t signature_high;
> > >      uint16_t signature;
> > >      int i;
> > > -    test_data data;
> > >  
> > > -    memset(&data, 0, sizeof(data));
> > > +    memset(data, 0, sizeof(*data));
> > >      args = g_strdup_printf("-net none -display none %s %s",
> > >                             params ? params : "", disk);
> > >      qtest_start(args);
> > > @@ -325,30 +354,61 @@ static void test_acpi_one(const char *params)
> > >      }
> > >      g_assert_cmphex(signature, ==, SIGNATURE);
> > >  
> > > -    test_acpi_rsdp_address(&data);
> > > -    test_acpi_rsdp_table(&data);
> > > -    test_acpi_rsdt_table(&data);
> > > -    test_acpi_fadt_table(&data);
> > > -    test_acpi_dsdt_table(&data);
> > > -    test_acpi_ssdt_tables(&data);
> > > -
> > > -    g_free(data.rsdt_tables_addr);
> > > -    for (i = 0; i < (data.rsdt_tables_nr - 1); ++i) {
> > > -        g_free(data.ssdt_tables[i].aml);
> > > -    }
> > > -    g_free(data.ssdt_tables);
> > > -    g_free(data.dsdt_table.aml);
> > > +    test_acpi_rsdp_address(data);
> > > +    test_acpi_rsdp_table(data);
> > > +    test_acpi_rsdt_table(data);
> > > +    test_acpi_fadt_table(data);
> > > +    test_acpi_facs_table(data);
> > > +    test_acpi_dsdt_table(data);
> > > +    test_acpi_ssdt_tables(data);
> > >  
> > >      qtest_quit(global_qtest);
> > >      g_free(args);
> > >  }
> > >  
> > > +static gint compare_table_signature(gconstpointer a, gconstpointer b)
> > > +{
> > > +    AcpiSdtTable *table1 = (AcpiSdtTable *)a;
> > > +    AcpiSdtTable *table2 = (AcpiSdtTable *)b;
> > > +    return (gint)table1->header.signature - 
> > > (gint)table2->header.signature;
> > > +}
> > > +
> > > +static void test_acpi_compare_machines(test_data *data1, test_data 
> > > *data2)
> > > +{
> > > +    int i;
> > > +
> > > +    /*
> > > +     * Check that both machine have the same acpi tables.
> > > +     * We only need to check the ssdt tables, because all
> > > +     * the other tables are always present */
> > > +    g_assert_cmpuint(data1->ssdt_tables->len, ==, 
> > > data2->ssdt_tables->len);
> > > +
> > > +    g_array_sort(data1->ssdt_tables, compare_table_signature);
> > > +    g_array_sort(data2->ssdt_tables, compare_table_signature);
> > > +
> > > +    for (i = 0; i < data1->ssdt_tables->len; ++i) {
> > > +        AcpiSdtTable *table1 = &g_array_index(data1->ssdt_tables, 
> > > AcpiSdtTable, i);
> > > +        AcpiSdtTable *table2 = &g_array_index(data2->ssdt_tables, 
> > > AcpiSdtTable, i);
> > > +
> > > +        g_assert_cmphex(table1->header.signature, ==, 
> > > table2->header.signature);
> > > +    }
> > > +}
> > > +
> > >  static void test_acpi_tcg(void)
> > >  {
> > > +    test_data data, data_16;
> > > +
> > >      /* Supplying -machine accel argument overrides the default (qtest).
> > >       * This is to make guest actually run.
> > >       */
> > > -    test_acpi_one("-machine accel=tcg");
> > > +    test_acpi_one("-machine accel=tcg", &data);
> > > +    test_acpi_one("-machine pc-i440fx-1.6,accel=tcg", &data_16);
> > 
> > I'm not sure this is a good idea: we can change tables and
> > they won't match 1.6 anymore.
> > How about storing expected tables under tests/?
> > This way we'll just update the expected values when we
> > change something.
> I was planning to do exactly that for the AML part (definition blocks),
> in this test I only wanted to ensure we have the same tables when
> we switch from bios supplied ACPI to qemu supplied ACPI.
> 
> Anyway, I should have used:
>     test_acpi_one("-machine pc-i440fx-1.7,accel=tcg", &data_17);
> for comparison and not:
>     test_acpi_one("-machine accel=tcg", &data);
> that tests for latest machine. (which may have different tables)
> 
> This test has also a nice side effect, that it tests
> a known good configuration and we will not have false
> positives in the test itself. (verify the test...)
> 
> What about making the change above and explicitly
> say this test compare 1.6 with 1.7 machines,
> and coding another unit-test that checks latest machines vs
> expected files?
> 
> Thanks,
> Marcel

Well it's possible that, by chance, -M 1.7 and -M 1.6
have identicaly tables ATM, but I don't see why would changing this
by bios be a regression.

> > 
> > > +
> > > +    /* for 1.6 the acpi tables come from bios */
> > > +    test_acpi_compare_machines(&data, &data_16);
> > > +
> > > +    free_test_data(&data);
> > > +    free_test_data(&data_16);
> > >  }
> > >  
> > >  int main(int argc, char *argv[])
> > > -- 
> > > 1.8.3.1
> 
>