QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: "Peter Maydell" <peter.maydell@linaro.org>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: Paul Burton <pburton@wavecomp.com>,
	Sagar Karandikar <sagark@eecs.berkeley.edu>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Jason Wang <jasowang@redhat.com>,
	Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
	QEMU Developers <qemu-devel@nongnu.org>,
	Alistair Francis <Alistair.Francis@wdc.com>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
	Markus Armbruster <armbru@redhat.com>,
	Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>,
	Richard Henderson <rth@twiddle.net>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Alistair Francis <alistair@alistair23.me>,
	qemu-arm <qemu-arm@nongnu.org>,
	David Gibson <david@gibson.dropbear.id.au>,
	"open list:RISC-V" <qemu-riscv@nongnu.org>,
	Bastian Koppelmann <kbastian@mail.uni-paderborn.de>,
	Andrew Baumann <Andrew.Baumann@microsoft.com>,
	Jean-Christophe Dubois <jcd@tribudubois.net>,
	qemu-ppc <qemu-ppc@nongnu.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Aurelien Jarno <aurelien@aurel32.net>
Subject: Re: [PATCH-for-5.0 12/12] hw/riscv/sifive_u: Add missing error-propagation code
Date: Tue, 31 Mar 2020 19:02:35 +0200
Message-ID: <e293f19c-7005-16f0-57df-55ce953fe0a4@redhat.com> (raw)
In-Reply-To: <CAFEAcA-26fHbOp5saM+XBCz72fzfz+=+xtiXGRtWnc6CMoiakA@mail.gmail.com>

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
> 



  reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25 19:18 [PATCH-for-5.0 00/12] hw: " Philippe Mathieu-Daudé
2020-03-25 19:18 ` [PATCH-for-5.0 01/12] scripts/coccinelle: Add script to catch missing error_propagate() calls Philippe Mathieu-Daudé
2020-03-26 21:32   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 02/12] hw/arm/bcm2835_peripherals: Add missing error-propagation code Philippe Mathieu-Daudé
2020-03-26 21:34   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 03/12] hw/arm/fsl-imx: " Philippe Mathieu-Daudé
2020-03-26 21:35   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 04/12] hw/arm/stm32fx05_soc: " Philippe Mathieu-Daudé
2020-03-25 20:51   ` Alistair Francis
2020-03-26 21:45   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 05/12] hw/i386/x86: " Philippe Mathieu-Daudé
2020-03-26 21:38   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 06/12] hw/dma/xilinx_axidma: " Philippe Mathieu-Daudé
2020-03-25 20:52   ` Alistair Francis
2020-03-26 21:46   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 07/12] hw/mips/cps: " Philippe Mathieu-Daudé
2020-03-26 21:43   ` Peter Maydell
2020-03-26 22:48   ` Aleksandar Markovic
2020-03-25 19:18 ` [PATCH-for-5.0 08/12] hw/mips/boston: " Philippe Mathieu-Daudé
2020-03-26 21:47   ` Peter Maydell
2020-03-26 22:50   ` Aleksandar Markovic
2020-03-25 19:18 ` [PATCH-for-5.0 09/12] hw/mips/mips_malta: " Philippe Mathieu-Daudé
2020-03-26 21:49   ` Peter Maydell
2020-03-26 22:49   ` Aleksandar Markovic
2020-03-25 19:18 ` [PATCH-for-5.0 10/12] hw/misc/macio/macio: " Philippe Mathieu-Daudé
2020-03-25 23:55   ` David Gibson
2020-03-26 21:50   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 11/12] hw/net/xilinx_axienet: " Philippe Mathieu-Daudé
2020-03-25 20:52   ` Alistair Francis
2020-03-26 21:51   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 12/12] hw/riscv/sifive_u: " Philippe Mathieu-Daudé
2020-03-25 20:52   ` Alistair Francis
2020-03-26 21:55   ` Peter Maydell
2020-03-31 17:02     ` Philippe Mathieu-Daudé [this message]
2020-03-31 17:03       ` Peter Maydell
2020-03-25 19:20 ` [PATCH-for-5.0 00/12] hw: " Philippe Mathieu-Daudé
2020-03-30  9:21   ` Stefan Hajnoczi
2020-04-06 17:47     ` Philippe Mathieu-Daudé
2020-03-31 13:23 ` Markus Armbruster
2020-04-03 17:53   ` Philippe Mathieu-Daudé
2020-04-04  5:55     ` Markus Armbruster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e293f19c-7005-16f0-57df-55ce953fe0a4@redhat.com \
    --to=philmd@redhat.com \
    --cc=Alistair.Francis@wdc.com \
    --cc=Andrew.Baumann@microsoft.com \
    --cc=aleksandar.qemu.devel@gmail.com \
    --cc=aleksandar.rikalo@rt-rk.com \
    --cc=alistair@alistair23.me \
    --cc=armbru@redhat.com \
    --cc=aurelien@aurel32.net \
    --cc=david@gibson.dropbear.id.au \
    --cc=edgar.iglesias@gmail.com \
    --cc=ehabkost@redhat.com \
    --cc=f4bug@amsat.org \
    --cc=jasowang@redhat.com \
    --cc=jcd@tribudubois.net \
    --cc=kbastian@mail.uni-paderborn.de \
    --cc=mark.cave-ayland@ilande.co.uk \
    --cc=mst@redhat.com \
    --cc=palmer@dabbelt.com \
    --cc=pbonzini@redhat.com \
    --cc=pburton@wavecomp.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --cc=qemu-riscv@nongnu.org \
    --cc=rth@twiddle.net \
    --cc=sagark@eecs.berkeley.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git