Re: [PATCH v1 13/25] hw/tricore: Add testdevice for tests in tests/tcg/

[Philippe Mathieu-Daudé]

On 4/26/21 12:15 PM, Alex Bennée wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> On 4/19/21 4:54 PM, Alex Bennée wrote:
>>> From: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
>>>
>>> this device is used to verify the correctness of regression tests by
>>> allowing guests to write their exit status to this device. This is then
>>> used by qemu to exit using the written status.
>>>
>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>> Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Message-Id: <20210305170045.869437-4-kbastian@mail.uni-paderborn.de>
>>> ---
>>>  include/hw/tricore/tricore_testdevice.h | 38 ++++++++++++
>>>  hw/tricore/tricore_testboard.c          |  8 +++
>>>  hw/tricore/tricore_testdevice.c         | 82 +++++++++++++++++++++++++
>>>  hw/tricore/meson.build                  |  1 +
>>>  4 files changed, 129 insertions(+)
>>>  create mode 100644 include/hw/tricore/tricore_testdevice.h
>>>  create mode 100644 hw/tricore/tricore_testdevice.c
>>
>>> +#include "hw/tricore/tricore_testdevice.h"
>>> +
>>> +static void tricore_testdevice_write(void *opaque, hwaddr offset,
>>> +                                      uint64_t value, unsigned size)
>>> +{
>>> +    exit(value);
>>
>>    ->  qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>>
>> I'd rather use a 2 steps check of value such watchdog devices do
>> (to be sure the guest is still in control and isn't nut).
> 
> This isn't any different to what we do for TARGET_SYS_EXIT_EXTENDED or
> the various other semihosting exits. Maybe we could do a better job of
> flagging that these devices (or features) give the guest an avenue to
> cause QEMU to shutdown but none of these are enabled by default.

My concern here is the console being modified and not being restored
correctly. Maybe not a problem for the current test, but could happens
later with more tests added, or the device re-used elsewhere.

This is a one-line change, which can be done later.

This concert also applies to the semihosting exit(). Can be done later too.

> 
>>
>> A general comments, all targets require a such test feature,
>> so we should have a generic user-creatable sysbus-testdev for that.
> 
> We also have the isa-debug-exit device used by x86. I believe there is
> also a PCI device (pci-testdev) used to submit error-exit results for
> kvm-unit-tests.
> 
> I'm all for modelling a cleaner abstraction that could be used by all
> these methods and avoiding multiple exit paths but I don't want to hold
> up Bastian's tests to a higher standard without addressing the other
> cases. In the meantime given it improves the testing situation for
> Tricore I don't think it's a major issue.

Agreed, not a major issue, my comment are not blocking this patch.

Thanks,

Phil.