diff mbox series

[2/5] bios-tables-test: teach test to use smbios 3.0 tables

Message ID 20220527165651.28092-3-jusual@redhat.com
State New
Headers show
Series hw/smbios: add core_count2 to smbios table type 4 | expand

Commit Message

Julia Suvorova May 27, 2022, 4:56 p.m. UTC
Introduce the 64-bit entry point. Since we no longer have a total
number of structures, stop checking for the new ones at the EOF
structure (type 127).

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 tests/qtest/bios-tables-test.c | 101 ++++++++++++++++++++++++---------
 1 file changed, 75 insertions(+), 26 deletions(-)

Comments

Ani Sinha May 30, 2022, 6:10 a.m. UTC | #1
On Fri, May 27, 2022 at 10:27 PM Julia Suvorova <jusual@redhat.com> wrote:
>
> Introduce the 64-bit entry point. Since we no longer have a total
> number of structures, stop checking for the new ones at the EOF
> structure (type 127).
>
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
>  tests/qtest/bios-tables-test.c | 101 ++++++++++++++++++++++++---------
>  1 file changed, 75 insertions(+), 26 deletions(-)
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index a4a46e97f0..0ba9d749a5 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -75,6 +75,14 @@
>  #define OEM_TEST_ARGS      "-machine x-oem-id=" OEM_ID ",x-oem-table-id=" \
>                             OEM_TABLE_ID
>
> +#define SMBIOS_VER21 0
> +#define SMBIOS_VER30 1
> +
> +typedef struct {
> +    struct smbios_21_entry_point ep21;
> +    struct smbios_30_entry_point ep30;
> +} smbios_entry_point;
> +
>  typedef struct {
>      bool tcg_only;
>      const char *machine;
> @@ -88,8 +96,8 @@ typedef struct {
>      uint64_t rsdp_addr;
>      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
>      GArray *tables;
> -    uint32_t smbios_ep_addr;
> -    struct smbios_21_entry_point smbios_ep_table;
> +    uint64_t smbios_ep_addr[2];
> +    smbios_entry_point smbios_ep_table;
>      uint16_t smbios_cpu_max_speed;
>      uint16_t smbios_cpu_curr_speed;
>      uint8_t *required_struct_types;
> @@ -533,10 +541,10 @@ static void test_acpi_asl(test_data *data)
>      free_test_data(&exp_data);
>  }
>
> -static bool smbios_ep_table_ok(test_data *data)
> +static bool smbios_ep2_table_ok(test_data *data)
>  {
> -    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> -    uint32_t addr = data->smbios_ep_addr;
> +    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table.ep21;
> +    uint32_t addr = data->smbios_ep_addr[SMBIOS_VER21];
>
>      qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
>      if (memcmp(ep_table->anchor_string, "_SM_", 4)) {
> @@ -559,29 +567,59 @@ static bool smbios_ep_table_ok(test_data *data)
>      return true;
>  }
>
> -static void test_smbios_entry_point(test_data *data)
> +static bool smbios_ep3_table_ok(test_data *data)
> +{
> +    struct smbios_30_entry_point *ep_table = &data->smbios_ep_table.ep30;
> +    uint64_t addr = data->smbios_ep_addr[SMBIOS_VER30];
> +
> +    qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
> +    if (memcmp(ep_table->anchor_string, "_SM3_", 5)) {
> +        return false;
> +    }
> +
> +    if (acpi_calc_checksum((uint8_t *)ep_table, sizeof *ep_table)) {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static int test_smbios_entry_point(test_data *data)
>  {
>      uint32_t off;
> +    bool found_ep2 = false, found_ep3 = false;
>
>      /* find smbios entry point structure */
>      for (off = 0xf0000; off < 0x100000; off += 0x10) {
> -        uint8_t sig[] = "_SM_";
> +        uint8_t sig[] = "_SM3_";
>          int i;
>
>          for (i = 0; i < sizeof sig - 1; ++i) {
>              sig[i] = qtest_readb(data->qts, off + i);
>          }
>
> -        if (!memcmp(sig, "_SM_", sizeof sig)) {
> +        if (!found_ep2 && !memcmp(sig, "_SM_", sizeof sig - 2)) {
>              /* signature match, but is this a valid entry point? */
> -            data->smbios_ep_addr = off;
> -            if (smbios_ep_table_ok(data)) {
> -                break;
> +            data->smbios_ep_addr[SMBIOS_VER21] = off;
> +            if (smbios_ep2_table_ok(data)) {
> +                found_ep2 = true;
> +            }
> +        } else if (!found_ep3 && !memcmp(sig, "_SM3_", sizeof sig - 1)) {
> +            data->smbios_ep_addr[SMBIOS_VER30] = off;
> +            if (smbios_ep3_table_ok(data)) {
> +                found_ep3 = true;
>              }
>          }
> +
> +        if (found_ep2 || found_ep3) {
> +            break;
> +        }
>      }
>
> -    g_assert_cmphex(off, <, 0x100000);
> +    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER21], <, 0x100000);
> +    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER30], <, 0x100000);
> +
> +    return found_ep3 ? SMBIOS_VER30 : SMBIOS_VER21;
>  }
>
>  static inline bool smbios_single_instance(uint8_t type)
> @@ -625,16 +663,23 @@ static bool smbios_cpu_test(test_data *data, uint32_t addr)
>      return true;
>  }
>
> -static void test_smbios_structs(test_data *data)
> +static void test_smbios_structs(test_data *data, int ver)
>  {
>      DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 };
> -    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> -    uint32_t addr = le32_to_cpu(ep_table->structure_table_address);
> -    int i, len, max_len = 0;
> +
> +    smbios_entry_point *ep_table = &data->smbios_ep_table;
> +    int i = 0, len, max_len = 0;
>      uint8_t type, prv, crt;
> +    uint64_t addr;
> +
> +    if (ver == SMBIOS_VER21) {
> +        addr = le32_to_cpu(ep_table->ep21.structure_table_address);
> +    } else {
> +        addr = le64_to_cpu(ep_table->ep30.structure_table_address);
> +    }
>
>      /* walk the smbios tables */
> -    for (i = 0; i < le16_to_cpu(ep_table->number_of_structures); i++) {
> +    do {
>
>          /* grab type and formatted area length from struct header */
>          type = qtest_readb(data->qts, addr);
> @@ -660,19 +705,23 @@ static void test_smbios_structs(test_data *data)
>          }
>
>          /* keep track of max. struct size */
> -        if (max_len < len) {
> +        if (ver == SMBIOS_VER21 && max_len < len) {
>              max_len = len;
> -            g_assert_cmpuint(max_len, <=, ep_table->max_structure_size);
> +            g_assert_cmpuint(max_len, <=, ep_table->ep21.max_structure_size);
>          }
>
>          /* start of next structure */
>          addr += len;
> -    }
>
> -    /* total table length and max struct size must match entry point values */
> -    g_assert_cmpuint(le16_to_cpu(ep_table->structure_table_length), ==,
> -                     addr - le32_to_cpu(ep_table->structure_table_address));
> -    g_assert_cmpuint(le16_to_cpu(ep_table->max_structure_size), ==, max_len);
> +    } while (ver == SMBIOS_VER21 ?
> +                (++i < le16_to_cpu(ep_table->ep21.number_of_structures)) : (type != 127));

This is quite an unreadable way to specify the loop condition.  I
wonder if there is a better way to structure this.
Secondly, instead of hardcoding 127 why do we not say (type < SMBIOS_MAX_TYPE) ?

> +
> +    if (ver == SMBIOS_VER21) {
> +        /* total table length and max struct size must match entry point values */
> +        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.structure_table_length), ==,
> +                         addr - le32_to_cpu(ep_table->ep21.structure_table_address));
> +        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.max_structure_size), ==, max_len);
> +    }
>
>      /* required struct types must all be present */
>      for (i = 0; i < data->required_struct_types_len; i++) {
> @@ -756,8 +805,8 @@ static void test_acpi_one(const char *params, test_data *data)
>       * https://bugs.launchpad.net/qemu/+bug/1821884
>       */
>      if (!use_uefi) {
> -        test_smbios_entry_point(data);
> -        test_smbios_structs(data);
> +        int ver = test_smbios_entry_point(data);
> +        test_smbios_structs(data, ver);
>      }
>
>      qtest_quit(data->qts);
> --
> 2.35.1
>
Julia Suvorova May 31, 2022, 12:39 p.m. UTC | #2
On Mon, May 30, 2022 at 8:11 AM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Fri, May 27, 2022 at 10:27 PM Julia Suvorova <jusual@redhat.com> wrote:
> >
> > Introduce the 64-bit entry point. Since we no longer have a total
> > number of structures, stop checking for the new ones at the EOF
> > structure (type 127).
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> >  tests/qtest/bios-tables-test.c | 101 ++++++++++++++++++++++++---------
> >  1 file changed, 75 insertions(+), 26 deletions(-)
> >
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index a4a46e97f0..0ba9d749a5 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -75,6 +75,14 @@
> >  #define OEM_TEST_ARGS      "-machine x-oem-id=" OEM_ID ",x-oem-table-id=" \
> >                             OEM_TABLE_ID
> >
> > +#define SMBIOS_VER21 0
> > +#define SMBIOS_VER30 1
> > +
> > +typedef struct {
> > +    struct smbios_21_entry_point ep21;
> > +    struct smbios_30_entry_point ep30;
> > +} smbios_entry_point;
> > +
> >  typedef struct {
> >      bool tcg_only;
> >      const char *machine;
> > @@ -88,8 +96,8 @@ typedef struct {
> >      uint64_t rsdp_addr;
> >      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
> >      GArray *tables;
> > -    uint32_t smbios_ep_addr;
> > -    struct smbios_21_entry_point smbios_ep_table;
> > +    uint64_t smbios_ep_addr[2];
> > +    smbios_entry_point smbios_ep_table;
> >      uint16_t smbios_cpu_max_speed;
> >      uint16_t smbios_cpu_curr_speed;
> >      uint8_t *required_struct_types;
> > @@ -533,10 +541,10 @@ static void test_acpi_asl(test_data *data)
> >      free_test_data(&exp_data);
> >  }
> >
> > -static bool smbios_ep_table_ok(test_data *data)
> > +static bool smbios_ep2_table_ok(test_data *data)
> >  {
> > -    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> > -    uint32_t addr = data->smbios_ep_addr;
> > +    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table.ep21;
> > +    uint32_t addr = data->smbios_ep_addr[SMBIOS_VER21];
> >
> >      qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
> >      if (memcmp(ep_table->anchor_string, "_SM_", 4)) {
> > @@ -559,29 +567,59 @@ static bool smbios_ep_table_ok(test_data *data)
> >      return true;
> >  }
> >
> > -static void test_smbios_entry_point(test_data *data)
> > +static bool smbios_ep3_table_ok(test_data *data)
> > +{
> > +    struct smbios_30_entry_point *ep_table = &data->smbios_ep_table.ep30;
> > +    uint64_t addr = data->smbios_ep_addr[SMBIOS_VER30];
> > +
> > +    qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
> > +    if (memcmp(ep_table->anchor_string, "_SM3_", 5)) {
> > +        return false;
> > +    }
> > +
> > +    if (acpi_calc_checksum((uint8_t *)ep_table, sizeof *ep_table)) {
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +static int test_smbios_entry_point(test_data *data)
> >  {
> >      uint32_t off;
> > +    bool found_ep2 = false, found_ep3 = false;
> >
> >      /* find smbios entry point structure */
> >      for (off = 0xf0000; off < 0x100000; off += 0x10) {
> > -        uint8_t sig[] = "_SM_";
> > +        uint8_t sig[] = "_SM3_";
> >          int i;
> >
> >          for (i = 0; i < sizeof sig - 1; ++i) {
> >              sig[i] = qtest_readb(data->qts, off + i);
> >          }
> >
> > -        if (!memcmp(sig, "_SM_", sizeof sig)) {
> > +        if (!found_ep2 && !memcmp(sig, "_SM_", sizeof sig - 2)) {
> >              /* signature match, but is this a valid entry point? */
> > -            data->smbios_ep_addr = off;
> > -            if (smbios_ep_table_ok(data)) {
> > -                break;
> > +            data->smbios_ep_addr[SMBIOS_VER21] = off;
> > +            if (smbios_ep2_table_ok(data)) {
> > +                found_ep2 = true;
> > +            }
> > +        } else if (!found_ep3 && !memcmp(sig, "_SM3_", sizeof sig - 1)) {
> > +            data->smbios_ep_addr[SMBIOS_VER30] = off;
> > +            if (smbios_ep3_table_ok(data)) {
> > +                found_ep3 = true;
> >              }
> >          }
> > +
> > +        if (found_ep2 || found_ep3) {
> > +            break;
> > +        }
> >      }
> >
> > -    g_assert_cmphex(off, <, 0x100000);
> > +    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER21], <, 0x100000);
> > +    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER30], <, 0x100000);
> > +
> > +    return found_ep3 ? SMBIOS_VER30 : SMBIOS_VER21;
> >  }
> >
> >  static inline bool smbios_single_instance(uint8_t type)
> > @@ -625,16 +663,23 @@ static bool smbios_cpu_test(test_data *data, uint32_t addr)
> >      return true;
> >  }
> >
> > -static void test_smbios_structs(test_data *data)
> > +static void test_smbios_structs(test_data *data, int ver)
> >  {
> >      DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 };
> > -    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> > -    uint32_t addr = le32_to_cpu(ep_table->structure_table_address);
> > -    int i, len, max_len = 0;
> > +
> > +    smbios_entry_point *ep_table = &data->smbios_ep_table;
> > +    int i = 0, len, max_len = 0;
> >      uint8_t type, prv, crt;
> > +    uint64_t addr;
> > +
> > +    if (ver == SMBIOS_VER21) {
> > +        addr = le32_to_cpu(ep_table->ep21.structure_table_address);
> > +    } else {
> > +        addr = le64_to_cpu(ep_table->ep30.structure_table_address);
> > +    }
> >
> >      /* walk the smbios tables */
> > -    for (i = 0; i < le16_to_cpu(ep_table->number_of_structures); i++) {
> > +    do {
> >
> >          /* grab type and formatted area length from struct header */
> >          type = qtest_readb(data->qts, addr);
> > @@ -660,19 +705,23 @@ static void test_smbios_structs(test_data *data)
> >          }
> >
> >          /* keep track of max. struct size */
> > -        if (max_len < len) {
> > +        if (ver == SMBIOS_VER21 && max_len < len) {
> >              max_len = len;
> > -            g_assert_cmpuint(max_len, <=, ep_table->max_structure_size);
> > +            g_assert_cmpuint(max_len, <=, ep_table->ep21.max_structure_size);
> >          }
> >
> >          /* start of next structure */
> >          addr += len;
> > -    }
> >
> > -    /* total table length and max struct size must match entry point values */
> > -    g_assert_cmpuint(le16_to_cpu(ep_table->structure_table_length), ==,
> > -                     addr - le32_to_cpu(ep_table->structure_table_address));
> > -    g_assert_cmpuint(le16_to_cpu(ep_table->max_structure_size), ==, max_len);
> > +    } while (ver == SMBIOS_VER21 ?
> > +                (++i < le16_to_cpu(ep_table->ep21.number_of_structures)) : (type != 127));
>
> This is quite an unreadable way to specify the loop condition.  I
> wonder if there is a better way to structure this.
> Secondly, instead of hardcoding 127 why do we not say (type < SMBIOS_MAX_TYPE) ?

Here we are looking for a special structure type 127. There can be up
to 256 structure types, and they can be placed before the final
structure.

But it seems like we need to change SMBIOS_MAX_TYPE in the test.

> > +
> > +    if (ver == SMBIOS_VER21) {
> > +        /* total table length and max struct size must match entry point values */
> > +        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.structure_table_length), ==,
> > +                         addr - le32_to_cpu(ep_table->ep21.structure_table_address));
> > +        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.max_structure_size), ==, max_len);
> > +    }
> >
> >      /* required struct types must all be present */
> >      for (i = 0; i < data->required_struct_types_len; i++) {
> > @@ -756,8 +805,8 @@ static void test_acpi_one(const char *params, test_data *data)
> >       * https://bugs.launchpad.net/qemu/+bug/1821884
> >       */
> >      if (!use_uefi) {
> > -        test_smbios_entry_point(data);
> > -        test_smbios_structs(data);
> > +        int ver = test_smbios_entry_point(data);
> > +        test_smbios_structs(data, ver);
> >      }
> >
> >      qtest_quit(data->qts);
> > --
> > 2.35.1
> >
>
Igor Mammedov June 2, 2022, 3:04 p.m. UTC | #3
On Fri, 27 May 2022 18:56:48 +0200
Julia Suvorova <jusual@redhat.com> wrote:

> Introduce the 64-bit entry point. Since we no longer have a total
> number of structures, stop checking for the new ones at the EOF
> structure (type 127).
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
>  tests/qtest/bios-tables-test.c | 101 ++++++++++++++++++++++++---------
>  1 file changed, 75 insertions(+), 26 deletions(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index a4a46e97f0..0ba9d749a5 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -75,6 +75,14 @@
>  #define OEM_TEST_ARGS      "-machine x-oem-id=" OEM_ID ",x-oem-table-id=" \
>                             OEM_TABLE_ID
>  
> +#define SMBIOS_VER21 0
> +#define SMBIOS_VER30 1
> +
> +typedef struct {
> +    struct smbios_21_entry_point ep21;
> +    struct smbios_30_entry_point ep30;
> +} smbios_entry_point;
> +
>  typedef struct {
>      bool tcg_only;
>      const char *machine;
> @@ -88,8 +96,8 @@ typedef struct {
>      uint64_t rsdp_addr;
>      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
>      GArray *tables;
> -    uint32_t smbios_ep_addr;
> -    struct smbios_21_entry_point smbios_ep_table;
> +    uint64_t smbios_ep_addr[2];
> +    smbios_entry_point smbios_ep_table;
>      uint16_t smbios_cpu_max_speed;
>      uint16_t smbios_cpu_curr_speed;
>      uint8_t *required_struct_types;
> @@ -533,10 +541,10 @@ static void test_acpi_asl(test_data *data)
>      free_test_data(&exp_data);
>  }
>  
> -static bool smbios_ep_table_ok(test_data *data)
> +static bool smbios_ep2_table_ok(test_data *data)
>  {
> -    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> -    uint32_t addr = data->smbios_ep_addr;
> +    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table.ep21;
> +    uint32_t addr = data->smbios_ep_addr[SMBIOS_VER21];
>  
>      qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
>      if (memcmp(ep_table->anchor_string, "_SM_", 4)) {
> @@ -559,29 +567,59 @@ static bool smbios_ep_table_ok(test_data *data)
>      return true;
>  }
>  
> -static void test_smbios_entry_point(test_data *data)
> +static bool smbios_ep3_table_ok(test_data *data)
> +{
> +    struct smbios_30_entry_point *ep_table = &data->smbios_ep_table.ep30;
> +    uint64_t addr = data->smbios_ep_addr[SMBIOS_VER30];
> +
> +    qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
> +    if (memcmp(ep_table->anchor_string, "_SM3_", 5)) {
> +        return false;
> +    }
> +
> +    if (acpi_calc_checksum((uint8_t *)ep_table, sizeof *ep_table)) {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static int test_smbios_entry_point(test_data *data)
>  {
>      uint32_t off;
> +    bool found_ep2 = false, found_ep3 = false;
>  
>      /* find smbios entry point structure */
>      for (off = 0xf0000; off < 0x100000; off += 0x10) {
> -        uint8_t sig[] = "_SM_";
> +        uint8_t sig[] = "_SM3_";

well I'd just add a separate sig3

>          int i;
>  
>          for (i = 0; i < sizeof sig - 1; ++i) {
>              sig[i] = qtest_readb(data->qts, off + i);
>          }
>  
> -        if (!memcmp(sig, "_SM_", sizeof sig)) {
> +        if (!found_ep2 && !memcmp(sig, "_SM_", sizeof sig - 2)) {

keep original v2 code and just add similar chunk for v3,
drop found_foo locals,
that should make it easier to read/follow
(i.e. less conditions to think about and no magic fiddling with the length of signature)

>              /* signature match, but is this a valid entry point? */
> -            data->smbios_ep_addr = off;
> -            if (smbios_ep_table_ok(data)) {
> -                break;
> +            data->smbios_ep_addr[SMBIOS_VER21] = off;
> +            if (smbios_ep2_table_ok(data)) {
> +                found_ep2 = true;
> +            }
> +        } else if (!found_ep3 && !memcmp(sig, "_SM3_", sizeof sig - 1)) {
> +            data->smbios_ep_addr[SMBIOS_VER30] = off;
> +            if (smbios_ep3_table_ok(data)) {
> +                found_ep3 = true;
>              }
>          }
> +
> +        if (found_ep2 || found_ep3) {
> +            break;
> +        }
>      }
>  
> -    g_assert_cmphex(off, <, 0x100000);
> +    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER21], <, 0x100000);
> +    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER30], <, 0x100000);
> +
> +    return found_ep3 ? SMBIOS_VER30 : SMBIOS_VER21;

and use content of data->smbios_ep_addr[] to return found version

>  }
>  
>  static inline bool smbios_single_instance(uint8_t type)
> @@ -625,16 +663,23 @@ static bool smbios_cpu_test(test_data *data, uint32_t addr)
>      return true;
>  }
>  
> -static void test_smbios_structs(test_data *data)
> +static void test_smbios_structs(test_data *data, int ver)
>  {
>      DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 };
> -    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> -    uint32_t addr = le32_to_cpu(ep_table->structure_table_address);
> -    int i, len, max_len = 0;
> +
> +    smbios_entry_point *ep_table = &data->smbios_ep_table;
> +    int i = 0, len, max_len = 0;
>      uint8_t type, prv, crt;
> +    uint64_t addr;
> +
> +    if (ver == SMBIOS_VER21) {
> +        addr = le32_to_cpu(ep_table->ep21.structure_table_address);
> +    } else {
> +        addr = le64_to_cpu(ep_table->ep30.structure_table_address);
> +    }
>  
>      /* walk the smbios tables */
> -    for (i = 0; i < le16_to_cpu(ep_table->number_of_structures); i++) {
> +    do {
>  
>          /* grab type and formatted area length from struct header */
>          type = qtest_readb(data->qts, addr);
> @@ -660,19 +705,23 @@ static void test_smbios_structs(test_data *data)
>          }
>  
>          /* keep track of max. struct size */
> -        if (max_len < len) {
> +        if (ver == SMBIOS_VER21 && max_len < len) {
>              max_len = len;
> -            g_assert_cmpuint(max_len, <=, ep_table->max_structure_size);
> +            g_assert_cmpuint(max_len, <=, ep_table->ep21.max_structure_size);
>          }
>  
>          /* start of next structure */
>          addr += len;
> -    }
>  
> -    /* total table length and max struct size must match entry point values */
> -    g_assert_cmpuint(le16_to_cpu(ep_table->structure_table_length), ==,
> -                     addr - le32_to_cpu(ep_table->structure_table_address));
> -    g_assert_cmpuint(le16_to_cpu(ep_table->max_structure_size), ==, max_len);
> +    } while (ver == SMBIOS_VER21 ?
> +                (++i < le16_to_cpu(ep_table->ep21.number_of_structures)) : (type != 127));
> +
> +    if (ver == SMBIOS_VER21) {
> +        /* total table length and max struct size must match entry point values */
> +        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.structure_table_length), ==,
> +                         addr - le32_to_cpu(ep_table->ep21.structure_table_address));
> +        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.max_structure_size), ==, max_len);
> +    }
>  
>      /* required struct types must all be present */
>      for (i = 0; i < data->required_struct_types_len; i++) {
> @@ -756,8 +805,8 @@ static void test_acpi_one(const char *params, test_data *data)
>       * https://bugs.launchpad.net/qemu/+bug/1821884
>       */
>      if (!use_uefi) {
> -        test_smbios_entry_point(data);
> -        test_smbios_structs(data);
> +        int ver = test_smbios_entry_point(data);
> +        test_smbios_structs(data, ver);
>      }
>  
>      qtest_quit(data->qts);
Julia Suvorova June 6, 2022, 10:52 a.m. UTC | #4
On Thu, Jun 2, 2022 at 5:04 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Fri, 27 May 2022 18:56:48 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
>
> > Introduce the 64-bit entry point. Since we no longer have a total
> > number of structures, stop checking for the new ones at the EOF
> > structure (type 127).
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> >  tests/qtest/bios-tables-test.c | 101 ++++++++++++++++++++++++---------
> >  1 file changed, 75 insertions(+), 26 deletions(-)
> >
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index a4a46e97f0..0ba9d749a5 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -75,6 +75,14 @@
> >  #define OEM_TEST_ARGS      "-machine x-oem-id=" OEM_ID ",x-oem-table-id=" \
> >                             OEM_TABLE_ID
> >
> > +#define SMBIOS_VER21 0
> > +#define SMBIOS_VER30 1
> > +
> > +typedef struct {
> > +    struct smbios_21_entry_point ep21;
> > +    struct smbios_30_entry_point ep30;
> > +} smbios_entry_point;
> > +
> >  typedef struct {
> >      bool tcg_only;
> >      const char *machine;
> > @@ -88,8 +96,8 @@ typedef struct {
> >      uint64_t rsdp_addr;
> >      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
> >      GArray *tables;
> > -    uint32_t smbios_ep_addr;
> > -    struct smbios_21_entry_point smbios_ep_table;
> > +    uint64_t smbios_ep_addr[2];
> > +    smbios_entry_point smbios_ep_table;
> >      uint16_t smbios_cpu_max_speed;
> >      uint16_t smbios_cpu_curr_speed;
> >      uint8_t *required_struct_types;
> > @@ -533,10 +541,10 @@ static void test_acpi_asl(test_data *data)
> >      free_test_data(&exp_data);
> >  }
> >
> > -static bool smbios_ep_table_ok(test_data *data)
> > +static bool smbios_ep2_table_ok(test_data *data)
> >  {
> > -    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> > -    uint32_t addr = data->smbios_ep_addr;
> > +    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table.ep21;
> > +    uint32_t addr = data->smbios_ep_addr[SMBIOS_VER21];
> >
> >      qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
> >      if (memcmp(ep_table->anchor_string, "_SM_", 4)) {
> > @@ -559,29 +567,59 @@ static bool smbios_ep_table_ok(test_data *data)
> >      return true;
> >  }
> >
> > -static void test_smbios_entry_point(test_data *data)
> > +static bool smbios_ep3_table_ok(test_data *data)
> > +{
> > +    struct smbios_30_entry_point *ep_table = &data->smbios_ep_table.ep30;
> > +    uint64_t addr = data->smbios_ep_addr[SMBIOS_VER30];
> > +
> > +    qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
> > +    if (memcmp(ep_table->anchor_string, "_SM3_", 5)) {
> > +        return false;
> > +    }
> > +
> > +    if (acpi_calc_checksum((uint8_t *)ep_table, sizeof *ep_table)) {
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +static int test_smbios_entry_point(test_data *data)
> >  {
> >      uint32_t off;
> > +    bool found_ep2 = false, found_ep3 = false;
> >
> >      /* find smbios entry point structure */
> >      for (off = 0xf0000; off < 0x100000; off += 0x10) {
> > -        uint8_t sig[] = "_SM_";
> > +        uint8_t sig[] = "_SM3_";
>
> well I'd just add a separate sig3

Ok

> >          int i;
> >
> >          for (i = 0; i < sizeof sig - 1; ++i) {
> >              sig[i] = qtest_readb(data->qts, off + i);
> >          }
> >
> > -        if (!memcmp(sig, "_SM_", sizeof sig)) {
> > +        if (!found_ep2 && !memcmp(sig, "_SM_", sizeof sig - 2)) {
>
> keep original v2 code and just add similar chunk for v3,
> drop found_foo locals,
> that should make it easier to read/follow
> (i.e. less conditions to think about and no magic fiddling with the length of signature)

The idea was to reuse existing code, but since it doesn't improve
things much, it makes sense to repeat it.

> >              /* signature match, but is this a valid entry point? */
> > -            data->smbios_ep_addr = off;
> > -            if (smbios_ep_table_ok(data)) {
> > -                break;
> > +            data->smbios_ep_addr[SMBIOS_VER21] = off;
> > +            if (smbios_ep2_table_ok(data)) {
> > +                found_ep2 = true;
> > +            }
> > +        } else if (!found_ep3 && !memcmp(sig, "_SM3_", sizeof sig - 1)) {
> > +            data->smbios_ep_addr[SMBIOS_VER30] = off;
> > +            if (smbios_ep3_table_ok(data)) {
> > +                found_ep3 = true;
> >              }
> >          }
> > +
> > +        if (found_ep2 || found_ep3) {
> > +            break;
> > +        }
> >      }
> >
> > -    g_assert_cmphex(off, <, 0x100000);
> > +    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER21], <, 0x100000);
> > +    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER30], <, 0x100000);
> > +
> > +    return found_ep3 ? SMBIOS_VER30 : SMBIOS_VER21;
>
> and use content of data->smbios_ep_addr[] to return found version

You mean check if it's initialized?

> >  }
> >
> >  static inline bool smbios_single_instance(uint8_t type)
> > @@ -625,16 +663,23 @@ static bool smbios_cpu_test(test_data *data, uint32_t addr)
> >      return true;
> >  }
> >
> > -static void test_smbios_structs(test_data *data)
> > +static void test_smbios_structs(test_data *data, int ver)
> >  {
> >      DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 };
> > -    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> > -    uint32_t addr = le32_to_cpu(ep_table->structure_table_address);
> > -    int i, len, max_len = 0;
> > +
> > +    smbios_entry_point *ep_table = &data->smbios_ep_table;
> > +    int i = 0, len, max_len = 0;
> >      uint8_t type, prv, crt;
> > +    uint64_t addr;
> > +
> > +    if (ver == SMBIOS_VER21) {
> > +        addr = le32_to_cpu(ep_table->ep21.structure_table_address);
> > +    } else {
> > +        addr = le64_to_cpu(ep_table->ep30.structure_table_address);
> > +    }
> >
> >      /* walk the smbios tables */
> > -    for (i = 0; i < le16_to_cpu(ep_table->number_of_structures); i++) {
> > +    do {
> >
> >          /* grab type and formatted area length from struct header */
> >          type = qtest_readb(data->qts, addr);
> > @@ -660,19 +705,23 @@ static void test_smbios_structs(test_data *data)
> >          }
> >
> >          /* keep track of max. struct size */
> > -        if (max_len < len) {
> > +        if (ver == SMBIOS_VER21 && max_len < len) {
> >              max_len = len;
> > -            g_assert_cmpuint(max_len, <=, ep_table->max_structure_size);
> > +            g_assert_cmpuint(max_len, <=, ep_table->ep21.max_structure_size);
> >          }
> >
> >          /* start of next structure */
> >          addr += len;
> > -    }
> >
> > -    /* total table length and max struct size must match entry point values */
> > -    g_assert_cmpuint(le16_to_cpu(ep_table->structure_table_length), ==,
> > -                     addr - le32_to_cpu(ep_table->structure_table_address));
> > -    g_assert_cmpuint(le16_to_cpu(ep_table->max_structure_size), ==, max_len);
> > +    } while (ver == SMBIOS_VER21 ?
> > +                (++i < le16_to_cpu(ep_table->ep21.number_of_structures)) : (type != 127));
> > +
> > +    if (ver == SMBIOS_VER21) {
> > +        /* total table length and max struct size must match entry point values */
> > +        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.structure_table_length), ==,
> > +                         addr - le32_to_cpu(ep_table->ep21.structure_table_address));
> > +        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.max_structure_size), ==, max_len);
> > +    }
> >
> >      /* required struct types must all be present */
> >      for (i = 0; i < data->required_struct_types_len; i++) {
> > @@ -756,8 +805,8 @@ static void test_acpi_one(const char *params, test_data *data)
> >       * https://bugs.launchpad.net/qemu/+bug/1821884
> >       */
> >      if (!use_uefi) {
> > -        test_smbios_entry_point(data);
> > -        test_smbios_structs(data);
> > +        int ver = test_smbios_entry_point(data);
> > +        test_smbios_structs(data, ver);
> >      }
> >
> >      qtest_quit(data->qts);
>
Igor Mammedov June 7, 2022, 9:55 a.m. UTC | #5
On Mon, 6 Jun 2022 12:52:00 +0200
Julia Suvorova <jusual@redhat.com> wrote:

> On Thu, Jun 2, 2022 at 5:04 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Fri, 27 May 2022 18:56:48 +0200
> > Julia Suvorova <jusual@redhat.com> wrote:
> >  
> > > Introduce the 64-bit entry point. Since we no longer have a total
> > > number of structures, stop checking for the new ones at the EOF
> > > structure (type 127).
> > >
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > ---
> > >  tests/qtest/bios-tables-test.c | 101 ++++++++++++++++++++++++---------
> > >  1 file changed, 75 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > > index a4a46e97f0..0ba9d749a5 100644
> > > --- a/tests/qtest/bios-tables-test.c
> > > +++ b/tests/qtest/bios-tables-test.c
> > > @@ -75,6 +75,14 @@
> > >  #define OEM_TEST_ARGS      "-machine x-oem-id=" OEM_ID ",x-oem-table-id=" \
> > >                             OEM_TABLE_ID
> > >
> > > +#define SMBIOS_VER21 0
> > > +#define SMBIOS_VER30 1
> > > +
> > > +typedef struct {
> > > +    struct smbios_21_entry_point ep21;
> > > +    struct smbios_30_entry_point ep30;
> > > +} smbios_entry_point;
> > > +
> > >  typedef struct {
> > >      bool tcg_only;
> > >      const char *machine;
> > > @@ -88,8 +96,8 @@ typedef struct {
> > >      uint64_t rsdp_addr;
> > >      uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
> > >      GArray *tables;
> > > -    uint32_t smbios_ep_addr;
> > > -    struct smbios_21_entry_point smbios_ep_table;
> > > +    uint64_t smbios_ep_addr[2];
> > > +    smbios_entry_point smbios_ep_table;
> > >      uint16_t smbios_cpu_max_speed;
> > >      uint16_t smbios_cpu_curr_speed;
> > >      uint8_t *required_struct_types;
> > > @@ -533,10 +541,10 @@ static void test_acpi_asl(test_data *data)
> > >      free_test_data(&exp_data);
> > >  }
> > >
> > > -static bool smbios_ep_table_ok(test_data *data)
> > > +static bool smbios_ep2_table_ok(test_data *data)
> > >  {
> > > -    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> > > -    uint32_t addr = data->smbios_ep_addr;
> > > +    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table.ep21;
> > > +    uint32_t addr = data->smbios_ep_addr[SMBIOS_VER21];
> > >
> > >      qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
> > >      if (memcmp(ep_table->anchor_string, "_SM_", 4)) {
> > > @@ -559,29 +567,59 @@ static bool smbios_ep_table_ok(test_data *data)
> > >      return true;
> > >  }
> > >
> > > -static void test_smbios_entry_point(test_data *data)
> > > +static bool smbios_ep3_table_ok(test_data *data)
> > > +{
> > > +    struct smbios_30_entry_point *ep_table = &data->smbios_ep_table.ep30;
> > > +    uint64_t addr = data->smbios_ep_addr[SMBIOS_VER30];
> > > +
> > > +    qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
> > > +    if (memcmp(ep_table->anchor_string, "_SM3_", 5)) {
> > > +        return false;
> > > +    }
> > > +
> > > +    if (acpi_calc_checksum((uint8_t *)ep_table, sizeof *ep_table)) {
> > > +        return false;
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > +
> > > +static int test_smbios_entry_point(test_data *data)
> > >  {
> > >      uint32_t off;
> > > +    bool found_ep2 = false, found_ep3 = false;
> > >
> > >      /* find smbios entry point structure */
> > >      for (off = 0xf0000; off < 0x100000; off += 0x10) {
> > > -        uint8_t sig[] = "_SM_";
> > > +        uint8_t sig[] = "_SM3_";  
> >
> > well I'd just add a separate sig3  
> 
> Ok
> 
> > >          int i;
> > >
> > >          for (i = 0; i < sizeof sig - 1; ++i) {
> > >              sig[i] = qtest_readb(data->qts, off + i);
> > >          }
> > >
> > > -        if (!memcmp(sig, "_SM_", sizeof sig)) {
> > > +        if (!found_ep2 && !memcmp(sig, "_SM_", sizeof sig - 2)) {  
> >
> > keep original v2 code and just add similar chunk for v3,
> > drop found_foo locals,
> > that should make it easier to read/follow
> > (i.e. less conditions to think about and no magic fiddling with the length of signature)  
> 
> The idea was to reuse existing code, but since it doesn't improve
> things much, it makes sense to repeat it.
> 
> > >              /* signature match, but is this a valid entry point? */
> > > -            data->smbios_ep_addr = off;
> > > -            if (smbios_ep_table_ok(data)) {
> > > -                break;
> > > +            data->smbios_ep_addr[SMBIOS_VER21] = off;
> > > +            if (smbios_ep2_table_ok(data)) {
> > > +                found_ep2 = true;
> > > +            }
> > > +        } else if (!found_ep3 && !memcmp(sig, "_SM3_", sizeof sig - 1)) {
> > > +            data->smbios_ep_addr[SMBIOS_VER30] = off;
> > > +            if (smbios_ep3_table_ok(data)) {
> > > +                found_ep3 = true;
> > >              }
> > >          }
> > > +
> > > +        if (found_ep2 || found_ep3) {
> > > +            break;
> > > +        }
> > >      }
> > >
> > > -    g_assert_cmphex(off, <, 0x100000);
> > > +    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER21], <, 0x100000);
> > > +    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER30], <, 0x100000);
> > > +
> > > +    return found_ep3 ? SMBIOS_VER30 : SMBIOS_VER21;  
> >
> > and use content of data->smbios_ep_addr[] to return found version  
> 
> You mean check if it's initialized?

yep, it's zeroed out initially and you can check if it's set to something else
after detection phase


> 
> > >  }
> > >
> > >  static inline bool smbios_single_instance(uint8_t type)
> > > @@ -625,16 +663,23 @@ static bool smbios_cpu_test(test_data *data, uint32_t addr)
> > >      return true;
> > >  }
> > >
> > > -static void test_smbios_structs(test_data *data)
> > > +static void test_smbios_structs(test_data *data, int ver)
> > >  {
> > >      DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 };
> > > -    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> > > -    uint32_t addr = le32_to_cpu(ep_table->structure_table_address);
> > > -    int i, len, max_len = 0;
> > > +
> > > +    smbios_entry_point *ep_table = &data->smbios_ep_table;
> > > +    int i = 0, len, max_len = 0;
> > >      uint8_t type, prv, crt;
> > > +    uint64_t addr;
> > > +
> > > +    if (ver == SMBIOS_VER21) {
> > > +        addr = le32_to_cpu(ep_table->ep21.structure_table_address);
> > > +    } else {
> > > +        addr = le64_to_cpu(ep_table->ep30.structure_table_address);
> > > +    }
> > >
> > >      /* walk the smbios tables */
> > > -    for (i = 0; i < le16_to_cpu(ep_table->number_of_structures); i++) {
> > > +    do {
> > >
> > >          /* grab type and formatted area length from struct header */
> > >          type = qtest_readb(data->qts, addr);
> > > @@ -660,19 +705,23 @@ static void test_smbios_structs(test_data *data)
> > >          }
> > >
> > >          /* keep track of max. struct size */
> > > -        if (max_len < len) {
> > > +        if (ver == SMBIOS_VER21 && max_len < len) {
> > >              max_len = len;
> > > -            g_assert_cmpuint(max_len, <=, ep_table->max_structure_size);
> > > +            g_assert_cmpuint(max_len, <=, ep_table->ep21.max_structure_size);
> > >          }
> > >
> > >          /* start of next structure */
> > >          addr += len;
> > > -    }
> > >
> > > -    /* total table length and max struct size must match entry point values */
> > > -    g_assert_cmpuint(le16_to_cpu(ep_table->structure_table_length), ==,
> > > -                     addr - le32_to_cpu(ep_table->structure_table_address));
> > > -    g_assert_cmpuint(le16_to_cpu(ep_table->max_structure_size), ==, max_len);
> > > +    } while (ver == SMBIOS_VER21 ?
> > > +                (++i < le16_to_cpu(ep_table->ep21.number_of_structures)) : (type != 127));
> > > +
> > > +    if (ver == SMBIOS_VER21) {
> > > +        /* total table length and max struct size must match entry point values */
> > > +        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.structure_table_length), ==,
> > > +                         addr - le32_to_cpu(ep_table->ep21.structure_table_address));
> > > +        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.max_structure_size), ==, max_len);
> > > +    }
> > >
> > >      /* required struct types must all be present */
> > >      for (i = 0; i < data->required_struct_types_len; i++) {
> > > @@ -756,8 +805,8 @@ static void test_acpi_one(const char *params, test_data *data)
> > >       * https://bugs.launchpad.net/qemu/+bug/1821884
> > >       */
> > >      if (!use_uefi) {
> > > -        test_smbios_entry_point(data);
> > > -        test_smbios_structs(data);
> > > +        int ver = test_smbios_entry_point(data);
> > > +        test_smbios_structs(data, ver);
> > >      }
> > >
> > >      qtest_quit(data->qts);  
> >  
>
diff mbox series

Patch

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index a4a46e97f0..0ba9d749a5 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -75,6 +75,14 @@ 
 #define OEM_TEST_ARGS      "-machine x-oem-id=" OEM_ID ",x-oem-table-id=" \
                            OEM_TABLE_ID
 
+#define SMBIOS_VER21 0
+#define SMBIOS_VER30 1
+
+typedef struct {
+    struct smbios_21_entry_point ep21;
+    struct smbios_30_entry_point ep30;
+} smbios_entry_point;
+
 typedef struct {
     bool tcg_only;
     const char *machine;
@@ -88,8 +96,8 @@  typedef struct {
     uint64_t rsdp_addr;
     uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
     GArray *tables;
-    uint32_t smbios_ep_addr;
-    struct smbios_21_entry_point smbios_ep_table;
+    uint64_t smbios_ep_addr[2];
+    smbios_entry_point smbios_ep_table;
     uint16_t smbios_cpu_max_speed;
     uint16_t smbios_cpu_curr_speed;
     uint8_t *required_struct_types;
@@ -533,10 +541,10 @@  static void test_acpi_asl(test_data *data)
     free_test_data(&exp_data);
 }
 
-static bool smbios_ep_table_ok(test_data *data)
+static bool smbios_ep2_table_ok(test_data *data)
 {
-    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
-    uint32_t addr = data->smbios_ep_addr;
+    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table.ep21;
+    uint32_t addr = data->smbios_ep_addr[SMBIOS_VER21];
 
     qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
     if (memcmp(ep_table->anchor_string, "_SM_", 4)) {
@@ -559,29 +567,59 @@  static bool smbios_ep_table_ok(test_data *data)
     return true;
 }
 
-static void test_smbios_entry_point(test_data *data)
+static bool smbios_ep3_table_ok(test_data *data)
+{
+    struct smbios_30_entry_point *ep_table = &data->smbios_ep_table.ep30;
+    uint64_t addr = data->smbios_ep_addr[SMBIOS_VER30];
+
+    qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
+    if (memcmp(ep_table->anchor_string, "_SM3_", 5)) {
+        return false;
+    }
+
+    if (acpi_calc_checksum((uint8_t *)ep_table, sizeof *ep_table)) {
+        return false;
+    }
+
+    return true;
+}
+
+static int test_smbios_entry_point(test_data *data)
 {
     uint32_t off;
+    bool found_ep2 = false, found_ep3 = false;
 
     /* find smbios entry point structure */
     for (off = 0xf0000; off < 0x100000; off += 0x10) {
-        uint8_t sig[] = "_SM_";
+        uint8_t sig[] = "_SM3_";
         int i;
 
         for (i = 0; i < sizeof sig - 1; ++i) {
             sig[i] = qtest_readb(data->qts, off + i);
         }
 
-        if (!memcmp(sig, "_SM_", sizeof sig)) {
+        if (!found_ep2 && !memcmp(sig, "_SM_", sizeof sig - 2)) {
             /* signature match, but is this a valid entry point? */
-            data->smbios_ep_addr = off;
-            if (smbios_ep_table_ok(data)) {
-                break;
+            data->smbios_ep_addr[SMBIOS_VER21] = off;
+            if (smbios_ep2_table_ok(data)) {
+                found_ep2 = true;
+            }
+        } else if (!found_ep3 && !memcmp(sig, "_SM3_", sizeof sig - 1)) {
+            data->smbios_ep_addr[SMBIOS_VER30] = off;
+            if (smbios_ep3_table_ok(data)) {
+                found_ep3 = true;
             }
         }
+
+        if (found_ep2 || found_ep3) {
+            break;
+        }
     }
 
-    g_assert_cmphex(off, <, 0x100000);
+    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER21], <, 0x100000);
+    g_assert_cmphex(data->smbios_ep_addr[SMBIOS_VER30], <, 0x100000);
+
+    return found_ep3 ? SMBIOS_VER30 : SMBIOS_VER21;
 }
 
 static inline bool smbios_single_instance(uint8_t type)
@@ -625,16 +663,23 @@  static bool smbios_cpu_test(test_data *data, uint32_t addr)
     return true;
 }
 
-static void test_smbios_structs(test_data *data)
+static void test_smbios_structs(test_data *data, int ver)
 {
     DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 };
-    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
-    uint32_t addr = le32_to_cpu(ep_table->structure_table_address);
-    int i, len, max_len = 0;
+
+    smbios_entry_point *ep_table = &data->smbios_ep_table;
+    int i = 0, len, max_len = 0;
     uint8_t type, prv, crt;
+    uint64_t addr;
+
+    if (ver == SMBIOS_VER21) {
+        addr = le32_to_cpu(ep_table->ep21.structure_table_address);
+    } else {
+        addr = le64_to_cpu(ep_table->ep30.structure_table_address);
+    }
 
     /* walk the smbios tables */
-    for (i = 0; i < le16_to_cpu(ep_table->number_of_structures); i++) {
+    do {
 
         /* grab type and formatted area length from struct header */
         type = qtest_readb(data->qts, addr);
@@ -660,19 +705,23 @@  static void test_smbios_structs(test_data *data)
         }
 
         /* keep track of max. struct size */
-        if (max_len < len) {
+        if (ver == SMBIOS_VER21 && max_len < len) {
             max_len = len;
-            g_assert_cmpuint(max_len, <=, ep_table->max_structure_size);
+            g_assert_cmpuint(max_len, <=, ep_table->ep21.max_structure_size);
         }
 
         /* start of next structure */
         addr += len;
-    }
 
-    /* total table length and max struct size must match entry point values */
-    g_assert_cmpuint(le16_to_cpu(ep_table->structure_table_length), ==,
-                     addr - le32_to_cpu(ep_table->structure_table_address));
-    g_assert_cmpuint(le16_to_cpu(ep_table->max_structure_size), ==, max_len);
+    } while (ver == SMBIOS_VER21 ?
+                (++i < le16_to_cpu(ep_table->ep21.number_of_structures)) : (type != 127));
+
+    if (ver == SMBIOS_VER21) {
+        /* total table length and max struct size must match entry point values */
+        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.structure_table_length), ==,
+                         addr - le32_to_cpu(ep_table->ep21.structure_table_address));
+        g_assert_cmpuint(le16_to_cpu(ep_table->ep21.max_structure_size), ==, max_len);
+    }
 
     /* required struct types must all be present */
     for (i = 0; i < data->required_struct_types_len; i++) {
@@ -756,8 +805,8 @@  static void test_acpi_one(const char *params, test_data *data)
      * https://bugs.launchpad.net/qemu/+bug/1821884
      */
     if (!use_uefi) {
-        test_smbios_entry_point(data);
-        test_smbios_structs(data);
+        int ver = test_smbios_entry_point(data);
+        test_smbios_structs(data, ver);
     }
 
     qtest_quit(data->qts);