Re: [Qemu-devel] [PATCH for-2.12] hw/riscv: Fix crashes with "-nodefaults"

[Peter Maydell <peter.maydell@linaro.org>]

On 23 March 2018 at 12:31, Thomas Huth <thuth@redhat.com> wrote:
> On 23.03.2018 10:56, Peter Maydell wrote:
>> On 23 March 2018 at 08:36, Thomas Huth <thuth@redhat.com> wrote:
>>> Most of the RISC-V boards currently crash when they are started with
>>> "-nodefaults", e.g.:
>>>
>>> $ gdb --args riscv32-softmmu/qemu-system-riscv32 -nodefaults -M sifive_e
>>> [...]
>>> (gdb) r
>>> Program received signal SIGSEGV, Segmentation fault.
>>> qemu_chr_fe_init ([...], s=s@entry=0x0, [...])
>>>     at chardev/char-fe.c:210
>>> 210         } else if (s->be) {
>>> (gdb) bt
>>>  0  0x00005555558695f8 in qemu_chr_fe_init ([...], s=s@entry=0x0, [...])
>>>     at chardev/char-fe.c:210
>>>  1  0x00005555556fb425 in sifive_uart_create ([...], chr=0x0, [...])
>>>     at hw/riscv/sifive_uart.c:169
>>>  2  0x00005555556f8cc4 in riscv_sifive_e_init (machine=[...])
>>>     at hw/riscv/sifive_e.c:164
>>>     [...]
>>>
>>> With "-nodefaults" there are no entries in serial_hds[], so qemu_chr_fe_init()
>>> finally tries to dereference a NULL pointer. Let's fix this problem by
>>> creating a "null" chardev in this case instead.
>>
>> Isn't it possible to fix this another level further down
>> by having qemu_chr_fe_init() &c handle having a NULL chardev?
>
> Not sure, ... I don't think that we should create a "null" device in
> chardev/char-fe.c - that would sound like a layer violation to me.

It already does treat a NULL Chardev* as being "do nothing"
most of the time: eg qemu_chr_fe_write(), qemu_chr_fe_read_all(),
qemu_chr_fe_ioctl(), qemu_chr_fe_set_handlers() &c all have
"if be->chr is NULL, do nothing" checks.

> So all we could do there is to set an error in "errp" or to simply
> "ignore" the NULL pointer (so that b->chr simply gets set to NULL here).

I think that the latter seems to be what the design intends:
a backend with a NULL chardev pointer is "backend with no
chardev attached at the moment". At least some of our UART devices
handle that as "just dump output into nothingness", which
seems logical to me.

Cc'ing Marc-André and Paolo as the chardev maintainers --
do you have a plan for what the intended design here is?

thanks
-- PMM