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" <mst@redhat.com>]

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 <marcel.a@redhat.com>
> ---
> 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.

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