Re: [PATCH v3 13/14] tests: acpi: updated expected blobs for TPM CRB

[Stefan Berger <stefanb@linux.ibm.com>]

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