Re: [RFC 5/6] bios-tables-test: Add Q35/TPM-TIS test

[Igor Mammedov <imammedo@redhat.com>]

On Mon,  1 Jun 2020 12:21:12 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> Test tables specific to the TPM-TIS instantiation.
> The TPM2 is added in the framework. Also the DSDT
> is updated with the TPM. The new function should be
> be usable for CRB as well, later one.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  tests/qtest/bios-tables-test.c | 60 ++++++++++++++++++++++++++++++++++
>  tests/qtest/Makefile.include   |  1 +
>  2 files changed, 61 insertions(+)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index c9843829b3..bbba98342c 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -57,6 +57,9 @@
>  #include "qemu/bitmap.h"
>  #include "acpi-utils.h"
>  #include "boot-sector.h"
> +#include "tpm-emu.h"
> +#include "hw/acpi/tpm.h"
> +
>  
>  #define MACHINE_PC "pc"
>  #define MACHINE_Q35 "q35"
> @@ -874,6 +877,60 @@ static void test_acpi_piix4_tcg_numamem(void)
>      free_test_data(&data);
>  }
>  
> +uint64_t tpm_tis_base_addr;
> +
> +struct tpm_test_data {
> +    const char *machine;
> +    const char *tpm_if;
> +};
> +
> +static void test_acpi_tcg_tpm(const void *context)
> +{

s/test_acpi_tcg_tpm/test_acpi_q35_tcg_tpm/

I'd try to keep test specific parameter within test function isnstead of pushing it up to main(),
drawback would be some code duplication for intializing test data and calling runner
but it's trivial and worked well so far. See for example test_acpi_piix4_tcg_bridge/test_acpi_q35_tcg_bridge. I might seem a waste but it's consictent with what we were doing
with bios tests.


> +    struct tpm_test_data *c = (struct tpm_test_data *)context;
> +    gchar *tmp_dir_name = g_strdup_printf("qemu-test_acpi_%s_tcg_%s.XXXXXX",
> +                                          c->machine, c->tpm_if);
> +    char *tmp_path = g_dir_make_tmp(tmp_dir_name, NULL);
> +    TestState test;
> +    test_data data;
> +    GThread *thread;
> +    char *args, *variant = g_strdup_printf(".%s", c->tpm_if);
maybe derive tpm_if from '.variant' if it's necessary at all?

> +
> +    tpm_tis_base_addr = TPM_TIS_ADDR_BASE;
hardcode it here, so in case QEMU regresses, test could notice?

> +
> +    module_call_init(MODULE_INIT_QOM);
why it's here?

> +
> +    test.addr = g_new0(SocketAddress, 1);
> +    test.addr->type = SOCKET_ADDRESS_TYPE_UNIX;
> +    test.addr->u.q_unix.path = g_build_filename(tmp_path, "sock", NULL);
> +    g_mutex_init(&test.data_mutex);
> +    g_cond_init(&test.data_cond);
> +    test.data_cond_signal = false;
> +
> +    thread = g_thread_new(NULL, tpm_emu_ctrl_thread, &test);
> +    tpm_emu_test_wait_cond(&test);
perhaps make a separate helper function from this chunk
so it could be reused from other TMP test functions.

> +
> +    memset(&data, 0, sizeof(data));
I'd init fields with initializer, see test_acpi_virt_tcg_numamem()

> +    data.machine = c->machine;
> +    data.variant = variant;
> +
> +    args = g_strdup_printf(
> +        " -chardev socket,id=chr,path=%s"
> +        " -tpmdev emulator,id=dev,chardev=chr"
> +        " -device tpm-%s,tpmdev=dev",
> +        test.addr->u.q_unix.path, c->tpm_if);
> +
> +    test_acpi_one(args, &data);
> +
> +    g_thread_join(thread);
> +    g_unlink(test.addr->u.q_unix.path);
> +    qapi_free_SocketAddress(test.addr);
> +    g_rmdir(tmp_path);
> +    g_free(variant);
> +    g_free(tmp_path);
> +    g_free(tmp_dir_name);
> +    free_test_data(&data);
> +}
> +
>  static void test_acpi_tcg_dimm_pxm(const char *machine)
>  {
>      test_data data;
> @@ -1028,6 +1085,7 @@ int main(int argc, char *argv[])
>  {
>      const char *arch = qtest_get_arch();
>      int ret;
> +    struct tpm_test_data tpm_q35_tis = {MACHINE_Q35, "tis"};

I'd hide this within test_acpi_tcg_tpm() as it's done in other test functions

>  
>      g_test_init(&argc, &argv, NULL);
>  
> @@ -1037,6 +1095,8 @@ int main(int argc, char *argv[])
>              return ret;
>          }
>  
> +        qtest_add_data_func("acpi/q35/tpm-tis",
> +                            &tpm_q35_tis, test_acpi_tcg_tpm);
>          qtest_add_func("acpi/piix4", test_acpi_piix4_tcg);
>          qtest_add_func("acpi/piix4/bridge", test_acpi_piix4_tcg_bridge);
>          qtest_add_func("acpi/q35", test_acpi_q35_tcg);
> diff --git a/tests/qtest/Makefile.include b/tests/qtest/Makefile.include
> index 9e5a51d033..5023fa413d 100644
> --- a/tests/qtest/Makefile.include
> +++ b/tests/qtest/Makefile.include
> @@ -262,6 +262,7 @@ tests/qtest/hd-geo-test$(EXESUF): tests/qtest/hd-geo-test.o $(libqos-obj-y)
>  tests/qtest/boot-order-test$(EXESUF): tests/qtest/boot-order-test.o $(libqos-obj-y)
>  tests/qtest/boot-serial-test$(EXESUF): tests/qtest/boot-serial-test.o $(libqos-obj-y)
>  tests/qtest/bios-tables-test$(EXESUF): tests/qtest/bios-tables-test.o \
> +        tests/qtest/tpm-emu.o $(test-io-obj-y) \
>  	tests/qtest/boot-sector.o tests/qtest/acpi-utils.o $(libqos-obj-y)
>  tests/qtest/pxe-test$(EXESUF): tests/qtest/pxe-test.o tests/qtest/boot-sector.o $(libqos-obj-y)
>  tests/qtest/microbit-test$(EXESUF): tests/qtest/microbit-test.o