m25p80: Warn the user when the backend file is too small for the device

[PATCH] m25p80: Warn the user when the backend file is too small for the device

Posted by Cédric Le Goater 1 year, 5 months ago

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.

Improve the reported error with a new message regarding the file size.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/block/m25p80.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 02adc87527..e0515e2a1e 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1606,6 +1606,14 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
     if (s->blk) {
         uint64_t perm = BLK_PERM_CONSISTENT_READ |
                         (blk_supports_write_perm(s->blk) ? BLK_PERM_WRITE : 0);
+
+        if (blk_getlength(s->blk) < s->size) {
+            error_setg(errp,
+                       "backend file is too small for flash device %s (%d MB)",
+                       object_class_get_name(OBJECT_CLASS(mc)), s->size >> 20);
+            return;
+        }
+
         ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
         if (ret < 0) {
             return;
-- 
2.38.1

Re: [PATCH] m25p80: Warn the user when the backend file is too small for the device

Posted by Peter Maydell 1 year, 5 months ago

On Tue, 15 Nov 2022 at 14:22, 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.

The commit message says we get an unhelpful error if the
file size "does not match"...

> Improve the reported error with a new message regarding the file size.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/block/m25p80.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 02adc87527..e0515e2a1e 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1606,6 +1606,14 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
>      if (s->blk) {
>          uint64_t perm = BLK_PERM_CONSISTENT_READ |
>                          (blk_supports_write_perm(s->blk) ? BLK_PERM_WRITE : 0);
> +
> +        if (blk_getlength(s->blk) < s->size) {

...but the code change is only checking for "too small".

What happens if the user provides a backing file that is too large ?

> +            error_setg(errp,
> +                       "backend file is too small for flash device %s (%d MB)",
> +                       object_class_get_name(OBJECT_CLASS(mc)), s->size >> 20);

This potentially reports to the user a size which isn't the
right one for them to use to set the size of the backing file,
if that required size isn't an exact number of MB.

> +            return;
> +        }
> +
>          ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
>          if (ret < 0) {
>              return;
> --
> 2.38.1

thanks
-- PMM

Re: [PATCH] m25p80: Warn the user when the backend file is too small for the device

Posted by Cédric Le Goater 1 year, 5 months ago

On 11/15/22 15:34, Peter Maydell wrote:
> On Tue, 15 Nov 2022 at 14:22, 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.
> 
> The commit message says we get an unhelpful error if the
> file size "does not match"...
> 
>> Improve the reported error with a new message regarding the file size.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/block/m25p80.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 02adc87527..e0515e2a1e 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -1606,6 +1606,14 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
>>       if (s->blk) {
>>           uint64_t perm = BLK_PERM_CONSISTENT_READ |
>>                           (blk_supports_write_perm(s->blk) ? BLK_PERM_WRITE : 0);
>> +
>> +        if (blk_getlength(s->blk) < s->size) {
> 
> ...but the code change is only checking for "too small".
> 
> What happens if the user provides a backing file that is too large ?

That's ok because the blk_pread() call following, which loads in RAM
the initial data, won't fail.

It might be better to enforce a strict check on the size to avoid
further confusion ? and change the error message to be clear.
  
> 
>> +            error_setg(errp,
>> +                       "backend file is too small for flash device %s (%d MB)",
>> +                       object_class_get_name(OBJECT_CLASS(mc)), s->size >> 20);
> 
> This potentially reports to the user a size which isn't the
> right one for them to use to set the size of the backing file,
> if that required size isn't an exact number of MB.

True. We have a few devices which size is below 1MB. Using a KB unit
should be fine.

Thanks,

C.

> 
>> +            return;
>> +        }
>> +
>>           ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
>>           if (ret < 0) {
>>               return;
>> --
>> 2.38.1
> 
> thanks
> -- PMM

Re: [PATCH] m25p80: Warn the user when the backend file is too small for the device

Posted by Peter Maydell 1 year, 5 months ago

On Tue, 15 Nov 2022 at 14:51, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 11/15/22 15:34, Peter Maydell wrote:
> > On Tue, 15 Nov 2022 at 14:22, 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.
> >
> > The commit message says we get an unhelpful error if the
> > file size "does not match"...
> >
> >> Improve the reported error with a new message regarding the file size.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>   hw/block/m25p80.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> >> index 02adc87527..e0515e2a1e 100644
> >> --- a/hw/block/m25p80.c
> >> +++ b/hw/block/m25p80.c
> >> @@ -1606,6 +1606,14 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
> >>       if (s->blk) {
> >>           uint64_t perm = BLK_PERM_CONSISTENT_READ |
> >>                           (blk_supports_write_perm(s->blk) ? BLK_PERM_WRITE : 0);
> >> +
> >> +        if (blk_getlength(s->blk) < s->size) {
> >
> > ...but the code change is only checking for "too small".
> >
> > What happens if the user provides a backing file that is too large ?
>
> That's ok because the blk_pread() call following, which loads in RAM
> the initial data, won't fail.
>
> It might be better to enforce a strict check on the size to avoid
> further confusion ? and change the error message to be clear.

Can we use blk_check_size_and_read_all() here rather than
a manual "check size, and then pread" ? That will take care
of the error message for you and make this device behave
the same way as other flash devices which use block backends.

thanks
-- PMM

Re: [PATCH] m25p80: Warn the user when the backend file is too small for the device

Posted by Cédric Le Goater 1 year, 5 months ago

On 11/15/22 15:55, Peter Maydell wrote:
> On Tue, 15 Nov 2022 at 14:51, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 11/15/22 15:34, Peter Maydell wrote:
>>> On Tue, 15 Nov 2022 at 14:22, 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.
>>>
>>> The commit message says we get an unhelpful error if the
>>> file size "does not match"...
>>>
>>>> Improve the reported error with a new message regarding the file size.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>    hw/block/m25p80.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>>>> index 02adc87527..e0515e2a1e 100644
>>>> --- a/hw/block/m25p80.c
>>>> +++ b/hw/block/m25p80.c
>>>> @@ -1606,6 +1606,14 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
>>>>        if (s->blk) {
>>>>            uint64_t perm = BLK_PERM_CONSISTENT_READ |
>>>>                            (blk_supports_write_perm(s->blk) ? BLK_PERM_WRITE : 0);
>>>> +
>>>> +        if (blk_getlength(s->blk) < s->size) {
>>>
>>> ...but the code change is only checking for "too small".
>>>
>>> What happens if the user provides a backing file that is too large ?
>>
>> That's ok because the blk_pread() call following, which loads in RAM
>> the initial data, won't fail.
>>
>> It might be better to enforce a strict check on the size to avoid
>> further confusion ? and change the error message to be clear.
> 
> Can we use blk_check_size_and_read_all() here rather than
> a manual "check size, and then pread" ? That will take care
> of the error message for you and make this device behave
> the same way as other flash devices which use block backends.

ok. I wasn't aware of this routine. I will check.

Thanks,
C.