| Message ID | 20230214171830.681594-2-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed: I2C fixes, -drive removal (first step) | expand |
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. > > 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? - Peter > > hw/block/m25p80.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > 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 >
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 >>
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 {