| Message ID | 20200325191830.16553-13-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Add missing error-propagation code | expand |
On Wed, Mar 25, 2020 at 12:28 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Running the coccinelle script produced: > > $ spatch \ > --macro-file scripts/cocci-macro-file.h --include-headers \ > --sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \ > --keep-comments --smpl-spacing --dir hw > > [[manual check required: error_propagate() might be missing in object_property_set_bool() hw/riscv/sifive_u.c:558:4]] > [[manual check required: error_propagate() might be missing in object_property_set_bool() hw/riscv/sifive_u.c:561:4]] > > Add the missing error_propagate() after manual review. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > hw/riscv/sifive_u.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > index 56351c4faa..01e44018cd 100644 > --- a/hw/riscv/sifive_u.c > +++ b/hw/riscv/sifive_u.c > @@ -473,113 +473,121 @@ static void riscv_sifive_u_machine_instance_init(Object *obj) > static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > { > MachineState *ms = MACHINE(qdev_get_machine()); > SiFiveUSoCState *s = RISCV_U_SOC(dev); > const struct MemmapEntry *memmap = sifive_u_memmap; > MemoryRegion *system_memory = get_system_memory(); > MemoryRegion *mask_rom = g_new(MemoryRegion, 1); > MemoryRegion *l2lim_mem = g_new(MemoryRegion, 1); > qemu_irq plic_gpios[SIFIVE_U_PLIC_NUM_SOURCES]; > char *plic_hart_config; > size_t plic_hart_config_len; > int i; > Error *err = NULL; > NICInfo *nd = &nd_table[0]; > > object_property_set_bool(OBJECT(&s->e_cpus), true, "realized", > &error_abort); > object_property_set_bool(OBJECT(&s->u_cpus), true, "realized", > &error_abort); > /* > * The cluster must be realized after the RISC-V hart array container, > * as the container's CPU object is only created on realize, and the > * CPU must exist and have been parented into the cluster before the > * cluster is realized. > */ > object_property_set_bool(OBJECT(&s->e_cluster), true, "realized", > &error_abort); > object_property_set_bool(OBJECT(&s->u_cluster), true, "realized", > &error_abort); > > /* boot rom */ > memory_region_init_rom(mask_rom, OBJECT(dev), "riscv.sifive.u.mrom", > memmap[SIFIVE_U_MROM].size, &error_fatal); > memory_region_add_subregion(system_memory, memmap[SIFIVE_U_MROM].base, > mask_rom); > > /* > * Add L2-LIM at reset size. > * This should be reduced in size as the L2 Cache Controller WayEnable > * register is incremented. Unfortunately I don't see a nice (or any) way > * to handle reducing or blocking out the L2 LIM while still allowing it > * be re returned to all enabled after a reset. For the time being, just > * leave it enabled all the time. This won't break anything, but will be > * too generous to misbehaving guests. > */ > memory_region_init_ram(l2lim_mem, NULL, "riscv.sifive.u.l2lim", > memmap[SIFIVE_U_L2LIM].size, &error_fatal); > memory_region_add_subregion(system_memory, memmap[SIFIVE_U_L2LIM].base, > l2lim_mem); > > /* create PLIC hart topology configuration string */ > plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) * > ms->smp.cpus; > plic_hart_config = g_malloc0(plic_hart_config_len); > for (i = 0; i < ms->smp.cpus; i++) { > if (i != 0) { > strncat(plic_hart_config, "," SIFIVE_U_PLIC_HART_CONFIG, > plic_hart_config_len); > } else { > strncat(plic_hart_config, "M", plic_hart_config_len); > } > plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); > } > > /* MMIO */ > s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base, > plic_hart_config, > SIFIVE_U_PLIC_NUM_SOURCES, > SIFIVE_U_PLIC_NUM_PRIORITIES, > SIFIVE_U_PLIC_PRIORITY_BASE, > SIFIVE_U_PLIC_PENDING_BASE, > SIFIVE_U_PLIC_ENABLE_BASE, > SIFIVE_U_PLIC_ENABLE_STRIDE, > SIFIVE_U_PLIC_CONTEXT_BASE, > SIFIVE_U_PLIC_CONTEXT_STRIDE, > memmap[SIFIVE_U_PLIC].size); > g_free(plic_hart_config); > sifive_uart_create(system_memory, memmap[SIFIVE_U_UART0].base, > serial_hd(0), qdev_get_gpio_in(DEVICE(s->plic), SIFIVE_U_UART0_IRQ)); > sifive_uart_create(system_memory, memmap[SIFIVE_U_UART1].base, > serial_hd(1), qdev_get_gpio_in(DEVICE(s->plic), SIFIVE_U_UART1_IRQ)); > sifive_clint_create(memmap[SIFIVE_U_CLINT].base, > memmap[SIFIVE_U_CLINT].size, ms->smp.cpus, > SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE, false); > > object_property_set_bool(OBJECT(&s->prci), true, "realized", &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base); > > object_property_set_bool(OBJECT(&s->otp), true, "realized", &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base); > > for (i = 0; i < SIFIVE_U_PLIC_NUM_SOURCES; i++) { > plic_gpios[i] = qdev_get_gpio_in(DEVICE(s->plic), i); > } > > if (nd->used) { > qemu_check_nic_model(nd, TYPE_CADENCE_GEM); > qdev_set_nic_properties(DEVICE(&s->gem), nd); > } > object_property_set_int(OBJECT(&s->gem), GEM_REVISION, "revision", > &error_abort); > object_property_set_bool(OBJECT(&s->gem), true, "realized", &err); > if (err) { > error_propagate(errp, err); > return; > } > sysbus_mmio_map(SYS_BUS_DEVICE(&s->gem), 0, memmap[SIFIVE_U_GEM].base); > sysbus_connect_irq(SYS_BUS_DEVICE(&s->gem), 0, > plic_gpios[SIFIVE_U_GEM_IRQ]); > > create_unimplemented_device("riscv.sifive.u.gem-mgmt", > memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size); > } > -- > 2.21.1 > >
On Wed, 25 Mar 2020 at 19:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Running the coccinelle script produced: > > $ spatch \ > --macro-file scripts/cocci-macro-file.h --include-headers \ > --sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \ > --keep-comments --smpl-spacing --dir hw > > [[manual check required: error_propagate() might be missing in object_property_set_bool() hw/riscv/sifive_u.c:558:4]] > [[manual check required: error_propagate() might be missing in object_property_set_bool() hw/riscv/sifive_u.c:561:4]] > > Add the missing error_propagate() after manual review. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/riscv/sifive_u.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > index 56351c4faa..01e44018cd 100644 > --- a/hw/riscv/sifive_u.c > +++ b/hw/riscv/sifive_u.c > @@ -473,113 +473,121 @@ static void riscv_sifive_u_machine_instance_init(Object *obj) > static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > { > MachineState *ms = MACHINE(qdev_get_machine()); > SiFiveUSoCState *s = RISCV_U_SOC(dev); > const struct MemmapEntry *memmap = sifive_u_memmap; > MemoryRegion *system_memory = get_system_memory(); > MemoryRegion *mask_rom = g_new(MemoryRegion, 1); > MemoryRegion *l2lim_mem = g_new(MemoryRegion, 1); > qemu_irq plic_gpios[SIFIVE_U_PLIC_NUM_SOURCES]; > char *plic_hart_config; > size_t plic_hart_config_len; > int i; > Error *err = NULL; > NICInfo *nd = &nd_table[0]; > > object_property_set_bool(OBJECT(&s->e_cpus), true, "realized", > &error_abort); > object_property_set_bool(OBJECT(&s->u_cpus), true, "realized", > &error_abort); > /* > * The cluster must be realized after the RISC-V hart array container, > * as the container's CPU object is only created on realize, and the > * CPU must exist and have been parented into the cluster before the > * cluster is realized. > */ > object_property_set_bool(OBJECT(&s->e_cluster), true, "realized", > &error_abort); > object_property_set_bool(OBJECT(&s->u_cluster), true, "realized", > &error_abort); Different bug noticed in passing: these really ought not to be using error_abort to realize things, as realize is a fairly likely-to-fail operation on most objects (either now or in the future if the object implementation changes). > > /* boot rom */ > memory_region_init_rom(mask_rom, OBJECT(dev), "riscv.sifive.u.mrom", > memmap[SIFIVE_U_MROM].size, &error_fatal); > memory_region_add_subregion(system_memory, memmap[SIFIVE_U_MROM].base, > mask_rom); > object_property_set_bool(OBJECT(&s->prci), true, "realized", &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base); > > object_property_set_bool(OBJECT(&s->otp), true, "realized", &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } The changes made in this patch are fine though: Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 3/26/20 10:55 PM, Peter Maydell wrote: > On Wed, 25 Mar 2020 at 19:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> Running the coccinelle script produced: >> >> $ spatch \ >> --macro-file scripts/cocci-macro-file.h --include-headers \ >> --sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \ >> --keep-comments --smpl-spacing --dir hw >> >> [[manual check required: error_propagate() might be missing in object_property_set_bool() hw/riscv/sifive_u.c:558:4]] >> [[manual check required: error_propagate() might be missing in object_property_set_bool() hw/riscv/sifive_u.c:561:4]] >> >> Add the missing error_propagate() after manual review. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/riscv/sifive_u.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c >> index 56351c4faa..01e44018cd 100644 >> --- a/hw/riscv/sifive_u.c >> +++ b/hw/riscv/sifive_u.c >> @@ -473,113 +473,121 @@ static void riscv_sifive_u_machine_instance_init(Object *obj) >> static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) >> { >> MachineState *ms = MACHINE(qdev_get_machine()); >> SiFiveUSoCState *s = RISCV_U_SOC(dev); >> const struct MemmapEntry *memmap = sifive_u_memmap; >> MemoryRegion *system_memory = get_system_memory(); >> MemoryRegion *mask_rom = g_new(MemoryRegion, 1); >> MemoryRegion *l2lim_mem = g_new(MemoryRegion, 1); >> qemu_irq plic_gpios[SIFIVE_U_PLIC_NUM_SOURCES]; >> char *plic_hart_config; >> size_t plic_hart_config_len; >> int i; >> Error *err = NULL; >> NICInfo *nd = &nd_table[0]; >> >> object_property_set_bool(OBJECT(&s->e_cpus), true, "realized", >> &error_abort); >> object_property_set_bool(OBJECT(&s->u_cpus), true, "realized", >> &error_abort); >> /* >> * The cluster must be realized after the RISC-V hart array container, >> * as the container's CPU object is only created on realize, and the >> * CPU must exist and have been parented into the cluster before the >> * cluster is realized. >> */ >> object_property_set_bool(OBJECT(&s->e_cluster), true, "realized", >> &error_abort); >> object_property_set_bool(OBJECT(&s->u_cluster), true, "realized", >> &error_abort); > > Different bug noticed in passing: these really ought not to be > using error_abort to realize things, as realize is a fairly > likely-to-fail operation on most objects (either now or in > the future if the object implementation changes). OK. > >> >> /* boot rom */ >> memory_region_init_rom(mask_rom, OBJECT(dev), "riscv.sifive.u.mrom", >> memmap[SIFIVE_U_MROM].size, &error_fatal); What about this memory_region_init_rom() call (and later memory_region_init_ram) using error_fatal, same reasoning right? >> memory_region_add_subregion(system_memory, memmap[SIFIVE_U_MROM].base, >> mask_rom); > >> object_property_set_bool(OBJECT(&s->prci), true, "realized", &err); >> + if (err) { >> + error_propagate(errp, err); >> + return; >> + } >> sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base); >> >> object_property_set_bool(OBJECT(&s->otp), true, "realized", &err); >> + if (err) { >> + error_propagate(errp, err); >> + return; >> + } > > The changes made in this patch are fine though: > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > thanks > -- PMM >
On Tue, 31 Mar 2020 at 18:02, Philippe Mathieu-Daudé <philmd@redhat.com> wrote > What about this memory_region_init_rom() call (and later > memory_region_init_ram) using error_fatal, same reasoning right? Yes. -- PMM
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index 56351c4faa..01e44018cd 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -473,113 +473,121 @@ static void riscv_sifive_u_machine_instance_init(Object *obj) static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) { MachineState *ms = MACHINE(qdev_get_machine()); SiFiveUSoCState *s = RISCV_U_SOC(dev); const struct MemmapEntry *memmap = sifive_u_memmap; MemoryRegion *system_memory = get_system_memory(); MemoryRegion *mask_rom = g_new(MemoryRegion, 1); MemoryRegion *l2lim_mem = g_new(MemoryRegion, 1); qemu_irq plic_gpios[SIFIVE_U_PLIC_NUM_SOURCES]; char *plic_hart_config; size_t plic_hart_config_len; int i; Error *err = NULL; NICInfo *nd = &nd_table[0]; object_property_set_bool(OBJECT(&s->e_cpus), true, "realized", &error_abort); object_property_set_bool(OBJECT(&s->u_cpus), true, "realized", &error_abort); /* * The cluster must be realized after the RISC-V hart array container, * as the container's CPU object is only created on realize, and the * CPU must exist and have been parented into the cluster before the * cluster is realized. */ object_property_set_bool(OBJECT(&s->e_cluster), true, "realized", &error_abort); object_property_set_bool(OBJECT(&s->u_cluster), true, "realized", &error_abort); /* boot rom */ memory_region_init_rom(mask_rom, OBJECT(dev), "riscv.sifive.u.mrom", memmap[SIFIVE_U_MROM].size, &error_fatal); memory_region_add_subregion(system_memory, memmap[SIFIVE_U_MROM].base, mask_rom); /* * Add L2-LIM at reset size. * This should be reduced in size as the L2 Cache Controller WayEnable * register is incremented. Unfortunately I don't see a nice (or any) way * to handle reducing or blocking out the L2 LIM while still allowing it * be re returned to all enabled after a reset. For the time being, just * leave it enabled all the time. This won't break anything, but will be * too generous to misbehaving guests. */ memory_region_init_ram(l2lim_mem, NULL, "riscv.sifive.u.l2lim", memmap[SIFIVE_U_L2LIM].size, &error_fatal); memory_region_add_subregion(system_memory, memmap[SIFIVE_U_L2LIM].base, l2lim_mem); /* create PLIC hart topology configuration string */ plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) * ms->smp.cpus; plic_hart_config = g_malloc0(plic_hart_config_len); for (i = 0; i < ms->smp.cpus; i++) { if (i != 0) { strncat(plic_hart_config, "," SIFIVE_U_PLIC_HART_CONFIG, plic_hart_config_len); } else { strncat(plic_hart_config, "M", plic_hart_config_len); } plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); } /* MMIO */ s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base, plic_hart_config, SIFIVE_U_PLIC_NUM_SOURCES, SIFIVE_U_PLIC_NUM_PRIORITIES, SIFIVE_U_PLIC_PRIORITY_BASE, SIFIVE_U_PLIC_PENDING_BASE, SIFIVE_U_PLIC_ENABLE_BASE, SIFIVE_U_PLIC_ENABLE_STRIDE, SIFIVE_U_PLIC_CONTEXT_BASE, SIFIVE_U_PLIC_CONTEXT_STRIDE, memmap[SIFIVE_U_PLIC].size); g_free(plic_hart_config); sifive_uart_create(system_memory, memmap[SIFIVE_U_UART0].base, serial_hd(0), qdev_get_gpio_in(DEVICE(s->plic), SIFIVE_U_UART0_IRQ)); sifive_uart_create(system_memory, memmap[SIFIVE_U_UART1].base, serial_hd(1), qdev_get_gpio_in(DEVICE(s->plic), SIFIVE_U_UART1_IRQ)); sifive_clint_create(memmap[SIFIVE_U_CLINT].base, memmap[SIFIVE_U_CLINT].size, ms->smp.cpus, SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE, false); object_property_set_bool(OBJECT(&s->prci), true, "realized", &err); + if (err) { + error_propagate(errp, err); + return; + } sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base); object_property_set_bool(OBJECT(&s->otp), true, "realized", &err); + if (err) { + error_propagate(errp, err); + return; + } sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base); for (i = 0; i < SIFIVE_U_PLIC_NUM_SOURCES; i++) { plic_gpios[i] = qdev_get_gpio_in(DEVICE(s->plic), i); } if (nd->used) { qemu_check_nic_model(nd, TYPE_CADENCE_GEM); qdev_set_nic_properties(DEVICE(&s->gem), nd); } object_property_set_int(OBJECT(&s->gem), GEM_REVISION, "revision", &error_abort); object_property_set_bool(OBJECT(&s->gem), true, "realized", &err); if (err) { error_propagate(errp, err); return; } sysbus_mmio_map(SYS_BUS_DEVICE(&s->gem), 0, memmap[SIFIVE_U_GEM].base); sysbus_connect_irq(SYS_BUS_DEVICE(&s->gem), 0, plic_gpios[SIFIVE_U_GEM_IRQ]); create_unimplemented_device("riscv.sifive.u.gem-mgmt", memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size); }
Running the coccinelle script produced: $ spatch \ --macro-file scripts/cocci-macro-file.h --include-headers \ --sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \ --keep-comments --smpl-spacing --dir hw [[manual check required: error_propagate() might be missing in object_property_set_bool() hw/riscv/sifive_u.c:558:4]] [[manual check required: error_propagate() might be missing in object_property_set_bool() hw/riscv/sifive_u.c:561:4]] Add the missing error_propagate() after manual review. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/riscv/sifive_u.c | 8 ++++++++ 1 file changed, 8 insertions(+)