From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40243) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WnlM3-0000U7-9l for qemu-devel@nongnu.org; Fri, 23 May 2014 05:01:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WnlLt-00041r-Le for qemu-devel@nongnu.org; Fri, 23 May 2014 05:01:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57031) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WnlLt-00041l-CY for qemu-devel@nongnu.org; Fri, 23 May 2014 05:01:21 -0400 Date: Fri, 23 May 2014 12:00:12 +0300 From: "Michael S. Tsirkin" Message-ID: <20140523090012.GA11815@redhat.com> References: <1400508595-23024-1-git-send-email-somlo@cmu.edu> <20140519194448.GA14502@redhat.com> <20140523012711.GA6121@crash.ini.cmu.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140523012711.GA6121@crash.ini.cmu.edu> Subject: Re: [Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gabriel L. Somlo" Cc: pbonzini@redhat.com, lersek@redhat.com, qemu-devel@nongnu.org, kraxel@redhat.com On Thu, May 22, 2014 at 09:27:11PM -0400, Gabriel L. Somlo wrote: > Michael, > > On Mon, May 19, 2014 at 10:44:48PM +0300, Michael S. Tsirkin wrote: > > One question: we don't seem to have unit-test for this > > interface in qemu, do we? > > I would like to see at least a basic test along the lines of > > tests/acpi-test.c: run bios to load tables from QEMU, > > then do some basic checks on the tables that bios loaded. > > So I took a closer look at tests/acpi-test.c today, and things slowly > started to shift into focus :) So now I have two questions: > > 1. There's a fairly complex setup (create a boot disk, start the > guest, loop around waiting for the bios to finish booting, watch > when your disk-based boot loader runs, etc.) before starting to > examine the guest memory for the presence and correctness of the acpi > tables. > > Would it make sense to rename this file to something like e.g. > tests/biostables-test.c, and add checks for smbios to the already > started and booted guest ? > > If not, I'd have to replicate most of your test-harness code, > which is almost half of acpi-test.c. That shouldn't be hard (you > already did the heavy lifting on that one), but I intuitively dislike > multiple cut'n'paste clones of significant code fragments :) Sure, fine. > 2. Assuming we can run smbios tests alongside acpi, what do you think > of the patch below ? I've currently stopped after finding and checking > the integrity of the smbios entry point structure. Not sure if I > really need to walk the tables themselves, or what I'd be testing for > if I did :) > > Feedback/comments/flames welcome ! :) > > Thanks much, > --Gabriel Looks good to me. > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > index 76fbccf..66cde26 100644 > --- a/tests/acpi-test.c > +++ b/tests/acpi-test.c > @@ -18,6 +18,7 @@ > #include "libqtest.h" > #include "qemu/compiler.h" > #include "hw/i386/acpi-defs.h" > +#include "hw/i386/smbios.h" > > #define MACHINE_PC "pc" > #define MACHINE_Q35 "q35" > @@ -46,6 +47,8 @@ typedef struct { > uint32_t *rsdt_tables_addr; > int rsdt_tables_nr; > GArray *tables; > + uint32_t smbios_ep_addr; > + struct smbios_entry_point smbios_ep_table; > } test_data; > > #define LOW(x) ((x) & 0xff) > @@ -220,6 +223,28 @@ static void test_acpi_rsdp_address(test_data *data) > data->rsdp_addr = off; > } > > +static void test_smbios_ep_address(test_data *data) > +{ > + uint32_t off; > + > + /* find smbios entry point structure */ > + for (off = 0xf0000; off < 0x100000; off += 0x10) { > + uint8_t sig[] = "_SM_"; > + int i; > + > + for (i = 0; i < sizeof sig - 1; ++i) { > + sig[i] = readb(off + i); > + } > + > + if (!memcmp(sig, "_SM_", sizeof sig)) { > + break; > + } > + } > + > + g_assert_cmphex(off, <, 0x100000); > + data->smbios_ep_addr = off; > +} > + > static void test_acpi_rsdp_table(test_data *data) > { > AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table; > @@ -238,6 +263,34 @@ static void test_acpi_rsdp_table(test_data *data) > g_assert(!acpi_checksum((uint8_t *)rsdp_table, 20)); > } > > +static void test_smbios_ep_table(test_data *data) > +{ > + struct smbios_entry_point *ep_table = &data->smbios_ep_table; > + uint32_t addr = data->smbios_ep_addr; > + > + ACPI_READ_ARRAY(ep_table->anchor_string, addr); > + g_assert(!memcmp(ep_table->anchor_string, "_SM_", 4)); > + ACPI_READ_FIELD(ep_table->checksum, addr); > + ACPI_READ_FIELD(ep_table->length, addr); > + ACPI_READ_FIELD(ep_table->smbios_major_version, addr); > + ACPI_READ_FIELD(ep_table->smbios_minor_version, addr); > + ACPI_READ_FIELD(ep_table->max_structure_size, addr); > + ACPI_READ_FIELD(ep_table->entry_point_revision, addr); > + ACPI_READ_ARRAY(ep_table->formatted_area, addr); > + ACPI_READ_ARRAY(ep_table->intermediate_anchor_string, addr); > + g_assert(!memcmp(ep_table->intermediate_anchor_string, "_DMI_", 5)); > + ACPI_READ_FIELD(ep_table->intermediate_checksum, addr); > + ACPI_READ_FIELD(ep_table->structure_table_length, addr); > + g_assert_cmpuint(ep_table->structure_table_length, >, 0); > + ACPI_READ_FIELD(ep_table->structure_table_address, addr); > + ACPI_READ_FIELD(ep_table->number_of_structures, addr); > + g_assert_cmpuint(ep_table->number_of_structures, >, 0); > + ACPI_READ_FIELD(ep_table->smbios_bcd_revision, addr); > + g_assert(!acpi_checksum((uint8_t *)ep_table, sizeof *ep_table)); > + g_assert(!acpi_checksum((uint8_t *)ep_table + 0x10, > + sizeof *ep_table - 0x10)); > +} > + > static void test_acpi_rsdt_table(test_data *data) > { > AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table; > @@ -633,6 +686,9 @@ static void test_acpi_one(const char *params, test_data *data) > } > } > > + test_smbios_ep_address(data); > + test_smbios_ep_table(data); > + > qtest_quit(global_qtest); > g_free(args); > }