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 <marce...@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.
> 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
>
>
