On Mon, 6 Jun 2022 12:52:00 +0200
Julia Suvorova <jus...@redhat.com> wrote:
> On Thu, Jun 2, 2022 at 5:04 PM Igor Mammedov <imamm...@redhat.com> wrote:
> >
> > On Fri, 27 May 2022 18:56:48 +0200
> > Julia Suvorova <jus...@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 <jus...@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);
> >
>
