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

[Cédric Le Goater]

On 11/16/22 09:28, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
>
> > On 11/16/22 07:56, Markus Armbruster wrote:
> > > Cédric Le Goater <clg@kaod.org> writes:
> > >
> > > > 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.
> > > >
> > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > ---
> > > >    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 02adc87527..68a757abf3 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"
> > > > @@ -1614,8 +1615,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 {
> > >
> > > Ignorant question: what does blk_pread() on short read?  Does it fail?
> >
> > an underlying call to blk_check_byte_request() makes it fail.
>
> Thanks!

I tried to understand how  blk_set_allow_write_beyond_eof() worked but
I lack knowledge on the block area.
 
> > > Or does it succeed, returning how much it read?  I tried to find an
> > > answer in function comments, no luck.
> > >
> > > Are there more instances of "we fill some fixed-size memory (such as a
> > > ROM or flash) from a block backend?"
> >
> > Yes. There are other similar devices :  nand, nvram, pnv_pnor, etc.
>
> I think they should all be converted to blk_check_size_and_read_all().
> Not a prerequisite for getting this patch merged.  Volunteers?

I can do the ones I introduced for the Aspeed and PPC machines.

Or we find a way to allow the underlying backend file to grow when
accessed beyond EOF but *not* beyond the flash device size. This might
be the complex part. The device model does some checks but the block
backend as no idea of the upper layer limitations.

Thanks,

C.