1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch

Alex Bennée posted 1 patch 6 years, 7 months ago
Test asan passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu failed
Test checkpatch failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190222154454.30289-1-alex.bennee@linaro.org
There is a newer version of this series
hw/block/pflash_cfi01.c | 40 +++++++++++++++++++++++++++++++++-------
1 file changed, 33 insertions(+), 7 deletions(-)
[Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
Posted by Alex Bennée 6 years, 7 months ago 
It looks like there was going to be code to check we had some sort of
alignment so lets replace it with an actual check. This is a bit more
useful than the enigmatic "failed to read the initial flash content"
when we attempt to read the number of bytes the device should have.

This is a potential confusing stumbling block when you move from using
-bios to using -drive if=pflash,file=blob,format=raw,readonly for
loading your firmware code. To mitigate that we automatically pad in
the read-only case and warn the user when we have performed magic to
enable things to Just Work (tm).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v3
  - tweak commit title/commentary
  - use total_len instead of device_len for checks
  - if the device is read-only do the padding for them
  - accept baking_len > total_len (how to warn_report with NULL *errp?)
v4
  - error check blk_getlength
  - optimise memset and use NOR erase pattern
  - restore singular device (overly confusing)
  - add warn_report for when we do magic
---
 hw/block/pflash_cfi01.c | 40 +++++++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 00c2efd0d7..c69ecc20a0 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -45,6 +45,7 @@
 #include "qemu/bitops.h"
 #include "qemu/host-utils.h"
 #include "qemu/log.h"
+#include "qemu/error-report.h"
 #include "hw/sysbus.h"
 #include "sysemu/sysemu.h"
 #include "trace.h"
@@ -714,13 +715,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
     device_len = sector_len_per_device * blocks_per_device;
 
-    /* XXX: to be fixed */
-#if 0
-    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
-        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
-        return NULL;
-#endif
-
     memory_region_init_rom_device(
         &pfl->mem, OBJECT(dev),
         &pflash_cfi01_ops,
@@ -747,6 +741,38 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
 
     if (pfl->blk) {
+        /*
+         * Validate the backing store is the right size for pflash
+         * devices. It should be padded to a multiple of the flash
+         * block size. If the device is read-only we can elide the
+         * check and just null pad the region first. If the user
+         * supplies a larger file we ignore the tail.
+         */
+        int64_t backing_len = blk_getlength(pfl->blk);
+        if (backing_len < 0) {
+            error_setg(errp, "unable to check size of backing file");
+            return;
+        }
+
+        if (backing_len < total_len) {
+            if (pfl->ro) {
+                size_t pad_bytes = total_len - backing_len;
+                /* pad with NOR erase pattern */
+                memset((uint8_t*)pfl->storage + backing_len, 0xff, pad_bytes);
+                warn_report("device needs %" PRIu64
+                            " bytes, padded with %zd 0xff bytes",
+                            total_len, pad_bytes);
+                total_len = backing_len;
+            } else {
+                error_setg(errp, "device needs %" PRIu64 " bytes, "
+                           "backing file provides only %" PRIu64 " bytes",
+                           total_len, backing_len);
+                return;
+            }
+        } else if (backing_len > total_len) {
+            warn_report("device needs %" PRIu64 " bytes, rest ignored", total_len);
+        }
+
         /* read the initial flash content */
         ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
 
-- 
2.20.1
Re: [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
Posted by no-reply@patchew.org 6 years, 7 months ago 
Patchew URL: https://patchew.org/QEMU/20190222154454.30289-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190222154454.30289-1-alex.bennee@linaro.org
Subject: [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190220010232.18731-1-philmd@redhat.com -> patchew/20190220010232.18731-1-philmd@redhat.com
Switched to a new branch 'test'
d617d4d4eb hw/block: better reporting on pflash backing file mismatch

=== OUTPUT BEGIN ===
ERROR: "(foo*)" should be "(foo *)"
#70: FILE: hw/block/pflash_cfi01.c:769:
+                memset((uint8_t*)pfl->storage + backing_len, 0xff, pad_bytes);

WARNING: line over 80 characters
#82: FILE: hw/block/pflash_cfi01.c:781:
+            warn_report("device needs %" PRIu64 " bytes, rest ignored", total_len);

total: 1 errors, 1 warnings, 58 lines checked

Commit d617d4d4eb13 (hw/block: better reporting on pflash backing file mismatch) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190222154454.30289-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
Posted by no-reply@patchew.org 6 years, 7 months ago 
Patchew URL: https://patchew.org/QEMU/20190222154454.30289-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190222154454.30289-1-alex.bennee@linaro.org
Type: series
Subject: [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1550863578-30859-1-git-send-email-aleksandar.markovic@rt-rk.com -> patchew/1550863578-30859-1-git-send-email-aleksandar.markovic@rt-rk.com
 * [new tag]         patchew/1550878686-23934-1-git-send-email-michelheily@gmail.com -> patchew/1550878686-23934-1-git-send-email-michelheily@gmail.com
 * [new tag]         patchew/1551185735-17154-1-git-send-email-aleksandar.markovic@rt-rk.com -> patchew/1551185735-17154-1-git-send-email-aleksandar.markovic@rt-rk.com
 - [tag update]      patchew/20190220145819.30969-1-berrange@redhat.com -> patchew/20190220145819.30969-1-berrange@redhat.com
 * [new tag]         patchew/20190220180112.28250-1-jsnow@redhat.com -> patchew/20190220180112.28250-1-jsnow@redhat.com
 * [new tag]         patchew/20190221101139.2224-1-stefanha@redhat.com -> patchew/20190221101139.2224-1-stefanha@redhat.com
 - [tag update]      patchew/20190222031413.20250-1-peterx@redhat.com -> patchew/20190222031413.20250-1-peterx@redhat.com
 * [new tag]         patchew/20190222141024.22217-1-kbastian@mail.uni-paderborn.de -> patchew/20190222141024.22217-1-kbastian@mail.uni-paderborn.de
 * [new tag]         patchew/20190222154454.30289-1-alex.bennee@linaro.org -> patchew/20190222154454.30289-1-alex.bennee@linaro.org
 * [new tag]         patchew/20190222204500.24434-1-alex.bennee@linaro.org -> patchew/20190222204500.24434-1-alex.bennee@linaro.org
 * [new tag]         patchew/20190225102433.22401-1-peter.maydell@linaro.org -> patchew/20190225102433.22401-1-peter.maydell@linaro.org
 * [new tag]         patchew/20190226045304.25618-1-david@gibson.dropbear.id.au -> patchew/20190226045304.25618-1-david@gibson.dropbear.id.au
 * [new tag]         patchew/20190226165013.24867-1-eblake@redhat.com -> patchew/20190226165013.24867-1-eblake@redhat.com
 * [new tag]         patchew/20190227131433.197063-1-vsementsov@virtuozzo.com -> patchew/20190227131433.197063-1-vsementsov@virtuozzo.com
 * [new tag]         patchew/20190227132413.13213-1-dgilbert@redhat.com -> patchew/20190227132413.13213-1-dgilbert@redhat.com
 * [new tag]         patchew/20190227135523.16952-1-berrange@redhat.com -> patchew/20190227135523.16952-1-berrange@redhat.com
 * [new tag]         patchew/20190227140629.1569-1-yuval.shaia@oracle.com -> patchew/20190227140629.1569-1-yuval.shaia@oracle.com
 * [new tag]         patchew/20190227144413.30975-1-stefanha@redhat.com -> patchew/20190227144413.30975-1-stefanha@redhat.com
 * [new tag]         patchew/20190227145324.26188-1-berrange@redhat.com -> patchew/20190227145324.26188-1-berrange@redhat.com
 * [new tag]         patchew/20190227145755.26556-1-berrange@redhat.com -> patchew/20190227145755.26556-1-berrange@redhat.com
 - [tag update]      patchew/cover.1550836631.git.berto@igalia.com -> patchew/cover.1550836631.git.berto@igalia.com
Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
Submodule 'roms/seabios-hppa' (https://github.com/hdeller/seabios-hppa.git) registered for path 'roms/seabios-hppa'
Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
Submodule 'tests/fp/berkeley-softfloat-3' (https://github.com/cota/berkeley-softfloat-3) registered for path 'tests/fp/berkeley-softfloat-3'
Submodule 'tests/fp/berkeley-testfloat-3' (https://github.com/cota/berkeley-testfloat-3) registered for path 'tests/fp/berkeley-testfloat-3'
Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into 'capstone'...
Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Cloning into 'roms/QemuMacDrivers'...
Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out 'a5b428e1c1eae703bdd62a3f527223c291ee3fdc'
Cloning into 'roms/ipxe'...
Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
Cloning into 'roms/openbios'...
Submodule path 'roms/openbios': checked out '3464681b2b5983df80086a40179d324102347da3'
Cloning into 'roms/openhackware'...
Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/qemu-palcode'...
Submodule path 'roms/qemu-palcode': checked out '51c237d7e20d05100eacadee2f61abc17e6bc097'
Cloning into 'roms/seabios'...
Submodule path 'roms/seabios': checked out 'a698c8995ffb2838296ec284fe3c4ad33dfca307'
Cloning into 'roms/seabios-hppa'...
Submodule path 'roms/seabios-hppa': checked out '1ef99a01572c2581c30e16e6fe69e9ea2ef92ce0'
Cloning into 'roms/sgabios'...
Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
Cloning into 'roms/skiboot'...
Submodule path 'roms/skiboot': checked out 'e0ee24c27a172bcf482f6f2bc905e6211c134bcc'
Cloning into 'roms/u-boot'...
Submodule path 'roms/u-boot': checked out 'd85ca029f257b53a96da6c2fb421e78a003a9943'
Cloning into 'roms/u-boot-sam460ex'...
Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
Cloning into 'tests/fp/berkeley-softfloat-3'...
Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'tests/fp/berkeley-testfloat-3'...
Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
Cloning into 'ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
Switched to a new branch 'test'
d617d4d hw/block: better reporting on pflash backing file mismatch

=== OUTPUT BEGIN ===
ERROR: "(foo*)" should be "(foo *)"
#70: FILE: hw/block/pflash_cfi01.c:769:
+                memset((uint8_t*)pfl->storage + backing_len, 0xff, pad_bytes);

WARNING: line over 80 characters
#82: FILE: hw/block/pflash_cfi01.c:781:
+            warn_report("device needs %" PRIu64 " bytes, rest ignored", total_len);

total: 1 errors, 1 warnings, 58 lines checked

Commit d617d4d4eb13 (hw/block: better reporting on pflash backing file mismatch) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190222154454.30289-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
Posted by Laszlo Ersek 6 years, 7 months ago 
Two trivial comments:

On 02/22/19 16:44, Alex Bennée wrote:
> It looks like there was going to be code to check we had some sort of
> alignment so lets replace it with an actual check. This is a bit more
> useful than the enigmatic "failed to read the initial flash content"
> when we attempt to read the number of bytes the device should have.
> 
> This is a potential confusing stumbling block when you move from using
> -bios to using -drive if=pflash,file=blob,format=raw,readonly for
> loading your firmware code. To mitigate that we automatically pad in
> the read-only case and warn the user when we have performed magic to
> enable things to Just Work (tm).
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v3
>   - tweak commit title/commentary
>   - use total_len instead of device_len for checks
>   - if the device is read-only do the padding for them
>   - accept baking_len > total_len (how to warn_report with NULL *errp?)
> v4
>   - error check blk_getlength
>   - optimise memset and use NOR erase pattern
>   - restore singular device (overly confusing)
>   - add warn_report for when we do magic
> ---
>  hw/block/pflash_cfi01.c | 40 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 00c2efd0d7..c69ecc20a0 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -45,6 +45,7 @@
>  #include "qemu/bitops.h"
>  #include "qemu/host-utils.h"
>  #include "qemu/log.h"
> +#include "qemu/error-report.h"
>  #include "hw/sysbus.h"
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
> @@ -714,13 +715,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>      device_len = sector_len_per_device * blocks_per_device;
>  
> -    /* XXX: to be fixed */
> -#if 0
> -    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
> -        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
> -        return NULL;
> -#endif
> -
>      memory_region_init_rom_device(
>          &pfl->mem, OBJECT(dev),
>          &pflash_cfi01_ops,
> @@ -747,6 +741,38 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>  
>      if (pfl->blk) {
> +        /*
> +         * Validate the backing store is the right size for pflash
> +         * devices. It should be padded to a multiple of the flash
> +         * block size. If the device is read-only we can elide the
> +         * check and just null pad the region first. If the user

(1) We no longer null-pad, but one-pad (bit-wise). I don't think it's
too confusing though.

> +         * supplies a larger file we ignore the tail.
> +         */
> +        int64_t backing_len = blk_getlength(pfl->blk);
> +        if (backing_len < 0) {
> +            error_setg(errp, "unable to check size of backing file");
> +            return;
> +        }
> +
> +        if (backing_len < total_len) {
> +            if (pfl->ro) {
> +                size_t pad_bytes = total_len - backing_len;
> +                /* pad with NOR erase pattern */
> +                memset((uint8_t*)pfl->storage + backing_len, 0xff, pad_bytes);
> +                warn_report("device needs %" PRIu64
> +                            " bytes, padded with %zd 0xff bytes",

(2) I think %zu would be more appropriate for size_t, but it's not a
showstopper.

> +                            total_len, pad_bytes);
> +                total_len = backing_len;
> +            } else {
> +                error_setg(errp, "device needs %" PRIu64 " bytes, "
> +                           "backing file provides only %" PRIu64 " bytes",
> +                           total_len, backing_len);
> +                return;
> +            }
> +        } else if (backing_len > total_len) {
> +            warn_report("device needs %" PRIu64 " bytes, rest ignored", total_len);
> +        }
> +
>          /* read the initial flash content */
>          ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
>  
> 

No need to repost just because of these.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.