Hi Igor, On 6/5/20 5:17 PM, Igor Mammedov wrote: > On Mon, 1 Jun 2020 12:21:12 +0200 > Eric Auger <eric.au...@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.au...@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. OK > > >> + 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 Without it, I get /x86_64/acpi/q35/tpm-tis: ** ERROR:qom/object.c:677:object_new_with_type: assertion failed: (type != NULL)
Thanks Eric > >> + >> + 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 >