Alex Bennée <alex.ben...@linaro.org> writes: > Laszlo Ersek <ler...@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.ben...@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.