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

[Peter Maydell]

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?
That would be nicer than requiring every UART model to handle
it, because inevitably people will forget about that corner case.

> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  For other boards / targets, see also:
>  https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05073.html
>
>  hw/riscv/riscv_htif.c  | 5 +++++
>  hw/riscv/sifive_uart.c | 5 +++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/hw/riscv/riscv_htif.c b/hw/riscv/riscv_htif.c
> index 3e17f30..d3d31ff 100644
> --- a/hw/riscv/riscv_htif.c
> +++ b/hw/riscv/riscv_htif.c
> @@ -245,9 +245,14 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, 
> MemoryRegion *main_mem,
>      s->pending_read = 0;
>      s->allow_tohost = 0;
>      s->fromhost_inprogress = 0;
> +
> +    if (!chr) {
> +        chr = qemu_chardev_new(NULL, TYPE_CHARDEV_NULL, NULL, &error_abort);
> +    }
>      qemu_chr_fe_init(&s->chr, chr, &error_abort);
>      qemu_chr_fe_set_handlers(&s->chr, htif_can_recv, htif_recv, htif_event,
>          htif_be_change, s, NULL, true);
> +
>      if (base) {
>          memory_region_init_io(&s->mmio, NULL, &htif_mm_ops, s,
>                              TYPE_HTIF_UART, size);

As an aside: this source file looks like it's a device, but
it isn't a QOM device. That seems like it needs fixing, and it
should probably be in hw/char or hw/misc.

> diff --git a/hw/riscv/sifive_uart.c b/hw/riscv/sifive_uart.c
> index b0c3798..2bde8bb 100644
> --- a/hw/riscv/sifive_uart.c
> +++ b/hw/riscv/sifive_uart.c

If this is a UART why isn't it in hw/char ? It should also
be a proper QOM device, and moved to the right place in
the source tree.

> @@ -166,9 +166,14 @@ SiFiveUARTState *sifive_uart_create(MemoryRegion 
> *address_space, hwaddr base,
>  {
>      SiFiveUARTState *s = g_malloc0(sizeof(SiFiveUARTState));
>      s->irq = irq;
> +
> +    if (!chr) {
> +        chr = qemu_chardev_new(NULL, TYPE_CHARDEV_NULL, NULL, &error_abort);
> +    }
>      qemu_chr_fe_init(&s->chr, chr, &error_abort);
>      qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx, uart_event,
>          uart_be_change, s, NULL, true);
> +
>      memory_region_init_io(&s->mmio, NULL, &uart_ops, s,
>                            TYPE_SIFIVE_UART, SIFIVE_UART_MAX);
>      memory_region_add_subregion(address_space, base, &s->mmio);
> --
> 1.8.3.1

thanks
-- PMM