Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Markus Armbruster <armbru@redhat.com>]

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 */
>>       }