Re: [Qemu-devel] [PATCH v8 1/2] pflash: Require backend size to match device, improve errors

[Laszlo Ersek]

On 03/19/19 17:35, Markus Armbruster wrote:
> 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 <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  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,
> 

Reviewed-by: Laszlo Ersek <address@hidden>

Bonus points for "eins.img" and "zwei.img"! :)

Thanks,
Laszlo