| Message ID | 20231114020927.62315-13-j@getutm.app |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: introduce TPM CRB SysBus device | expand |
Hi On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne <j@getutm.app> wrote: > > Signed-off-by: Joelle van Dyne <j@getutm.app> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> nit: you also added tests for x86, could be a different patch? For arm, the test fails until next patch with: # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-991279.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine virt -accel tcg -nodefaults -nographic -drive if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 -cpu cortex-a57 -chardev socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev -accel qtest Warning! zero length expected file 'tests/data/acpi/virt/TPM2.crb-device.tpm2' Warning! zero length expected file 'tests/data/acpi/virt/DSDT.crb-device.tpm2' acpi-test: Warning! binary file mismatch. Actual [aml:/tmp/aml-GO4ME2], Expected [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files. acpi-test: Warning! binary file mismatch. Actual [aml:/tmp/aml-6N4ME2], Expected [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files. to see ASL diff between mismatched files install IASL, rebuild QEMU from scratch and re-run tests with V=1 environment variable set** ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion failed: (all_tables_match) not ok /aarch64/acpi/virt/tpm2-crb - ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion failed: (all_tables_match) Bail out! qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: Resource temporarily unavailable Unexpected error in qio_channel_socket_writev() at ../io/channel-socket.c:622: /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable to write to socket: Bad file descriptor > --- > tests/qtest/bios-tables-test.c | 43 ++++++++++++++++++++++++++++++++-- > 1 file changed, 41 insertions(+), 2 deletions(-) > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > index 71af5cf69f..bb4ebf00c1 100644 > --- a/tests/qtest/bios-tables-test.c > +++ b/tests/qtest/bios-tables-test.c > @@ -1447,6 +1447,28 @@ static void test_acpi_piix4_tcg_numamem(void) > > uint64_t tpm_tis_base_addr; > > +static test_data tcg_tpm_test_data(const char *machine) > +{ > + if (g_strcmp0(machine, "virt") == 0) { > + test_data data = { > + .machine = "virt", > + .tcg_only = true, > + .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd", > + .uefi_fl2 = "pc-bios/edk2-arm-vars.fd", > + .cd = > + "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2", > + .ram_start = 0x40000000ULL, > + .scan_len = 128ULL * 1024 * 1024, > + }; > + return data; > + } else { > + test_data data = { > + .machine = machine, > + }; > + return data; > + } > +} > + > static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if, > uint64_t base, enum TPMVersion tpm_version) > { > @@ -1454,7 +1476,7 @@ static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if, > machine, tpm_if); > char *tmp_path = g_dir_make_tmp(tmp_dir_name, NULL); > TPMTestState test; > - test_data data = {}; > + test_data data = tcg_tpm_test_data(machine); > GThread *thread; > const char *suffix = tpm_version == TPM_VERSION_2_0 ? "tpm2" : "tpm12"; > char *args, *variant = g_strdup_printf(".%s.%s", tpm_if, suffix); > @@ -1474,13 +1496,14 @@ static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if, > thread = g_thread_new(NULL, tpm_emu_ctrl_thread, &test); > tpm_emu_test_wait_cond(&test); > > - data.machine = machine; > data.variant = variant; > > args = g_strdup_printf( > + " %s" > " -chardev socket,id=chr,path=%s" > " -tpmdev emulator,id=dev,chardev=chr" > " -device tpm-%s,tpmdev=dev", > + g_strcmp0(machine, "virt") == 0 ? "-cpu cortex-a57" : "", > test.addr->u.q_unix.path, tpm_if); > > test_acpi_one(args, &data); > @@ -1506,6 +1529,16 @@ static void test_acpi_q35_tcg_tpm12_tis(void) > test_acpi_tcg_tpm("q35", "tis", 0xFED40000, TPM_VERSION_1_2); > } > > +static void test_acpi_q35_tcg_tpm2_crb(void) > +{ > + test_acpi_tcg_tpm("q35", "crb", 0xFED40000, TPM_VERSION_2_0); > +} > + > +static void test_acpi_virt_tcg_tpm2_crb(void) > +{ > + test_acpi_tcg_tpm("virt", "crb-device", 0xFED40000, TPM_VERSION_2_0); > +} > + > static void test_acpi_tcg_dimm_pxm(const char *machine) > { > test_data data = {}; > @@ -2212,6 +2245,9 @@ int main(int argc, char *argv[]) > qtest_add_func("acpi/q35/tpm12-tis", > test_acpi_q35_tcg_tpm12_tis); > } > + if (tpm_model_is_available("-machine q35", "tpm-crb")) { > + qtest_add_func("acpi/q35/tpm2-crb", test_acpi_q35_tcg_tpm2_crb); > + } > qtest_add_func("acpi/q35/bridge", test_acpi_q35_tcg_bridge); > qtest_add_func("acpi/q35/no-acpi-hotplug", > test_acpi_q35_tcg_no_acpi_hotplug); > @@ -2301,6 +2337,9 @@ int main(int argc, char *argv[]) > qtest_add_func("acpi/virt/viot", test_acpi_virt_viot); > } > } > + if (tpm_model_is_available("-machine virt", "tpm-crb")) { > + qtest_add_func("acpi/virt/tpm2-crb", test_acpi_virt_tcg_tpm2_crb); > + } > } > ret = g_test_run(); > boot_sector_cleanup(disk); > -- > 2.41.0 > >
On 11/14/23 04:36, Marc-André Lureau wrote: > Hi > > On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne <j@getutm.app> wrote: >> >> Signed-off-by: Joelle van Dyne <j@getutm.app> >> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > > nit: you also added tests for x86, could be a different patch? > > For arm, the test fails until next patch with: > > # starting QEMU: exec ./qemu-system-aarch64 -qtest > unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev > socket,path=/tmp/qtest-991279.qmp,id=char0 -mon > chardev=char0,mode=control -display none -audio none -machine virt > -accel tcg -nodefaults -nographic -drive > if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on > -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on > -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 > -cpu cortex-a57 -chardev > socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock > -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev > -accel qtest > Warning! zero length expected file 'tests/data/acpi/virt/TPM2.crb-device.tpm2' > Warning! zero length expected file 'tests/data/acpi/virt/DSDT.crb-device.tpm2' > acpi-test: Warning! binary file mismatch. Actual > [aml:/tmp/aml-GO4ME2], Expected > [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. > See source file tests/qtest/bios-tables-test.c for instructions on how > to update expected files. > acpi-test: Warning! binary file mismatch. Actual > [aml:/tmp/aml-6N4ME2], Expected > [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. > See source file tests/qtest/bios-tables-test.c for instructions on how > to update expected files. > to see ASL diff between mismatched files install IASL, rebuild QEMU > from scratch and re-run tests with V=1 environment variable set** > ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion > failed: (all_tables_match) > not ok /aarch64/acpi/virt/tpm2-crb - > ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion > failed: (all_tables_match) > Bail out! > qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: > Resource temporarily unavailable > Unexpected error in qio_channel_socket_writev() at ../io/channel-socket.c:622: > /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable > to write to socket: Bad file descriptor I had seen errors like these as well. However, the first error appears when applying the series after getting it using 'b4 am' -- it complains about patch 13. I always take patch 13 & 14 from my email client. Both of these here pass: QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/bios-tables-test QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/bios-tables-test
On 11/14/23 04:36, Marc-André Lureau wrote: > Hi > > On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne <j@getutm.app> wrote: >> >> Signed-off-by: Joelle van Dyne <j@getutm.app> >> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > > nit: you also added tests for x86, could be a different patch? > > For arm, the test fails until next patch with: > > # starting QEMU: exec ./qemu-system-aarch64 -qtest > unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev > socket,path=/tmp/qtest-991279.qmp,id=char0 -mon > chardev=char0,mode=control -display none -audio none -machine virt > -accel tcg -nodefaults -nographic -drive > if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on > -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on > -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 > -cpu cortex-a57 -chardev > socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock > -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev > -accel qtest > Warning! zero length expected file 'tests/data/acpi/virt/TPM2.crb-device.tpm2' > Warning! zero length expected file 'tests/data/acpi/virt/DSDT.crb-device.tpm2' > acpi-test: Warning! binary file mismatch. Actual > [aml:/tmp/aml-GO4ME2], Expected > [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. > See source file tests/qtest/bios-tables-test.c for instructions on how > to update expected files. > acpi-test: Warning! binary file mismatch. Actual > [aml:/tmp/aml-6N4ME2], Expected > [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. > See source file tests/qtest/bios-tables-test.c for instructions on how > to update expected files. > to see ASL diff between mismatched files install IASL, rebuild QEMU > from scratch and re-run tests with V=1 environment variable set** > ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion > failed: (all_tables_match) > not ok /aarch64/acpi/virt/tpm2-crb - > ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion > failed: (all_tables_match) > Bail out! > qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: > Resource temporarily unavailable > Unexpected error in qio_channel_socket_writev() at ../io/channel-socket.c:622: > /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable > to write to socket: Bad file descriptor > Travis testing on s390x I see the following failures for this patchset (search for 'ERROR'): https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363 Summary of Failures: 134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test ERROR 0.70s killed by signal 6 SIGABRT 219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test ERROR 0.88s killed by signal 6 SIGABRT Summary of Failures: 271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test ERROR 0.59s killed by signal 6 SIGABRT My guess is it's an endianess issue on big endian machines due to reading from the ROM device where we lost the .endianess: +const MemoryRegionOps tpm_crb_memory_ops = { + .read = tpm_crb_mmio_read, + .write = tpm_crb_mmio_write, + .endianness = DEVICE_LITTLE_ENDIAN, + .valid = { + .min_access_size = 1, + .max_access_size = 4, + }, +};
On 11/14/23 13:03, Stefan Berger wrote: > > > On 11/14/23 04:36, Marc-André Lureau wrote: >> Hi >> >> On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne <j@getutm.app> wrote: >>> >>> Signed-off-by: Joelle van Dyne <j@getutm.app> >>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >> >> nit: you also added tests for x86, could be a different patch? >> >> For arm, the test fails until next patch with: >> >> # starting QEMU: exec ./qemu-system-aarch64 -qtest >> unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev >> socket,path=/tmp/qtest-991279.qmp,id=char0 -mon >> chardev=char0,mode=control -display none -audio none -machine virt >> -accel tcg -nodefaults -nographic -drive >> if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on >> -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on >> -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 >> -cpu cortex-a57 -chardev >> socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock >> -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev >> -accel qtest >> Warning! zero length expected file >> 'tests/data/acpi/virt/TPM2.crb-device.tpm2' >> Warning! zero length expected file >> 'tests/data/acpi/virt/DSDT.crb-device.tpm2' >> acpi-test: Warning! binary file mismatch. Actual >> [aml:/tmp/aml-GO4ME2], Expected >> [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. >> See source file tests/qtest/bios-tables-test.c for instructions on how >> to update expected files. >> acpi-test: Warning! binary file mismatch. Actual >> [aml:/tmp/aml-6N4ME2], Expected >> [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. >> See source file tests/qtest/bios-tables-test.c for instructions on how >> to update expected files. >> to see ASL diff between mismatched files install IASL, rebuild QEMU >> from scratch and re-run tests with V=1 environment variable set** >> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion >> failed: (all_tables_match) >> not ok /aarch64/acpi/virt/tpm2-crb - >> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion >> failed: (all_tables_match) >> Bail out! >> qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: >> Resource temporarily unavailable >> Unexpected error in qio_channel_socket_writev() at >> ../io/channel-socket.c:622: >> /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable >> to write to socket: Bad file descriptor >> > > Travis testing on s390x I see the following failures for this patchset > (search for 'ERROR'): > > https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363 > > Summary of Failures: > > 134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test > ERROR 0.70s killed by signal 6 SIGABRT > > 219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test > ERROR 0.88s killed by signal 6 SIGABRT > > > Summary of Failures: > > 271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test > ERROR 0.59s killed by signal 6 SIGABRT > > > My guess is it's an endianess issue on big endian machines due to > reading from the ROM device where we lost the .endianess: > > +const MemoryRegionOps tpm_crb_memory_ops = { > + .read = tpm_crb_mmio_read, > + .write = tpm_crb_mmio_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .valid = { > + .min_access_size = 1, > + .max_access_size = 4, > + }, > +}; > I think we need a 2nd set of registers to support the endianess conversion. It's not exactly nice, though. Basically the saved_regs could be used for this directly, even though I did not do that but introduced n_regs: https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91 This patch allows the tests on s390x to run farther but the execution of the command doesn't seem to work maybe due to command data that were also written in wrong endianess. I don't know. I would have to get access to a big endian / s390 machine to be able to fix it.
On 11/14/23 16:05, Stefan Berger wrote: > > > On 11/14/23 13:03, Stefan Berger wrote: >> >> >> On 11/14/23 04:36, Marc-André Lureau wrote: >>> Hi >>> >>> On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne <j@getutm.app> wrote: >>>> >>>> Signed-off-by: Joelle van Dyne <j@getutm.app> >>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >>> >>> nit: you also added tests for x86, could be a different patch? >>> >>> For arm, the test fails until next patch with: >>> >>> # starting QEMU: exec ./qemu-system-aarch64 -qtest >>> unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev >>> socket,path=/tmp/qtest-991279.qmp,id=char0 -mon >>> chardev=char0,mode=control -display none -audio none -machine virt >>> -accel tcg -nodefaults -nographic -drive >>> if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on >>> -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on >>> -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 >>> -cpu cortex-a57 -chardev >>> socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock >>> -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev >>> -accel qtest >>> Warning! zero length expected file >>> 'tests/data/acpi/virt/TPM2.crb-device.tpm2' >>> Warning! zero length expected file >>> 'tests/data/acpi/virt/DSDT.crb-device.tpm2' >>> acpi-test: Warning! binary file mismatch. Actual >>> [aml:/tmp/aml-GO4ME2], Expected >>> [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. >>> See source file tests/qtest/bios-tables-test.c for instructions on how >>> to update expected files. >>> acpi-test: Warning! binary file mismatch. Actual >>> [aml:/tmp/aml-6N4ME2], Expected >>> [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. >>> See source file tests/qtest/bios-tables-test.c for instructions on how >>> to update expected files. >>> to see ASL diff between mismatched files install IASL, rebuild QEMU >>> from scratch and re-run tests with V=1 environment variable set** >>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion >>> failed: (all_tables_match) >>> not ok /aarch64/acpi/virt/tpm2-crb - >>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion >>> failed: (all_tables_match) >>> Bail out! >>> qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: >>> Resource temporarily unavailable >>> Unexpected error in qio_channel_socket_writev() at >>> ../io/channel-socket.c:622: >>> /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable >>> to write to socket: Bad file descriptor >>> >> >> Travis testing on s390x I see the following failures for this patchset >> (search for 'ERROR'): >> >> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363 >> >> Summary of Failures: >> >> 134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test >> ERROR 0.70s killed by signal 6 SIGABRT >> >> 219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test >> ERROR 0.88s killed by signal 6 SIGABRT >> >> >> Summary of Failures: >> >> 271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test >> ERROR 0.59s killed by signal 6 SIGABRT >> >> >> My guess is it's an endianess issue on big endian machines due to >> reading from the ROM device where we lost the .endianess: >> >> +const MemoryRegionOps tpm_crb_memory_ops = { >> + .read = tpm_crb_mmio_read, >> + .write = tpm_crb_mmio_write, >> + .endianness = DEVICE_LITTLE_ENDIAN, >> + .valid = { >> + .min_access_size = 1, >> + .max_access_size = 4, >> + }, >> +}; >> > > I think we need a 2nd set of registers to support the endianess > conversion. It's not exactly nice, though. Basically the saved_regs > could be used for this directly, even though I did not do that but > introduced n_regs: > https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91 > > This patch allows the tests on s390x to run farther but the execution of > the command doesn't seem to work maybe due to command data that were > also written in wrong endianess. I don't know. I would have to get > access to a big endian / s390 machine to be able to fix it. > > The latest version now passes on Travis s390x: https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267245220
On Tue, Nov 14, 2023 at 4:12 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 11/14/23 16:05, Stefan Berger wrote: > > > > > > On 11/14/23 13:03, Stefan Berger wrote: > >> > >> > >> On 11/14/23 04:36, Marc-André Lureau wrote: > >>> Hi > >>> > >>> On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne <j@getutm.app> wrote: > >>>> > >>>> Signed-off-by: Joelle van Dyne <j@getutm.app> > >>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > >>> > >>> nit: you also added tests for x86, could be a different patch? > >>> > >>> For arm, the test fails until next patch with: > >>> > >>> # starting QEMU: exec ./qemu-system-aarch64 -qtest > >>> unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev > >>> socket,path=/tmp/qtest-991279.qmp,id=char0 -mon > >>> chardev=char0,mode=control -display none -audio none -machine virt > >>> -accel tcg -nodefaults -nographic -drive > >>> if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on > >>> -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on > >>> -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 > >>> -cpu cortex-a57 -chardev > >>> socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock > >>> -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev > >>> -accel qtest > >>> Warning! zero length expected file > >>> 'tests/data/acpi/virt/TPM2.crb-device.tpm2' > >>> Warning! zero length expected file > >>> 'tests/data/acpi/virt/DSDT.crb-device.tpm2' > >>> acpi-test: Warning! binary file mismatch. Actual > >>> [aml:/tmp/aml-GO4ME2], Expected > >>> [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. > >>> See source file tests/qtest/bios-tables-test.c for instructions on how > >>> to update expected files. > >>> acpi-test: Warning! binary file mismatch. Actual > >>> [aml:/tmp/aml-6N4ME2], Expected > >>> [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. > >>> See source file tests/qtest/bios-tables-test.c for instructions on how > >>> to update expected files. > >>> to see ASL diff between mismatched files install IASL, rebuild QEMU > >>> from scratch and re-run tests with V=1 environment variable set** > >>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion > >>> failed: (all_tables_match) > >>> not ok /aarch64/acpi/virt/tpm2-crb - > >>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion > >>> failed: (all_tables_match) > >>> Bail out! > >>> qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: > >>> Resource temporarily unavailable > >>> Unexpected error in qio_channel_socket_writev() at > >>> ../io/channel-socket.c:622: > >>> /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable > >>> to write to socket: Bad file descriptor > >>> > >> > >> Travis testing on s390x I see the following failures for this patchset > >> (search for 'ERROR'): > >> > >> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363 > >> > >> Summary of Failures: > >> > >> 134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test > >> ERROR 0.70s killed by signal 6 SIGABRT > >> > >> 219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test > >> ERROR 0.88s killed by signal 6 SIGABRT > >> > >> > >> Summary of Failures: > >> > >> 271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test > >> ERROR 0.59s killed by signal 6 SIGABRT > >> > >> > >> My guess is it's an endianess issue on big endian machines due to > >> reading from the ROM device where we lost the .endianess: > >> > >> +const MemoryRegionOps tpm_crb_memory_ops = { > >> + .read = tpm_crb_mmio_read, > >> + .write = tpm_crb_mmio_write, > >> + .endianness = DEVICE_LITTLE_ENDIAN, > >> + .valid = { > >> + .min_access_size = 1, > >> + .max_access_size = 4, > >> + }, > >> +}; > >> > > > > I think we need a 2nd set of registers to support the endianess > > conversion. It's not exactly nice, though. Basically the saved_regs > > could be used for this directly, even though I did not do that but > > introduced n_regs: > > https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91 > > > > This patch allows the tests on s390x to run farther but the execution of > > the command doesn't seem to work maybe due to command data that were > > also written in wrong endianess. I don't know. I would have to get > > access to a big endian / s390 machine to be able to fix it. > > > > > > The latest version now passes on Travis s390x: > https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267245220 > Are the tests failing on S390X due to the added code or are they failing because previously it was untested? I don't think the original code took account of endianness and that should be fixed, but feels like it should be in a separate patch set?
On 11/23/23 19:56, Joelle van Dyne wrote: > On Tue, Nov 14, 2023 at 4:12 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 11/14/23 16:05, Stefan Berger wrote: >>> >>> >>> On 11/14/23 13:03, Stefan Berger wrote: >>>> >>>> >>>> On 11/14/23 04:36, Marc-André Lureau wrote: >>>>> Hi >>>>> >>>>> On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne <j@getutm.app> wrote: >>>>>> >>>>>> Signed-off-by: Joelle van Dyne <j@getutm.app> >>>>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >>>>> >>>>> nit: you also added tests for x86, could be a different patch? >>>>> >>>>> For arm, the test fails until next patch with: >>>>> >>>>> # starting QEMU: exec ./qemu-system-aarch64 -qtest >>>>> unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev >>>>> socket,path=/tmp/qtest-991279.qmp,id=char0 -mon >>>>> chardev=char0,mode=control -display none -audio none -machine virt >>>>> -accel tcg -nodefaults -nographic -drive >>>>> if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on >>>>> -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on >>>>> -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 >>>>> -cpu cortex-a57 -chardev >>>>> socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock >>>>> -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev >>>>> -accel qtest >>>>> Warning! zero length expected file >>>>> 'tests/data/acpi/virt/TPM2.crb-device.tpm2' >>>>> Warning! zero length expected file >>>>> 'tests/data/acpi/virt/DSDT.crb-device.tpm2' >>>>> acpi-test: Warning! binary file mismatch. Actual >>>>> [aml:/tmp/aml-GO4ME2], Expected >>>>> [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. >>>>> See source file tests/qtest/bios-tables-test.c for instructions on how >>>>> to update expected files. >>>>> acpi-test: Warning! binary file mismatch. Actual >>>>> [aml:/tmp/aml-6N4ME2], Expected >>>>> [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. >>>>> See source file tests/qtest/bios-tables-test.c for instructions on how >>>>> to update expected files. >>>>> to see ASL diff between mismatched files install IASL, rebuild QEMU >>>>> from scratch and re-run tests with V=1 environment variable set** >>>>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion >>>>> failed: (all_tables_match) >>>>> not ok /aarch64/acpi/virt/tpm2-crb - >>>>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion >>>>> failed: (all_tables_match) >>>>> Bail out! >>>>> qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: >>>>> Resource temporarily unavailable >>>>> Unexpected error in qio_channel_socket_writev() at >>>>> ../io/channel-socket.c:622: >>>>> /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable >>>>> to write to socket: Bad file descriptor >>>>> >>>> >>>> Travis testing on s390x I see the following failures for this patchset >>>> (search for 'ERROR'): >>>> >>>> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363 >>>> >>>> Summary of Failures: >>>> >>>> 134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test >>>> ERROR 0.70s killed by signal 6 SIGABRT >>>> >>>> 219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test >>>> ERROR 0.88s killed by signal 6 SIGABRT >>>> >>>> >>>> Summary of Failures: >>>> >>>> 271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test >>>> ERROR 0.59s killed by signal 6 SIGABRT >>>> >>>> >>>> My guess is it's an endianess issue on big endian machines due to >>>> reading from the ROM device where we lost the .endianess: >>>> >>>> +const MemoryRegionOps tpm_crb_memory_ops = { >>>> + .read = tpm_crb_mmio_read, >>>> + .write = tpm_crb_mmio_write, >>>> + .endianness = DEVICE_LITTLE_ENDIAN, >>>> + .valid = { >>>> + .min_access_size = 1, >>>> + .max_access_size = 4, >>>> + }, >>>> +}; >>>> >>> >>> I think we need a 2nd set of registers to support the endianess >>> conversion. It's not exactly nice, though. Basically the saved_regs >>> could be used for this directly, even though I did not do that but >>> introduced n_regs: >>> https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91 >>> >>> This patch allows the tests on s390x to run farther but the execution of >>> the command doesn't seem to work maybe due to command data that were >>> also written in wrong endianess. I don't know. I would have to get >>> access to a big endian / s390 machine to be able to fix it. >>> >>> >> >> The latest version now passes on Travis s390x: >> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267245220 >> > > Are the tests failing on S390X due to the added code or are they > failing because previously it was untested? I don't think the original > code took account of endianness and that should be fixed, but feels > like it should be in a separate patch set? They are failing because something like the topmost one or two patches as in this branch here are missing for a big endian host: https://github.com/stefanberger/qemu-tpm/tree/joelle.v5%2B2nd_registers
On Fri, Nov 24, 2023 at 8:17 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 11/23/23 19:56, Joelle van Dyne wrote: > > On Tue, Nov 14, 2023 at 4:12 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > >> > >> > >> > >> On 11/14/23 16:05, Stefan Berger wrote: > >>> > >>> > >>> On 11/14/23 13:03, Stefan Berger wrote: > >>>> > >>>> > >>>> On 11/14/23 04:36, Marc-André Lureau wrote: > >>>>> Hi > >>>>> > >>>>> On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne <j@getutm.app> wrote: > >>>>>> > >>>>>> Signed-off-by: Joelle van Dyne <j@getutm.app> > >>>>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > >>>>> > >>>>> nit: you also added tests for x86, could be a different patch? > >>>>> > >>>>> For arm, the test fails until next patch with: > >>>>> > >>>>> # starting QEMU: exec ./qemu-system-aarch64 -qtest > >>>>> unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev > >>>>> socket,path=/tmp/qtest-991279.qmp,id=char0 -mon > >>>>> chardev=char0,mode=control -display none -audio none -machine virt > >>>>> -accel tcg -nodefaults -nographic -drive > >>>>> if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on > >>>>> -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on > >>>>> -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 > >>>>> -cpu cortex-a57 -chardev > >>>>> socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock > >>>>> -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev > >>>>> -accel qtest > >>>>> Warning! zero length expected file > >>>>> 'tests/data/acpi/virt/TPM2.crb-device.tpm2' > >>>>> Warning! zero length expected file > >>>>> 'tests/data/acpi/virt/DSDT.crb-device.tpm2' > >>>>> acpi-test: Warning! binary file mismatch. Actual > >>>>> [aml:/tmp/aml-GO4ME2], Expected > >>>>> [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. > >>>>> See source file tests/qtest/bios-tables-test.c for instructions on how > >>>>> to update expected files. > >>>>> acpi-test: Warning! binary file mismatch. Actual > >>>>> [aml:/tmp/aml-6N4ME2], Expected > >>>>> [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. > >>>>> See source file tests/qtest/bios-tables-test.c for instructions on how > >>>>> to update expected files. > >>>>> to see ASL diff between mismatched files install IASL, rebuild QEMU > >>>>> from scratch and re-run tests with V=1 environment variable set** > >>>>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion > >>>>> failed: (all_tables_match) > >>>>> not ok /aarch64/acpi/virt/tpm2-crb - > >>>>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion > >>>>> failed: (all_tables_match) > >>>>> Bail out! > >>>>> qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: > >>>>> Resource temporarily unavailable > >>>>> Unexpected error in qio_channel_socket_writev() at > >>>>> ../io/channel-socket.c:622: > >>>>> /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable > >>>>> to write to socket: Bad file descriptor > >>>>> > >>>> > >>>> Travis testing on s390x I see the following failures for this patchset > >>>> (search for 'ERROR'): > >>>> > >>>> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363 > >>>> > >>>> Summary of Failures: > >>>> > >>>> 134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test > >>>> ERROR 0.70s killed by signal 6 SIGABRT > >>>> > >>>> 219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test > >>>> ERROR 0.88s killed by signal 6 SIGABRT > >>>> > >>>> > >>>> Summary of Failures: > >>>> > >>>> 271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test > >>>> ERROR 0.59s killed by signal 6 SIGABRT > >>>> > >>>> > >>>> My guess is it's an endianess issue on big endian machines due to > >>>> reading from the ROM device where we lost the .endianess: > >>>> > >>>> +const MemoryRegionOps tpm_crb_memory_ops = { > >>>> + .read = tpm_crb_mmio_read, > >>>> + .write = tpm_crb_mmio_write, > >>>> + .endianness = DEVICE_LITTLE_ENDIAN, > >>>> + .valid = { > >>>> + .min_access_size = 1, > >>>> + .max_access_size = 4, > >>>> + }, > >>>> +}; > >>>> > >>> > >>> I think we need a 2nd set of registers to support the endianess > >>> conversion. It's not exactly nice, though. Basically the saved_regs > >>> could be used for this directly, even though I did not do that but > >>> introduced n_regs: > >>> https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91 > >>> > >>> This patch allows the tests on s390x to run farther but the execution of > >>> the command doesn't seem to work maybe due to command data that were > >>> also written in wrong endianess. I don't know. I would have to get > >>> access to a big endian / s390 machine to be able to fix it. > >>> > >>> > >> > >> The latest version now passes on Travis s390x: > >> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267245220 > >> > > > > Are the tests failing on S390X due to the added code or are they > > failing because previously it was untested? I don't think the original > > code took account of endianness and that should be fixed, but feels > > like it should be in a separate patch set? > > They are failing because something like the topmost one or two patches > as in this branch here are missing for a big endian host: > > https://github.com/stefanberger/qemu-tpm/tree/joelle.v5%2B2nd_registers Right, but is this issue new due to the patchset? i.e. if just the tests were added without the other patches, would they still fail? If so, I think the fixes should be in another patchset while we disable them in this patchset.
On 11/24/23 11:21, Joelle van Dyne wrote: > On Fri, Nov 24, 2023 at 8:17 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 11/23/23 19:56, Joelle van Dyne wrote: >>> On Tue, Nov 14, 2023 at 4:12 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>> >>>> >>>> >>>> On 11/14/23 16:05, Stefan Berger wrote: >>>>> >>>>> >>>>> On 11/14/23 13:03, Stefan Berger wrote: >>>>>> >>>>>> >>>>>> On 11/14/23 04:36, Marc-André Lureau wrote: >>>>>>> Hi >>>>>>> >>>>>>> On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne <j@getutm.app> wrote: >>>>>>>> >>>>>>>> Signed-off-by: Joelle van Dyne <j@getutm.app> >>>>>>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >>>>>>> >>>>>>> nit: you also added tests for x86, could be a different patch? >>>>>>> >>>>>>> For arm, the test fails until next patch with: >>>>>>> >>>>>>> # starting QEMU: exec ./qemu-system-aarch64 -qtest >>>>>>> unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev >>>>>>> socket,path=/tmp/qtest-991279.qmp,id=char0 -mon >>>>>>> chardev=char0,mode=control -display none -audio none -machine virt >>>>>>> -accel tcg -nodefaults -nographic -drive >>>>>>> if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on >>>>>>> -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on >>>>>>> -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 >>>>>>> -cpu cortex-a57 -chardev >>>>>>> socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock >>>>>>> -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev >>>>>>> -accel qtest >>>>>>> Warning! zero length expected file >>>>>>> 'tests/data/acpi/virt/TPM2.crb-device.tpm2' >>>>>>> Warning! zero length expected file >>>>>>> 'tests/data/acpi/virt/DSDT.crb-device.tpm2' >>>>>>> acpi-test: Warning! binary file mismatch. Actual >>>>>>> [aml:/tmp/aml-GO4ME2], Expected >>>>>>> [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. >>>>>>> See source file tests/qtest/bios-tables-test.c for instructions on how >>>>>>> to update expected files. >>>>>>> acpi-test: Warning! binary file mismatch. Actual >>>>>>> [aml:/tmp/aml-6N4ME2], Expected >>>>>>> [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. >>>>>>> See source file tests/qtest/bios-tables-test.c for instructions on how >>>>>>> to update expected files. >>>>>>> to see ASL diff between mismatched files install IASL, rebuild QEMU >>>>>>> from scratch and re-run tests with V=1 environment variable set** >>>>>>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion >>>>>>> failed: (all_tables_match) >>>>>>> not ok /aarch64/acpi/virt/tpm2-crb - >>>>>>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion >>>>>>> failed: (all_tables_match) >>>>>>> Bail out! >>>>>>> qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: >>>>>>> Resource temporarily unavailable >>>>>>> Unexpected error in qio_channel_socket_writev() at >>>>>>> ../io/channel-socket.c:622: >>>>>>> /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable >>>>>>> to write to socket: Bad file descriptor >>>>>>> >>>>>> >>>>>> Travis testing on s390x I see the following failures for this patchset >>>>>> (search for 'ERROR'): >>>>>> >>>>>> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363 >>>>>> >>>>>> Summary of Failures: >>>>>> >>>>>> 134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test >>>>>> ERROR 0.70s killed by signal 6 SIGABRT >>>>>> >>>>>> 219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test >>>>>> ERROR 0.88s killed by signal 6 SIGABRT >>>>>> >>>>>> >>>>>> Summary of Failures: >>>>>> >>>>>> 271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test >>>>>> ERROR 0.59s killed by signal 6 SIGABRT >>>>>> >>>>>> >>>>>> My guess is it's an endianess issue on big endian machines due to >>>>>> reading from the ROM device where we lost the .endianess: >>>>>> >>>>>> +const MemoryRegionOps tpm_crb_memory_ops = { >>>>>> + .read = tpm_crb_mmio_read, >>>>>> + .write = tpm_crb_mmio_write, >>>>>> + .endianness = DEVICE_LITTLE_ENDIAN, >>>>>> + .valid = { >>>>>> + .min_access_size = 1, >>>>>> + .max_access_size = 4, >>>>>> + }, >>>>>> +}; >>>>>> >>>>> >>>>> I think we need a 2nd set of registers to support the endianess >>>>> conversion. It's not exactly nice, though. Basically the saved_regs >>>>> could be used for this directly, even though I did not do that but >>>>> introduced n_regs: >>>>> https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91 >>>>> >>>>> This patch allows the tests on s390x to run farther but the execution of >>>>> the command doesn't seem to work maybe due to command data that were >>>>> also written in wrong endianess. I don't know. I would have to get >>>>> access to a big endian / s390 machine to be able to fix it. >>>>> >>>>> >>>> >>>> The latest version now passes on Travis s390x: >>>> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267245220 >>>> >>> >>> Are the tests failing on S390X due to the added code or are they >>> failing because previously it was untested? I don't think the original >>> code took account of endianness and that should be fixed, but feels >>> like it should be in a separate patch set? >> >> They are failing because something like the topmost one or two patches >> as in this branch here are missing for a big endian host: >> >> https://github.com/stefanberger/qemu-tpm/tree/joelle.v5%2B2nd_registers > > Right, but is this issue new due to the patchset? i.e. if just the Yes, it is due to this patchset. The reason is that CRB switched to a ROMD interface where the fact that the MMIO registers are little endian got lost for existing x86_64 support. > tests were added without the other patches, would they still fail? If > so, I think the fixes should be in another patchset while we disable > them in this patchset.
On Fri, Nov 24, 2023 at 8:26 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 11/24/23 11:21, Joelle van Dyne wrote: > > On Fri, Nov 24, 2023 at 8:17 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > >> > >> > >> > >> On 11/23/23 19:56, Joelle van Dyne wrote: > >>> On Tue, Nov 14, 2023 at 4:12 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>> > >>>> > >>>> > >>>> On 11/14/23 16:05, Stefan Berger wrote: > >>>>> > >>>>> > >>>>> On 11/14/23 13:03, Stefan Berger wrote: > >>>>>> > >>>>>> > >>>>>> On 11/14/23 04:36, Marc-André Lureau wrote: > >>>>>>> Hi > >>>>>>> > >>>>>>> On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne <j@getutm.app> wrote: > >>>>>>>> > >>>>>>>> Signed-off-by: Joelle van Dyne <j@getutm.app> > >>>>>>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > >>>>>>> > >>>>>>> nit: you also added tests for x86, could be a different patch? > >>>>>>> > >>>>>>> For arm, the test fails until next patch with: > >>>>>>> > >>>>>>> # starting QEMU: exec ./qemu-system-aarch64 -qtest > >>>>>>> unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev > >>>>>>> socket,path=/tmp/qtest-991279.qmp,id=char0 -mon > >>>>>>> chardev=char0,mode=control -display none -audio none -machine virt > >>>>>>> -accel tcg -nodefaults -nographic -drive > >>>>>>> if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on > >>>>>>> -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on > >>>>>>> -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 > >>>>>>> -cpu cortex-a57 -chardev > >>>>>>> socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock > >>>>>>> -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev > >>>>>>> -accel qtest > >>>>>>> Warning! zero length expected file > >>>>>>> 'tests/data/acpi/virt/TPM2.crb-device.tpm2' > >>>>>>> Warning! zero length expected file > >>>>>>> 'tests/data/acpi/virt/DSDT.crb-device.tpm2' > >>>>>>> acpi-test: Warning! binary file mismatch. Actual > >>>>>>> [aml:/tmp/aml-GO4ME2], Expected > >>>>>>> [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. > >>>>>>> See source file tests/qtest/bios-tables-test.c for instructions on how > >>>>>>> to update expected files. > >>>>>>> acpi-test: Warning! binary file mismatch. Actual > >>>>>>> [aml:/tmp/aml-6N4ME2], Expected > >>>>>>> [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. > >>>>>>> See source file tests/qtest/bios-tables-test.c for instructions on how > >>>>>>> to update expected files. > >>>>>>> to see ASL diff between mismatched files install IASL, rebuild QEMU > >>>>>>> from scratch and re-run tests with V=1 environment variable set** > >>>>>>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion > >>>>>>> failed: (all_tables_match) > >>>>>>> not ok /aarch64/acpi/virt/tpm2-crb - > >>>>>>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion > >>>>>>> failed: (all_tables_match) > >>>>>>> Bail out! > >>>>>>> qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: > >>>>>>> Resource temporarily unavailable > >>>>>>> Unexpected error in qio_channel_socket_writev() at > >>>>>>> ../io/channel-socket.c:622: > >>>>>>> /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable > >>>>>>> to write to socket: Bad file descriptor > >>>>>>> > >>>>>> > >>>>>> Travis testing on s390x I see the following failures for this patchset > >>>>>> (search for 'ERROR'): > >>>>>> > >>>>>> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363 > >>>>>> > >>>>>> Summary of Failures: > >>>>>> > >>>>>> 134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test > >>>>>> ERROR 0.70s killed by signal 6 SIGABRT > >>>>>> > >>>>>> 219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test > >>>>>> ERROR 0.88s killed by signal 6 SIGABRT > >>>>>> > >>>>>> > >>>>>> Summary of Failures: > >>>>>> > >>>>>> 271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test > >>>>>> ERROR 0.59s killed by signal 6 SIGABRT > >>>>>> > >>>>>> > >>>>>> My guess is it's an endianess issue on big endian machines due to > >>>>>> reading from the ROM device where we lost the .endianess: > >>>>>> > >>>>>> +const MemoryRegionOps tpm_crb_memory_ops = { > >>>>>> + .read = tpm_crb_mmio_read, > >>>>>> + .write = tpm_crb_mmio_write, > >>>>>> + .endianness = DEVICE_LITTLE_ENDIAN, > >>>>>> + .valid = { > >>>>>> + .min_access_size = 1, > >>>>>> + .max_access_size = 4, > >>>>>> + }, > >>>>>> +}; > >>>>>> > >>>>> > >>>>> I think we need a 2nd set of registers to support the endianess > >>>>> conversion. It's not exactly nice, though. Basically the saved_regs > >>>>> could be used for this directly, even though I did not do that but > >>>>> introduced n_regs: > >>>>> https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91 > >>>>> > >>>>> This patch allows the tests on s390x to run farther but the execution of > >>>>> the command doesn't seem to work maybe due to command data that were > >>>>> also written in wrong endianess. I don't know. I would have to get > >>>>> access to a big endian / s390 machine to be able to fix it. > >>>>> > >>>>> > >>>> > >>>> The latest version now passes on Travis s390x: > >>>> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267245220 > >>>> > >>> > >>> Are the tests failing on S390X due to the added code or are they > >>> failing because previously it was untested? I don't think the original > >>> code took account of endianness and that should be fixed, but feels > >>> like it should be in a separate patch set? > >> > >> They are failing because something like the topmost one or two patches > >> as in this branch here are missing for a big endian host: > >> > >> https://github.com/stefanberger/qemu-tpm/tree/joelle.v5%2B2nd_registers > > > > Right, but is this issue new due to the patchset? i.e. if just the > > Yes, it is due to this patchset. The reason is that CRB switched to a > ROMD interface where the fact that the MMIO registers are little endian > got lost for existing x86_64 support. > > > tests were added without the other patches, would they still fail? If > > so, I think the fixes should be in another patchset while we disable > > them in this patchset. I see, how do you want to best integrate your changes? Do you want me to squash your changes into the patch that introduces the code? Or do you want them to be separate commits?
On 11/24/23 21:39, Joelle van Dyne wrote: > On Fri, Nov 24, 2023 at 8:26 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 11/24/23 11:21, Joelle van Dyne wrote: >>> On Fri, Nov 24, 2023 at 8:17 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>> >>>> >>>> >>>> On 11/23/23 19:56, Joelle van Dyne wrote: >>>>> On Tue, Nov 14, 2023 at 4:12 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 11/14/23 16:05, Stefan Berger wrote: >>>>>>> >>>>>>> >>>>>>> On 11/14/23 13:03, Stefan Berger wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 11/14/23 04:36, Marc-André Lureau wrote: >>>>>>>>> Hi >>>>>>>>> >>>>>>>>> On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne <j@getutm.app> wrote: >>>>>>>>>> >>>>>>>>>> Signed-off-by: Joelle van Dyne <j@getutm.app> >>>>>>>>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >>>>>>>>> >>>>>>>>> nit: you also added tests for x86, could be a different patch? >>>>>>>>> >>>>>>>>> For arm, the test fails until next patch with: >>>>>>>>> >>>>>>>>> # starting QEMU: exec ./qemu-system-aarch64 -qtest >>>>>>>>> unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev >>>>>>>>> socket,path=/tmp/qtest-991279.qmp,id=char0 -mon >>>>>>>>> chardev=char0,mode=control -display none -audio none -machine virt >>>>>>>>> -accel tcg -nodefaults -nographic -drive >>>>>>>>> if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on >>>>>>>>> -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on >>>>>>>>> -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 >>>>>>>>> -cpu cortex-a57 -chardev >>>>>>>>> socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock >>>>>>>>> -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev >>>>>>>>> -accel qtest >>>>>>>>> Warning! zero length expected file >>>>>>>>> 'tests/data/acpi/virt/TPM2.crb-device.tpm2' >>>>>>>>> Warning! zero length expected file >>>>>>>>> 'tests/data/acpi/virt/DSDT.crb-device.tpm2' >>>>>>>>> acpi-test: Warning! binary file mismatch. Actual >>>>>>>>> [aml:/tmp/aml-GO4ME2], Expected >>>>>>>>> [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. >>>>>>>>> See source file tests/qtest/bios-tables-test.c for instructions on how >>>>>>>>> to update expected files. >>>>>>>>> acpi-test: Warning! binary file mismatch. Actual >>>>>>>>> [aml:/tmp/aml-6N4ME2], Expected >>>>>>>>> [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. >>>>>>>>> See source file tests/qtest/bios-tables-test.c for instructions on how >>>>>>>>> to update expected files. >>>>>>>>> to see ASL diff between mismatched files install IASL, rebuild QEMU >>>>>>>>> from scratch and re-run tests with V=1 environment variable set** >>>>>>>>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion >>>>>>>>> failed: (all_tables_match) >>>>>>>>> not ok /aarch64/acpi/virt/tpm2-crb - >>>>>>>>> ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion >>>>>>>>> failed: (all_tables_match) >>>>>>>>> Bail out! >>>>>>>>> qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: >>>>>>>>> Resource temporarily unavailable >>>>>>>>> Unexpected error in qio_channel_socket_writev() at >>>>>>>>> ../io/channel-socket.c:622: >>>>>>>>> /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable >>>>>>>>> to write to socket: Bad file descriptor >>>>>>>>> >>>>>>>> >>>>>>>> Travis testing on s390x I see the following failures for this patchset >>>>>>>> (search for 'ERROR'): >>>>>>>> >>>>>>>> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363 >>>>>>>> >>>>>>>> Summary of Failures: >>>>>>>> >>>>>>>> 134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test >>>>>>>> ERROR 0.70s killed by signal 6 SIGABRT >>>>>>>> >>>>>>>> 219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test >>>>>>>> ERROR 0.88s killed by signal 6 SIGABRT >>>>>>>> >>>>>>>> >>>>>>>> Summary of Failures: >>>>>>>> >>>>>>>> 271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test >>>>>>>> ERROR 0.59s killed by signal 6 SIGABRT >>>>>>>> >>>>>>>> >>>>>>>> My guess is it's an endianess issue on big endian machines due to >>>>>>>> reading from the ROM device where we lost the .endianess: >>>>>>>> >>>>>>>> +const MemoryRegionOps tpm_crb_memory_ops = { >>>>>>>> + .read = tpm_crb_mmio_read, >>>>>>>> + .write = tpm_crb_mmio_write, >>>>>>>> + .endianness = DEVICE_LITTLE_ENDIAN, >>>>>>>> + .valid = { >>>>>>>> + .min_access_size = 1, >>>>>>>> + .max_access_size = 4, >>>>>>>> + }, >>>>>>>> +}; >>>>>>>> >>>>>>> >>>>>>> I think we need a 2nd set of registers to support the endianess >>>>>>> conversion. It's not exactly nice, though. Basically the saved_regs >>>>>>> could be used for this directly, even though I did not do that but >>>>>>> introduced n_regs: >>>>>>> https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91 >>>>>>> >>>>>>> This patch allows the tests on s390x to run farther but the execution of >>>>>>> the command doesn't seem to work maybe due to command data that were >>>>>>> also written in wrong endianess. I don't know. I would have to get >>>>>>> access to a big endian / s390 machine to be able to fix it. >>>>>>> >>>>>>> >>>>>> >>>>>> The latest version now passes on Travis s390x: >>>>>> https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267245220 >>>>>> >>>>> >>>>> Are the tests failing on S390X due to the added code or are they >>>>> failing because previously it was untested? I don't think the original >>>>> code took account of endianness and that should be fixed, but feels >>>>> like it should be in a separate patch set? >>>> >>>> They are failing because something like the topmost one or two patches >>>> as in this branch here are missing for a big endian host: >>>> >>>> https://github.com/stefanberger/qemu-tpm/tree/joelle.v5%2B2nd_registers >>> >>> Right, but is this issue new due to the patchset? i.e. if just the >> >> Yes, it is due to this patchset. The reason is that CRB switched to a >> ROMD interface where the fact that the MMIO registers are little endian >> got lost for existing x86_64 support. >> >>> tests were added without the other patches, would they still fail? If >>> so, I think the fixes should be in another patchset while we disable >>> them in this patchset. > > I see, how do you want to best integrate your changes? Do you want me > to squash your changes into the patch that introduces the code? Or do > you want them to be separate commits? I think squashing them in would be the right thing to do. You may want to append _LE to the macros in the 2nd patch if you take them. Stefan
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index 71af5cf69f..bb4ebf00c1 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -1447,6 +1447,28 @@ static void test_acpi_piix4_tcg_numamem(void) uint64_t tpm_tis_base_addr; +static test_data tcg_tpm_test_data(const char *machine) +{ + if (g_strcmp0(machine, "virt") == 0) { + test_data data = { + .machine = "virt", + .tcg_only = true, + .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd", + .uefi_fl2 = "pc-bios/edk2-arm-vars.fd", + .cd = + "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2", + .ram_start = 0x40000000ULL, + .scan_len = 128ULL * 1024 * 1024, + }; + return data; + } else { + test_data data = { + .machine = machine, + }; + return data; + } +} + static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if, uint64_t base, enum TPMVersion tpm_version) { @@ -1454,7 +1476,7 @@ static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if, machine, tpm_if); char *tmp_path = g_dir_make_tmp(tmp_dir_name, NULL); TPMTestState test; - test_data data = {}; + test_data data = tcg_tpm_test_data(machine); GThread *thread; const char *suffix = tpm_version == TPM_VERSION_2_0 ? "tpm2" : "tpm12"; char *args, *variant = g_strdup_printf(".%s.%s", tpm_if, suffix); @@ -1474,13 +1496,14 @@ static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if, thread = g_thread_new(NULL, tpm_emu_ctrl_thread, &test); tpm_emu_test_wait_cond(&test); - data.machine = machine; data.variant = variant; args = g_strdup_printf( + " %s" " -chardev socket,id=chr,path=%s" " -tpmdev emulator,id=dev,chardev=chr" " -device tpm-%s,tpmdev=dev", + g_strcmp0(machine, "virt") == 0 ? "-cpu cortex-a57" : "", test.addr->u.q_unix.path, tpm_if); test_acpi_one(args, &data); @@ -1506,6 +1529,16 @@ static void test_acpi_q35_tcg_tpm12_tis(void) test_acpi_tcg_tpm("q35", "tis", 0xFED40000, TPM_VERSION_1_2); } +static void test_acpi_q35_tcg_tpm2_crb(void) +{ + test_acpi_tcg_tpm("q35", "crb", 0xFED40000, TPM_VERSION_2_0); +} + +static void test_acpi_virt_tcg_tpm2_crb(void) +{ + test_acpi_tcg_tpm("virt", "crb-device", 0xFED40000, TPM_VERSION_2_0); +} + static void test_acpi_tcg_dimm_pxm(const char *machine) { test_data data = {}; @@ -2212,6 +2245,9 @@ int main(int argc, char *argv[]) qtest_add_func("acpi/q35/tpm12-tis", test_acpi_q35_tcg_tpm12_tis); } + if (tpm_model_is_available("-machine q35", "tpm-crb")) { + qtest_add_func("acpi/q35/tpm2-crb", test_acpi_q35_tcg_tpm2_crb); + } qtest_add_func("acpi/q35/bridge", test_acpi_q35_tcg_bridge); qtest_add_func("acpi/q35/no-acpi-hotplug", test_acpi_q35_tcg_no_acpi_hotplug); @@ -2301,6 +2337,9 @@ int main(int argc, char *argv[]) qtest_add_func("acpi/virt/viot", test_acpi_virt_viot); } } + if (tpm_model_is_available("-machine virt", "tpm-crb")) { + qtest_add_func("acpi/virt/tpm2-crb", test_acpi_virt_tcg_tpm2_crb); + } } ret = g_test_run(); boot_sector_cleanup(disk);