Re: [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
Patchew URL: https://patchew.org/QEMU/20190222154454.30289-1-alex.ben...@linaro.org/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20190222154454.30289-1-alex.ben...@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.marko...@rt-rk.com -> patchew/1550863578-30859-1-git-send-email-aleksandar.marko...@rt-rk.com * [new tag] patchew/1550878686-23934-1-git-send-email-michelhe...@gmail.com -> patchew/1550878686-23934-1-git-send-email-michelhe...@gmail.com * [new tag] patchew/1551185735-17154-1-git-send-email-aleksandar.marko...@rt-rk.com -> patchew/1551185735-17154-1-git-send-email-aleksandar.marko...@rt-rk.com - [tag update] patchew/20190220145819.30969-1-berra...@redhat.com -> patchew/20190220145819.30969-1-berra...@redhat.com * [new tag] patchew/20190220180112.28250-1-js...@redhat.com -> patchew/20190220180112.28250-1-js...@redhat.com * [new tag] patchew/20190221101139.2224-1-stefa...@redhat.com -> patchew/20190221101139.2224-1-stefa...@redhat.com - [tag update] patchew/20190222031413.20250-1-pet...@redhat.com -> patchew/20190222031413.20250-1-pet...@redhat.com * [new tag] patchew/20190222141024.22217-1-kbast...@mail.uni-paderborn.de -> patchew/20190222141024.22217-1-kbast...@mail.uni-paderborn.de * [new tag] patchew/20190222154454.30289-1-alex.ben...@linaro.org -> patchew/20190222154454.30289-1-alex.ben...@linaro.org * [new tag] patchew/2019004500.24434-1-alex.ben...@linaro.org -> patchew/2019004500.24434-1-alex.ben...@linaro.org * [new tag] patchew/20190225102433.22401-1-peter.mayd...@linaro.org -> patchew/20190225102433.22401-1-peter.mayd...@linaro.org * [new tag] patchew/20190226045304.25618-1-da...@gibson.dropbear.id.au -> patchew/20190226045304.25618-1-da...@gibson.dropbear.id.au * [new tag] patchew/20190226165013.24867-1-ebl...@redhat.com -> patchew/20190226165013.24867-1-ebl...@redhat.com * [new tag] patchew/20190227131433.197063-1-vsement...@virtuozzo.com -> patchew/20190227131433.197063-1-vsement...@virtuozzo.com * [new tag] patchew/20190227132413.13213-1-dgilb...@redhat.com -> patchew/20190227132413.13213-1-dgilb...@redhat.com * [new tag] patchew/20190227135523.16952-1-berra...@redhat.com -> patchew/20190227135523.16952-1-berra...@redhat.com * [new tag] patchew/20190227140629.1569-1-yuval.sh...@oracle.com -> patchew/20190227140629.1569-1-yuval.sh...@oracle.com * [new tag] patchew/20190227144413.30975-1-stefa...@redhat.com -> patchew/20190227144413.30975-1-stefa...@redhat.com * [new tag] patchew/20190227145324.26188-1-berra...@redhat.com -> patchew/20190227145324.26188-1-berra...@redhat.com * [new tag] patchew/20190227145755.26556-1-berra...@redhat.com -> patchew/20190227145755.26556-1-berra...@redhat.com - [tag update] patchew/cover.1550836631.git.be...@igalia.com -> patchew/cover.1550836631.git.be...@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
Re: [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
Patchew URL:
https://patchew.org/QEMU/20190222154454.30289-1-alex.ben...@linaro.org/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20190222154454.30289-1-alex.ben...@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-phi...@redhat.com ->
patchew/20190220010232.18731-1-phi...@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.ben...@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
Re: [Qemu-devel] [PATCH v4] hw/block: better reporting on pflash backing file mismatch
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
>
> ---
> 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
Thanks!
Laszlo
