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);
/* 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, 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
© 2016 - 2026 Red Hat, Inc.