[v4,0/3] SMBIOS cleanup round

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 :)

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

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

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" <gsomlo@gmail.com> writes:

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

Err, isn't that what a rename does?

> 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 :)

Did you rename with git-mv?  Did you diff with rename detection on?  See
diff.renames in git-config(1).

[...]

On Mon, May 26, 2014 at 08:30:48AM +0200, Markus Armbruster wrote:
> >
> > 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.
> 
> Err, isn't that what a rename does?

Being somewhere in the middle of the Git learning curve, to me a
rename mostly means editing a string in a filesystem directory entry,
without touching the actual content :) 

> > 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 :)
> 
> Did you rename with git-mv?  Did you diff with rename detection on?  See
> diff.renames in git-config(1).

Aaaah, that's what I was missing! This makes for a much more
"articulate" looking patch (which I'll be sending out shortly) :)

Thanks a ton,
--Gabriel

On 05/27/14 15:00, Gabriel L. Somlo wrote:
> On Mon, May 26, 2014 at 08:30:48AM +0200, Markus Armbruster wrote:
>>>
>>> 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.
>>
>> Err, isn't that what a rename does?
> 
> Being somewhere in the middle of the Git learning curve, to me a
> rename mostly means editing a string in a filesystem directory entry,
> without touching the actual content :) 
> 
>>> 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 :)
>>
>> Did you rename with git-mv?  Did you diff with rename detection on?  See
>> diff.renames in git-config(1).
> 
> Aaaah, that's what I was missing! This makes for a much more
> "articulate" looking patch (which I'll be sending out shortly) :)

(Side note: do this for projects that use git "natively"; don't do it
for projects that will want to apply the patch in SVN too (or even
primarily), eg. edk2. SVN chokes on git rename directives.

Side note #2: ratcheting up the "diff.renameLimit" setting will make
your cherry-picks and rebases take much more CPU hungry (potentially),
but they will also recognize renames over much longer histories, and
save you many manual conflict resolutions. See
<https://blogs.atlassian.com/2011/10/confluence_git_rename_merge_oh_my/>.)

Thanks
Laszlo