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

[Marcel Apfelbaum]

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

> 
> > +
> > +    /* 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