Re: [PATCH 1/8] m25p80: Improve error when the backend file size does not match the device

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 15/2/23 20:52, Peter Delevoryas wrote:
> On Tue, Feb 14, 2023 at 06:18:23PM +0100, Cédric Le Goater wrote:
>> Currently, when a block backend is attached to a m25p80 device and the
>> associated file size does not match the flash model, QEMU complains
>> with the error message "failed to read the initial flash content".
>> This is confusing for the user.
>>
>> Use blk_check_size_and_read_all() instead of blk_pread() to improve
>> the reported error.

Could you reference commit 06f1521795?

>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Message-Id: <20221115151000.2080833-1-clg@kaod.org>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>    breakage with commit a4b15a8b9e ("pflash: Only read non-zero parts
>>    of backend image") when using -snaphot.
> 
> 
> I guess it's not obvious to me, what broke?
> 
> 1. BlockBackend *blk = -drive file=blob.mtd,format=raw,if=mtd,snapshot=on
> 2. uint8_t *storage = malloc(...)
> 3. blk_check_size_and_read_all(blk, storage, size)
> 4. commit a4b15a8b9e
>      for sector in blk:
>          if any(b != 0 for b in sector):
>              memcpy(&storage[...], sector, sizeof(sector))
>          else:
>              # Skip memcpy of zeroes
> 
> But this is probably a regression for us because we actually expect the zeroes
> to be copied?

Yes... Commit a4b15a8b9e mostly considered cloud payload optimization.

Since EEPROMs are usually "small", this could be kludged as:

-- >8 --
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1615,6 +1615,7 @@ static void m25p80_realize(SSIPeripheral *ss, 
Error **errp)
          trace_m25p80_binding(s);
          s->storage = blk_blockalign(s->blk, s->size);

+        memset(s->storage, ERASED_BYTE, s->size);
          if (blk_pread(s->blk, 0, s->size, s->storage, 0) < 0) {
              error_setg(errp, "failed to read the initial flash content");
              return;
---

On emulation, it is simpler to ask the user to provide a block image
with the same size of the emulated device. See commit 06f1521795
("pflash: Require backend size to match device, improve errors") for
more information.

>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 802d2eb021..dc5ffbc4ff 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -24,6 +24,7 @@
>>   #include "qemu/osdep.h"
>>   #include "qemu/units.h"
>>   #include "sysemu/block-backend.h"
>> +#include "hw/block/block.h"
>>   #include "hw/qdev-properties.h"
>>   #include "hw/qdev-properties-system.h"
>>   #include "hw/ssi/ssi.h"
>> @@ -1615,8 +1616,7 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
>>           trace_m25p80_binding(s);
>>           s->storage = blk_blockalign(s->blk, s->size);
>>   
>> -        if (blk_pread(s->blk, 0, s->size, s->storage, 0) < 0) {
>> -            error_setg(errp, "failed to read the initial flash content");
>> +        if (!blk_check_size_and_read_all(s->blk, s->storage, s->size, errp)) {
>>               return;
>>           }
>>       } else {
>> -- 
>> 2.39.1
>>