| Message ID | 20210419145435.14083-14-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | testing/next (hexagon/tricore/test cc) | expand |
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). A general comments, all targets require a such test feature, so we should have a generic user-creatable sysbus-testdev for that. Regards, Phil.
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. > > 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. > > Regards, > > Phil. -- Alex Bennée
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.
diff --git a/include/hw/tricore/tricore_testdevice.h b/include/hw/tricore/tricore_testdevice.h new file mode 100644 index 0000000000..2c56c51bcb --- /dev/null +++ b/include/hw/tricore/tricore_testdevice.h @@ -0,0 +1,38 @@ +/* + * Copyright (c) 2018-2021 Bastian Koppelmann Paderborn University + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see <http://www.gnu.org/licenses/>. + */ + + +#ifndef HW_TRICORE_TESTDEV_H +#define HW_TRICORE_TESTDEV_H + +#include "hw/sysbus.h" +#include "hw/hw.h" + +#define TYPE_TRICORE_TESTDEVICE "tricore_testdevice" +#define TRICORE_TESTDEVICE(obj) \ + OBJECT_CHECK(TriCoreTestDeviceState, (obj), TYPE_TRICORE_TESTDEVICE) + +typedef struct { + /* <private> */ + SysBusDevice parent_obj; + + /* <public> */ + MemoryRegion iomem; + +} TriCoreTestDeviceState; + +#endif diff --git a/hw/tricore/tricore_testboard.c b/hw/tricore/tricore_testboard.c index 12ea1490fd..9791986a3e 100644 --- a/hw/tricore/tricore_testboard.c +++ b/hw/tricore/tricore_testboard.c @@ -28,6 +28,7 @@ #include "exec/address-spaces.h" #include "elf.h" #include "hw/tricore/tricore.h" +#include "hw/tricore/tricore_testdevice.h" #include "qemu/error-report.h" @@ -57,6 +58,7 @@ static void tricore_testboard_init(MachineState *machine, int board_id) { TriCoreCPU *cpu; CPUTriCoreState *env; + TriCoreTestDeviceState *test_dev; MemoryRegion *sysmem = get_system_memory(); MemoryRegion *ext_cram = g_new(MemoryRegion, 1); @@ -88,6 +90,12 @@ static void tricore_testboard_init(MachineState *machine, int board_id) memory_region_add_subregion(sysmem, 0xf0050000, pcp_data); memory_region_add_subregion(sysmem, 0xf0060000, pcp_text); + test_dev = g_new(TriCoreTestDeviceState, 1); + object_initialize(test_dev, sizeof(TriCoreTestDeviceState), + TYPE_TRICORE_TESTDEVICE); + memory_region_add_subregion(sysmem, 0xf0000000, &test_dev->iomem); + + tricoretb_binfo.ram_size = machine->ram_size; tricoretb_binfo.kernel_filename = machine->kernel_filename; diff --git a/hw/tricore/tricore_testdevice.c b/hw/tricore/tricore_testdevice.c new file mode 100644 index 0000000000..a1563aa568 --- /dev/null +++ b/hw/tricore/tricore_testdevice.c @@ -0,0 +1,82 @@ +/* + * Copyright (c) 2018-2021 Bastian Koppelmann Paderborn University + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see <http://www.gnu.org/licenses/>. + */ + +#include "qemu/osdep.h" +#include "hw/sysbus.h" +#include "hw/qdev-properties.h" +#include "hw/tricore/tricore_testdevice.h" + +static void tricore_testdevice_write(void *opaque, hwaddr offset, + uint64_t value, unsigned size) +{ + exit(value); +} + +static uint64_t tricore_testdevice_read(void *opaque, hwaddr offset, + unsigned size) +{ + return 0xdeadbeef; +} + +static void tricore_testdevice_reset(DeviceState *dev) +{ +} + +static const MemoryRegionOps tricore_testdevice_ops = { + .read = tricore_testdevice_read, + .write = tricore_testdevice_write, + .valid = { + .min_access_size = 4, + .max_access_size = 4, + }, + .endianness = DEVICE_NATIVE_ENDIAN, +}; + +static void tricore_testdevice_init(Object *obj) +{ + TriCoreTestDeviceState *s = TRICORE_TESTDEVICE(obj); + /* map memory */ + memory_region_init_io(&s->iomem, OBJECT(s), &tricore_testdevice_ops, s, + "tricore_testdevice", 0x4); +} + +static Property tricore_testdevice_properties[] = { + DEFINE_PROP_END_OF_LIST() +}; + +static void tricore_testdevice_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + device_class_set_props(dc, tricore_testdevice_properties); + dc->reset = tricore_testdevice_reset; +} + +static const TypeInfo tricore_testdevice_info = { + .name = TYPE_TRICORE_TESTDEVICE, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(TriCoreTestDeviceState), + .instance_init = tricore_testdevice_init, + .class_init = tricore_testdevice_class_init, +}; + +static void tricore_testdevice_register_types(void) +{ + type_register_static(&tricore_testdevice_info); +} + +type_init(tricore_testdevice_register_types) diff --git a/hw/tricore/meson.build b/hw/tricore/meson.build index 77ff6fd137..47e36bb077 100644 --- a/hw/tricore/meson.build +++ b/hw/tricore/meson.build @@ -1,5 +1,6 @@ tricore_ss = ss.source_set() tricore_ss.add(when: 'CONFIG_TRICORE', if_true: files('tricore_testboard.c')) +tricore_ss.add(when: 'CONFIG_TRICORE', if_true: files('tricore_testdevice.c')) tricore_ss.add(when: 'CONFIG_TRIBOARD', if_true: files('triboard.c')) tricore_ss.add(when: 'CONFIG_TC27X_SOC', if_true: files('tc27x_soc.c'))