[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 01/88] ppc/pnv: Add a PNOR model
From: |
Peter Maydell |
Subject: |
Re: [PULL 01/88] ppc/pnv: Add a PNOR model |
Date: |
Tue, 7 Jan 2020 14:43:25 +0000 |
On Tue, 17 Dec 2019 at 04:43, David Gibson <address@hidden> wrote:
>
> From: Cédric Le Goater <address@hidden>
>
> On a POWERPC PowerNV system, the host firmware is stored in a PNOR
> flash chip which contents is mapped on the LPC bus. This model adds a
> simple dummy device to map the contents of a block device in the host
> address space.
>
> Signed-off-by: Cédric Le Goater <address@hidden>
> Message-Id: <address@hidden>
> Signed-off-by: David Gibson <address@hidden>
> ---
> hw/ppc/Makefile.objs | 4 +-
> hw/ppc/pnv.c | 14 ++++
> hw/ppc/pnv_pnor.c | 135 ++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/pnv.h | 3 +
> include/hw/ppc/pnv_pnor.h | 25 +++++++
> 5 files changed, 180 insertions(+), 1 deletion(-)
> create mode 100644 hw/ppc/pnv_pnor.c
> create mode 100644 include/hw/ppc/pnv_pnor.h
Hi; Coverity finds some issues in this patch:
> +static void pnv_pnor_update(PnvPnor *s, int offset, int size)
> +{
> + int offset_end;
> +
> + if (s->blk) {
> + return;
> + }
> +
> + offset_end = offset + size;
> + offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
> + offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
> +
> + blk_pwrite(s->blk, offset, s->storage + offset,
> + offset_end - offset, 0);
Here we call blk_pwrite() but don't check whether it
succeeded or failed. (CID 1412228)
> +static void pnv_pnor_realize(DeviceState *dev, Error **errp)
> +{
> + PnvPnor *s = PNV_PNOR(dev);
> + int ret;
> +
> + if (s->blk) {
> + uint64_t perm = BLK_PERM_CONSISTENT_READ |
> + (blk_is_read_only(s->blk) ? 0 : BLK_PERM_WRITE);
> + ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
> + if (ret < 0) {
> + return;
> + }
> +
> + s->size = blk_getlength(s->blk);
> + if (s->size <= 0) {
blk_getlength() returns an int64_t, but s->size is a uint32_t.
This means that this attempt to check for <= 0 doesn't
actually catch the negative values which are errors...
> + error_setg(errp, "failed to get flash size");
> + return;
> + }
> +
> + s->storage = blk_blockalign(s->blk, s->size);
...so we'll pass a very large positive number to
blk_blockalign() (since it takse a size_t argument), which
Coverity correctly identifies as doing the wrong thing.
(CID 1412226)
Side note: the blk functions here seem a bit inconsistent:
blk_getlength() returns int64_t
blk_blockalign() takes size_t
blk_pread() takes int
> +
> + if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
> + error_setg(errp, "failed to read the initial flash content");
> + return;
> + }
> + } else {
> + s->storage = blk_blockalign(NULL, s->size);
> + memset(s->storage, 0xFF, s->size);
> + }
> +
> + memory_region_init_io(&s->mmio, OBJECT(s), &pnv_pnor_ops, s,
> + TYPE_PNV_PNOR, s->size);
> +}
thanks
-- PMM
- Re: [PULL 01/88] ppc/pnv: Add a PNOR model,
Peter Maydell <=