| Message ID | 20190307093723.655-2-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v6,1/4] hw/block: better reporting on pflash backing file mismatch | expand |
Markus Armbruster <armbru@redhat.com> writes: > From: Alex Bennée <alex.bennee@linaro.org> > > It looks like there was going to be code to check we had some sort of > alignment so let's replace it with an actual check. Reject undersized > images with "device needs N bytes, backing file provides only M > bytes". 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. Continue to accept oversized images, but warn > "device needs N bytes, rest ignored". > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/block/pflash_cfi01.c | 29 ++++++++++++++++++++++------- > 1 file changed, 22 insertions(+), 7 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index 9d1c356eb6..75ce8ef489 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" > @@ -730,13 +731,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,6 +757,27 @@ 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, "unable to check size of backing file"); > + return; > + } > + > + if (backing_len < total_len) { > + 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); > + } As discussed elsewhere I'm happy for this just to error out with a useful error message. > + > /* read the initial flash content */ > ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); -- Alex Bennée
Alex Bennée <alex.bennee@linaro.org> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> From: Alex Bennée <alex.bennee@linaro.org> >> >> It looks like there was going to be code to check we had some sort of >> alignment so let's replace it with an actual check. Reject undersized >> images with "device needs N bytes, backing file provides only M >> bytes". 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. Continue to accept oversized images, but warn >> "device needs N bytes, rest ignored". >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> hw/block/pflash_cfi01.c | 29 ++++++++++++++++++++++------- >> 1 file changed, 22 insertions(+), 7 deletions(-) >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index 9d1c356eb6..75ce8ef489 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" >> @@ -730,13 +731,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,6 +757,27 @@ 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, "unable to check size of backing file"); >> + return; >> + } >> + >> + if (backing_len < total_len) { >> + 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); >> + } > > As discussed elsewhere I'm happy for this just to error out with a > useful error message. I'll do that in the next spin. Thanks! >> + >> /* read the initial flash content */ >> ret = blk_pread(pfl->blk, 0, pfl->storage, total_len); > > > -- > Alex Bennée
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index 9d1c356eb6..75ce8ef489 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" @@ -730,13 +731,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,6 +757,27 @@ 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, "unable to check size of backing file"); + return; + } + + if (backing_len < total_len) { + 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);