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

[Michael S. Tsirkin]

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);
>  }