Le mar. 19 mars 2019 17:35, Markus Armbruster <arm...@redhat.com> a écrit :
> 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 <alex.ben...@linaro.org>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
> 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.17.2
>
Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>
