On 4/26/21 12:15 PM, Alex Bennée wrote:
> Philippe Mathieu-Daudé <f4...@amsat.org> writes:
>> On 4/19/21 4:54 PM, Alex Bennée wrote:
>>> From: Bastian Koppelmann <kbast...@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.ben...@linaro.org>
>>> Signed-off-by: Bastian Koppelmann <kbast...@mail.uni-paderborn.de>
>>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
>>> Message-Id: <20210305170045.869437-4-kbast...@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.
