* [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
@ 2019-02-22 15:44 Alex Bennée
2019-02-22 18:44 ` Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Alex Bennée @ 2019-02-22 15:44 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, armbru, stappers, Alex Bennée
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
2019-02-22 15:44 [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch Alex Bennée
@ 2019-02-22 18:44 ` Laszlo Ersek
2019-02-27 17:59 ` no-reply
2019-02-27 18:02 ` no-reply
2 siblings, 0 replies; 4+ messages in thread
From: Laszlo Ersek @ 2019-02-22 18:44 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: armbru, stappers
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
2019-02-22 15:44 [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch Alex Bennée
2019-02-22 18:44 ` Laszlo Ersek
@ 2019-02-27 17:59 ` no-reply
2019-02-27 18:02 ` no-reply
2 siblings, 0 replies; 4+ messages in thread
From: no-reply @ 2019-02-27 17:59 UTC (permalink / raw)
To: alex.bennee; +Cc: fam, qemu-devel, stappers, lersek, armbru
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
2019-02-22 15:44 [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch Alex Bennée
2019-02-22 18:44 ` Laszlo Ersek
2019-02-27 17:59 ` no-reply
@ 2019-02-27 18:02 ` no-reply
2 siblings, 0 replies; 4+ messages in thread
From: no-reply @ 2019-02-27 18:02 UTC (permalink / raw)
To: alex.bennee; +Cc: fam, qemu-devel, stappers, lersek, armbru
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-02-27 18:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-22 15:44 [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch Alex Bennée
2019-02-22 18:44 ` Laszlo Ersek
2019-02-27 17:59 ` no-reply
2019-02-27 18:02 ` no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.