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);
