Re: [PATCH v2] m25p80: Improve error when the backend file size does not match the device
On [2022 Nov 15] Tue 16:10:00, 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. > > Signed-off-by: Cédric Le Goater Reviewed-by: Francisco Iglesias > --- > 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 { > -- > 2.38.1 >
Re: [PATCH v2] m25p80: Improve error when the backend file size does not match the device
On 11/16/22 09:28, Markus Armbruster wrote: Cédric Le Goater writes: On 11/16/22 07:56, Markus Armbruster wrote: Cédric Le Goater 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 --- 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.
Re: [PATCH v2] m25p80: Improve error when the backend file size does not match the device
Cédric Le Goater writes: > On 11/16/22 07:56, Markus Armbruster wrote: >> Cédric Le Goater 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 >>> --- >>> 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! >> 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?
Re: [PATCH v2] m25p80: Improve error when the backend file size does not match the device
On 11/16/22 07:56, Markus Armbruster wrote: Cédric Le Goater 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 --- 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. 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. C.
Re: [PATCH v2] m25p80: Improve error when the backend file size does not match the device
Cédric Le Goater 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 > --- > 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? 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?"
Re: [PATCH v2] m25p80: Improve error when the backend file size does not match the device
On Wed, Nov 16, 2022 at 1:13 AM 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. > > Signed-off-by: Cédric Le Goater Reviewed-by: Alistair Francis Alistair > --- > 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 { > -- > 2.38.1 > >
Re: [PATCH v2] m25p80: Improve error when the backend file size does not match the device
On Tue, Nov 15, 2022 at 04:50:11PM +0100, Philippe Mathieu-Daudé wrote: > On 15/11/22 16:10, 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. > > > > Signed-off-by: Cédric Le Goater > > --- > > hw/block/m25p80.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > Reviewed-by: Philippe Mathieu-Daudé > Thanks Cedric! Reviewed-by: Peter Delevoryas
Re: [PATCH v2] m25p80: Improve error when the backend file size does not match the device
On 15/11/22 16:10, 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. Signed-off-by: Cédric Le Goater --- hw/block/m25p80.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2] m25p80: Improve error when the backend file size does not match the device
On Tue, 15 Nov 2022 at 15:10, 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. > > Signed-off-by: Cédric Le Goater > --- > 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 { > -- > 2.38.1 Reviewed-by: Peter Maydell thanks -- PMM
[PATCH v2] m25p80: Improve error when the backend file size does not match the device
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 --- 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 { -- 2.38.1