* [PATCH v2 1/6] hw/input/stellaris_input: Rename to stellaris_gamepad
2023-10-30 11:47 [PATCH v2 0/6] arm/stellaris: convert gamepad input device to qdev Peter Maydell
@ 2023-10-30 11:47 ` Peter Maydell
2023-10-30 11:47 ` [PATCH v2 2/6] hw/input/stellaris_gamepad: Rename structs to our usual convention Peter Maydell
` (4 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2023-10-30 11:47 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Philippe Mathieu-Daudé, Kevin Wolf
This source file implements a stellaris gamepad device; rename
it so that it is a closer match to the device name.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/input/{gamepad.h => stellaris_gamepad.h} | 6 +++---
hw/arm/stellaris.c | 2 +-
hw/input/{stellaris_input.c => stellaris_gamepad.c} | 2 +-
hw/arm/Kconfig | 2 +-
hw/input/Kconfig | 2 +-
hw/input/meson.build | 2 +-
6 files changed, 8 insertions(+), 8 deletions(-)
rename include/hw/input/{gamepad.h => stellaris_gamepad.h} (77%)
rename hw/input/{stellaris_input.c => stellaris_gamepad.c} (98%)
diff --git a/include/hw/input/gamepad.h b/include/hw/input/stellaris_gamepad.h
similarity index 77%
rename from include/hw/input/gamepad.h
rename to include/hw/input/stellaris_gamepad.h
index 6f6aa2406aa..23cfd3c95f3 100644
--- a/include/hw/input/gamepad.h
+++ b/include/hw/input/stellaris_gamepad.h
@@ -8,11 +8,11 @@
* See the COPYING file in the top-level directory.
*/
-#ifndef HW_INPUT_GAMEPAD_H
-#define HW_INPUT_GAMEPAD_H
+#ifndef HW_INPUT_STELLARIS_GAMEPAD_H
+#define HW_INPUT_STELLARIS_GAMEPAD_H
-/* stellaris_input.c */
+/* stellaris_gamepad.c */
void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode);
#endif
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index aa5b0ddfaa5..96585dd7106 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -23,7 +23,7 @@
#include "sysemu/sysemu.h"
#include "hw/arm/armv7m.h"
#include "hw/char/pl011.h"
-#include "hw/input/gamepad.h"
+#include "hw/input/stellaris_gamepad.h"
#include "hw/irq.h"
#include "hw/watchdog/cmsdk-apb-watchdog.h"
#include "migration/vmstate.h"
diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_gamepad.c
similarity index 98%
rename from hw/input/stellaris_input.c
rename to hw/input/stellaris_gamepad.c
index e6ee5e11f1b..3bab557cab3 100644
--- a/hw/input/stellaris_input.c
+++ b/hw/input/stellaris_gamepad.c
@@ -8,7 +8,7 @@
*/
#include "qemu/osdep.h"
-#include "hw/input/gamepad.h"
+#include "hw/input/stellaris_gamepad.h"
#include "hw/irq.h"
#include "migration/vmstate.h"
#include "ui/console.h"
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 7e683484405..841f3131ea5 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -291,7 +291,7 @@ config STELLARIS
select SSD0303 # OLED display
select SSD0323 # OLED display
select SSI_SD
- select STELLARIS_INPUT
+ select STELLARIS_GAMEPAD
select STELLARIS_ENET # ethernet
select STELLARIS_GPTM # general purpose timer module
select UNIMP
diff --git a/hw/input/Kconfig b/hw/input/Kconfig
index 55865bb3869..f86e98c8293 100644
--- a/hw/input/Kconfig
+++ b/hw/input/Kconfig
@@ -20,7 +20,7 @@ config PL050
config PS2
bool
-config STELLARIS_INPUT
+config STELLARIS_GAMEPAD
bool
config TSC2005
diff --git a/hw/input/meson.build b/hw/input/meson.build
index c0d44821800..640556bbbcc 100644
--- a/hw/input/meson.build
+++ b/hw/input/meson.build
@@ -5,7 +5,7 @@ system_ss.add(when: 'CONFIG_LM832X', if_true: files('lm832x.c'))
system_ss.add(when: 'CONFIG_PCKBD', if_true: files('pckbd.c'))
system_ss.add(when: 'CONFIG_PL050', if_true: files('pl050.c'))
system_ss.add(when: 'CONFIG_PS2', if_true: files('ps2.c'))
-system_ss.add(when: 'CONFIG_STELLARIS_INPUT', if_true: files('stellaris_input.c'))
+system_ss.add(when: 'CONFIG_STELLARIS_GAMEPAD', if_true: files('stellaris_gamepad.c'))
system_ss.add(when: 'CONFIG_TSC2005', if_true: files('tsc2005.c'))
system_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: files('virtio-input.c'))
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 2/6] hw/input/stellaris_gamepad: Rename structs to our usual convention
2023-10-30 11:47 [PATCH v2 0/6] arm/stellaris: convert gamepad input device to qdev Peter Maydell
2023-10-30 11:47 ` [PATCH v2 1/6] hw/input/stellaris_input: Rename to stellaris_gamepad Peter Maydell
@ 2023-10-30 11:47 ` Peter Maydell
2023-10-30 11:47 ` [PATCH v2 3/6] qdev: Add qdev_prop_set_array() Peter Maydell
` (3 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2023-10-30 11:47 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Philippe Mathieu-Daudé, Kevin Wolf
Rename the structs in stellaris_gamepad.c to our now-standard
CamelCase convention.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/input/stellaris_gamepad.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
index 3bab557cab3..377101a4035 100644
--- a/hw/input/stellaris_gamepad.c
+++ b/hw/input/stellaris_gamepad.c
@@ -17,17 +17,17 @@ typedef struct {
qemu_irq irq;
int keycode;
uint8_t pressed;
-} gamepad_button;
+} StellarisGamepadButton;
typedef struct {
- gamepad_button *buttons;
+ StellarisGamepadButton *buttons;
int num_buttons;
int extension;
-} gamepad_state;
+} StellarisGamepad;
static void stellaris_gamepad_put_key(void * opaque, int keycode)
{
- gamepad_state *s = (gamepad_state *)opaque;
+ StellarisGamepad *s = (StellarisGamepad *)opaque;
int i;
int down;
@@ -55,7 +55,7 @@ static const VMStateDescription vmstate_stellaris_button = {
.version_id = 0,
.minimum_version_id = 0,
.fields = (VMStateField[]) {
- VMSTATE_UINT8(pressed, gamepad_button),
+ VMSTATE_UINT8(pressed, StellarisGamepadButton),
VMSTATE_END_OF_LIST()
}
};
@@ -65,11 +65,11 @@ static const VMStateDescription vmstate_stellaris_gamepad = {
.version_id = 2,
.minimum_version_id = 2,
.fields = (VMStateField[]) {
- VMSTATE_INT32(extension, gamepad_state),
- VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, gamepad_state,
+ VMSTATE_INT32(extension, StellarisGamepad),
+ VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, StellarisGamepad,
num_buttons,
vmstate_stellaris_button,
- gamepad_button),
+ StellarisGamepadButton),
VMSTATE_END_OF_LIST()
}
};
@@ -77,11 +77,11 @@ static const VMStateDescription vmstate_stellaris_gamepad = {
/* Returns an array of 5 output slots. */
void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode)
{
- gamepad_state *s;
+ StellarisGamepad *s;
int i;
- s = g_new0(gamepad_state, 1);
- s->buttons = g_new0(gamepad_button, n);
+ s = g_new0(StellarisGamepad, 1);
+ s->buttons = g_new0(StellarisGamepadButton, n);
for (i = 0; i < n; i++) {
s->buttons[i].irq = irq[i];
s->buttons[i].keycode = keycode[i];
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 3/6] qdev: Add qdev_prop_set_array()
2023-10-30 11:47 [PATCH v2 0/6] arm/stellaris: convert gamepad input device to qdev Peter Maydell
2023-10-30 11:47 ` [PATCH v2 1/6] hw/input/stellaris_input: Rename to stellaris_gamepad Peter Maydell
2023-10-30 11:47 ` [PATCH v2 2/6] hw/input/stellaris_gamepad: Rename structs to our usual convention Peter Maydell
@ 2023-10-30 11:47 ` Peter Maydell
2023-10-30 11:48 ` [PATCH v2 4/6] hw/input/stellaris_gamepad: Remove StellarisGamepadButton struct Peter Maydell
` (2 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2023-10-30 11:47 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Philippe Mathieu-Daudé, Kevin Wolf
From: Kevin Wolf <kwolf@redhat.com>
Instead of exposing the ugly hack of how we represent arrays in qdev (a
static "foo-len" property and after it is set, dynamically created
"foo[i]" properties) to boards, add an interface that allows setting the
whole array at once.
Once all internal users of devices with array properties have been
converted to use this function, we can change the implementation to move
away from this hack.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/qdev-properties.h | 3 +++
hw/core/qdev-properties.c | 21 +++++++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index e1df08876c6..7fa2fdb7c94 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -206,6 +206,9 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char *name,
const uint8_t *value);
void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
+/* Takes ownership of @values */
+void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values);
+
void *object_field_prop_ptr(Object *obj, Property *prop);
void qdev_prop_register_global(GlobalProperty *prop);
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 357b8761b54..950ef48e013 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -3,12 +3,14 @@
#include "qapi/error.h"
#include "qapi/qapi-types-misc.h"
#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qlist.h"
#include "qemu/ctype.h"
#include "qemu/error-report.h"
#include "qapi/visitor.h"
#include "qemu/units.h"
#include "qemu/cutils.h"
#include "qdev-prop-internal.h"
+#include "qom/qom-qobject.h"
void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
Error **errp)
@@ -739,6 +741,25 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
&error_abort);
}
+void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
+{
+ const QListEntry *entry;
+ g_autofree char *prop_len = g_strdup_printf("len-%s", name);
+ uint32_t i = 0;
+
+ object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
+ &error_abort);
+
+ QLIST_FOREACH_ENTRY(values, entry) {
+ g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
+ object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
+ &error_abort);
+ i++;
+ }
+
+ qobject_unref(values);
+}
+
static GPtrArray *global_props(void)
{
static GPtrArray *gp;
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 4/6] hw/input/stellaris_gamepad: Remove StellarisGamepadButton struct
2023-10-30 11:47 [PATCH v2 0/6] arm/stellaris: convert gamepad input device to qdev Peter Maydell
` (2 preceding siblings ...)
2023-10-30 11:47 ` [PATCH v2 3/6] qdev: Add qdev_prop_set_array() Peter Maydell
@ 2023-10-30 11:48 ` Peter Maydell
2023-10-30 11:48 ` [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev Peter Maydell
2023-10-30 11:48 ` [PATCH v2 6/6] hw/input/stellaris_gamepad: Convert to qemu_input_handler_register() Peter Maydell
5 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2023-10-30 11:48 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Philippe Mathieu-Daudé, Kevin Wolf
Currently for each button on the device we have a
StellarisGamepadButton struct which has the irq, keycode and pressed
state for it. When we convert to qdev, the qdev property and GPIO
APIs are going to require that we have separate arrays for the irqs
and keycodes. Convert from array-of-structs to three separate arrays
in preparation.
This is a migration compatibility break for the stellaris boards
(lm3s6965evb, lm3s811evb).
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
--
v1=>v2: mention migration compat break in commit message;
bump version fields in vmstate
---
hw/input/stellaris_gamepad.c | 47 ++++++++++++------------------------
1 file changed, 16 insertions(+), 31 deletions(-)
diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
index 377101a4035..82ddc47a26d 100644
--- a/hw/input/stellaris_gamepad.c
+++ b/hw/input/stellaris_gamepad.c
@@ -14,15 +14,11 @@
#include "ui/console.h"
typedef struct {
- qemu_irq irq;
- int keycode;
- uint8_t pressed;
-} StellarisGamepadButton;
-
-typedef struct {
- StellarisGamepadButton *buttons;
- int num_buttons;
+ uint32_t num_buttons;
int extension;
+ qemu_irq *irqs;
+ uint32_t *keycodes;
+ uint8_t *pressed;
} StellarisGamepad;
static void stellaris_gamepad_put_key(void * opaque, int keycode)
@@ -40,36 +36,23 @@ static void stellaris_gamepad_put_key(void * opaque, int keycode)
keycode = (keycode & 0x7f) | s->extension;
for (i = 0; i < s->num_buttons; i++) {
- if (s->buttons[i].keycode == keycode
- && s->buttons[i].pressed != down) {
- s->buttons[i].pressed = down;
- qemu_set_irq(s->buttons[i].irq, down);
+ if (s->keycodes[i] == keycode && s->pressed[i] != down) {
+ s->pressed[i] = down;
+ qemu_set_irq(s->irqs[i], down);
}
}
s->extension = 0;
}
-static const VMStateDescription vmstate_stellaris_button = {
- .name = "stellaris_button",
- .version_id = 0,
- .minimum_version_id = 0,
- .fields = (VMStateField[]) {
- VMSTATE_UINT8(pressed, StellarisGamepadButton),
- VMSTATE_END_OF_LIST()
- }
-};
-
static const VMStateDescription vmstate_stellaris_gamepad = {
.name = "stellaris_gamepad",
- .version_id = 2,
- .minimum_version_id = 2,
+ .version_id = 3,
+ .minimum_version_id = 3,
.fields = (VMStateField[]) {
VMSTATE_INT32(extension, StellarisGamepad),
- VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, StellarisGamepad,
- num_buttons,
- vmstate_stellaris_button,
- StellarisGamepadButton),
+ VMSTATE_VARRAY_UINT32(pressed, StellarisGamepad, num_buttons,
+ 0, vmstate_info_uint8, uint8_t),
VMSTATE_END_OF_LIST()
}
};
@@ -81,10 +64,12 @@ void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode)
int i;
s = g_new0(StellarisGamepad, 1);
- s->buttons = g_new0(StellarisGamepadButton, n);
+ s->irqs = g_new0(qemu_irq, n);
+ s->keycodes = g_new0(uint32_t, n);
+ s->pressed = g_new0(uint8_t, n);
for (i = 0; i < n; i++) {
- s->buttons[i].irq = irq[i];
- s->buttons[i].keycode = keycode[i];
+ s->irqs[i] = irq[i];
+ s->keycodes[i] = keycode[i];
}
s->num_buttons = n;
qemu_add_kbd_event_handler(stellaris_gamepad_put_key, s);
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev
2023-10-30 11:47 [PATCH v2 0/6] arm/stellaris: convert gamepad input device to qdev Peter Maydell
` (3 preceding siblings ...)
2023-10-30 11:48 ` [PATCH v2 4/6] hw/input/stellaris_gamepad: Remove StellarisGamepadButton struct Peter Maydell
@ 2023-10-30 11:48 ` Peter Maydell
2023-10-30 13:52 ` Philippe Mathieu-Daudé
2023-10-30 20:37 ` Mark Cave-Ayland
2023-10-30 11:48 ` [PATCH v2 6/6] hw/input/stellaris_gamepad: Convert to qemu_input_handler_register() Peter Maydell
5 siblings, 2 replies; 17+ messages in thread
From: Peter Maydell @ 2023-10-30 11:48 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Philippe Mathieu-Daudé, Kevin Wolf
Convert the hw/input/stellaris_input device to qdev.
The interface uses an array property for the board to specify the
keycodes to use, so the s->keycodes memory is now allocated by the
array-property machinery.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
v1->v2: drop private/public comment lines
---
include/hw/input/stellaris_gamepad.h | 22 ++++++++-
hw/arm/stellaris.c | 26 +++++++---
hw/input/stellaris_gamepad.c | 73 +++++++++++++++++++---------
3 files changed, 89 insertions(+), 32 deletions(-)
diff --git a/include/hw/input/stellaris_gamepad.h b/include/hw/input/stellaris_gamepad.h
index 23cfd3c95f3..6140b889a28 100644
--- a/include/hw/input/stellaris_gamepad.h
+++ b/include/hw/input/stellaris_gamepad.h
@@ -11,8 +11,26 @@
#ifndef HW_INPUT_STELLARIS_GAMEPAD_H
#define HW_INPUT_STELLARIS_GAMEPAD_H
+#include "hw/sysbus.h"
+#include "qom/object.h"
-/* stellaris_gamepad.c */
-void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode);
+/*
+ * QEMU interface:
+ * + QOM array property "keycodes": uint32_t QEMU keycodes to handle
+ * + unnamed GPIO outputs: one per keycode, in the same order as the
+ * "keycodes" array property entries; asserted when key is down
+ */
+
+#define TYPE_STELLARIS_GAMEPAD "stellaris-gamepad"
+OBJECT_DECLARE_SIMPLE_TYPE(StellarisGamepad, STELLARIS_GAMEPAD)
+
+struct StellarisGamepad {
+ SysBusDevice parent_obj;
+ uint32_t num_buttons;
+ qemu_irq *irqs;
+ uint32_t *keycodes;
+ uint8_t *pressed;
+ int extension;
+};
#endif
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 96585dd7106..707b0dae375 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -31,6 +31,7 @@
#include "hw/timer/stellaris-gptm.h"
#include "hw/qdev-clock.h"
#include "qom/object.h"
+#include "qapi/qmp/qlist.h"
#define GPIO_A 0
#define GPIO_B 1
@@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
}
if (board->peripherals & BP_GAMEPAD) {
- qemu_irq gpad_irq[5];
+ QList *gpad_keycode_list = qlist_new();
static const int gpad_keycode[5] = { 0xc8, 0xd0, 0xcb, 0xcd, 0x1d };
+ DeviceState *gpad;
- gpad_irq[0] = qemu_irq_invert(gpio_in[GPIO_E][0]); /* up */
- gpad_irq[1] = qemu_irq_invert(gpio_in[GPIO_E][1]); /* down */
- gpad_irq[2] = qemu_irq_invert(gpio_in[GPIO_E][2]); /* left */
- gpad_irq[3] = qemu_irq_invert(gpio_in[GPIO_E][3]); /* right */
- gpad_irq[4] = qemu_irq_invert(gpio_in[GPIO_F][1]); /* select */
+ gpad = qdev_new(TYPE_STELLARIS_GAMEPAD);
+ for (i = 0; i < ARRAY_SIZE(gpad_keycode); i++) {
+ qlist_append_int(gpad_keycode_list, gpad_keycode[i]);
+ }
+ qdev_prop_set_array(gpad, "keycodes", gpad_keycode_list);
+ sysbus_realize_and_unref(SYS_BUS_DEVICE(gpad), &error_fatal);
- stellaris_gamepad_init(5, gpad_irq, gpad_keycode);
+ qdev_connect_gpio_out(gpad, 0,
+ qemu_irq_invert(gpio_in[GPIO_E][0])); /* up */
+ qdev_connect_gpio_out(gpad, 1,
+ qemu_irq_invert(gpio_in[GPIO_E][1])); /* down */
+ qdev_connect_gpio_out(gpad, 2,
+ qemu_irq_invert(gpio_in[GPIO_E][2])); /* left */
+ qdev_connect_gpio_out(gpad, 3,
+ qemu_irq_invert(gpio_in[GPIO_E][3])); /* right */
+ qdev_connect_gpio_out(gpad, 4,
+ qemu_irq_invert(gpio_in[GPIO_F][1])); /* select */
}
for (i = 0; i < 7; i++) {
if (board->dc4 & (1 << i)) {
diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
index 82ddc47a26d..6ccf0e80adc 100644
--- a/hw/input/stellaris_gamepad.c
+++ b/hw/input/stellaris_gamepad.c
@@ -8,19 +8,13 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "hw/input/stellaris_gamepad.h"
#include "hw/irq.h"
+#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
#include "ui/console.h"
-typedef struct {
- uint32_t num_buttons;
- int extension;
- qemu_irq *irqs;
- uint32_t *keycodes;
- uint8_t *pressed;
-} StellarisGamepad;
-
static void stellaris_gamepad_put_key(void * opaque, int keycode)
{
StellarisGamepad *s = (StellarisGamepad *)opaque;
@@ -57,22 +51,55 @@ static const VMStateDescription vmstate_stellaris_gamepad = {
}
};
-/* Returns an array of 5 output slots. */
-void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode)
+static void stellaris_gamepad_realize(DeviceState *dev, Error **errp)
{
- StellarisGamepad *s;
- int i;
+ StellarisGamepad *s = STELLARIS_GAMEPAD(dev);
- s = g_new0(StellarisGamepad, 1);
- s->irqs = g_new0(qemu_irq, n);
- s->keycodes = g_new0(uint32_t, n);
- s->pressed = g_new0(uint8_t, n);
- for (i = 0; i < n; i++) {
- s->irqs[i] = irq[i];
- s->keycodes[i] = keycode[i];
+ if (s->num_buttons == 0) {
+ error_setg(errp, "keycodes property array must be set");
+ return;
}
- s->num_buttons = n;
- qemu_add_kbd_event_handler(stellaris_gamepad_put_key, s);
- vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
- &vmstate_stellaris_gamepad, s);
+
+ s->irqs = g_new0(qemu_irq, s->num_buttons);
+ s->pressed = g_new0(uint8_t, s->num_buttons);
+ qdev_init_gpio_out(dev, s->irqs, s->num_buttons);
+ qemu_add_kbd_event_handler(stellaris_gamepad_put_key, dev);
}
+
+static void stellaris_gamepad_reset_enter(Object *obj, ResetType type)
+{
+ StellarisGamepad *s = STELLARIS_GAMEPAD(obj);
+
+ memset(s->pressed, 0, s->num_buttons * sizeof(uint8_t));
+}
+
+static Property stellaris_gamepad_properties[] = {
+ DEFINE_PROP_ARRAY("keycodes", StellarisGamepad, num_buttons,
+ keycodes, qdev_prop_uint32, uint32_t),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void stellaris_gamepad_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ ResettableClass *rc = RESETTABLE_CLASS(klass);
+
+ rc->phases.enter = stellaris_gamepad_reset_enter;
+ dc->realize = stellaris_gamepad_realize;
+ dc->vmsd = &vmstate_stellaris_gamepad;
+ device_class_set_props(dc, stellaris_gamepad_properties);
+}
+
+static const TypeInfo stellaris_gamepad_info = {
+ .name = TYPE_STELLARIS_GAMEPAD,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(StellarisGamepad),
+ .class_init = stellaris_gamepad_class_init,
+};
+
+static void stellaris_gamepad_register_types(void)
+{
+ type_register_static(&stellaris_gamepad_info);
+}
+
+type_init(stellaris_gamepad_register_types);
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev
2023-10-30 11:48 ` [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev Peter Maydell
@ 2023-10-30 13:52 ` Philippe Mathieu-Daudé
2023-10-30 14:25 ` Markus Armbruster
2023-10-30 14:41 ` Peter Maydell
2023-10-30 20:37 ` Mark Cave-Ayland
1 sibling, 2 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-30 13:52 UTC (permalink / raw)
To: Peter Maydell, Markus Armbruster; +Cc: Kevin Wolf, qemu-arm, qemu-devel
Hi Peter,
Cc'ing Markus for QObject.
On 30/10/23 12:48, Peter Maydell wrote:
> Convert the hw/input/stellaris_input device to qdev.
>
> The interface uses an array property for the board to specify the
> keycodes to use, so the s->keycodes memory is now allocated by the
> array-property machinery.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> v1->v2: drop private/public comment lines
> ---
> include/hw/input/stellaris_gamepad.h | 22 ++++++++-
> hw/arm/stellaris.c | 26 +++++++---
> hw/input/stellaris_gamepad.c | 73 +++++++++++++++++++---------
> 3 files changed, 89 insertions(+), 32 deletions(-)
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 96585dd7106..707b0dae375 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -31,6 +31,7 @@
> #include "hw/timer/stellaris-gptm.h"
> #include "hw/qdev-clock.h"
> #include "qom/object.h"
> +#include "qapi/qmp/qlist.h"
>
> #define GPIO_A 0
> #define GPIO_B 1
> @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
> sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
> }
> if (board->peripherals & BP_GAMEPAD) {
> - qemu_irq gpad_irq[5];
> + QList *gpad_keycode_list = qlist_new();
I'm trying to understand better qlist_new(), but unfortunately there
is not much documentation. Looking at how the allocated list was
released, I found use of g_autoptr in tests/unit/check-qobject.c,
so I tried:
g_autoptr(QList) gpad_keycode_list = qlist_new();
But QEMU crashes elsewhere which seems unrelated:
* thread #2, stop reason = signal SIGABRT
* frame #0: 0x8b1eb11c libsystem_kernel.dylib`__pthread_kill + 8
frame #1: 0x8b222cc0 libsystem_pthread.dylib`pthread_kill + 288
frame #2: 0x8b132a50 libsystem_c.dylib`abort + 180
frame #3: 0x8b049b08 libsystem_malloc.dylib`malloc_vreport + 908
frame #4: 0x8b06924c libsystem_malloc.dylib`malloc_zone_error + 104
frame #5: 0x8b05b094
libsystem_malloc.dylib`nanov2_guard_corruption_detected + 44
frame #6: 0x8b05a2a8
libsystem_malloc.dylib`nanov2_allocate_outlined + 404
frame #7: 0x0201fdc0 libglib-2.0.0.dylib`g_malloc0 + 36
frame #8: 0x02007718 libglib-2.0.0.dylib`g_hash_table_setup_storage
+ 76
frame #9: 0x020076b0 libglib-2.0.0.dylib`g_hash_table_new_full + 96
frame #10: 0x003a9920 qemu-system-ppc`object_unref [inlined]
object_property_del_all(obj=0x42023e00) at object.c:635:34
frame #11: 0x003a9914 qemu-system-ppc`object_unref [inlined]
object_finalize(data=0x42023e00) at object.c:707:5
frame #12: 0x003a990c
qemu-system-ppc`object_unref(objptr=0x42023e00) at object.c:1216:9
frame #13: 0x00355114 qemu-system-ppc`address_space_dispatch_free
at physmem.c:1001:9
frame #14: 0x003550fc qemu-system-ppc`address_space_dispatch_free
at physmem.c:1010:9
frame #15: 0x003550e0
qemu-system-ppc`address_space_dispatch_free(d=0x000060000385d680) at
physmem.c:2473:5
frame #16: 0x00349438
qemu-system-ppc`flatview_destroy(view=0x000060000385d640) at memory.c:295:9
frame #17: 0x00524920
qemu-system-ppc`call_rcu_thread(opaque=<unavailable>) at rcu.c:301:13
frame #18: 0x0051c1f0
qemu-system-ppc`qemu_thread_start(args=<unavailable>) at
qemu-thread-posix.c:541:9
frame #19: 0x8b223034 libsystem_pthread.dylib`_pthread_start + 136
However when running 'make check-unit', qobject_is_equal_list_test()
is successful, so I'm a bit confused...
> static const int gpad_keycode[5] = { 0xc8, 0xd0, 0xcb, 0xcd, 0x1d };
> + DeviceState *gpad;
>
> - gpad_irq[0] = qemu_irq_invert(gpio_in[GPIO_E][0]); /* up */
> - gpad_irq[1] = qemu_irq_invert(gpio_in[GPIO_E][1]); /* down */
> - gpad_irq[2] = qemu_irq_invert(gpio_in[GPIO_E][2]); /* left */
> - gpad_irq[3] = qemu_irq_invert(gpio_in[GPIO_E][3]); /* right */
> - gpad_irq[4] = qemu_irq_invert(gpio_in[GPIO_F][1]); /* select */
> + gpad = qdev_new(TYPE_STELLARIS_GAMEPAD);
> + for (i = 0; i < ARRAY_SIZE(gpad_keycode); i++) {
> + qlist_append_int(gpad_keycode_list, gpad_keycode[i]);
> + }
> + qdev_prop_set_array(gpad, "keycodes", gpad_keycode_list);
> + sysbus_realize_and_unref(SYS_BUS_DEVICE(gpad), &error_fatal);
>
> - stellaris_gamepad_init(5, gpad_irq, gpad_keycode);
> + qdev_connect_gpio_out(gpad, 0,
> + qemu_irq_invert(gpio_in[GPIO_E][0])); /* up */
> + qdev_connect_gpio_out(gpad, 1,
> + qemu_irq_invert(gpio_in[GPIO_E][1])); /* down */
> + qdev_connect_gpio_out(gpad, 2,
> + qemu_irq_invert(gpio_in[GPIO_E][2])); /* left */
> + qdev_connect_gpio_out(gpad, 3,
> + qemu_irq_invert(gpio_in[GPIO_E][3])); /* right */
> + qdev_connect_gpio_out(gpad, 4,
> + qemu_irq_invert(gpio_in[GPIO_F][1])); /* select */
> }
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev
2023-10-30 13:52 ` Philippe Mathieu-Daudé
@ 2023-10-30 14:25 ` Markus Armbruster
2023-10-31 8:16 ` Philippe Mathieu-Daudé
2023-10-30 14:41 ` Peter Maydell
1 sibling, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2023-10-30 14:25 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Markus Armbruster, Kevin Wolf, qemu-arm,
qemu-devel
Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> Hi Peter,
>
> Cc'ing Markus for QObject.
>
> On 30/10/23 12:48, Peter Maydell wrote:
>> Convert the hw/input/stellaris_input device to qdev.
>> The interface uses an array property for the board to specify the
>> keycodes to use, so the s->keycodes memory is now allocated by the
>> array-property machinery.
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> v1->v2: drop private/public comment lines
>> ---
>> include/hw/input/stellaris_gamepad.h | 22 ++++++++-
>> hw/arm/stellaris.c | 26 +++++++---
>> hw/input/stellaris_gamepad.c | 73 +++++++++++++++++++---------
>> 3 files changed, 89 insertions(+), 32 deletions(-)
>
>
>> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
>> index 96585dd7106..707b0dae375 100644
>> --- a/hw/arm/stellaris.c
>> +++ b/hw/arm/stellaris.c
>> @@ -31,6 +31,7 @@
>> #include "hw/timer/stellaris-gptm.h"
>> #include "hw/qdev-clock.h"
>> #include "qom/object.h"
>> +#include "qapi/qmp/qlist.h"
>> #define GPIO_A 0
>> #define GPIO_B 1
>> @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>> sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
>> }
>> if (board->peripherals & BP_GAMEPAD) {
>> - qemu_irq gpad_irq[5];
>> + QList *gpad_keycode_list = qlist_new();
>
> I'm trying to understand better qlist_new(), but unfortunately there
> is not much documentation. Looking at how the allocated list was
> released, I found use of g_autoptr in tests/unit/check-qobject.c,
> so I tried:
>
> g_autoptr(QList) gpad_keycode_list = qlist_new();
QObject and its subtypes QDict, QList, QString, ... are reference
counted. qFOO_new() ist to be paired with qFOO_unref() or
qobject_unref().
Your use of g_autoptr(QList) should work.
> But QEMU crashes elsewhere which seems unrelated:
>
> * thread #2, stop reason = signal SIGABRT
> * frame #0: 0x8b1eb11c libsystem_kernel.dylib`__pthread_kill + 8
> frame #1: 0x8b222cc0 libsystem_pthread.dylib`pthread_kill + 288
> frame #2: 0x8b132a50 libsystem_c.dylib`abort + 180
> frame #3: 0x8b049b08 libsystem_malloc.dylib`malloc_vreport + 908
> frame #4: 0x8b06924c libsystem_malloc.dylib`malloc_zone_error + 104
> frame #5: 0x8b05b094 libsystem_malloc.dylib`nanov2_guard_corruption_detected + 44
> frame #6: 0x8b05a2a8 libsystem_malloc.dylib`nanov2_allocate_outlined + 404
> frame #7: 0x0201fdc0 libglib-2.0.0.dylib`g_malloc0 + 36
> frame #8: 0x02007718 libglib-2.0.0.dylib`g_hash_table_setup_storage + 76
> frame #9: 0x020076b0 libglib-2.0.0.dylib`g_hash_table_new_full + 96
> frame #10: 0x003a9920 qemu-system-ppc`object_unref [inlined] object_property_del_all(obj=0x42023e00) at object.c:635:34
> frame #11: 0x003a9914 qemu-system-ppc`object_unref [inlined] object_finalize(data=0x42023e00) at object.c:707:5
> frame #12: 0x003a990c qemu-system-ppc`object_unref(objptr=0x42023e00) at object.c:1216:9
> frame #13: 0x00355114 qemu-system-ppc`address_space_dispatch_free at physmem.c:1001:9
> frame #14: 0x003550fc qemu-system-ppc`address_space_dispatch_free at physmem.c:1010:9
> frame #15: 0x003550e0 qemu-system-ppc`address_space_dispatch_free(d=0x000060000385d680) at physmem.c:2473:5
> frame #16: 0x00349438 qemu-system-ppc`flatview_destroy(view=0x000060000385d640) at memory.c:295:9
> frame #17: 0x00524920 qemu-system-ppc`call_rcu_thread(opaque=<unavailable>) at rcu.c:301:13
> frame #18: 0x0051c1f0 qemu-system-ppc`qemu_thread_start(args=<unavailable>) at qemu-thread-posix.c:541:9
> frame #19: 0x8b223034 libsystem_pthread.dylib`_pthread_start + 136
Can't see QList or QObject in this backtrace. object_unref() is QOM,
not QObject.
> However when running 'make check-unit', qobject_is_equal_list_test()
> is successful, so I'm a bit confused...
>
>> static const int gpad_keycode[5] = { 0xc8, 0xd0, 0xcb, 0xcd, 0x1d };
>> + DeviceState *gpad;
>> - gpad_irq[0] = qemu_irq_invert(gpio_in[GPIO_E][0]); /* up */
>> - gpad_irq[1] = qemu_irq_invert(gpio_in[GPIO_E][1]); /* down */
>> - gpad_irq[2] = qemu_irq_invert(gpio_in[GPIO_E][2]); /* left */
>> - gpad_irq[3] = qemu_irq_invert(gpio_in[GPIO_E][3]); /* right */
>> - gpad_irq[4] = qemu_irq_invert(gpio_in[GPIO_F][1]); /* select */
>> + gpad = qdev_new(TYPE_STELLARIS_GAMEPAD);
>> + for (i = 0; i < ARRAY_SIZE(gpad_keycode); i++) {
>> + qlist_append_int(gpad_keycode_list, gpad_keycode[i]);
>> + }
>> + qdev_prop_set_array(gpad, "keycodes", gpad_keycode_list);
>> + sysbus_realize_and_unref(SYS_BUS_DEVICE(gpad), &error_fatal);
>> - stellaris_gamepad_init(5, gpad_irq, gpad_keycode);
>> + qdev_connect_gpio_out(gpad, 0,
>> + qemu_irq_invert(gpio_in[GPIO_E][0])); /* up */
>> + qdev_connect_gpio_out(gpad, 1,
>> + qemu_irq_invert(gpio_in[GPIO_E][1])); /* down */
>> + qdev_connect_gpio_out(gpad, 2,
>> + qemu_irq_invert(gpio_in[GPIO_E][2])); /* left */
>> + qdev_connect_gpio_out(gpad, 3,
>> + qemu_irq_invert(gpio_in[GPIO_E][3])); /* right */
>> + qdev_connect_gpio_out(gpad, 4,
>> + qemu_irq_invert(gpio_in[GPIO_F][1])); /* select */
>> }
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev
2023-10-30 14:25 ` Markus Armbruster
@ 2023-10-31 8:16 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-31 8:16 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Peter Maydell, Kevin Wolf, qemu-arm, qemu-devel
On 30/10/23 15:25, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
>> Hi Peter,
>>
>> Cc'ing Markus for QObject.
>>
>> On 30/10/23 12:48, Peter Maydell wrote:
>>> Convert the hw/input/stellaris_input device to qdev.
>>> The interface uses an array property for the board to specify the
>>> keycodes to use, so the s->keycodes memory is now allocated by the
>>> array-property machinery.
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> v1->v2: drop private/public comment lines
>>> ---
>>> include/hw/input/stellaris_gamepad.h | 22 ++++++++-
>>> hw/arm/stellaris.c | 26 +++++++---
>>> hw/input/stellaris_gamepad.c | 73 +++++++++++++++++++---------
>>> 3 files changed, 89 insertions(+), 32 deletions(-)
>>
>>
>>> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
>>> index 96585dd7106..707b0dae375 100644
>>> --- a/hw/arm/stellaris.c
>>> +++ b/hw/arm/stellaris.c
>>> @@ -31,6 +31,7 @@
>>> #include "hw/timer/stellaris-gptm.h"
>>> #include "hw/qdev-clock.h"
>>> #include "qom/object.h"
>>> +#include "qapi/qmp/qlist.h"
>>> #define GPIO_A 0
>>> #define GPIO_B 1
>>> @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>>> sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
>>> }
>>> if (board->peripherals & BP_GAMEPAD) {
>>> - qemu_irq gpad_irq[5];
>>> + QList *gpad_keycode_list = qlist_new();
>>
>> I'm trying to understand better qlist_new(), but unfortunately there
>> is not much documentation. Looking at how the allocated list was
>> released, I found use of g_autoptr in tests/unit/check-qobject.c,
>> so I tried:
>>
>> g_autoptr(QList) gpad_keycode_list = qlist_new();
>
> QObject and its subtypes QDict, QList, QString, ... are reference
> counted. qFOO_new() ist to be paired with qFOO_unref() or
> qobject_unref().
>
> Your use of g_autoptr(QList) should work.
Peter figured qdev_prop_set_array() takes the ownership, so using
g_autoptr triggers a double-free:
https://lore.kernel.org/qemu-devel/CAFEAcA_GC8ypM2Y94KCU9Q_dntF6Na+igu-+0JZJ+MvPFE_HcA@mail.gmail.com/
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev
2023-10-30 13:52 ` Philippe Mathieu-Daudé
2023-10-30 14:25 ` Markus Armbruster
@ 2023-10-30 14:41 ` Peter Maydell
2023-10-30 18:28 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2023-10-30 14:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Markus Armbruster, Kevin Wolf, qemu-arm, qemu-devel
On Mon, 30 Oct 2023 at 13:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> Cc'ing Markus for QObject.
>
> On 30/10/23 12:48, Peter Maydell wrote:
> > Convert the hw/input/stellaris_input device to qdev.
> >
> > The interface uses an array property for the board to specify the
> > keycodes to use, so the s->keycodes memory is now allocated by the
> > array-property machinery.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > v1->v2: drop private/public comment lines
> > ---
> > include/hw/input/stellaris_gamepad.h | 22 ++++++++-
> > hw/arm/stellaris.c | 26 +++++++---
> > hw/input/stellaris_gamepad.c | 73 +++++++++++++++++++---------
> > 3 files changed, 89 insertions(+), 32 deletions(-)
>
>
> > diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> > index 96585dd7106..707b0dae375 100644
> > --- a/hw/arm/stellaris.c
> > +++ b/hw/arm/stellaris.c
> > @@ -31,6 +31,7 @@
> > #include "hw/timer/stellaris-gptm.h"
> > #include "hw/qdev-clock.h"
> > #include "qom/object.h"
> > +#include "qapi/qmp/qlist.h"
> >
> > #define GPIO_A 0
> > #define GPIO_B 1
> > @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
> > sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
> > }
> > if (board->peripherals & BP_GAMEPAD) {
> > - qemu_irq gpad_irq[5];
> > + QList *gpad_keycode_list = qlist_new();
>
> I'm trying to understand better qlist_new(), but unfortunately there
> is not much documentation. Looking at how the allocated list was
> released, I found use of g_autoptr in tests/unit/check-qobject.c,
> so I tried:
>
> g_autoptr(QList) gpad_keycode_list = qlist_new();
The API for qdev_prop_set_array() documents that it takes ownership
of the list you pass it (and it ends up calling qobject_unref() on it).
So I think adding g_autoptr() here will result in the memory being
double-freed (once inside qobject_unref() when the refcount
goes to 0, and once when g_autoptr frees it as it goes out of scope)...
> * thread #2, stop reason = signal SIGABRT
> * frame #0: 0x8b1eb11c libsystem_kernel.dylib`__pthread_kill + 8
> frame #1: 0x8b222cc0 libsystem_pthread.dylib`pthread_kill + 288
> frame #2: 0x8b132a50 libsystem_c.dylib`abort + 180
> frame #3: 0x8b049b08 libsystem_malloc.dylib`malloc_vreport + 908
> frame #4: 0x8b06924c libsystem_malloc.dylib`malloc_zone_error + 104
> frame #5: 0x8b05b094
> libsystem_malloc.dylib`nanov2_guard_corruption_detected + 44
> frame #6: 0x8b05a2a8
> libsystem_malloc.dylib`nanov2_allocate_outlined + 404
> frame #7: 0x0201fdc0 libglib-2.0.0.dylib`g_malloc0 + 36
> frame #8: 0x02007718 libglib-2.0.0.dylib`g_hash_table_setup_storage
> + 76
> frame #9: 0x020076b0 libglib-2.0.0.dylib`g_hash_table_new_full + 96
...which is probably why a later memory operation runs into a
malloc data corruption assertion.
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev
2023-10-30 14:41 ` Peter Maydell
@ 2023-10-30 18:28 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-30 18:28 UTC (permalink / raw)
To: Peter Maydell; +Cc: Markus Armbruster, Kevin Wolf, qemu-arm, qemu-devel
On 30/10/23 15:41, Peter Maydell wrote:
> On Mon, 30 Oct 2023 at 13:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> Cc'ing Markus for QObject.
>>
>> On 30/10/23 12:48, Peter Maydell wrote:
>>> Convert the hw/input/stellaris_input device to qdev.
>>>
>>> The interface uses an array property for the board to specify the
>>> keycodes to use, so the s->keycodes memory is now allocated by the
>>> array-property machinery.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> v1->v2: drop private/public comment lines
>>> ---
>>> include/hw/input/stellaris_gamepad.h | 22 ++++++++-
>>> hw/arm/stellaris.c | 26 +++++++---
>>> hw/input/stellaris_gamepad.c | 73 +++++++++++++++++++---------
>>> 3 files changed, 89 insertions(+), 32 deletions(-)
>>
>>
>>> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
>>> index 96585dd7106..707b0dae375 100644
>>> --- a/hw/arm/stellaris.c
>>> +++ b/hw/arm/stellaris.c
>>> @@ -31,6 +31,7 @@
>>> #include "hw/timer/stellaris-gptm.h"
>>> #include "hw/qdev-clock.h"
>>> #include "qom/object.h"
>>> +#include "qapi/qmp/qlist.h"
>>>
>>> #define GPIO_A 0
>>> #define GPIO_B 1
>>> @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>>> sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
>>> }
>>> if (board->peripherals & BP_GAMEPAD) {
>>> - qemu_irq gpad_irq[5];
>>> + QList *gpad_keycode_list = qlist_new();
>>
>> I'm trying to understand better qlist_new(), but unfortunately there
>> is not much documentation. Looking at how the allocated list was
>> released, I found use of g_autoptr in tests/unit/check-qobject.c,
>> so I tried:
>>
>> g_autoptr(QList) gpad_keycode_list = qlist_new();
>
> The API for qdev_prop_set_array() documents that it takes ownership
> of the list you pass it (and it ends up calling qobject_unref() on it).
> So I think adding g_autoptr() here will result in the memory being
> double-freed (once inside qobject_unref() when the refcount
> goes to 0, and once when g_autoptr frees it as it goes out of scope)...
Ah, I missed how qdev_prop_set_array() is involved.
>> * thread #2, stop reason = signal SIGABRT
>> * frame #0: 0x8b1eb11c libsystem_kernel.dylib`__pthread_kill + 8
>> frame #1: 0x8b222cc0 libsystem_pthread.dylib`pthread_kill + 288
>> frame #2: 0x8b132a50 libsystem_c.dylib`abort + 180
>> frame #3: 0x8b049b08 libsystem_malloc.dylib`malloc_vreport + 908
>> frame #4: 0x8b06924c libsystem_malloc.dylib`malloc_zone_error + 104
>> frame #5: 0x8b05b094
>> libsystem_malloc.dylib`nanov2_guard_corruption_detected + 44
>> frame #6: 0x8b05a2a8
>> libsystem_malloc.dylib`nanov2_allocate_outlined + 404
>> frame #7: 0x0201fdc0 libglib-2.0.0.dylib`g_malloc0 + 36
>> frame #8: 0x02007718 libglib-2.0.0.dylib`g_hash_table_setup_storage
>> + 76
>> frame #9: 0x020076b0 libglib-2.0.0.dylib`g_hash_table_new_full + 96
>
> ...which is probably why a later memory operation runs into a
> malloc data corruption assertion.
Yes, this is certainly the reason. Thanks for the explanation!
Phil.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev
2023-10-30 11:48 ` [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev Peter Maydell
2023-10-30 13:52 ` Philippe Mathieu-Daudé
@ 2023-10-30 20:37 ` Mark Cave-Ayland
2023-10-31 13:55 ` Peter Maydell
1 sibling, 1 reply; 17+ messages in thread
From: Mark Cave-Ayland @ 2023-10-30 20:37 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Cc: Philippe Mathieu-Daudé, Kevin Wolf
On 30/10/2023 11:48, Peter Maydell wrote:
> Convert the hw/input/stellaris_input device to qdev.
>
> The interface uses an array property for the board to specify the
> keycodes to use, so the s->keycodes memory is now allocated by the
> array-property machinery.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> v1->v2: drop private/public comment lines
> ---
> include/hw/input/stellaris_gamepad.h | 22 ++++++++-
> hw/arm/stellaris.c | 26 +++++++---
> hw/input/stellaris_gamepad.c | 73 +++++++++++++++++++---------
> 3 files changed, 89 insertions(+), 32 deletions(-)
>
> diff --git a/include/hw/input/stellaris_gamepad.h b/include/hw/input/stellaris_gamepad.h
> index 23cfd3c95f3..6140b889a28 100644
> --- a/include/hw/input/stellaris_gamepad.h
> +++ b/include/hw/input/stellaris_gamepad.h
> @@ -11,8 +11,26 @@
> #ifndef HW_INPUT_STELLARIS_GAMEPAD_H
> #define HW_INPUT_STELLARIS_GAMEPAD_H
>
> +#include "hw/sysbus.h"
> +#include "qom/object.h"
>
> -/* stellaris_gamepad.c */
> -void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode);
> +/*
> + * QEMU interface:
> + * + QOM array property "keycodes": uint32_t QEMU keycodes to handle
> + * + unnamed GPIO outputs: one per keycode, in the same order as the
> + * "keycodes" array property entries; asserted when key is down
> + */
> +
> +#define TYPE_STELLARIS_GAMEPAD "stellaris-gamepad"
> +OBJECT_DECLARE_SIMPLE_TYPE(StellarisGamepad, STELLARIS_GAMEPAD)
> +
> +struct StellarisGamepad {
> + SysBusDevice parent_obj;
Minor style comment: for QOM types there should be an empty line after parent_obj to
aid identification (as per
https://qemu.readthedocs.io/en/master/devel/style.html#qemu-object-model-declarations).
> + uint32_t num_buttons;
> + qemu_irq *irqs;
> + uint32_t *keycodes;
> + uint8_t *pressed;
> + int extension;
> +};
>
> #endif
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 96585dd7106..707b0dae375 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -31,6 +31,7 @@
> #include "hw/timer/stellaris-gptm.h"
> #include "hw/qdev-clock.h"
> #include "qom/object.h"
> +#include "qapi/qmp/qlist.h"
>
> #define GPIO_A 0
> #define GPIO_B 1
> @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
> sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
> }
> if (board->peripherals & BP_GAMEPAD) {
> - qemu_irq gpad_irq[5];
> + QList *gpad_keycode_list = qlist_new();
> static const int gpad_keycode[5] = { 0xc8, 0xd0, 0xcb, 0xcd, 0x1d };
> + DeviceState *gpad;
>
> - gpad_irq[0] = qemu_irq_invert(gpio_in[GPIO_E][0]); /* up */
> - gpad_irq[1] = qemu_irq_invert(gpio_in[GPIO_E][1]); /* down */
> - gpad_irq[2] = qemu_irq_invert(gpio_in[GPIO_E][2]); /* left */
> - gpad_irq[3] = qemu_irq_invert(gpio_in[GPIO_E][3]); /* right */
> - gpad_irq[4] = qemu_irq_invert(gpio_in[GPIO_F][1]); /* select */
> + gpad = qdev_new(TYPE_STELLARIS_GAMEPAD);
> + for (i = 0; i < ARRAY_SIZE(gpad_keycode); i++) {
> + qlist_append_int(gpad_keycode_list, gpad_keycode[i]);
> + }
> + qdev_prop_set_array(gpad, "keycodes", gpad_keycode_list);
> + sysbus_realize_and_unref(SYS_BUS_DEVICE(gpad), &error_fatal);
>
> - stellaris_gamepad_init(5, gpad_irq, gpad_keycode);
> + qdev_connect_gpio_out(gpad, 0,
> + qemu_irq_invert(gpio_in[GPIO_E][0])); /* up */
> + qdev_connect_gpio_out(gpad, 1,
> + qemu_irq_invert(gpio_in[GPIO_E][1])); /* down */
> + qdev_connect_gpio_out(gpad, 2,
> + qemu_irq_invert(gpio_in[GPIO_E][2])); /* left */
> + qdev_connect_gpio_out(gpad, 3,
> + qemu_irq_invert(gpio_in[GPIO_E][3])); /* right */
> + qdev_connect_gpio_out(gpad, 4,
> + qemu_irq_invert(gpio_in[GPIO_F][1])); /* select */
> }
> for (i = 0; i < 7; i++) {
> if (board->dc4 & (1 << i)) {
> diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
> index 82ddc47a26d..6ccf0e80adc 100644
> --- a/hw/input/stellaris_gamepad.c
> +++ b/hw/input/stellaris_gamepad.c
> @@ -8,19 +8,13 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qapi/error.h"
> #include "hw/input/stellaris_gamepad.h"
> #include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> #include "migration/vmstate.h"
> #include "ui/console.h"
>
> -typedef struct {
> - uint32_t num_buttons;
> - int extension;
> - qemu_irq *irqs;
> - uint32_t *keycodes;
> - uint8_t *pressed;
> -} StellarisGamepad;
> -
> static void stellaris_gamepad_put_key(void * opaque, int keycode)
> {
> StellarisGamepad *s = (StellarisGamepad *)opaque;
> @@ -57,22 +51,55 @@ static const VMStateDescription vmstate_stellaris_gamepad = {
> }
> };
>
> -/* Returns an array of 5 output slots. */
> -void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode)
> +static void stellaris_gamepad_realize(DeviceState *dev, Error **errp)
> {
> - StellarisGamepad *s;
> - int i;
> + StellarisGamepad *s = STELLARIS_GAMEPAD(dev);
>
> - s = g_new0(StellarisGamepad, 1);
> - s->irqs = g_new0(qemu_irq, n);
> - s->keycodes = g_new0(uint32_t, n);
> - s->pressed = g_new0(uint8_t, n);
> - for (i = 0; i < n; i++) {
> - s->irqs[i] = irq[i];
> - s->keycodes[i] = keycode[i];
> + if (s->num_buttons == 0) {
> + error_setg(errp, "keycodes property array must be set");
> + return;
> }
> - s->num_buttons = n;
> - qemu_add_kbd_event_handler(stellaris_gamepad_put_key, s);
> - vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
> - &vmstate_stellaris_gamepad, s);
> +
> + s->irqs = g_new0(qemu_irq, s->num_buttons);
> + s->pressed = g_new0(uint8_t, s->num_buttons);
> + qdev_init_gpio_out(dev, s->irqs, s->num_buttons);
> + qemu_add_kbd_event_handler(stellaris_gamepad_put_key, dev);
> }
> +
> +static void stellaris_gamepad_reset_enter(Object *obj, ResetType type)
> +{
> + StellarisGamepad *s = STELLARIS_GAMEPAD(obj);
> +
> + memset(s->pressed, 0, s->num_buttons * sizeof(uint8_t));
> +}
> +
> +static Property stellaris_gamepad_properties[] = {
> + DEFINE_PROP_ARRAY("keycodes", StellarisGamepad, num_buttons,
> + keycodes, qdev_prop_uint32, uint32_t),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void stellaris_gamepad_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + ResettableClass *rc = RESETTABLE_CLASS(klass);
> +
> + rc->phases.enter = stellaris_gamepad_reset_enter;
> + dc->realize = stellaris_gamepad_realize;
> + dc->vmsd = &vmstate_stellaris_gamepad;
> + device_class_set_props(dc, stellaris_gamepad_properties);
> +}
> +
> +static const TypeInfo stellaris_gamepad_info = {
> + .name = TYPE_STELLARIS_GAMEPAD,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(StellarisGamepad),
> + .class_init = stellaris_gamepad_class_init,
> +};
> +
> +static void stellaris_gamepad_register_types(void)
> +{
> + type_register_static(&stellaris_gamepad_info);
> +}
> +
> +type_init(stellaris_gamepad_register_types);
Is it worth converting this to use DEFINE_TYPES() during the conversion? I know Phil
has considered some automation to remove the type_init() boilerplate for the majority
of cases.
ATB,
Mark.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev
2023-10-30 20:37 ` Mark Cave-Ayland
@ 2023-10-31 13:55 ` Peter Maydell
2023-10-31 14:05 ` Peter Maydell
0 siblings, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2023-10-31 13:55 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-arm, qemu-devel, Philippe Mathieu-Daudé, Kevin Wolf
On Mon, 30 Oct 2023 at 20:38, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 30/10/2023 11:48, Peter Maydell wrote:
>
> > Convert the hw/input/stellaris_input device to qdev.
> >
> > The interface uses an array property for the board to specify the
> > keycodes to use, so the s->keycodes memory is now allocated by the
> > array-property machinery.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > v1->v2: drop private/public comment lines
> > ---
> > include/hw/input/stellaris_gamepad.h | 22 ++++++++-
> > hw/arm/stellaris.c | 26 +++++++---
> > hw/input/stellaris_gamepad.c | 73 +++++++++++++++++++---------
> > 3 files changed, 89 insertions(+), 32 deletions(-)
> >
> > diff --git a/include/hw/input/stellaris_gamepad.h b/include/hw/input/stellaris_gamepad.h
> > index 23cfd3c95f3..6140b889a28 100644
> > --- a/include/hw/input/stellaris_gamepad.h
> > +++ b/include/hw/input/stellaris_gamepad.h
> > @@ -11,8 +11,26 @@
> > #ifndef HW_INPUT_STELLARIS_GAMEPAD_H
> > #define HW_INPUT_STELLARIS_GAMEPAD_H
> >
> > +#include "hw/sysbus.h"
> > +#include "qom/object.h"
> >
> > -/* stellaris_gamepad.c */
> > -void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode);
> > +/*
> > + * QEMU interface:
> > + * + QOM array property "keycodes": uint32_t QEMU keycodes to handle
> > + * + unnamed GPIO outputs: one per keycode, in the same order as the
> > + * "keycodes" array property entries; asserted when key is down
> > + */
> > +
> > +#define TYPE_STELLARIS_GAMEPAD "stellaris-gamepad"
> > +OBJECT_DECLARE_SIMPLE_TYPE(StellarisGamepad, STELLARIS_GAMEPAD)
> > +
> > +struct StellarisGamepad {
> > + SysBusDevice parent_obj;
>
> Minor style comment: for QOM types there should be an empty line after parent_obj to
> aid identification (as per
> https://qemu.readthedocs.io/en/master/devel/style.html#qemu-object-model-declarations).
Fixed.
> > +static const TypeInfo stellaris_gamepad_info = {
> > + .name = TYPE_STELLARIS_GAMEPAD,
> > + .parent = TYPE_SYS_BUS_DEVICE,
> > + .instance_size = sizeof(StellarisGamepad),
> > + .class_init = stellaris_gamepad_class_init,
> > +};
> > +
> > +static void stellaris_gamepad_register_types(void)
> > +{
> > + type_register_static(&stellaris_gamepad_info);
> > +}
> > +
> > +type_init(stellaris_gamepad_register_types);
>
> Is it worth converting this to use DEFINE_TYPES() during the conversion? I know Phil
> has considered some automation to remove the type_init() boilerplate for the majority
> of cases.
I could, I guess. It seems a bit awkward that DEFINE_TYPES()
wants you to pass it an array even when you only have one type,
though, which is going to be a very common use case.
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev
2023-10-31 13:55 ` Peter Maydell
@ 2023-10-31 14:05 ` Peter Maydell
2023-10-31 14:55 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2023-10-31 14:05 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-arm, qemu-devel, Philippe Mathieu-Daudé, Kevin Wolf
On Tue, 31 Oct 2023 at 13:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 30 Oct 2023 at 20:38, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
> >
> > On 30/10/2023 11:48, Peter Maydell wrote:
> > Is it worth converting this to use DEFINE_TYPES() during the conversion? I know Phil
> > has considered some automation to remove the type_init() boilerplate for the majority
> > of cases.
>
> I could, I guess. It seems a bit awkward that DEFINE_TYPES()
> wants you to pass it an array even when you only have one type,
> though, which is going to be a very common use case.
I'm going to squash this into this patch:
diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
index 6ccf0e80adc..d42ba4f0582 100644
--- a/hw/input/stellaris_gamepad.c
+++ b/hw/input/stellaris_gamepad.c
@@ -90,16 +90,13 @@ static void
stellaris_gamepad_class_init(ObjectClass *klass, void *data)
device_class_set_props(dc, stellaris_gamepad_properties);
}
-static const TypeInfo stellaris_gamepad_info = {
- .name = TYPE_STELLARIS_GAMEPAD,
- .parent = TYPE_SYS_BUS_DEVICE,
- .instance_size = sizeof(StellarisGamepad),
- .class_init = stellaris_gamepad_class_init,
+static const TypeInfo stellaris_gamepad_info[] = {
+ {
+ .name = TYPE_STELLARIS_GAMEPAD,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(StellarisGamepad),
+ .class_init = stellaris_gamepad_class_init,
+ },
};
-static void stellaris_gamepad_register_types(void)
-{
- type_register_static(&stellaris_gamepad_info);
-}
-
-type_init(stellaris_gamepad_register_types);
+DEFINE_TYPES(stellaris_gamepad_info);
The array is a bit awkward, but it's overall better than having
to define the register-types function.
thanks
-- PMM
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev
2023-10-31 14:05 ` Peter Maydell
@ 2023-10-31 14:55 ` Philippe Mathieu-Daudé
2023-10-31 15:36 ` Peter Maydell
0 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-31 14:55 UTC (permalink / raw)
To: Peter Maydell, Mark Cave-Ayland; +Cc: qemu-arm, qemu-devel, Kevin Wolf
On 31/10/23 15:05, Peter Maydell wrote:
> On Tue, 31 Oct 2023 at 13:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Mon, 30 Oct 2023 at 20:38, Mark Cave-Ayland
>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>
>>> On 30/10/2023 11:48, Peter Maydell wrote:
>>> Is it worth converting this to use DEFINE_TYPES() during the conversion? I know Phil
>>> has considered some automation to remove the type_init() boilerplate for the majority
>>> of cases.
>>
>> I could, I guess. It seems a bit awkward that DEFINE_TYPES()
>> wants you to pass it an array even when you only have one type,
>> though, which is going to be a very common use case.
For single type, there is no point beside enforcing a QOM style.
I'll update docs/devel/qom.rst...
> I'm going to squash this into this patch:
> diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
> index 6ccf0e80adc..d42ba4f0582 100644
> --- a/hw/input/stellaris_gamepad.c
> +++ b/hw/input/stellaris_gamepad.c
> @@ -90,16 +90,13 @@ static void
> stellaris_gamepad_class_init(ObjectClass *klass, void *data)
> device_class_set_props(dc, stellaris_gamepad_properties);
> }
>
> -static const TypeInfo stellaris_gamepad_info = {
> - .name = TYPE_STELLARIS_GAMEPAD,
> - .parent = TYPE_SYS_BUS_DEVICE,
> - .instance_size = sizeof(StellarisGamepad),
> - .class_init = stellaris_gamepad_class_init,
> +static const TypeInfo stellaris_gamepad_info[] = {
> + {
> + .name = TYPE_STELLARIS_GAMEPAD,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(StellarisGamepad),
> + .class_init = stellaris_gamepad_class_init,
> + },
> };
>
> -static void stellaris_gamepad_register_types(void)
> -{
> - type_register_static(&stellaris_gamepad_info);
> -}
> -
> -type_init(stellaris_gamepad_register_types);
> +DEFINE_TYPES(stellaris_gamepad_info);
>
>
> The array is a bit awkward, but it's overall better than having
> to define the register-types function.
Thank you!
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev
2023-10-31 14:55 ` Philippe Mathieu-Daudé
@ 2023-10-31 15:36 ` Peter Maydell
0 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2023-10-31 15:36 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Mark Cave-Ayland, qemu-arm, qemu-devel, Kevin Wolf
On Tue, 31 Oct 2023 at 14:55, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 31/10/23 15:05, Peter Maydell wrote:
> > On Tue, 31 Oct 2023 at 13:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Mon, 30 Oct 2023 at 20:38, Mark Cave-Ayland
> >> <mark.cave-ayland@ilande.co.uk> wrote:
> >>>
> >>> On 30/10/2023 11:48, Peter Maydell wrote:
> >>> Is it worth converting this to use DEFINE_TYPES() during the conversion? I know Phil
> >>> has considered some automation to remove the type_init() boilerplate for the majority
> >>> of cases.
> >>
> >> I could, I guess. It seems a bit awkward that DEFINE_TYPES()
> >> wants you to pass it an array even when you only have one type,
> >> though, which is going to be a very common use case.
>
> For single type, there is no point beside enforcing a QOM style.
>
> I'll update docs/devel/qom.rst...
I do like that the macro means you're not writing an actual
function for the registration.
We could I guess have a DEFINE_TYPE() macro that was similar
to DEFINE_TYPES but emitted a function that called
type_register_static() instead of type_register_static_array().
Is that worth having? I'm not sure.
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 6/6] hw/input/stellaris_gamepad: Convert to qemu_input_handler_register()
2023-10-30 11:47 [PATCH v2 0/6] arm/stellaris: convert gamepad input device to qdev Peter Maydell
` (4 preceding siblings ...)
2023-10-30 11:48 ` [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev Peter Maydell
@ 2023-10-30 11:48 ` Peter Maydell
5 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2023-10-30 11:48 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Philippe Mathieu-Daudé, Kevin Wolf
Now that we have converted to qdev, we can use the newer
qemu_input_handler_register() API rather than the legacy
qemu_add_kbd_event_handler().
Since we only have one user, take the opportunity to convert
from scancodes to QCodes, rather than using
qemu_input_key_value_to_scancode() (which adds an 0xe0
prefix and encodes up/down indication in the scancode,
which our old handler function then had to reverse). That
lets us drop the old state field which was tracking whether
we were halfway through a two-byte scancode.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
v1->v2: bump vmstate version number again
mark QemuInputHandler struct as const
---
include/hw/input/stellaris_gamepad.h | 2 +-
hw/arm/stellaris.c | 6 ++++-
hw/input/stellaris_gamepad.c | 37 +++++++++++++---------------
3 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/include/hw/input/stellaris_gamepad.h b/include/hw/input/stellaris_gamepad.h
index 6140b889a28..b0c66a35531 100644
--- a/include/hw/input/stellaris_gamepad.h
+++ b/include/hw/input/stellaris_gamepad.h
@@ -17,6 +17,7 @@
/*
* QEMU interface:
* + QOM array property "keycodes": uint32_t QEMU keycodes to handle
+ * (these are QCodes, ie the Q_KEY_* values)
* + unnamed GPIO outputs: one per keycode, in the same order as the
* "keycodes" array property entries; asserted when key is down
*/
@@ -30,7 +31,6 @@ struct StellarisGamepad {
qemu_irq *irqs;
uint32_t *keycodes;
uint8_t *pressed;
- int extension;
};
#endif
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 707b0dae375..dd90f686bfa 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -32,6 +32,7 @@
#include "hw/qdev-clock.h"
#include "qom/object.h"
#include "qapi/qmp/qlist.h"
+#include "ui/input.h"
#define GPIO_A 0
#define GPIO_B 1
@@ -1276,7 +1277,10 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
}
if (board->peripherals & BP_GAMEPAD) {
QList *gpad_keycode_list = qlist_new();
- static const int gpad_keycode[5] = { 0xc8, 0xd0, 0xcb, 0xcd, 0x1d };
+ static const int gpad_keycode[5] = {
+ Q_KEY_CODE_UP, Q_KEY_CODE_DOWN, Q_KEY_CODE_LEFT,
+ Q_KEY_CODE_RIGHT, Q_KEY_CODE_CTRL,
+ };
DeviceState *gpad;
gpad = qdev_new(TYPE_STELLARIS_GAMEPAD);
diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
index 6ccf0e80adc..3533becff5d 100644
--- a/hw/input/stellaris_gamepad.c
+++ b/hw/input/stellaris_gamepad.c
@@ -15,42 +15,39 @@
#include "migration/vmstate.h"
#include "ui/console.h"
-static void stellaris_gamepad_put_key(void * opaque, int keycode)
+static void stellaris_gamepad_event(DeviceState *dev, QemuConsole *src,
+ InputEvent *evt)
{
- StellarisGamepad *s = (StellarisGamepad *)opaque;
+ StellarisGamepad *s = STELLARIS_GAMEPAD(dev);
+ InputKeyEvent *key = evt->u.key.data;
+ int qcode = qemu_input_key_value_to_qcode(key->key);
int i;
- int down;
-
- if (keycode == 0xe0 && !s->extension) {
- s->extension = 0x80;
- return;
- }
-
- down = (keycode & 0x80) == 0;
- keycode = (keycode & 0x7f) | s->extension;
for (i = 0; i < s->num_buttons; i++) {
- if (s->keycodes[i] == keycode && s->pressed[i] != down) {
- s->pressed[i] = down;
- qemu_set_irq(s->irqs[i], down);
+ if (s->keycodes[i] == qcode && s->pressed[i] != key->down) {
+ s->pressed[i] = key->down;
+ qemu_set_irq(s->irqs[i], key->down);
}
}
-
- s->extension = 0;
}
static const VMStateDescription vmstate_stellaris_gamepad = {
.name = "stellaris_gamepad",
- .version_id = 3,
- .minimum_version_id = 3,
+ .version_id = 4,
+ .minimum_version_id = 4,
.fields = (VMStateField[]) {
- VMSTATE_INT32(extension, StellarisGamepad),
VMSTATE_VARRAY_UINT32(pressed, StellarisGamepad, num_buttons,
0, vmstate_info_uint8, uint8_t),
VMSTATE_END_OF_LIST()
}
};
+static const QemuInputHandler stellaris_gamepad_handler = {
+ .name = "Stellaris Gamepad",
+ .mask = INPUT_EVENT_MASK_KEY,
+ .event = stellaris_gamepad_event,
+};
+
static void stellaris_gamepad_realize(DeviceState *dev, Error **errp)
{
StellarisGamepad *s = STELLARIS_GAMEPAD(dev);
@@ -63,7 +60,7 @@ static void stellaris_gamepad_realize(DeviceState *dev, Error **errp)
s->irqs = g_new0(qemu_irq, s->num_buttons);
s->pressed = g_new0(uint8_t, s->num_buttons);
qdev_init_gpio_out(dev, s->irqs, s->num_buttons);
- qemu_add_kbd_event_handler(stellaris_gamepad_put_key, dev);
+ qemu_input_handler_register(dev, &stellaris_gamepad_handler);
}
static void stellaris_gamepad_reset_enter(Object *obj, ResetType type)
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread