diff mbox series

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

Message ID 20210419145435.14083-14-alex.bennee@linaro.org (mailing list archive)
State New, archived
Headers show
Series testing/next (hexagon/tricore/test cc) | expand

Commit Message

Alex Bennée April 19, 2021, 2:54 p.m. UTC
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

Comments

Philippe Mathieu-Daudé April 26, 2021, 9:37 a.m. UTC | #1
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.
Alex Bennée April 26, 2021, 10:15 a.m. UTC | #2
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.
Philippe Mathieu-Daudé April 26, 2021, 12:03 p.m. UTC | #3
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 mbox series

Patch

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'))