From: Markus Armbruster <armbru@redhat.com>
To: Laszlo Ersek <lersek@redhat.com>
Cc: qemu-devel@nongnu.org, kwolf@redhat.com, philmd@redhat.com,
alex.bennee@linaro.org, qemu-block@nongnu.org, mreitz@redhat.com
Subject: Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors
Date: Fri, 08 Mar 2019 13:28:54 +0100 [thread overview]
Message-ID: <875zsteeih.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <481d398a-e26c-6c0a-20b7-7466ae1ebf1f@redhat.com> (Laszlo Ersek's message of "Fri, 8 Mar 2019 12:47:52 +0100")
Laszlo Ersek <lersek@redhat.com> writes:
> On 03/08/19 07:24, Markus Armbruster wrote:
>> From: Alex Bennée <alex.bennee@linaro.org>
>>
>> We reject undersized backends with a rather enigmatic "failed to read
>> the initial flash content" error.
>>
>> 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 as "device requires N bytes, block backend 'NAME' provides M
>> bytes".
>>
>> Improve the error for actual read failures to "can't read initial
>> flash content from block backend 'NAME'.
>>
>> We disabled code to limit device sizes to 8, 16, 32 or 64MiB more than
>> a decade ago in commit 95d1f3edd5e and c8b153d7949, v0.9.1. Bury.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> hw/block/pflash_cfi01.c | 31 ++++++++++++++++++++++---------
>> hw/block/pflash_cfi02.c | 31 +++++++++++++++++++++++--------
>> 2 files changed, 45 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 9d1c356eb6..2e3a05c0b3 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -730,13 +730,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,
>> @@ -763,12 +756,32 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>> }
>>
>> if (pfl->blk) {
>> + /*
>> + * Validate the backing store is the right size for pflash
>> + * devices. 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, "can't get size of block backend '%s'",
>> + blk_name(pfl->blk));
>> + return;
>> + }
>> + if (backing_len != total_len) {
>> + error_setg(errp, "device requires %" PRIu64 " bytes, "
>> + "block backend '%s' provides %" PRIu64 " bytes",
>> + total_len, blk_name(pfl->blk), backing_len);
>> + return;
>> + }
>> +
>> /* read the initial flash content */
>> ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
>> -
>> if (ret < 0) {
>> vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
>> - error_setg(errp, "failed to read the initial flash content");
>> + error_setg(errp, "can't read initial flash content"
>> + " from block backend '%s'",
>> + blk_name(pfl->blk));
>> return;
>> }
>> }
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index 33779ce807..45b88d65d8 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -550,12 +550,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>> }
>>
>> chip_len = pfl->sector_len * pfl->nb_blocs;
>> - /* 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->orig_mem, OBJECT(pfl), pfl->be ?
>> &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
>> @@ -581,11 +575,32 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>> }
>>
>> if (pfl->blk) {
>> + /*
>> + * Validate the backing store is the right size for pflash
>> + * devices. 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, "can't get size of block backend '%s'",
>> + blk_name(pfl->blk));
>> + return;
>> + }
>> + if (backing_len != chip_len) {
>> + error_setg(errp, "device requires %" PRIu32 " bytes, "
>> + "block backend '%s' provides %" PRIu64 " bytes",
>> + chip_len, blk_name(pfl->blk), backing_len);
>> + return;
>> + }
>> +
>> /* read the initial flash content */
>> ret = blk_pread(pfl->blk, 0, pfl->storage, chip_len);
>> if (ret < 0) {
>> - vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl));
>> - error_setg(errp, "failed to read the initial flash content");
>> + vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
>> + error_setg(errp, "can't read initial flash content"
>> + " from block backend '%s'",
>> + blk_name(pfl->blk));
>> return;
>> }
>> }
>>
>
> This one has got to be one of the longest bike-shedding sessions! :)
>
> I'm fine with this patch, but I could suggest two improvements.
>
> (1) When blk_getlength() fails, we could format the negative error code
> returned by it into the error message.
I can do that.
> (2) We could extract the common code to a new function in
> "hw/block/block.c". (It says "Common code for block device models" on
> the tin.)
There's so much common code in these two files even before this patch...
> I realize the vmstate_unregister_ram() rollbacks are not identical
> between cfi01 and cfi02. However, that difference only draws my
> attention to the fact that the unregister calls are not made on the
> other error branches in this patch anyway!
You're right, that needs fixing.
> So we should have a common function like this:
>
> void pflash_cfi_read_initial_content(BlockBackend *blk, uint64_t size,
> void *buf, Error **errp)
> {
> int64_t backing_len = blk_getlength(blk);
> int ret;
>
> if (backing_len < 0) {
> error_setg(...);
> return;
> }
>
> if (backing_len != size) {
> error_setg(...);
> return;
> }
>
> ret = blk_pread(blk, 0, buf, size);
> if (ret < 0) {
> error_setg(...);
> }
> }
>
> And at the call sites, we can do
>
> if (pfl->blk) {
> Error *local_err = NULL;
>
> pflash_cfi_read_initial_content(pfl->blk,
> total_len OR chip_len,
> pfl->storage,
> &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> vmstate_unregister_ram(...); // as appropriate for cfiXX
> return;
> }
> }
I'll give it a try for v8. Thanks!
next prev parent reply other threads:[~2019-03-08 12:29 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-08 6:24 [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors Markus Armbruster
2019-03-08 10:00 ` Alex Bennée
2019-03-08 12:16 ` Markus Armbruster
2019-03-08 11:47 ` Laszlo Ersek
2019-03-08 12:28 ` Markus Armbruster [this message]
2019-03-08 13:35 ` Kevin Wolf
2019-03-08 14:09 ` Laszlo Ersek
2019-03-08 14:26 ` Philippe Mathieu-Daudé
2019-03-08 14:29 ` Markus Armbruster
2019-03-08 15:40 ` Kevin Wolf
2019-03-08 16:45 ` Philippe Mathieu-Daudé
2019-03-08 17:03 ` Markus Armbruster
2019-03-08 17:18 ` Kevin Wolf
2019-03-08 18:51 ` Philippe Mathieu-Daudé
2019-03-09 9:20 ` Markus Armbruster
2019-03-08 14:40 ` Philippe Mathieu-Daudé
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=875zsteeih.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=alex.bennee@linaro.org \
--cc=kwolf@redhat.com \
--cc=lersek@redhat.com \
--cc=mreitz@redhat.com \
--cc=philmd@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).