Re: [PATCH v5 12/14] tests: acpi: implement TPM CRB tests for ARM virt

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

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