I think I actually messed up and committed an older version of the
expected blobs. Since these are newly added for the new tests, they
should be passing without any additional work.
On Mon, Oct 30, 2023 at 5:35 PM Stefan Berger <stef...@linux.ibm.com> wrote:
>
>
> On 10/30/23 18:42, Stefan Berger wrote:
> >
> > On 10/29/23 02:03, Joelle van Dyne wrote:
> >> Signed-off-by: Joelle van Dyne <j...@getutm.app>
> >
> > I see this error here with the test cases:
> >
> >
> > | 364/377 ERROR:../tests/qtest/bios-tables-test.c:535:test_acpi_asl:
> > assertion failed: (all_tables_match) ERROR
> > 364/377 qemu:qtest+qtest-x86_64 /
> > qtest-x86_64/bios-tables-test ERROR 34.83s killed
> > by signal 6 SIGABRT
> > >>> QTEST_QEMU_BINARY=./qemu-system-x86_64 MALLOC_PERTURB_=200
> > QTEST_QEMU_IMG=./qemu-img
> > G_TEST_DBUS_DAEMON=/home/stefanb/qemu-tpm/tests/dbus-vmstate-daemon.sh
> > QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
> > PYTHON=/home/stefanb/qemu-tpm/build/pyvenv/bin/python3
> > /home/stefanb/qemu-tpm/build/tests/qtest/bios-tables-test --tap -k
> > --------------------------------------------------------------- 8<
> > ---------------------------------------------------------------
> >
> > $ diff tests/data/acpi/virt/TPM2.crb-device.dsl /tmp/aml-98C6D2.dsl
> > 6c6
> > < * Disassembly of tests/data/acpi/virt/TPM2.crb-device.tpm2, Mon Oct
> > 30 18:30:00 2023
> > ---
> > > * Disassembly of /tmp/aml-98C6D2, Mon Oct 30 18:29:29 2023
> > 16c16
> > < [009h 0009 1] Checksum : BA
> > ---
> > > [009h 0009 1] Checksum : C2
> > 30c30
> > < [044h 0068 8] Log Address : 0000000043D10000
> > ---
> > > [044h 0068 8] Log Address : 0000000043C90000
> >
> > The diff is in the address of the TPM log ... Not good. I don't know
> > how we could have it ignore the address or not build the TPM2 table
> > with an address for a log. It would be good to have test cases.
> >
> > Stefan
>
>
> The log that the TPM2 ACPI table points to is needed for a BIOS, UEFI
> does not need it (but we don't typically know whether BIOS or UEFI will
> run). So we could introduce a property no-acpi-log and have the test
> cases set this to 'on' and get a NULL pointer for the 'Log Address'. You
> could use the following in a patch:
>
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index acc654382e..2b2de34201 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2224,6 +2224,7 @@ void build_tpm2(GArray *table_data, BIOSLinker
> *linker, GArray *tcpalog,
> uint32_t start_method;
> AcpiTable table = { .sig = "TPM2", .rev = 4,
> .oem_id = oem_id, .oem_table_id = oem_table_id };
> + bool acpi_log = true;
>
> acpi_table_begin(&table, table_data);
>
> @@ -2238,6 +2239,7 @@ void build_tpm2(GArray *table_data, BIOSLinker
> *linker, GArray *tcpalog,
> control_area_start_address = TPM_CRB_ADDR_CTRL;
> start_method = TPM2_START_METHOD_CRB;
> } else if (TPM_IS_CRB_SYSBUS(tpmif)) {
> + acpi_log = !object_property_get_bool(OBJECT(tpmif),
> "no-acpi-log", NULL);
> baseaddr = object_property_get_uint(OBJECT(tpmif),
> "x-baseaddr", NULL);
> control_area_start_address = baseaddr + A_CRB_CTRL_REQ;
> start_method = TPM2_START_METHOD_CRB;
> @@ -2253,20 +2255,25 @@ void build_tpm2(GArray *table_data, BIOSLinker
> *linker, GArray *tcpalog,
> g_array_append_vals(table_data, &start_method_params,
> ARRAY_SIZE(start_method_params));
>
> - /* Log Area Minimum Length */
> - build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
> + if (acpi_log) {
> + /* Log Area Minimum Length */
> + build_append_int_noprefix(table_data,
> TPM_LOG_AREA_MINIMUM_SIZE, 4);
>
> - acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
> - bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
> - false);
> + acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
> + bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE,
> tcpalog, 1,
> + false);
>
> - log_addr_offset = table_data->len;
> + log_addr_offset = table_data->len;
>
> - /* Log Area Start Address to be filled by Guest linker */
> - build_append_int_noprefix(table_data, 0, 8);
> - bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> - log_addr_offset, 8,
> - ACPI_BUILD_TPMLOG_FILE, 0);
> + /* Log Area Start Address to be filled by Guest linker */
> + build_append_int_noprefix(table_data, 0, 8);
> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> + log_addr_offset, 8,
> + ACPI_BUILD_TPMLOG_FILE, 0);
> + } else {
> + build_append_int_noprefix(table_data, 0, 4);
> + build_append_int_noprefix(table_data, 0, 8);
> + }
> acpi_table_end(linker, &table);
> }
> #endif
> diff --git a/hw/tpm/tpm_crb_sysbus.c b/hw/tpm/tpm_crb_sysbus.c
> index c10a8b5639..aeeaba512b 100644
> --- a/hw/tpm/tpm_crb_sysbus.c
> +++ b/hw/tpm/tpm_crb_sysbus.c
> @@ -35,6 +35,7 @@ struct TPMCRBStateSysBus {
> TPMCRBState state;
> uint64_t baseaddr;
> uint64_t size;
> + bool no_acpi_log;
> };
>
> OBJECT_DECLARE_SIMPLE_TYPE(TPMCRBStateSysBus, TPM_CRB_SYSBUS)
> @@ -74,6 +75,8 @@ static Property tpm_crb_sysbus_properties[] = {
> baseaddr, TPM_CRB_ADDR_BASE),
> DEFINE_PROP_UINT64("x-size", TPMCRBStateSysBus,
> size, TPM_CRB_ADDR_SIZE),
> + DEFINE_PROP_BOOL("no-acpi-log", TPMCRBStateSysBus,
> + no_acpi_log, FALSE),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index dea2a18158..9e8a02f924 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -1400,7 +1400,7 @@ static void test_acpi_tcg_tpm(const char *machine,
> const char *tpm_if,
> " %s"
> " -chardev socket,id=chr,path=%s"
> " -tpmdev emulator,id=dev,chardev=chr"
> - " -device tpm-%s,tpmdev=dev",
> + " -device tpm-%s,tpmdev=dev,no-acpi-log=on",
> g_strcmp0(machine, "virt") == 0 ? "-cpu cortex-a57" : "",
> test.addr->u.q_unix.path, tpm_if);
>
>
