| Message ID | 20190214155714.8779-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/block: report when pflash backing file isn't aligned | expand |
On 02/14/19 16:57, 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. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > hw/block/pflash_cfi01.c | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index bffb4c40e7..f3251b236c 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -722,12 +722,19 @@ 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 > + /* > + * Validate the backing store is the right size for pflash > + * devices. It has to be padded to a multiple of the flash block > + * size. > + */ > + if (pfl->blk) { > + uint64_t backing_len = blk_getlength(pfl->blk); > + if (device_len != backing_len) { > + error_setg(errp, "backing file wrong size " > + "(%" PRId64 " != %" PRId64")", backing_len, device_len); > + return; > + } > + } > > memory_region_init_rom_device( > &pfl->mem, OBJECT(dev), > I have two suggestions: - backing_len and device_len are both uint64_t; we should print them with PRIu64 - from a user POV, I find it more useful if the error message also shows which quantity is which, not just two inequal numbers. I don't feel too strongly about this, so up to you. Thanks, Laszlo
Laszlo Ersek <lersek@redhat.com> writes: > On 02/14/19 16:57, 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. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> hw/block/pflash_cfi01.c | 19 +++++++++++++------ >> 1 file changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index bffb4c40e7..f3251b236c 100644 >> --- a/hw/block/pflash_cfi01.c >> +++ b/hw/block/pflash_cfi01.c >> @@ -722,12 +722,19 @@ 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 >> + /* >> + * Validate the backing store is the right size for pflash >> + * devices. It has to be padded to a multiple of the flash block >> + * size. >> + */ >> + if (pfl->blk) { >> + uint64_t backing_len = blk_getlength(pfl->blk); >> + if (device_len != backing_len) { >> + error_setg(errp, "backing file wrong size " >> + "(%" PRId64 " != %" PRId64")", backing_len, device_len); >> + return; >> + } >> + } >> >> memory_region_init_rom_device( >> &pfl->mem, OBJECT(dev), >> > > I have two suggestions: > - backing_len and device_len are both uint64_t; we should print them > with PRIu64 blk_getlength actually returns int64_t for some reason (do signed lengths even make sense? maybe it's for error handling?). But sure I can make it PRIu64 > - from a user POV, I find it more useful if the error message also shows > which quantity is which, not just two inequal numbers. How about: "backing file size (%) not enough for whole device (%)" ? > > I don't feel too strongly about this, so up to you. > > Thanks, > Laszlo -- Alex Bennée
On Thu, Feb 14, 2019 at 10:38:35PM +0000, Alex Bennée wrote: > Laszlo Ersek <lersek@redhat.com> writes: > > On 02/14/19 16:57, Alex Bennée wrote: > >> + /* > >> + * Validate the backing store is the right size for pflash > >> + * devices. It has to be padded to a multiple of the flash block > >> + * size. > >> + */ > >> + if (pfl->blk) { > >> + uint64_t backing_len = blk_getlength(pfl->blk); > >> + if (device_len != backing_len) { > >> + error_setg(errp, "backing file wrong size " > >> + "(%" PRId64 " != %" PRId64")", backing_len, device_len); > >> + return; > >> + } > >> + } > >> > >> memory_region_init_rom_device( > >> &pfl->mem, OBJECT(dev), > >> > > > - from a user POV, I find it more useful if the error message also shows > > which quantity is which, not just two inequal numbers. > > How about: > > "backing file size (%) not enough for whole device (%)" > > ? > from a user POV, I find it more useful if the error messsage resembles the actual test in the source. Above is test != How about "backing file size (%) not equal to whole device size (%)" ? Groeten Geert Stappers
Alex Bennée <alex.bennee@linaro.org> writes: > Laszlo Ersek <lersek@redhat.com> writes: > >> On 02/14/19 16:57, 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. >>> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> --- >>> hw/block/pflash_cfi01.c | 19 +++++++++++++------ >>> 1 file changed, 13 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >>> index bffb4c40e7..f3251b236c 100644 >>> --- a/hw/block/pflash_cfi01.c >>> +++ b/hw/block/pflash_cfi01.c >>> @@ -722,12 +722,19 @@ 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 >>> + /* >>> + * Validate the backing store is the right size for pflash >>> + * devices. It has to be padded to a multiple of the flash block >>> + * size. >>> + */ >>> + if (pfl->blk) { >>> + uint64_t backing_len = blk_getlength(pfl->blk); >>> + if (device_len != backing_len) { >>> + error_setg(errp, "backing file wrong size " >>> + "(%" PRId64 " != %" PRId64")", backing_len, device_len); >>> + return; >>> + } >>> + } >>> >>> memory_region_init_rom_device( >>> &pfl->mem, OBJECT(dev), >>> >> >> I have two suggestions: >> - backing_len and device_len are both uint64_t; we should print them >> with PRIu64 > > blk_getlength actually returns int64_t for some reason (do signed > lengths even make sense? maybe it's for error handling?). But sure I can > make it PRIu64 Use of signed integers for file offsets is pervasive in the block layer. It's convenient when we return either a non-negative offset or a negative error code. It's admittedly sloppy anywhere else. >> - from a user POV, I find it more useful if the error message also shows >> which quantity is which, not just two inequal numbers. > > How about: > > "backing file size (%) not enough for whole device (%)" > > ? Not bad. Another one, avoiding parentheses: "device needs DDD bytes, backing file provides only BBB bytes" > >> >> I don't feel too strongly about this, so up to you.
On 02/14/19 23:38, Alex Bennée wrote: > > Laszlo Ersek <lersek@redhat.com> writes: > >> On 02/14/19 16:57, 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. >>> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> --- >>> hw/block/pflash_cfi01.c | 19 +++++++++++++------ >>> 1 file changed, 13 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >>> index bffb4c40e7..f3251b236c 100644 >>> --- a/hw/block/pflash_cfi01.c >>> +++ b/hw/block/pflash_cfi01.c >>> @@ -722,12 +722,19 @@ 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 >>> + /* >>> + * Validate the backing store is the right size for pflash >>> + * devices. It has to be padded to a multiple of the flash block >>> + * size. >>> + */ >>> + if (pfl->blk) { >>> + uint64_t backing_len = blk_getlength(pfl->blk); >>> + if (device_len != backing_len) { >>> + error_setg(errp, "backing file wrong size " >>> + "(%" PRId64 " != %" PRId64")", backing_len, device_len); >>> + return; >>> + } >>> + } >>> >>> memory_region_init_rom_device( >>> &pfl->mem, OBJECT(dev), >>> >> >> I have two suggestions: >> - backing_len and device_len are both uint64_t; we should print them >> with PRIu64 > > blk_getlength actually returns int64_t for some reason (do signed > lengths even make sense? maybe it's for error handling?). Ah, sorry, I didn't realize. I guess we should cover negative values then explicitly? Not sure. > But sure I can > make it PRIu64 > >> - from a user POV, I find it more useful if the error message also shows >> which quantity is which, not just two inequal numbers. > > How about: > > "backing file size (%) not enough for whole device (%)" "not enough" means "<", but the C expression uses "!=". How about "backing file size (...) does not match device size (...)"? Or "does not equal", whichever sounds more palatable. Thanks! Laszlo
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index bffb4c40e7..f3251b236c 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -722,12 +722,19 @@ 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 + /* + * Validate the backing store is the right size for pflash + * devices. It has to be padded to a multiple of the flash block + * size. + */ + if (pfl->blk) { + uint64_t backing_len = blk_getlength(pfl->blk); + if (device_len != backing_len) { + error_setg(errp, "backing file wrong size " + "(%" PRId64 " != %" PRId64")", backing_len, device_len); + return; + } + } memory_region_init_rom_device( &pfl->mem, OBJECT(dev),
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. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- hw/block/pflash_cfi01.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)