Re: [Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round

[Gabriel L. Somlo]

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