From 06f1521795207359a395996c253c306f4ab7586e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 19 Mar 2019 17:35:50 +0100 Subject: [PATCH] pflash: Require backend size to match device, improve errors MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit We reject undersized backends with a rather enigmatic "failed to read the initial flash content" error. For instance: $ qemu-system-ppc64 -S -display none -M sam460ex -drive if=pflash,format=raw,file=eins.img qemu-system-ppc64: Initialization of device cfi.pflash02 failed: failed to read the initial flash content We happily accept oversized images, ignoring their tail. Throwing away parts of firmware that way is pretty much certain to end in an even more enigmatic failure to boot. Require the backend's size to match the device's size exactly. Report mismatch like this: qemu-system-ppc64: Initialization of device cfi.pflash01 failed: device requires 1048576 bytes, block backend provides 512 bytes Improve the error for actual read failures to "can't read block backend". To avoid duplicating even more code between the two pflash device models, do all that in new helper blk_check_size_and_read_all(). The error reporting can still be confusing. For instance: qemu-system-ppc64 -S -display none -M taihu -drive if=pflash,format=raw,file=eins.img -drive if=pflash,unit=1,format=raw,file=zwei.img qemu-system-ppc64: Initialization of device cfi.pflash02 failed: device requires 2097152 bytes, block backend provides 512 bytes Leaves the user guessing which of the two -drive is wrong. Mention the issue in a TODO comment. Suggested-by: Alex Bennée Signed-off-by: Markus Armbruster Message-Id: <20190319163551.32499-2-armbru@redhat.com> Reviewed-by: Laszlo Ersek Reviewed-by: Alex Bennée Reviewed-by: Philippe Mathieu-Daudé --- hw/block/block.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- hw/block/pflash_cfi01.c | 8 +++----- hw/block/pflash_cfi02.c | 7 +++---- include/hw/block/block.h | 7 ++++++- 4 files changed, 59 insertions(+), 11 deletions(-) diff --git a/hw/block/block.c b/hw/block/block.c index cf0eb826f1..bf56c7612b 100644 --- a/hw/block/block.c +++ b/hw/block/block.c @@ -13,7 +13,53 @@ #include "hw/block/block.h" #include "qapi/error.h" #include "qapi/qapi-types-block.h" -#include "qemu/error-report.h" + +/* + * Read the entire contents of @blk into @buf. + * @blk's contents must be @size bytes, and @size must be at most + * BDRV_REQUEST_MAX_BYTES. + * On success, return true. + * On failure, store an error through @errp and return false. + * Note that the error messages do not identify the block backend. + * TODO Since callers don't either, this can result in confusing + * errors. + * This function not intended for actual block devices, which read on + * demand. It's for things like memory devices that (ab)use a block + * backend to provide persistence. + */ +bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size, + Error **errp) +{ + int64_t blk_len; + int ret; + + blk_len = blk_getlength(blk); + if (blk_len < 0) { + error_setg_errno(errp, -blk_len, + "can't get size of block backend"); + return false; + } + if (blk_len != size) { + error_setg(errp, "device requires %" HWADDR_PRIu " bytes, " + "block backend provides %" PRIu64 " bytes", + size, blk_len); + return false; + } + + /* + * We could loop for @size > BDRV_REQUEST_MAX_BYTES, but if we + * ever get to the point we want to read *gigabytes* here, we + * should probably rework the device to be more like an actual + * block device and read only on demand. + */ + assert(size <= BDRV_REQUEST_MAX_BYTES); + ret = blk_pread(blk, 0, buf, size); + if (ret < 0) { + error_setg_errno(errp, -ret, "can't read block backend"); + return false; + } + return true; +} void blkconf_blocksizes(BlockConf *conf) { diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index 125f70b8e4..9937739a82 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -38,6 +38,7 @@ #include "qemu/osdep.h" #include "hw/hw.h" +#include "hw/block/block.h" #include "hw/block/flash.h" #include "sysemu/block-backend.h" #include "qapi/error.h" @@ -763,12 +764,9 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) } if (pfl->blk) { - /* read the initial flash content */ - ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); - - if (ret < 0) { + if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len, + errp)) { vmstate_unregister_ram(&pfl->mem, DEVICE(pfl)); - error_setg(errp, "failed to read the initial flash content"); return; } } diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c index c9db430611..9b934305fa 100644 --- a/hw/block/pflash_cfi02.c +++ b/hw/block/pflash_cfi02.c @@ -37,6 +37,7 @@ #include "qemu/osdep.h" #include "hw/hw.h" +#include "hw/block/block.h" #include "hw/block/flash.h" #include "qapi/error.h" #include "qemu/timer.h" @@ -581,11 +582,9 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp) } if (pfl->blk) { - /* read the initial flash content */ - ret = blk_pread(pfl->blk, 0, pfl->storage, chip_len); - if (ret < 0) { + if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, chip_len, + errp)) { vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl)); - error_setg(errp, "failed to read the initial flash content"); return; } } diff --git a/include/hw/block/block.h b/include/hw/block/block.h index e9f9e2223f..d06f25aa0f 100644 --- a/include/hw/block/block.h +++ b/include/hw/block/block.h @@ -11,7 +11,7 @@ #ifndef HW_BLOCK_H #define HW_BLOCK_H -#include "qemu-common.h" +#include "exec/hwaddr.h" #include "qapi/qapi-types-block-core.h" /* Configuration */ @@ -70,6 +70,11 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) DEFINE_PROP_BLOCKDEV_ON_ERROR("werror", _state, _conf.werror, \ BLOCKDEV_ON_ERROR_AUTO) +/* Backend access helpers */ + +bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size, + Error **errp); + /* Configuration helpers */ bool blkconf_geometry(BlockConf *conf, int *trans, -- 2.11.0