From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49678) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wnwae-0008Rg-GX for qemu-devel@nongnu.org; Fri, 23 May 2014 17:01:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WnwaY-0008NF-Hp for qemu-devel@nongnu.org; Fri, 23 May 2014 17:01:20 -0400 Received: from mail-qc0-x236.google.com ([2607:f8b0:400d:c01::236]:35404) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WnwaY-0008N3-C7 for qemu-devel@nongnu.org; Fri, 23 May 2014 17:01:14 -0400 Received: by mail-qc0-f182.google.com with SMTP id e16so8861061qcx.41 for ; Fri, 23 May 2014 14:01:13 -0700 (PDT) Date: Fri, 23 May 2014 17:01:11 -0400 From: "Gabriel L. Somlo" Message-ID: <20140523210111.GB16630@crash.ini.cmu.edu> References: <1400508595-23024-1-git-send-email-somlo@cmu.edu> <20140519194448.GA14502@redhat.com> <20140523012711.GA6121@crash.ini.cmu.edu> <20140523090012.GA11815@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140523090012.GA11815@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: pbonzini@redhat.com, lersek@redhat.com, qemu-devel@nongnu.org, kraxel@redhat.com On Fri, May 23, 2014 at 12:00:12PM +0300, Michael S. Tsirkin wrote: > > 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. So I was about to send a patch with acpi-test.c renamed to bios-tables-test.c, but the patch is basically removing all of acpi-test.c, and creating a new file bios-tables-test.c. Do you have a better way to rename the file first, and then I can send a patch against it ? Or should we give up on renaming it altogether ? Or should I just bite the bullet and cut'n'paste your test harness into a new file specific to smbios ? It's not particularly important to me which way we go -- I want to do the right thing, whatever you decide that is :) Thanks, --Gabriel > > 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); > > } -- ------------------------------------ Gabriel L. Somlo Director of Computing Services Information Networking Institute Carnegie Mellon University 4616 Henry St., Pittsburgh, PA 15213 +1.412.268.9310 www.ini.cmu.edu