| Message ID | 20231030114802.3671871-6-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm/stellaris: convert gamepad input device to qdev | expand |
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 */ > }
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 */ >> }
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
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.
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.
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/
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
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
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!
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
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);