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

[Ani Sinha]

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
>