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

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

[Cédric Le Goater]

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 {
-- 
2.38.1

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

[Peter Maydell]

On Tue, 15 Nov 2022 at 15:10, Cédric Le Goater <clg@kaod.org> 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 <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 {
> --
> 2.38.1

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

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 <clg@kaod.org>
> ---
>   hw/block/m25p80.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Peter Delevoryas]

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 <clg@kaod.org>
> > ---
> >   hw/block/m25p80.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

Thanks Cedric!

Reviewed-by: Peter Delevoryas <peter@pjd.dev>

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

[Alistair Francis]

On Wed, Nov 16, 2022 at 1:13 AM Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

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

[Markus Armbruster]

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?
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

[Cédric Le Goater]

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.

> 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

[Markus Armbruster]

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!

>> 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

[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.

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

[Francisco Iglesias]

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 <clg@kaod.org>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  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
>