Message ID | 20140523012711.GA6121@crash.ini.cmu.edu |
---|---|
State | New |
Headers | show |
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
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); }