[v5,09/10] ACPI ERST: qtest for ERST

Commit Message

Eric DeVolder June 30, 2021, 7:07 p.m. UTC

This change provides a qtest that locates and then does a simple
interrogation of the ERST feature within the guest.

Signed-off-by: Eric DeVolder <[e-mail]>
---
 tests/qtest/erst-test.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/qtest/meson.build |   2 +
 2 files changed, 131 insertions(+)
 create mode 100644 tests/qtest/erst-test.c

Comments

Igor Mammedov July 20, 2021, 1:38 p.m. UTC | #1

On Wed, 30 Jun 2021 15:07:20 -0400
Eric DeVolder <[e-mail]> wrote:

> This change provides a qtest that locates and then does a simple
> interrogation of the ERST feature within the guest.
> 
> Signed-off-by: Eric DeVolder <[e-mail]>
> ---
>  tests/qtest/erst-test.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qtest/meson.build |   2 +
>  2 files changed, 131 insertions(+)
>  create mode 100644 tests/qtest/erst-test.c
> 
> diff --git a/tests/qtest/erst-test.c b/tests/qtest/erst-test.c
> new file mode 100644
> index 0000000..ce014c1
> --- /dev/null
> +++ b/tests/qtest/erst-test.c
> @@ -0,0 +1,129 @@
> +/*
> + * QTest testcase for ACPI ERST
> + *
> + * Copyright (c) 2021 Oracle
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/bitmap.h"
> +#include "qemu/uuid.h"
> +#include "hw/acpi/acpi-defs.h"
> +#include "boot-sector.h"
> +#include "acpi-utils.h"
> +#include "libqos/libqtest.h"
> +#include "qapi/qmp/qdict.h"
> +
> +#define RSDP_ADDR_INVALID 0x100000 /* RSDP must be below this address */
> +
> +static uint64_t acpi_find_erst(QTestState *qts)
> +{
> +    uint32_t rsdp_offset;
> +    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
> +    uint32_t rsdt_len, table_length;
> +    uint8_t *rsdt, *ent;
> +    uint64_t base = 0;
> +
> +    /* Wait for guest firmware to finish and start the payload. */
> +    boot_sector_test(qts);
> +
> +    /* Tables should be initialized now. */
> +    rsdp_offset = acpi_find_rsdp_address(qts);
> +
> +    g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
> +
> +    acpi_fetch_rsdp_table(qts, rsdp_offset, rsdp_table);
> +    acpi_fetch_table(qts, &rsdt, &rsdt_len, &rsdp_table[16 /* RsdtAddress */],
> +                     4, "RSDT", true);
> +
> +    ACPI_FOREACH_RSDT_ENTRY(rsdt, rsdt_len, ent, 4 /* Entry size */) {
> +        uint8_t *table_aml;
> +        acpi_fetch_table(qts, &table_aml, &table_length, ent, 4, NULL, true);
> +        if (!memcmp(table_aml + 0 /* Header Signature */, "ERST", 4)) {
> +            /*
> +             * Picking up ERST base address from the Register Region
> +             * specified as part of the first Serialization Instruction
> +             * Action (which is a Begin Write Operation).
> +             */
> +            memcpy(&base, &table_aml[56], sizeof(base));
> +            g_free(table_aml);
> +            break;
> +        }
> +        g_free(table_aml);
> +    }
> +    g_free(rsdt);
> +    return base;
> +}
I'd drop this, bios-tables-test should do ACPI table check
as for PCI device itself you can test it with qtest accelerator
that allows to instantiate it and access registers directly
without overhead of running actual guest.

As example you can look into megasas-test.c, ivshmem-test.c
or other PCI device tests.

> +static char disk[] = "tests/erst-test-disk-XXXXXX";
> +
> +#define ERST_CMD()                              \
> +    "-accel kvm -accel tcg "                    \
> +    "-object memory-backend-file," \
> +      "id=erstnvram,mem-path=tests/acpi-erst-XXXXXX,size=0x10000,share=on " \
> +    "-device acpi-erst,memdev=erstnvram " \
> +    "-drive id=hd0,if=none,file=%s,format=raw " \
> +    "-device ide-hd,drive=hd0 ", disk
> +
> +static void erst_get_error_log_address_range(void)
> +{
> +    QTestState *qts;
> +    uint64_t log_address_range = 0;
> +    unsigned log_address_length = 0;
> +    unsigned log_address_attr = 0;
> +
> +    qts = qtest_initf(ERST_CMD());
> +
> +    uint64_t base = acpi_find_erst(qts);
> +    g_assert(base != 0);
> +
> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE command */
> +    qtest_writel(qts, base + 0, 0xD);
> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE result */
> +    log_address_range = qtest_readq(qts, base + 8);\
> +
> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE_LENGTH command */
> +    qtest_writel(qts, base + 0, 0xE);
> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE_LENGTH result */
> +    log_address_length = qtest_readq(qts, base + 8);\
> +
> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES command */
> +    qtest_writel(qts, base + 0, 0xF);
> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES result */
> +    log_address_attr = qtest_readq(qts, base + 8);\
> +
> +    /* Check log_address_range is not 0,~0 or base */
> +    g_assert(log_address_range != base);
> +    g_assert(log_address_range != 0);
> +    g_assert(log_address_range != ~0UL);
> +
> +    /* Check log_address_length is ERST_RECORD_SIZE */
> +    g_assert(log_address_length == (8 * 1024));
> +
> +    /* Check log_address_attr is 0 */
> +    g_assert(log_address_attr == 0);
> +
> +    qtest_quit(qts);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    int ret;
> +
> +    ret = boot_sector_init(disk);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    qtest_add_func("/erst/get-error-log-address-range",
> +                   erst_get_error_log_address_range);
> +
> +    ret = g_test_run();
> +    boot_sector_cleanup(disk);
> +
> +    return ret;
> +}
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 0c76738..deae443 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -66,6 +66,7 @@ qtests_i386 = \
>    (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) +              \
>    (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) +   \
>    (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +                 \
> +  (config_all_devices.has_key('CONFIG_ACPI') ? ['erst-test'] : []) +                 \
>    qtests_pci +                                                                              \
>    ['fdc-test',
>     'ide-test',
> @@ -237,6 +238,7 @@ qtests = {
>    'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
>    'cdrom-test': files('boot-sector.c'),
>    'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
> +  'erst-test': files('erst-test.c', 'boot-sector.c', 'acpi-utils.c'),
>    'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
>    'migration-test': files('migration-helpers.c'),
>    'pxe-test': files('boot-sector.c'),

Eric DeVolder July 21, 2021, 4:18 p.m. UTC | #2

On 7/20/21 8:38 AM, Igor Mammedov wrote:
> On Wed, 30 Jun 2021 15:07:20 -0400
> Eric DeVolder <[e-mail]> wrote:
> 
>> This change provides a qtest that locates and then does a simple
>> interrogation of the ERST feature within the guest.
>>
>> Signed-off-by: Eric DeVolder <[e-mail]>
>> ---
>>   tests/qtest/erst-test.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qtest/meson.build |   2 +
>>   2 files changed, 131 insertions(+)
>>   create mode 100644 tests/qtest/erst-test.c
>>
>> diff --git a/tests/qtest/erst-test.c b/tests/qtest/erst-test.c
>> new file mode 100644
>> index 0000000..ce014c1
>> --- /dev/null
>> +++ b/tests/qtest/erst-test.c
>> @@ -0,0 +1,129 @@
>> +/*
>> + * QTest testcase for ACPI ERST
>> + *
>> + * Copyright (c) 2021 Oracle
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/bitmap.h"
>> +#include "qemu/uuid.h"
>> +#include "hw/acpi/acpi-defs.h"
>> +#include "boot-sector.h"
>> +#include "acpi-utils.h"
>> +#include "libqos/libqtest.h"
>> +#include "qapi/qmp/qdict.h"
>> +
>> +#define RSDP_ADDR_INVALID 0x100000 /* RSDP must be below this address */
>> +
>> +static uint64_t acpi_find_erst(QTestState *qts)
>> +{
>> +    uint32_t rsdp_offset;
>> +    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
>> +    uint32_t rsdt_len, table_length;
>> +    uint8_t *rsdt, *ent;
>> +    uint64_t base = 0;
>> +
>> +    /* Wait for guest firmware to finish and start the payload. */
>> +    boot_sector_test(qts);
>> +
>> +    /* Tables should be initialized now. */
>> +    rsdp_offset = acpi_find_rsdp_address(qts);
>> +
>> +    g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
>> +
>> +    acpi_fetch_rsdp_table(qts, rsdp_offset, rsdp_table);
>> +    acpi_fetch_table(qts, &rsdt, &rsdt_len, &rsdp_table[16 /* RsdtAddress */],
>> +                     4, "RSDT", true);
>> +
>> +    ACPI_FOREACH_RSDT_ENTRY(rsdt, rsdt_len, ent, 4 /* Entry size */) {
>> +        uint8_t *table_aml;
>> +        acpi_fetch_table(qts, &table_aml, &table_length, ent, 4, NULL, true);
>> +        if (!memcmp(table_aml + 0 /* Header Signature */, "ERST", 4)) {
>> +            /*
>> +             * Picking up ERST base address from the Register Region
>> +             * specified as part of the first Serialization Instruction
>> +             * Action (which is a Begin Write Operation).
>> +             */
>> +            memcpy(&base, &table_aml[56], sizeof(base));
>> +            g_free(table_aml);
>> +            break;
>> +        }
>> +        g_free(table_aml);
>> +    }
>> +    g_free(rsdt);
>> +    return base;
>> +}
> I'd drop this, bios-tables-test should do ACPI table check
> as for PCI device itself you can test it with qtest accelerator
> that allows to instantiate it and access registers directly
> without overhead of running actual guest.
Yes, bios-tables-test checks the ACPI table, but not the functionality.
This test has actually caught a problem/bug during development.

> 
> As example you can look into megasas-test.c, ivshmem-test.c
> or other PCI device tests.
But I'll look at these and see about migrating to this approach.

> 
>> +static char disk[] = "tests/erst-test-disk-XXXXXX";
>> +
>> +#define ERST_CMD()                              \
>> +    "-accel kvm -accel tcg "                    \
>> +    "-object memory-backend-file," \
>> +      "id=erstnvram,mem-path=tests/acpi-erst-XXXXXX,size=0x10000,share=on " \
>> +    "-device acpi-erst,memdev=erstnvram " \
>> +    "-drive id=hd0,if=none,file=%s,format=raw " \
>> +    "-device ide-hd,drive=hd0 ", disk
>> +
>> +static void erst_get_error_log_address_range(void)
>> +{
>> +    QTestState *qts;
>> +    uint64_t log_address_range = 0;
>> +    unsigned log_address_length = 0;
>> +    unsigned log_address_attr = 0;
>> +
>> +    qts = qtest_initf(ERST_CMD());
>> +
>> +    uint64_t base = acpi_find_erst(qts);
>> +    g_assert(base != 0);
>> +
>> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE command */
>> +    qtest_writel(qts, base + 0, 0xD);
>> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE result */
>> +    log_address_range = qtest_readq(qts, base + 8);\
>> +
>> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE_LENGTH command */
>> +    qtest_writel(qts, base + 0, 0xE);
>> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE_LENGTH result */
>> +    log_address_length = qtest_readq(qts, base + 8);\
>> +
>> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES command */
>> +    qtest_writel(qts, base + 0, 0xF);
>> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES result */
>> +    log_address_attr = qtest_readq(qts, base + 8);\
>> +
>> +    /* Check log_address_range is not 0,~0 or base */
>> +    g_assert(log_address_range != base);
>> +    g_assert(log_address_range != 0);
>> +    g_assert(log_address_range != ~0UL);
>> +
>> +    /* Check log_address_length is ERST_RECORD_SIZE */
>> +    g_assert(log_address_length == (8 * 1024));
>> +
>> +    /* Check log_address_attr is 0 */
>> +    g_assert(log_address_attr == 0);
>> +
>> +    qtest_quit(qts);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    int ret;
>> +
>> +    ret = boot_sector_init(disk);
>> +    if (ret) {
>> +        return ret;
>> +    }
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    qtest_add_func("/erst/get-error-log-address-range",
>> +                   erst_get_error_log_address_range);
>> +
>> +    ret = g_test_run();
>> +    boot_sector_cleanup(disk);
>> +
>> +    return ret;
>> +}
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index 0c76738..deae443 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -66,6 +66,7 @@ qtests_i386 = \
>>     (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) +              \
>>     (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) +   \
>>     (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +                 \
>> +  (config_all_devices.has_key('CONFIG_ACPI') ? ['erst-test'] : []) +                 \
>>     qtests_pci +                                                                              \
>>     ['fdc-test',
>>      'ide-test',
>> @@ -237,6 +238,7 @@ qtests = {
>>     'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
>>     'cdrom-test': files('boot-sector.c'),
>>     'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
>> +  'erst-test': files('erst-test.c', 'boot-sector.c', 'acpi-utils.c'),
>>     'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
>>     'migration-test': files('migration-helpers.c'),
>>     'pxe-test': files('boot-sector.c'),
>

Igor Mammedov July 26, 2021, 11:45 a.m. UTC | #3

On Wed, 21 Jul 2021 11:18:44 -0500
Eric DeVolder <[e-mail]> wrote:

> On 7/20/21 8:38 AM, Igor Mammedov wrote:
> > On Wed, 30 Jun 2021 15:07:20 -0400
> > Eric DeVolder <[e-mail]> wrote:
> >   
> >> This change provides a qtest that locates and then does a simple
> >> interrogation of the ERST feature within the guest.
> >>
> >> Signed-off-by: Eric DeVolder <[e-mail]>
> >> ---
> >>   tests/qtest/erst-test.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>   tests/qtest/meson.build |   2 +
> >>   2 files changed, 131 insertions(+)
> >>   create mode 100644 tests/qtest/erst-test.c
> >>
> >> diff --git a/tests/qtest/erst-test.c b/tests/qtest/erst-test.c
> >> new file mode 100644
> >> index 0000000..ce014c1
> >> --- /dev/null
> >> +++ b/tests/qtest/erst-test.c
> >> @@ -0,0 +1,129 @@
> >> +/*
> >> + * QTest testcase for ACPI ERST
> >> + *
> >> + * Copyright (c) 2021 Oracle
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "qemu/bitmap.h"
> >> +#include "qemu/uuid.h"
> >> +#include "hw/acpi/acpi-defs.h"
> >> +#include "boot-sector.h"
> >> +#include "acpi-utils.h"
> >> +#include "libqos/libqtest.h"
> >> +#include "qapi/qmp/qdict.h"
> >> +
> >> +#define RSDP_ADDR_INVALID 0x100000 /* RSDP must be below this address */
> >> +
> >> +static uint64_t acpi_find_erst(QTestState *qts)
> >> +{
> >> +    uint32_t rsdp_offset;
> >> +    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
> >> +    uint32_t rsdt_len, table_length;
> >> +    uint8_t *rsdt, *ent;
> >> +    uint64_t base = 0;
> >> +
> >> +    /* Wait for guest firmware to finish and start the payload. */
> >> +    boot_sector_test(qts);
> >> +
> >> +    /* Tables should be initialized now. */
> >> +    rsdp_offset = acpi_find_rsdp_address(qts);
> >> +
> >> +    g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
> >> +
> >> +    acpi_fetch_rsdp_table(qts, rsdp_offset, rsdp_table);
> >> +    acpi_fetch_table(qts, &rsdt, &rsdt_len, &rsdp_table[16 /* RsdtAddress */],
> >> +                     4, "RSDT", true);
> >> +
> >> +    ACPI_FOREACH_RSDT_ENTRY(rsdt, rsdt_len, ent, 4 /* Entry size */) {
> >> +        uint8_t *table_aml;
> >> +        acpi_fetch_table(qts, &table_aml, &table_length, ent, 4, NULL, true);
> >> +        if (!memcmp(table_aml + 0 /* Header Signature */, "ERST", 4)) {
> >> +            /*
> >> +             * Picking up ERST base address from the Register Region
> >> +             * specified as part of the first Serialization Instruction
> >> +             * Action (which is a Begin Write Operation).
> >> +             */
> >> +            memcpy(&base, &table_aml[56], sizeof(base));
> >> +            g_free(table_aml);
> >> +            break;
> >> +        }
> >> +        g_free(table_aml);
> >> +    }
> >> +    g_free(rsdt);
> >> +    return base;
> >> +}  
> > I'd drop this, bios-tables-test should do ACPI table check
> > as for PCI device itself you can test it with qtest accelerator
> > that allows to instantiate it and access registers directly
> > without overhead of running actual guest.  
> Yes, bios-tables-test checks the ACPI table, but not the functionality.
> This test has actually caught a problem/bug during development.

What I'm saying is not to drop test, but rather use qtest
accelerator to test PCI hardware registers. Which doesn't run
guest code. but lets you directly program/access PCI device.

So instead of searching/parsing ERST table, you'd program BARs
manually on behalf of BIOS, and then test that it works as expected.

As for ACPI tables, we don't have complete testing infrastructure
in tree, bios-tables-test, only tests matching to committed
reference blobs. And verifying that reference blob is correct,
is manual process currently.

To test whole stack one could write an optional acceptance test,
which would run actual guest (if you wish to add that, you can look at
docs/devel/testing.rst "Acceptance tests ...").



> > As example you can look into megasas-test.c, ivshmem-test.c
> > or other PCI device tests.  
> But I'll look at these and see about migrating to this approach.
> 
> >   
> >> +static char disk[] = "tests/erst-test-disk-XXXXXX";
> >> +
> >> +#define ERST_CMD()                              \
> >> +    "-accel kvm -accel tcg "                    \
> >> +    "-object memory-backend-file," \
> >> +      "id=erstnvram,mem-path=tests/acpi-erst-XXXXXX,size=0x10000,share=on " \
> >> +    "-device acpi-erst,memdev=erstnvram " \
> >> +    "-drive id=hd0,if=none,file=%s,format=raw " \
> >> +    "-device ide-hd,drive=hd0 ", disk
> >> +
> >> +static void erst_get_error_log_address_range(void)
> >> +{
> >> +    QTestState *qts;
> >> +    uint64_t log_address_range = 0;
> >> +    unsigned log_address_length = 0;
> >> +    unsigned log_address_attr = 0;
> >> +
> >> +    qts = qtest_initf(ERST_CMD());
> >> +
> >> +    uint64_t base = acpi_find_erst(qts);
> >> +    g_assert(base != 0);
> >> +
> >> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE command */
> >> +    qtest_writel(qts, base + 0, 0xD);
> >> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE result */
> >> +    log_address_range = qtest_readq(qts, base + 8);\
> >> +
> >> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE_LENGTH command */
> >> +    qtest_writel(qts, base + 0, 0xE);
> >> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE_LENGTH result */
> >> +    log_address_length = qtest_readq(qts, base + 8);\
> >> +
> >> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES command */
> >> +    qtest_writel(qts, base + 0, 0xF);
> >> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES result */
> >> +    log_address_attr = qtest_readq(qts, base + 8);\
> >> +
> >> +    /* Check log_address_range is not 0,~0 or base */
> >> +    g_assert(log_address_range != base);
> >> +    g_assert(log_address_range != 0);
> >> +    g_assert(log_address_range != ~0UL);
> >> +
> >> +    /* Check log_address_length is ERST_RECORD_SIZE */
> >> +    g_assert(log_address_length == (8 * 1024));
> >> +
> >> +    /* Check log_address_attr is 0 */
> >> +    g_assert(log_address_attr == 0);
> >> +
> >> +    qtest_quit(qts);
> >> +}
> >> +
> >> +int main(int argc, char **argv)
> >> +{
> >> +    int ret;
> >> +
> >> +    ret = boot_sector_init(disk);
> >> +    if (ret) {
> >> +        return ret;
> >> +    }
> >> +
> >> +    g_test_init(&argc, &argv, NULL);
> >> +
> >> +    qtest_add_func("/erst/get-error-log-address-range",
> >> +                   erst_get_error_log_address_range);
> >> +
> >> +    ret = g_test_run();
> >> +    boot_sector_cleanup(disk);
> >> +
> >> +    return ret;
> >> +}
> >> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> >> index 0c76738..deae443 100644
> >> --- a/tests/qtest/meson.build
> >> +++ b/tests/qtest/meson.build
> >> @@ -66,6 +66,7 @@ qtests_i386 = \
> >>     (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) +              \
> >>     (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) +   \
> >>     (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +                 \
> >> +  (config_all_devices.has_key('CONFIG_ACPI') ? ['erst-test'] : []) +                 \
> >>     qtests_pci +                                                                              \
> >>     ['fdc-test',
> >>      'ide-test',
> >> @@ -237,6 +238,7 @@ qtests = {
> >>     'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
> >>     'cdrom-test': files('boot-sector.c'),
> >>     'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
> >> +  'erst-test': files('erst-test.c', 'boot-sector.c', 'acpi-utils.c'),
> >>     'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
> >>     'migration-test': files('migration-helpers.c'),
> >>     'pxe-test': files('boot-sector.c'),  
> >   
>

Eric DeVolder July 26, 2021, 8:06 p.m. UTC | #4

On 7/26/21 6:45 AM, Igor Mammedov wrote:
> On Wed, 21 Jul 2021 11:18:44 -0500
> Eric DeVolder <[e-mail]> wrote:
> 
>> On 7/20/21 8:38 AM, Igor Mammedov wrote:
>>> On Wed, 30 Jun 2021 15:07:20 -0400
>>> Eric DeVolder <[e-mail]> wrote:
>>>    
>>>> This change provides a qtest that locates and then does a simple
>>>> interrogation of the ERST feature within the guest.
>>>>
>>>> Signed-off-by: Eric DeVolder <[e-mail]>
>>>> ---
>>>>    tests/qtest/erst-test.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    tests/qtest/meson.build |   2 +
>>>>    2 files changed, 131 insertions(+)
>>>>    create mode 100644 tests/qtest/erst-test.c
>>>>
>>>> diff --git a/tests/qtest/erst-test.c b/tests/qtest/erst-test.c
>>>> new file mode 100644
>>>> index 0000000..ce014c1
>>>> --- /dev/null
>>>> +++ b/tests/qtest/erst-test.c
>>>> @@ -0,0 +1,129 @@
>>>> +/*
>>>> + * QTest testcase for ACPI ERST
>>>> + *
>>>> + * Copyright (c) 2021 Oracle
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qemu/bitmap.h"
>>>> +#include "qemu/uuid.h"
>>>> +#include "hw/acpi/acpi-defs.h"
>>>> +#include "boot-sector.h"
>>>> +#include "acpi-utils.h"
>>>> +#include "libqos/libqtest.h"
>>>> +#include "qapi/qmp/qdict.h"
>>>> +
>>>> +#define RSDP_ADDR_INVALID 0x100000 /* RSDP must be below this address */
>>>> +
>>>> +static uint64_t acpi_find_erst(QTestState *qts)
>>>> +{
>>>> +    uint32_t rsdp_offset;
>>>> +    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
>>>> +    uint32_t rsdt_len, table_length;
>>>> +    uint8_t *rsdt, *ent;
>>>> +    uint64_t base = 0;
>>>> +
>>>> +    /* Wait for guest firmware to finish and start the payload. */
>>>> +    boot_sector_test(qts);
>>>> +
>>>> +    /* Tables should be initialized now. */
>>>> +    rsdp_offset = acpi_find_rsdp_address(qts);
>>>> +
>>>> +    g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
>>>> +
>>>> +    acpi_fetch_rsdp_table(qts, rsdp_offset, rsdp_table);
>>>> +    acpi_fetch_table(qts, &rsdt, &rsdt_len, &rsdp_table[16 /* RsdtAddress */],
>>>> +                     4, "RSDT", true);
>>>> +
>>>> +    ACPI_FOREACH_RSDT_ENTRY(rsdt, rsdt_len, ent, 4 /* Entry size */) {
>>>> +        uint8_t *table_aml;
>>>> +        acpi_fetch_table(qts, &table_aml, &table_length, ent, 4, NULL, true);
>>>> +        if (!memcmp(table_aml + 0 /* Header Signature */, "ERST", 4)) {
>>>> +            /*
>>>> +             * Picking up ERST base address from the Register Region
>>>> +             * specified as part of the first Serialization Instruction
>>>> +             * Action (which is a Begin Write Operation).
>>>> +             */
>>>> +            memcpy(&base, &table_aml[56], sizeof(base));
>>>> +            g_free(table_aml);
>>>> +            break;
>>>> +        }
>>>> +        g_free(table_aml);
>>>> +    }
>>>> +    g_free(rsdt);
>>>> +    return base;
>>>> +}
>>> I'd drop this, bios-tables-test should do ACPI table check
>>> as for PCI device itself you can test it with qtest accelerator
>>> that allows to instantiate it and access registers directly
>>> without overhead of running actual guest.
>> Yes, bios-tables-test checks the ACPI table, but not the functionality.
>> This test has actually caught a problem/bug during development.
> 
> What I'm saying is not to drop test, but rather use qtest
> accelerator to test PCI hardware registers. Which doesn't run
> guest code. but lets you directly program/access PCI device.
> 
> So instead of searching/parsing ERST table, you'd program BARs
> manually on behalf of BIOS, and then test that it works as expected.
> 
> As for ACPI tables, we don't have complete testing infrastructure
> in tree, bios-tables-test, only tests matching to committed
> reference blobs. And verifying that reference blob is correct,
> is manual process currently.
> 
> To test whole stack one could write an optional acceptance test,
> which would run actual guest (if you wish to add that, you can look at
> docs/devel/testing.rst "Acceptance tests ...").
> 

I've reworked this to pattern it after ivshmem test.

> 
> 
>>> As example you can look into megasas-test.c, ivshmem-test.c
>>> or other PCI device tests.
>> But I'll look at these and see about migrating to this approach.
>>
>>>    
>>>> +static char disk[] = "tests/erst-test-disk-XXXXXX";
>>>> +
>>>> +#define ERST_CMD()                              \
>>>> +    "-accel kvm -accel tcg "                    \
>>>> +    "-object memory-backend-file," \
>>>> +      "id=erstnvram,mem-path=tests/acpi-erst-XXXXXX,size=0x10000,share=on " \
>>>> +    "-device acpi-erst,memdev=erstnvram " \
>>>> +    "-drive id=hd0,if=none,file=%s,format=raw " \
>>>> +    "-device ide-hd,drive=hd0 ", disk
>>>> +
>>>> +static void erst_get_error_log_address_range(void)
>>>> +{
>>>> +    QTestState *qts;
>>>> +    uint64_t log_address_range = 0;
>>>> +    unsigned log_address_length = 0;
>>>> +    unsigned log_address_attr = 0;
>>>> +
>>>> +    qts = qtest_initf(ERST_CMD());
>>>> +
>>>> +    uint64_t base = acpi_find_erst(qts);
>>>> +    g_assert(base != 0);
>>>> +
>>>> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE command */
>>>> +    qtest_writel(qts, base + 0, 0xD);
>>>> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE result */
>>>> +    log_address_range = qtest_readq(qts, base + 8);\
>>>> +
>>>> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE_LENGTH command */
>>>> +    qtest_writel(qts, base + 0, 0xE);
>>>> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE_LENGTH result */
>>>> +    log_address_length = qtest_readq(qts, base + 8);\
>>>> +
>>>> +    /* Issue GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES command */
>>>> +    qtest_writel(qts, base + 0, 0xF);
>>>> +    /* Read GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES result */
>>>> +    log_address_attr = qtest_readq(qts, base + 8);\
>>>> +
>>>> +    /* Check log_address_range is not 0,~0 or base */
>>>> +    g_assert(log_address_range != base);
>>>> +    g_assert(log_address_range != 0);
>>>> +    g_assert(log_address_range != ~0UL);
>>>> +
>>>> +    /* Check log_address_length is ERST_RECORD_SIZE */
>>>> +    g_assert(log_address_length == (8 * 1024));
>>>> +
>>>> +    /* Check log_address_attr is 0 */
>>>> +    g_assert(log_address_attr == 0);
>>>> +
>>>> +    qtest_quit(qts);
>>>> +}
>>>> +
>>>> +int main(int argc, char **argv)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    ret = boot_sector_init(disk);
>>>> +    if (ret) {
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    g_test_init(&argc, &argv, NULL);
>>>> +
>>>> +    qtest_add_func("/erst/get-error-log-address-range",
>>>> +                   erst_get_error_log_address_range);
>>>> +
>>>> +    ret = g_test_run();
>>>> +    boot_sector_cleanup(disk);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>>>> index 0c76738..deae443 100644
>>>> --- a/tests/qtest/meson.build
>>>> +++ b/tests/qtest/meson.build
>>>> @@ -66,6 +66,7 @@ qtests_i386 = \
>>>>      (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) +              \
>>>>      (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) +   \
>>>>      (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +                 \
>>>> +  (config_all_devices.has_key('CONFIG_ACPI') ? ['erst-test'] : []) +                 \
>>>>      qtests_pci +                                                                              \
>>>>      ['fdc-test',
>>>>       'ide-test',
>>>> @@ -237,6 +238,7 @@ qtests = {
>>>>      'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
>>>>      'cdrom-test': files('boot-sector.c'),
>>>>      'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
>>>> +  'erst-test': files('erst-test.c', 'boot-sector.c', 'acpi-utils.c'),
>>>>      'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
>>>>      'migration-test': files('migration-helpers.c'),
>>>>      'pxe-test': files('boot-sector.c'),
>>>    
>>
>

Patch

diff --git a/tests/qtest/erst-test.c b/tests/qtest/erst-test.c
new file mode 100644
index 0000000..ce014c1
--- /dev/null
+++ b/tests/qtest/erst-test.c
@@ -0,0 +1,129 @@ 
+/*
+ * QTest testcase for ACPI ERST
+ *
+ * Copyright (c) 2021 Oracle
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/bitmap.h"
+#include "qemu/uuid.h"
+#include "hw/acpi/acpi-defs.h"
+#include "boot-sector.h"
+#include "acpi-utils.h"
+#include "libqos/libqtest.h"
+#include "qapi/qmp/qdict.h"
+
+#define RSDP_ADDR_INVALID 0x100000 /* RSDP must be below this address */
+
+static uint64_t acpi_find_erst(QTestState *qts)
+{
+    uint32_t rsdp_offset;
+    uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
+    uint32_t rsdt_len, table_length;
+    uint8_t *rsdt, *ent;
+    uint64_t base = 0;
+
+    /* Wait for guest firmware to finish and start the payload. */
+    boot_sector_test(qts);
+
+    /* Tables should be initialized now. */
+    rsdp_offset = acpi_find_rsdp_address(qts);
+
+    g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
+
+    acpi_fetch_rsdp_table(qts, rsdp_offset, rsdp_table);
+    acpi_fetch_table(qts, &rsdt, &rsdt_len, &rsdp_table[16 /* RsdtAddress */],
+                     4, "RSDT", true);
+
+    ACPI_FOREACH_RSDT_ENTRY(rsdt, rsdt_len, ent, 4 /* Entry size */) {
+        uint8_t *table_aml;
+        acpi_fetch_table(qts, &table_aml, &table_length, ent, 4, NULL, true);
+        if (!memcmp(table_aml + 0 /* Header Signature */, "ERST", 4)) {
+            /*
+             * Picking up ERST base address from the Register Region
+             * specified as part of the first Serialization Instruction
+             * Action (which is a Begin Write Operation).
+             */
+            memcpy(&base, &table_aml[56], sizeof(base));
+            g_free(table_aml);
+            break;
+        }
+        g_free(table_aml);
+    }
+    g_free(rsdt);
+    return base;
+}
+
+static char disk[] = "tests/erst-test-disk-XXXXXX";
+
+#define ERST_CMD()                              \
+    "-accel kvm -accel tcg "                    \
+    "-object memory-backend-file," \
+      "id=erstnvram,mem-path=tests/acpi-erst-XXXXXX,size=0x10000,share=on " \
+    "-device acpi-erst,memdev=erstnvram " \
+    "-drive id=hd0,if=none,file=%s,format=raw " \
+    "-device ide-hd,drive=hd0 ", disk
+
+static void erst_get_error_log_address_range(void)
+{
+    QTestState *qts;
+    uint64_t log_address_range = 0;
+    unsigned log_address_length = 0;
+    unsigned log_address_attr = 0;
+
+    qts = qtest_initf(ERST_CMD());
+
+    uint64_t base = acpi_find_erst(qts);
+    g_assert(base != 0);
+
+    /* Issue GET_ERROR_LOG_ADDRESS_RANGE command */
+    qtest_writel(qts, base + 0, 0xD);
+    /* Read GET_ERROR_LOG_ADDRESS_RANGE result */
+    log_address_range = qtest_readq(qts, base + 8);\
+
+    /* Issue GET_ERROR_LOG_ADDRESS_RANGE_LENGTH command */
+    qtest_writel(qts, base + 0, 0xE);
+    /* Read GET_ERROR_LOG_ADDRESS_RANGE_LENGTH result */
+    log_address_length = qtest_readq(qts, base + 8);\
+
+    /* Issue GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES command */
+    qtest_writel(qts, base + 0, 0xF);
+    /* Read GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES result */
+    log_address_attr = qtest_readq(qts, base + 8);\
+
+    /* Check log_address_range is not 0,~0 or base */
+    g_assert(log_address_range != base);
+    g_assert(log_address_range != 0);
+    g_assert(log_address_range != ~0UL);
+
+    /* Check log_address_length is ERST_RECORD_SIZE */
+    g_assert(log_address_length == (8 * 1024));
+
+    /* Check log_address_attr is 0 */
+    g_assert(log_address_attr == 0);
+
+    qtest_quit(qts);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    ret = boot_sector_init(disk);
+    if (ret) {
+        return ret;
+    }
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("/erst/get-error-log-address-range",
+                   erst_get_error_log_address_range);
+
+    ret = g_test_run();
+    boot_sector_cleanup(disk);
+
+    return ret;
+}
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 0c76738..deae443 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -66,6 +66,7 @@  qtests_i386 = \
   (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) +              \
   (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) +   \
   (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +                 \
+  (config_all_devices.has_key('CONFIG_ACPI') ? ['erst-test'] : []) +                 \
   qtests_pci +                                                                              \
   ['fdc-test',
    'ide-test',
@@ -237,6 +238,7 @@  qtests = {
   'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
   'cdrom-test': files('boot-sector.c'),
   'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
+  'erst-test': files('erst-test.c', 'boot-sector.c', 'acpi-utils.c'),
   'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
   'migration-test': files('migration-helpers.c'),
   'pxe-test': files('boot-sector.c'),