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

[Markus Armbruster]

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,
-- 
2.17.2