Re: [Qemu-devel] [PATCH 19/38] char: make some qemu_chr_fe skip if no driver

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 16/2/23 15:23, Marc-André Lureau wrote:
> Hi Philippe
> 
> On Thu, Feb 16, 2023 at 2:14 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> Hi Marc-André,
>>
>> [very old patch...]
>>
>> On 22/10/16 11:52, Marc-André Lureau wrote:
>>> In most cases, front ends do not care about the side effect of
>>> CharBackend, so we can simply skip the checks and call the qemu_chr_fe
>>> functions even without associated CharDriver.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    hw/arm/pxa2xx.c           |  8 +++-----
>>>    hw/arm/strongarm.c        | 16 ++++++---------
>>>    hw/char/bcm2835_aux.c     | 18 ++++++-----------
>>>    hw/char/cadence_uart.c    | 24 +++++++---------------
>>
>>>    qemu-char.c               | 51 ++++++++++++++++++++++++++++++++++++++---------
>>>    include/sysemu/char.h     | 40 +++++++++++++++++++++++++------------
>>>    22 files changed, 156 insertions(+), 191 deletions(-)
>>
>>
>>> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
>>> index 4459b2d..291818e 100644
>>> --- a/hw/char/cadence_uart.c
>>> +++ b/hw/char/cadence_uart.c
>>> @@ -142,9 +142,7 @@ static void uart_rx_reset(CadenceUARTState *s)
>>>    {
>>>        s->rx_wpos = 0;
>>>        s->rx_count = 0;
>>> -    if (s->chr.chr) {
>>> -        qemu_chr_fe_accept_input(&s->chr);
>>> -    }
>>> +    qemu_chr_fe_accept_input(&s->chr);
>>
>> I'm trying to understand this change. This code comes from:
>>
>> commit 9121d02cb33c96b444a3973579f5edc119597e81
>>
>>       char/cadence_uart: Fix reset for unattached instances
>>
>>       commit 1db8b5efe0c2b5000e50691eea61264a615f43de introduced an issue
>>       where QEMU would segfault if you have an unattached Cadence UART.
>>
>>       Fix by guarding the flush-on-reset logic on there being a qemu_chr
>>       attachment.
>>
>> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
>> index 131370a74b..4d457f8c65 100644
>> --- a/hw/char/cadence_uart.c
>> +++ b/hw/char/cadence_uart.c
>> @@ -157,7 +157,9 @@ static void uart_rx_reset(UartState *s)
>>    {
>>        s->rx_wpos = 0;
>>        s->rx_count = 0;
>> -    qemu_chr_accept_input(s->chr);
>> +    if (s->chr) {
>> +        qemu_chr_accept_input(s->chr);
>> +    }
>>
>> When resetting the xlnx-zcu102 machine, I hit:
>>
>> (lldb) bt
>> * thread #1, queue = 'com.apple.main-thread', stop reason =
>> EXC_BAD_ACCESS (code=1, address=0x50)
>>     * frame #0: 0x10020a740 gd_vc_send_chars(vc=0x000000000) at
>> gtk.c:1759:41 [opt]
>>       frame #1: 0x100636264 qemu_chr_fe_accept_input(be=<unavailable>) at
>> char-fe.c:159:9 [opt]
>>       frame #2: 0x1000608e0 cadence_uart_reset_hold [inlined]
>> uart_rx_reset(s=0x10810a960) at cadence_uart.c:158:5 [opt]
>>       frame #3: 0x1000608d4 cadence_uart_reset_hold(obj=0x10810a960) at
>> cadence_uart.c:530:5 [opt]
>>       frame #4: 0x100580ab4 resettable_phase_hold(obj=0x10810a960,
>> opaque=0x000000000, type=<unavailable>) at resettable.c:0 [opt]
>>       frame #5: 0x10057d1b0 bus_reset_child_foreach(obj=<unavailable>,
>> cb=(resettable_phase_hold at resettable.c:162), opaque=0x000000000,
>> type=RESET_TYPE_COLD) at bus.c:97:13 [opt]
>>       frame #6: 0x1005809f8 resettable_phase_hold [inlined]
>> resettable_child_foreach(rc=0x000060000332d2c0, obj=0x0000600002c1c180,
>> cb=<unavailable>, opaque=0x000000000, type=RESET_TYPE_COLD) at
>> resettable.c:96:9 [opt]
>>       frame #7: 0x1005809d8 resettable_phase_hold(obj=0x0000600002c1c180,
>> opaque=0x000000000, type=RESET_TYPE_COLD) at resettable.c:173:5 [opt]
>>       frame #8: 0x1005803a0
>> resettable_assert_reset(obj=0x0000600002c1c180, type=<unavailable>) at
>> resettable.c:60:5 [opt]
>>       frame #9: 0x10058027c resettable_reset(obj=0x0000600002c1c180,
>> type=RESET_TYPE_COLD) at resettable.c:45:5 [opt]
>>
>> Doing similar to commit 9121d02cb3...:
>>
>> -- >8 --
>> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
>> index c069a30842..deadee1788 100644
>> --- a/hw/char/cadence_uart.c
>> +++ b/hw/char/cadence_uart.c
>> @@ -155,7 +155,9 @@ static void uart_rx_reset(CadenceUARTState *s)
>>    {
>>        s->rx_wpos = 0;
>>        s->rx_count = 0;
>> -    qemu_chr_fe_accept_input(&s->chr);
>> +    if (qemu_chr_fe_backend_open(&s->chr)) {
>> +        qemu_chr_fe_accept_input(&s->chr);
>> +    }
>>    }
>> ---
>>
>> ... fixes the issue but I'm not sure 1/ this is a correct use of the
>> chardev API and 2/ this is how the HW work at reset.
> 
> The trouble is that GTK/VTE console/chardev creation is done later.
> 
> I think we should rather fix ui/gtk.c, as this could happen with other
> char frontends:
> 
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 4817623c8f..dfaf6d33c3 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -1783,7 +1783,9 @@ static void gd_vc_chr_accept_input(Chardev *chr)
>       VCChardev *vcd = VC_CHARDEV(chr);
>       VirtualConsole *vc = vcd->console;
> 
> -    gd_vc_send_chars(vc);
> +    if (vc) {
> +        gd_vc_send_chars(vc);
> +    }

Easy :) If you send a proper patch, feel free to include:

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!