Hi Drew,
On 9/6/21 11:58 AM, Andrew Jones wrote:
> On Mon, Sep 06, 2021 at 11:20:31AM +0100, Alexandru Elisei wrote:
>> Hi Drew,
>>
>> Sorry for taking so long to reply, been busy with other things.
>>
>> On 7/12/21 5:36 PM, Andrew Jones wrote:
>>> On Fri, Jul 02, 2021 at 05:31:18PM +0100, Alexandru Elisei wrote:
>>>> The arm64 tests can be run under kvmtool, which doesn't emulate a
>>>> chr-testdev device. In preparation for adding run script support for
>>>> kvmtool, print the test exit status so the scripts can pick it up and
>>>> correctly mark the test as pass or fail.
>>>>
>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
>>>> ---
>>>> lib/chr-testdev.h | 1 +
>>>> lib/arm/io.c | 10 +++++++++-
>>>> lib/chr-testdev.c | 5 +++++
>>>> 3 files changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/chr-testdev.h b/lib/chr-testdev.h
>>>> index ffd9a851aa9b..09b4b424670e 100644
>>>> --- a/lib/chr-testdev.h
>>>> +++ b/lib/chr-testdev.h
>>>> @@ -11,4 +11,5 @@
>>>> */
>>>> extern void chr_testdev_init(void);
>>>> extern void chr_testdev_exit(int code);
>>>> +extern bool chr_testdev_available(void);
>>>> #endif
>>>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>>>> index 343e10822263..9e62b571a91b 100644
>>>> --- a/lib/arm/io.c
>>>> +++ b/lib/arm/io.c
>>>> @@ -125,7 +125,15 @@ extern void halt(int code);
>>>>
>>>> void exit(int code)
>>>> {
>>>> - chr_testdev_exit(code);
>>>> + if (chr_testdev_available()) {
>>>> + chr_testdev_exit(code);
>>> chr_testdev_exit() already has a 'if !vcon goto out' in it, so you can
>>> just call it unconditionally. No need for chr_testdev_available().
>> I'm not sure what you mean. There has to be a way to check if chr-testdev is
>> available, and if it's not present on the system, to print the EXIT: STATUS
>> message, and vcon is static in chr-testdev.c.
>>
>> Are you suggesting that we move the message to chr_testdev_exit(code)?
> I'm saying you can unconditionally call chr_testdev_exit(), because it
> only conditionally does anything, and on the same condition that you're
> adding (vcon != NULL).
>
> $ /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,accel=tcg -cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/selftest.flat
> ABORT: selftest: no test specified
> SUMMARY: 0 tests
> $ echo $?
> 127
> $ /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,accel=tcg -cpu cortex-a57 -display none -serial stdio -kernel arm/selftest.flat
> ABORT: selftest: no test specified
> SUMMARY: 0 tests
> $ echo $?
> 0
>
> See, no explosions when the device is removed. Just a lack of return code.
Yup, this makes sense, this is exactly what happens today with kvmtool.
>
> Also, since chr_testdev_exit() exits, any calls after it won't happen. So
> the exit print statement doesn't need to be in an else clause. That said,
> I think the print statement should come first in order to also put it in
> the qemu output logs. We might as well have consistent output between qemu
> and kvmtool.
Makes sense, I'll move the printf before chr_testdev_exit(). Thanks for the quick
reply!
Thanks,
Alex
>
> Thanks,
> drew
>
>
>> Thanks,
>>
>> Alex
>>
>>>> + } else {
>>>> + /*
>>>> + * Print the test return code in the format used by chr-testdev
>>>> + * so the runner script can parse it.
>>>> + */
>>>> + printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1);
>>>> + }
>>>> psci_system_off();
>>>> halt(code);
>>>> __builtin_unreachable();
>>>> diff --git a/lib/chr-testdev.c b/lib/chr-testdev.c
>>>> index b3c641a833fe..301e73a6c064 100644
>>>> --- a/lib/chr-testdev.c
>>>> +++ b/lib/chr-testdev.c
>>>> @@ -68,3 +68,8 @@ void chr_testdev_init(void)
>>>> in_vq = vqs[0];
>>>> out_vq = vqs[1];
>>>> }
>>>> +
>>>> +bool chr_testdev_available(void)
>>>> +{
>>>> + return vcon != NULL;
>>>> +}
>>>> --
>>>> 2.32.0
>>>>
>>> Thanks,
>>> drew
>>>
