* [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors @ 2019-03-08 6:24 Markus Armbruster 2019-03-08 10:00 ` Alex Bennée 2019-03-08 11:47 ` Laszlo Ersek 0 siblings, 2 replies; 16+ messages in thread From: Markus Armbruster @ 2019-03-08 6:24 UTC (permalink / raw) To: qemu-devel; +Cc: alex.bennee, lersek, philmd, kwolf, mreitz, qemu-block 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; } } -- 2.17.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 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 1 sibling, 1 reply; 16+ messages in thread From: Alex Bennée @ 2019-03-08 10:00 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, lersek, philmd, kwolf, mreitz, qemu-block Markus Armbruster <armbru@redhat.com> writes: > 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. > + */ We need to drop the last sentence... > + 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. > + */ And here. > + 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; > } > } Otherwise all good... not that a r-b means anything from me ;-) -- Alex Bennée ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 2019-03-08 10:00 ` Alex Bennée @ 2019-03-08 12:16 ` Markus Armbruster 0 siblings, 0 replies; 16+ messages in thread From: Markus Armbruster @ 2019-03-08 12:16 UTC (permalink / raw) To: Alex Bennée; +Cc: kwolf, qemu-block, lersek, qemu-devel, mreitz, philmd Alex Bennée <alex.bennee@linaro.org> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> 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. >> + */ > > We need to drop the last sentence... D'oh! >> + 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. >> + */ > > And here. > >> + 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; >> } >> } > > Otherwise all good... not that a r-b means anything from me ;-) v8 coming up... thanks! ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 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 11:47 ` Laszlo Ersek 2019-03-08 12:28 ` Markus Armbruster 2019-03-08 14:40 ` Philippe Mathieu-Daudé 1 sibling, 2 replies; 16+ messages in thread From: Laszlo Ersek @ 2019-03-08 11:47 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: alex.bennee, philmd, kwolf, mreitz, qemu-block 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. (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.) 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! 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; } } Thanks Laszlo ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 2019-03-08 11:47 ` Laszlo Ersek @ 2019-03-08 12:28 ` Markus Armbruster 2019-03-08 13:35 ` Kevin Wolf 2019-03-08 14:40 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 16+ messages in thread From: Markus Armbruster @ 2019-03-08 12:28 UTC (permalink / raw) To: Laszlo Ersek; +Cc: qemu-devel, kwolf, philmd, alex.bennee, qemu-block, mreitz 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! ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 2019-03-08 12:28 ` Markus Armbruster @ 2019-03-08 13:35 ` Kevin Wolf 2019-03-08 14:09 ` Laszlo Ersek ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Kevin Wolf @ 2019-03-08 13:35 UTC (permalink / raw) To: Markus Armbruster Cc: Laszlo Ersek, qemu-devel, philmd, alex.bennee, qemu-block, mreitz Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben: > Laszlo Ersek <lersek@redhat.com> writes: > > 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. By using error_setg_errno(), I assume. Not throwing away error details is always good. > > (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... My understanding is that hw/block/block.c contains code that is potentially useful to all kinds of block devices, not random code that two specific similar devices happen to share. If we want to deduplicate some code in the flash devices, without any expectation that other devices will use it at some point, I'd rather create a new source file hw/block/pflash_common.c or something like that. Kevin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 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 2 siblings, 0 replies; 16+ messages in thread From: Laszlo Ersek @ 2019-03-08 14:09 UTC (permalink / raw) To: Kevin Wolf, Markus Armbruster Cc: qemu-devel, philmd, alex.bennee, qemu-block, mreitz On 03/08/19 14:35, Kevin Wolf wrote: > Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben: >> Laszlo Ersek <lersek@redhat.com> writes: >>> 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. > > By using error_setg_errno(), I assume. Not throwing away error details > is always good. > >>> (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... > > My understanding is that hw/block/block.c contains code that is > potentially useful to all kinds of block devices, not random code that > two specific similar devices happen to share. Ah, OK. > If we want to deduplicate some code in the flash devices, without any > expectation that other devices will use it at some point, I'd rather > create a new source file hw/block/pflash_common.c or something like > that. Sure, that makes a lot of sense then. Cheers Laszlo ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 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 2 siblings, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2019-03-08 14:26 UTC (permalink / raw) To: Kevin Wolf, Markus Armbruster Cc: Laszlo Ersek, qemu-devel, alex.bennee, qemu-block, mreitz On 3/8/19 2:35 PM, Kevin Wolf wrote: > Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben: >> Laszlo Ersek <lersek@redhat.com> writes: >>> 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. > > By using error_setg_errno(), I assume. Not throwing away error details > is always good. > >>> (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... > > My understanding is that hw/block/block.c contains code that is > potentially useful to all kinds of block devices, not random code that > two specific similar devices happen to share. > > If we want to deduplicate some code in the flash devices, without any > expectation that other devices will use it at some point, I'd rather > create a new source file hw/block/pflash_common.c or something like > that. Agreed, I like that suggestion too. Actually we already have been talking about a such cleanup/refactor/rewrite with Markus, but this will wait the next QEMU dev cycle. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 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 2 siblings, 1 reply; 16+ messages in thread From: Markus Armbruster @ 2019-03-08 14:29 UTC (permalink / raw) To: Kevin Wolf Cc: qemu-block, philmd, qemu-devel, Laszlo Ersek, mreitz, alex.bennee Kevin Wolf <kwolf@redhat.com> writes: > Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben: >> Laszlo Ersek <lersek@redhat.com> writes: >> > 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. > > By using error_setg_errno(), I assume. Not throwing away error details > is always good. > >> > (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... > > My understanding is that hw/block/block.c contains code that is > potentially useful to all kinds of block devices, not random code that > two specific similar devices happen to share. > > If we want to deduplicate some code in the flash devices, without any > expectation that other devices will use it at some point, I'd rather > create a new source file hw/block/pflash_common.c or something like > that. Yes. The helper I came up with (appended) isn't really specific to flash devices. Would it be okay for hw/block/block.c even though only the two flash devices use it for now? 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 '%s'", blk_name(blk)); return false; } if (blk_len != size) { error_setg(errp, "device requires %" PRIu64 " bytes, " "block backend '%s' provides %" PRIu64 " bytes", size, blk_name(blk), blk_len); return false; } /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */ 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 '%s'", blk_name(blk)); return false; } return true; } ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 2019-03-08 14:29 ` Markus Armbruster @ 2019-03-08 15:40 ` Kevin Wolf 2019-03-08 16:45 ` Philippe Mathieu-Daudé ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Kevin Wolf @ 2019-03-08 15:40 UTC (permalink / raw) To: Markus Armbruster Cc: qemu-block, philmd, qemu-devel, Laszlo Ersek, mreitz, alex.bennee Am 08.03.2019 um 15:29 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben: > >> Laszlo Ersek <lersek@redhat.com> writes: > >> > 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. > > > > By using error_setg_errno(), I assume. Not throwing away error details > > is always good. > > > >> > (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... > > > > My understanding is that hw/block/block.c contains code that is > > potentially useful to all kinds of block devices, not random code that > > two specific similar devices happen to share. > > > > If we want to deduplicate some code in the flash devices, without any > > expectation that other devices will use it at some point, I'd rather > > create a new source file hw/block/pflash_common.c or something like > > that. > > Yes. > > The helper I came up with (appended) isn't really specific to flash > devices. Would it be okay for hw/block/block.c even though only the two > flash devices use it for now? Hm, it feels more like a helper for devices that can't decide whether they want to be a block device or not. Or that actually don't want to be a block device, but use a BlockBackend anyway. Reading in the whole image isn't something that a normal block device would do. But yes, it doesn't have flash-specific knowledge, even though I hope that it's functionality that will remain very specific to these two devices. So it's your call, I don't have a strong opinion either way. > > 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 '%s'", > blk_name(blk)); > return false; > } > if (blk_len != size) { > error_setg(errp, "device requires %" PRIu64 " bytes, " > "block backend '%s' provides %" PRIu64 " bytes", > size, blk_name(blk), blk_len); Should size use HWADDR_PRIu? I'm not sure if printing the BlockBackend name is a good idea because hopefully one day the BlockBackend will be anonymous even for the flash devices. > return false; > } > > /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */ > assert(size <= BDRV_REQUEST_MAX_BYTES); I don't think we'd ever want to read in more than 2 GB into a memory buffer. Before we even get close to this point, the devices should be reworked to be more like an actual block device and read only what is actually accessed. > ret = blk_pread(blk, 0, buf, size); > if (ret < 0) { > error_setg_errno(errp, -ret, "can't read block backend '%s'", > blk_name(blk)); > return false; > } > return true; > } Kevin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 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 18:51 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2019-03-08 16:45 UTC (permalink / raw) To: Kevin Wolf, Markus Armbruster Cc: qemu-block, qemu-devel, Laszlo Ersek, mreitz, alex.bennee On 3/8/19 4:40 PM, Kevin Wolf wrote: > Am 08.03.2019 um 15:29 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >>> Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben: >>>> Laszlo Ersek <lersek@redhat.com> writes: >>>>> 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. >>> >>> By using error_setg_errno(), I assume. Not throwing away error details >>> is always good. >>> >>>>> (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... >>> >>> My understanding is that hw/block/block.c contains code that is >>> potentially useful to all kinds of block devices, not random code that >>> two specific similar devices happen to share. >>> >>> If we want to deduplicate some code in the flash devices, without any >>> expectation that other devices will use it at some point, I'd rather >>> create a new source file hw/block/pflash_common.c or something like >>> that. >> >> Yes. >> >> The helper I came up with (appended) isn't really specific to flash >> devices. Would it be okay for hw/block/block.c even though only the two >> flash devices use it for now? > > Hm, it feels more like a helper for devices that can't decide whether > they want to be a block device or not. Or that actually don't want to be > a block device, but use a BlockBackend anyway. Reading in the whole > image isn't something that a normal block device would do. > > But yes, it doesn't have flash-specific knowledge, even though I hope > that it's functionality that will remain very specific to these two > devices. > > So it's your call, I don't have a strong opinion either way. > >> >> 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 '%s'", >> blk_name(blk)); >> return false; >> } >> if (blk_len != size) { >> error_setg(errp, "device requires %" PRIu64 " bytes, " >> "block backend '%s' provides %" PRIu64 " bytes", >> size, blk_name(blk), blk_len); > > Should size use HWADDR_PRIu? > > I'm not sure if printing the BlockBackend name is a good idea because > hopefully one day the BlockBackend will be anonymous even for the flash > devices. > >> return false; >> } >> >> /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */ >> assert(size <= BDRV_REQUEST_MAX_BYTES); > > I don't think we'd ever want to read in more than 2 GB into a memory > buffer. Before we even get close to this point, the devices should be > reworked to be more like an actual block device and read only what is > actually accessed. [bikeshedding again] So you eventually found the root problem of this device. It tries to use files as 'raw' format block. Since the pflash devices provide write access, we create a block device in memory. We should: - use raw files as ROM (without using a pflash device) - enforce the pflash device to use block formatted files - provide a tool to help converting raw file to pflash block, or improve the device documentation. >> ret = blk_pread(blk, 0, buf, size); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "can't read block backend '%s'", >> blk_name(blk)); >> return false; >> } >> return true; >> } > > Kevin > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 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é 2 siblings, 1 reply; 16+ messages in thread From: Markus Armbruster @ 2019-03-08 17:03 UTC (permalink / raw) To: Kevin Wolf Cc: qemu-block, Laszlo Ersek, qemu-devel, philmd, mreitz, alex.bennee Kevin Wolf <kwolf@redhat.com> writes: > Am 08.03.2019 um 15:29 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben: >> >> Laszlo Ersek <lersek@redhat.com> writes: >> >> > 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. >> > >> > By using error_setg_errno(), I assume. Not throwing away error details >> > is always good. >> > >> >> > (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... >> > >> > My understanding is that hw/block/block.c contains code that is >> > potentially useful to all kinds of block devices, not random code that >> > two specific similar devices happen to share. >> > >> > If we want to deduplicate some code in the flash devices, without any >> > expectation that other devices will use it at some point, I'd rather >> > create a new source file hw/block/pflash_common.c or something like >> > that. >> >> Yes. >> >> The helper I came up with (appended) isn't really specific to flash >> devices. Would it be okay for hw/block/block.c even though only the two >> flash devices use it for now? > > Hm, it feels more like a helper for devices that can't decide whether > they want to be a block device or not. Or that actually don't want to be > a block device, but use a BlockBackend anyway. I think the latter's precisely what the two pflash devices are. > Reading in the whole > image isn't something that a normal block device would do. No argument. > But yes, it doesn't have flash-specific knowledge, even though I hope > that it's functionality that will remain very specific to these two > devices. > > So it's your call, I don't have a strong opinion either way. Understood. >> 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 '%s'", >> blk_name(blk)); >> return false; >> } >> if (blk_len != size) { >> error_setg(errp, "device requires %" PRIu64 " bytes, " >> "block backend '%s' provides %" PRIu64 " bytes", >> size, blk_name(blk), blk_len); > > Should size use HWADDR_PRIu? Yes. > I'm not sure if printing the BlockBackend name is a good idea because > hopefully one day the BlockBackend will be anonymous even for the flash > devices. Hmm. Tell me what else I can use to identify the troublemaker to the user. For the second error, I could sidestep the issue and point to frontend instead (oh, now I have an even thornier issue). The first and third error are *backend* errors, only related to the frontend since the frontend happens to use them. I could perhaps identify them as "backend of frontend F". If we want to do that going forward, we need suitable helpers, so we don't reinvent this wheel in every block frontend. >> return false; >> } >> >> /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */ >> assert(size <= BDRV_REQUEST_MAX_BYTES); > > I don't think we'd ever want to read in more than 2 GB into a memory > buffer. Before we even get close to this point, the devices should be > reworked to be more like an actual block device and read only what is > actually accessed. I'll steal from that to turn the comment into a proper excuse for my lazy assertion. Thanks! >> ret = blk_pread(blk, 0, buf, size); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "can't read block backend '%s'", >> blk_name(blk)); >> return false; >> } >> return true; >> } > > Kevin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 2019-03-08 17:03 ` Markus Armbruster @ 2019-03-08 17:18 ` Kevin Wolf 0 siblings, 0 replies; 16+ messages in thread From: Kevin Wolf @ 2019-03-08 17:18 UTC (permalink / raw) To: Markus Armbruster Cc: qemu-block, Laszlo Ersek, qemu-devel, philmd, mreitz, alex.bennee Am 08.03.2019 um 18:03 hat Markus Armbruster geschrieben: > >> 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 '%s'", > >> blk_name(blk)); > >> return false; > >> } > >> if (blk_len != size) { > >> error_setg(errp, "device requires %" PRIu64 " bytes, " > >> "block backend '%s' provides %" PRIu64 " bytes", > >> size, blk_name(blk), blk_len); > > > > Should size use HWADDR_PRIu? > > Yes. > > > I'm not sure if printing the BlockBackend name is a good idea because > > hopefully one day the BlockBackend will be anonymous even for the flash > > devices. > > Hmm. Tell me what else I can use to identify the troublemaker to the > user. My hope was that a caller of this would prefix the right context. For example, if the device were created by -device, the error message would be prefixed with the whole "-device driver=pflash...:" string, which gives enough context to the user. Machine code that instantiates the device based on -drive should probably do something similar. Kevin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 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 18:51 ` Philippe Mathieu-Daudé 2019-03-09 9:20 ` Markus Armbruster 2 siblings, 1 reply; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2019-03-08 18:51 UTC (permalink / raw) To: Kevin Wolf, Markus Armbruster Cc: qemu-block, qemu-devel, Laszlo Ersek, mreitz, alex.bennee On 3/8/19 4:40 PM, Kevin Wolf wrote: > Am 08.03.2019 um 15:29 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >>> Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben: >>>> Laszlo Ersek <lersek@redhat.com> writes: >>>>> 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. >>> >>> By using error_setg_errno(), I assume. Not throwing away error details >>> is always good. >>> >>>>> (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... >>> >>> My understanding is that hw/block/block.c contains code that is >>> potentially useful to all kinds of block devices, not random code that >>> two specific similar devices happen to share. >>> >>> If we want to deduplicate some code in the flash devices, without any >>> expectation that other devices will use it at some point, I'd rather >>> create a new source file hw/block/pflash_common.c or something like >>> that. >> >> Yes. >> >> The helper I came up with (appended) isn't really specific to flash >> devices. Would it be okay for hw/block/block.c even though only the two >> flash devices use it for now? > > Hm, it feels more like a helper for devices that can't decide whether > they want to be a block device or not. Or that actually don't want to be > a block device, but use a BlockBackend anyway. Reading in the whole > image isn't something that a normal block device would do. > > But yes, it doesn't have flash-specific knowledge, even though I hope > that it's functionality that will remain very specific to these two > devices. > > So it's your call, I don't have a strong opinion either way. > >> >> 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 '%s'", >> blk_name(blk)); >> return false; >> } >> if (blk_len != size) { >> error_setg(errp, "device requires %" PRIu64 " bytes, " >> "block backend '%s' provides %" PRIu64 " bytes", >> size, blk_name(blk), blk_len); > > Should size use HWADDR_PRIu? > > I'm not sure if printing the BlockBackend name is a good idea because > hopefully one day the BlockBackend will be anonymous even for the flash > devices. > >> return false; >> } >> >> /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */ >> assert(size <= BDRV_REQUEST_MAX_BYTES); > > I don't think we'd ever want to read in more than 2 GB into a memory > buffer. Before we even get close to this point, the devices should be > reworked to be more like an actual block device and read only what is > actually accessed. The biggest NOR available in the market is 256 MiB (bigger size is barely ChipSelect MMIO-addressable). Maybe you can use: #define NOR_FLASH_MAX_BYTES (256 * MiB) and refuse bigger flashes. We could also check what is the widest chip-select range addressable by all the supported architectures. I don't think it's worth it. > >> ret = blk_pread(blk, 0, buf, size); OK, this function is named blk_check_size_and_read_all. Here we read_all. Refactoring this device we should be able to read at most sizeof(the biggest sector). But this implies some serious work. >> if (ret < 0) { >> error_setg_errno(errp, -ret, "can't read block backend '%s'", >> blk_name(blk)); >> return false; >> } >> return true; >> } > > Kevin > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 2019-03-08 18:51 ` Philippe Mathieu-Daudé @ 2019-03-09 9:20 ` Markus Armbruster 0 siblings, 0 replies; 16+ messages in thread From: Markus Armbruster @ 2019-03-09 9:20 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Kevin Wolf, Markus Armbruster, alex.bennee, Laszlo Ersek, qemu-devel, qemu-block, mreitz Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 3/8/19 4:40 PM, Kevin Wolf wrote: >> Am 08.03.2019 um 15:29 hat Markus Armbruster geschrieben: >>> Kevin Wolf <kwolf@redhat.com> writes: >>> >>>> Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben: >>>>> Laszlo Ersek <lersek@redhat.com> writes: >>>>>> 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. >>>> >>>> By using error_setg_errno(), I assume. Not throwing away error details >>>> is always good. >>>> >>>>>> (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... >>>> >>>> My understanding is that hw/block/block.c contains code that is >>>> potentially useful to all kinds of block devices, not random code that >>>> two specific similar devices happen to share. >>>> >>>> If we want to deduplicate some code in the flash devices, without any >>>> expectation that other devices will use it at some point, I'd rather >>>> create a new source file hw/block/pflash_common.c or something like >>>> that. >>> >>> Yes. >>> >>> The helper I came up with (appended) isn't really specific to flash >>> devices. Would it be okay for hw/block/block.c even though only the two >>> flash devices use it for now? >> >> Hm, it feels more like a helper for devices that can't decide whether >> they want to be a block device or not. Or that actually don't want to be >> a block device, but use a BlockBackend anyway. Reading in the whole >> image isn't something that a normal block device would do. >> >> But yes, it doesn't have flash-specific knowledge, even though I hope >> that it's functionality that will remain very specific to these two >> devices. >> >> So it's your call, I don't have a strong opinion either way. >> >>> >>> 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 '%s'", >>> blk_name(blk)); >>> return false; >>> } >>> if (blk_len != size) { >>> error_setg(errp, "device requires %" PRIu64 " bytes, " >>> "block backend '%s' provides %" PRIu64 " bytes", >>> size, blk_name(blk), blk_len); >> >> Should size use HWADDR_PRIu? >> >> I'm not sure if printing the BlockBackend name is a good idea because >> hopefully one day the BlockBackend will be anonymous even for the flash >> devices. >> >>> return false; >>> } >>> >>> /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */ >>> assert(size <= BDRV_REQUEST_MAX_BYTES); >> >> I don't think we'd ever want to read in more than 2 GB into a memory >> buffer. Before we even get close to this point, the devices should be >> reworked to be more like an actual block device and read only what is >> actually accessed. > > The biggest NOR available in the market is 256 MiB (bigger size is > barely ChipSelect MMIO-addressable). > > Maybe you can use: > > #define NOR_FLASH_MAX_BYTES (256 * MiB) > > and refuse bigger flashes. The comment next to the definition of property "width" in pflash_cfi01.c suggests the device model can emulate a bunch of flash chips wired together: /* width here is the overall width of this QEMU device in bytes. * The QEMU device may be emulating a number of flash devices * wired up in parallel; the width of each individual flash * device should be specified via device-width. If the individual * devices have a maximum width which is greater than the width * they are being used for, this maximum width should be set via * max-device-width (which otherwise defaults to device-width). * So for instance a 32-bit wide QEMU flash device made from four * 16-bit flash devices used in 8-bit wide mode would be configured * with width = 4, device-width = 1, max-device-width = 2. * * If device-width is not specified we default to backwards * compatible behaviour which is a bad emulation of two * 16 bit devices making up a 32 bit wide QEMU device. This * is deprecated for new uses of this device. */ > We could also check what is the widest chip-select range addressable > by all the supported architectures. I don't think it's worth it. > >> >>> ret = blk_pread(blk, 0, buf, size); > > OK, this function is named blk_check_size_and_read_all. Here we > read_all. Refactoring this device we should be able to read at > most sizeof(the biggest sector). > > But this implies some serious work. Here's another thing to consider: shadowing in RAM. Attractive when you got the RAM, and it's faster than (parallel) flash. If you shadow anyway, you can just as well use serial flash, and throw in compression. Now, emulated pflash can execute code just as fast es emulated RAM. The question is what kind of hardware the firmware expects. Even if it supports a variety of hardware, it may still have preferences. >>> if (ret < 0) { >>> error_setg_errno(errp, -ret, "can't read block backend '%s'", >>> blk_name(blk)); >>> return false; >>> } >>> return true; >>> } >> >> Kevin >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors 2019-03-08 11:47 ` Laszlo Ersek 2019-03-08 12:28 ` Markus Armbruster @ 2019-03-08 14:40 ` Philippe Mathieu-Daudé 1 sibling, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2019-03-08 14:40 UTC (permalink / raw) To: Laszlo Ersek, Markus Armbruster, qemu-devel Cc: alex.bennee, kwolf, mreitz, qemu-block On 3/8/19 12:47 PM, Laszlo Ersek wrote: > 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. This one is a worthwhile a v8. > (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.) Good idea, but I'd postpone that for the next dev cycle. > 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! > > 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; > } > } > > Thanks > Laszlo > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-03-09 9:21 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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é
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.