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

[Thomas Huth]

On 23.03.2018 10:56, Peter Maydell wrote:
> On 23 March 2018 at 08:36, Thomas Huth <address@hidden> 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 ([...], address@hidden, [...])
>>     at chardev/char-fe.c:210
>> 210         } else if (s->be) {
>> (gdb) bt
>>  0  0x00005555558695f8 in qemu_chr_fe_init ([...], address@hidden, [...])
>>     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.

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).

In the first case, we'd run into abort()s instead, since lots of caller
call this function with errp = &error_abort. So that does also not sound
very user-friendly (unless we change the error_aborts into error_fatals
maybe).

In the second case, QEMU seems either bails out later in serial_mm_init
(when calling serial_realize_core()) with:

$ riscv32-softmmu/qemu-system-riscv32 -nodefaults -M virt
qemu-system-riscv32: Can't create serial device, empty char device

... or it seems to start in case of the other riscv machines. Not sure
whether we can trust all the remaining functions to deal correctly with
a NULL pointer in b->chr here though... I guess we could try and find
out later where there are other crashes in that case.

Opinions?

> That would be nicer than requiring every UART model to handle
> it, because inevitably people will forget about that corner case.

At least we need some more automatic qtest coverage here... (e.g.
https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05033.html)

 Thomas