On 23 March 2018 at 08:36, Thomas Huth <th...@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?
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 <th...@redhat.com>
> ---
> 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
