[Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Markus Armbruster]

From: Alex Bennée <alex.bennee@linaro.org>

We reject undersized backends with a rather enigmatic "failed to read
the initial flash content" error.

We happily accept oversized images, ignoring their tail.  Throwing
away parts of firmware that way is pretty much certain to end in an
even more enigmatic failure to boot.

Require the backend's size to match the device's size exactly.  Report
mismatch as "device requires N bytes, block backend 'NAME' provides M
bytes".

Improve the error for actual read failures to "can't read initial
flash content from block backend 'NAME'.

We disabled code to limit device sizes to 8, 16, 32 or 64MiB more than
a decade ago in commit 95d1f3edd5e and c8b153d7949, v0.9.1.  Bury.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/block/pflash_cfi01.c | 31 ++++++++++++++++++++++---------
 hw/block/pflash_cfi02.c | 31 +++++++++++++++++++++++--------
 2 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 9d1c356eb6..2e3a05c0b3 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -730,13 +730,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
     device_len = sector_len_per_device * blocks_per_device;
 
-    /* XXX: to be fixed */
-#if 0
-    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
-        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
-        return NULL;
-#endif
-
     memory_region_init_rom_device(
         &pfl->mem, OBJECT(dev),
         &pflash_cfi01_ops,
@@ -763,12 +756,32 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
 
     if (pfl->blk) {
+        /*
+         * Validate the backing store is the right size for pflash
+         * devices. If the user supplies a larger file we ignore the
+         * tail.
+         */
+        int64_t backing_len = blk_getlength(pfl->blk);
+
+        if (backing_len < 0) {
+            error_setg(errp, "can't get size of block backend '%s'",
+                       blk_name(pfl->blk));
+            return;
+        }
+        if (backing_len != total_len) {
+            error_setg(errp, "device requires %" PRIu64 " bytes, "
+                       "block backend '%s' provides %" PRIu64 " bytes",
+                       total_len, blk_name(pfl->blk), backing_len);
+            return;
+        }
+
         /* read the initial flash content */
         ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
-
         if (ret < 0) {
             vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
-            error_setg(errp, "failed to read the initial flash content");
+            error_setg(errp, "can't read initial flash content"
+                       " from block backend '%s'",
+                       blk_name(pfl->blk));
             return;
         }
     }
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 33779ce807..45b88d65d8 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -550,12 +550,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     }
 
     chip_len = pfl->sector_len * pfl->nb_blocs;
-    /* XXX: to be fixed */
-#if 0
-    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
-        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
-        return NULL;
-#endif
 
     memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl), pfl->be ?
                                   &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
@@ -581,11 +575,32 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     }
 
     if (pfl->blk) {
+        /*
+         * Validate the backing store is the right size for pflash
+         * devices. If the user supplies a larger file we ignore the
+         * tail.
+         */
+        int64_t backing_len = blk_getlength(pfl->blk);
+
+        if (backing_len < 0) {
+            error_setg(errp, "can't get size of block backend '%s'",
+                       blk_name(pfl->blk));
+            return;
+        }
+        if (backing_len != chip_len) {
+            error_setg(errp, "device requires %" PRIu32 " bytes, "
+                       "block backend '%s' provides %" PRIu64 " bytes",
+                       chip_len, blk_name(pfl->blk), backing_len);
+            return;
+        }
+
         /* read the initial flash content */
         ret = blk_pread(pfl->blk, 0, pfl->storage, chip_len);
         if (ret < 0) {
-            vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl));
-            error_setg(errp, "failed to read the initial flash content");
+            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
+            error_setg(errp, "can't read initial flash content"
+                       " from block backend '%s'",
+                       blk_name(pfl->blk));
             return;
         }
     }
-- 
2.17.2

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Alex Bennée]

Markus Armbruster <armbru@redhat.com> writes:

> From: Alex Bennée <alex.bennee@linaro.org>
>
> We reject undersized backends with a rather enigmatic "failed to read
> the initial flash content" error.
>
> We happily accept oversized images, ignoring their tail.  Throwing
> away parts of firmware that way is pretty much certain to end in an
> even more enigmatic failure to boot.
>
> Require the backend's size to match the device's size exactly.  Report
> mismatch as "device requires N bytes, block backend 'NAME' provides M
> bytes".
>
> Improve the error for actual read failures to "can't read initial
> flash content from block backend 'NAME'.
>
> We disabled code to limit device sizes to 8, 16, 32 or 64MiB more than
> a decade ago in commit 95d1f3edd5e and c8b153d7949, v0.9.1.  Bury.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 31 ++++++++++++++++++++++---------
>  hw/block/pflash_cfi02.c | 31 +++++++++++++++++++++++--------
>  2 files changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 9d1c356eb6..2e3a05c0b3 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -730,13 +730,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>      device_len = sector_len_per_device * blocks_per_device;
>
> -    /* XXX: to be fixed */
> -#if 0
> -    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
> -        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
> -        return NULL;
> -#endif
> -
>      memory_region_init_rom_device(
>          &pfl->mem, OBJECT(dev),
>          &pflash_cfi01_ops,
> @@ -763,12 +756,32 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>
>      if (pfl->blk) {
> +        /*
> +         * Validate the backing store is the right size for pflash
> +         * devices. If the user supplies a larger file we ignore the
> +         * tail.
> +         */

We need to drop the last sentence...

> +        int64_t backing_len = blk_getlength(pfl->blk);
> +
> +        if (backing_len < 0) {
> +            error_setg(errp, "can't get size of block backend '%s'",
> +                       blk_name(pfl->blk));
> +            return;
> +        }
> +        if (backing_len != total_len) {
> +            error_setg(errp, "device requires %" PRIu64 " bytes, "
> +                       "block backend '%s' provides %" PRIu64 " bytes",
> +                       total_len, blk_name(pfl->blk), backing_len);
> +            return;
> +        }
> +
>          /* read the initial flash content */
>          ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
> -
>          if (ret < 0) {
>              vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
> -            error_setg(errp, "failed to read the initial flash content");
> +            error_setg(errp, "can't read initial flash content"
> +                       " from block backend '%s'",
> +                       blk_name(pfl->blk));
>              return;
>          }
>      }
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 33779ce807..45b88d65d8 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -550,12 +550,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      }
>
>      chip_len = pfl->sector_len * pfl->nb_blocs;
> -    /* XXX: to be fixed */
> -#if 0
> -    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
> -        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
> -        return NULL;
> -#endif
>
>      memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl), pfl->be ?
>                                    &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
> @@ -581,11 +575,32 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      }
>
>      if (pfl->blk) {
> +        /*
> +         * Validate the backing store is the right size for pflash
> +         * devices. If the user supplies a larger file we ignore the
> +         * tail.
> +         */

And here.

> +        int64_t backing_len = blk_getlength(pfl->blk);
> +
> +        if (backing_len < 0) {
> +            error_setg(errp, "can't get size of block backend '%s'",
> +                       blk_name(pfl->blk));
> +            return;
> +        }
> +        if (backing_len != chip_len) {
> +            error_setg(errp, "device requires %" PRIu32 " bytes, "
> +                       "block backend '%s' provides %" PRIu64 " bytes",
> +                       chip_len, blk_name(pfl->blk), backing_len);
> +            return;
> +        }
> +
>          /* read the initial flash content */
>          ret = blk_pread(pfl->blk, 0, pfl->storage, chip_len);
>          if (ret < 0) {
> -            vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl));
> -            error_setg(errp, "failed to read the initial flash content");
> +            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
> +            error_setg(errp, "can't read initial flash content"
> +                       " from block backend '%s'",
> +                       blk_name(pfl->blk));
>              return;
>          }
>      }

Otherwise all good... not that a r-b means anything from me ;-)

--
Alex Bennée

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Laszlo Ersek]

On 03/08/19 07:24, Markus Armbruster wrote:
> From: Alex Bennée <alex.bennee@linaro.org>
> 
> We reject undersized backends with a rather enigmatic "failed to read
> the initial flash content" error.
> 
> We happily accept oversized images, ignoring their tail.  Throwing
> away parts of firmware that way is pretty much certain to end in an
> even more enigmatic failure to boot.
> 
> Require the backend's size to match the device's size exactly.  Report
> mismatch as "device requires N bytes, block backend 'NAME' provides M
> bytes".
> 
> Improve the error for actual read failures to "can't read initial
> flash content from block backend 'NAME'.
> 
> We disabled code to limit device sizes to 8, 16, 32 or 64MiB more than
> a decade ago in commit 95d1f3edd5e and c8b153d7949, v0.9.1.  Bury.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 31 ++++++++++++++++++++++---------
>  hw/block/pflash_cfi02.c | 31 +++++++++++++++++++++++--------
>  2 files changed, 45 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 9d1c356eb6..2e3a05c0b3 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -730,13 +730,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>      device_len = sector_len_per_device * blocks_per_device;
>  
> -    /* XXX: to be fixed */
> -#if 0
> -    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
> -        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
> -        return NULL;
> -#endif
> -
>      memory_region_init_rom_device(
>          &pfl->mem, OBJECT(dev),
>          &pflash_cfi01_ops,
> @@ -763,12 +756,32 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>  
>      if (pfl->blk) {
> +        /*
> +         * Validate the backing store is the right size for pflash
> +         * devices. If the user supplies a larger file we ignore the
> +         * tail.
> +         */
> +        int64_t backing_len = blk_getlength(pfl->blk);
> +
> +        if (backing_len < 0) {
> +            error_setg(errp, "can't get size of block backend '%s'",
> +                       blk_name(pfl->blk));
> +            return;
> +        }
> +        if (backing_len != total_len) {
> +            error_setg(errp, "device requires %" PRIu64 " bytes, "
> +                       "block backend '%s' provides %" PRIu64 " bytes",
> +                       total_len, blk_name(pfl->blk), backing_len);
> +            return;
> +        }
> +
>          /* read the initial flash content */
>          ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
> -
>          if (ret < 0) {
>              vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
> -            error_setg(errp, "failed to read the initial flash content");
> +            error_setg(errp, "can't read initial flash content"
> +                       " from block backend '%s'",
> +                       blk_name(pfl->blk));
>              return;
>          }
>      }
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 33779ce807..45b88d65d8 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -550,12 +550,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      }
>  
>      chip_len = pfl->sector_len * pfl->nb_blocs;
> -    /* XXX: to be fixed */
> -#if 0
> -    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
> -        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
> -        return NULL;
> -#endif
>  
>      memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl), pfl->be ?
>                                    &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
> @@ -581,11 +575,32 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      }
>  
>      if (pfl->blk) {
> +        /*
> +         * Validate the backing store is the right size for pflash
> +         * devices. If the user supplies a larger file we ignore the
> +         * tail.
> +         */
> +        int64_t backing_len = blk_getlength(pfl->blk);
> +
> +        if (backing_len < 0) {
> +            error_setg(errp, "can't get size of block backend '%s'",
> +                       blk_name(pfl->blk));
> +            return;
> +        }
> +        if (backing_len != chip_len) {
> +            error_setg(errp, "device requires %" PRIu32 " bytes, "
> +                       "block backend '%s' provides %" PRIu64 " bytes",
> +                       chip_len, blk_name(pfl->blk), backing_len);
> +            return;
> +        }
> +
>          /* read the initial flash content */
>          ret = blk_pread(pfl->blk, 0, pfl->storage, chip_len);
>          if (ret < 0) {
> -            vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl));
> -            error_setg(errp, "failed to read the initial flash content");
> +            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
> +            error_setg(errp, "can't read initial flash content"
> +                       " from block backend '%s'",
> +                       blk_name(pfl->blk));
>              return;
>          }
>      }
> 

This one has got to be one of the longest bike-shedding sessions! :)

I'm fine with this patch, but I could suggest two improvements.

(1) When blk_getlength() fails, we could format the negative error code
returned by it into the error message.

(2) We could extract the common code to a new function in
"hw/block/block.c". (It says "Common code for block device models" on
the tin.)


I realize the vmstate_unregister_ram() rollbacks are not identical
between cfi01 and cfi02. However, that difference only draws my
attention to the fact that the unregister calls are not made on the
other error branches in this patch anyway!

So we should have a common function like this:

void pflash_cfi_read_initial_content(BlockBackend *blk, uint64_t size,
                                     void *buf, Error **errp)
{
    int64_t backing_len = blk_getlength(blk);
    int ret;

    if (backing_len < 0) {
        error_setg(...);
        return;
    }

    if (backing_len != size) {
        error_setg(...);
        return;
    }

    ret = blk_pread(blk, 0, buf, size);
    if (ret < 0) {
        error_setg(...);
    }
}

And at the call sites, we can do

    if (pfl->blk) {
        Error *local_err = NULL;

        pflash_cfi_read_initial_content(pfl->blk,
                                        total_len OR chip_len,
                                        pfl->storage,
                                        &local_err);
        if (local_err) {
            error_propagate(errp, local_err);
            vmstate_unregister_ram(...); // as appropriate for cfiXX
            return;
        }
    }

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Markus Armbruster]

Alex Bennée <alex.bennee@linaro.org> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> From: Alex Bennée <alex.bennee@linaro.org>
>>
>> We reject undersized backends with a rather enigmatic "failed to read
>> the initial flash content" error.
>>
>> We happily accept oversized images, ignoring their tail.  Throwing
>> away parts of firmware that way is pretty much certain to end in an
>> even more enigmatic failure to boot.
>>
>> Require the backend's size to match the device's size exactly.  Report
>> mismatch as "device requires N bytes, block backend 'NAME' provides M
>> bytes".
>>
>> Improve the error for actual read failures to "can't read initial
>> flash content from block backend 'NAME'.
>>
>> We disabled code to limit device sizes to 8, 16, 32 or 64MiB more than
>> a decade ago in commit 95d1f3edd5e and c8b153d7949, v0.9.1.  Bury.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/block/pflash_cfi01.c | 31 ++++++++++++++++++++++---------
>>  hw/block/pflash_cfi02.c | 31 +++++++++++++++++++++++--------
>>  2 files changed, 45 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 9d1c356eb6..2e3a05c0b3 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -730,13 +730,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>>      }
>>      device_len = sector_len_per_device * blocks_per_device;
>>
>> -    /* XXX: to be fixed */
>> -#if 0
>> -    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
>> -        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
>> -        return NULL;
>> -#endif
>> -
>>      memory_region_init_rom_device(
>>          &pfl->mem, OBJECT(dev),
>>          &pflash_cfi01_ops,
>> @@ -763,12 +756,32 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>>      }
>>
>>      if (pfl->blk) {
>> +        /*
>> +         * Validate the backing store is the right size for pflash
>> +         * devices. If the user supplies a larger file we ignore the
>> +         * tail.
>> +         */
>
> We need to drop the last sentence...

D'oh!

>> +        int64_t backing_len = blk_getlength(pfl->blk);
>> +
>> +        if (backing_len < 0) {
>> +            error_setg(errp, "can't get size of block backend '%s'",
>> +                       blk_name(pfl->blk));
>> +            return;
>> +        }
>> +        if (backing_len != total_len) {
>> +            error_setg(errp, "device requires %" PRIu64 " bytes, "
>> +                       "block backend '%s' provides %" PRIu64 " bytes",
>> +                       total_len, blk_name(pfl->blk), backing_len);
>> +            return;
>> +        }
>> +
>>          /* read the initial flash content */
>>          ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
>> -
>>          if (ret < 0) {
>>              vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
>> -            error_setg(errp, "failed to read the initial flash content");
>> +            error_setg(errp, "can't read initial flash content"
>> +                       " from block backend '%s'",
>> +                       blk_name(pfl->blk));
>>              return;
>>          }
>>      }
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index 33779ce807..45b88d65d8 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -550,12 +550,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>>      }
>>
>>      chip_len = pfl->sector_len * pfl->nb_blocs;
>> -    /* XXX: to be fixed */
>> -#if 0
>> -    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
>> -        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
>> -        return NULL;
>> -#endif
>>
>>      memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl), pfl->be ?
>>                                    &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
>> @@ -581,11 +575,32 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>>      }
>>
>>      if (pfl->blk) {
>> +        /*
>> +         * Validate the backing store is the right size for pflash
>> +         * devices. If the user supplies a larger file we ignore the
>> +         * tail.
>> +         */
>
> And here.
>
>> +        int64_t backing_len = blk_getlength(pfl->blk);
>> +
>> +        if (backing_len < 0) {
>> +            error_setg(errp, "can't get size of block backend '%s'",
>> +                       blk_name(pfl->blk));
>> +            return;
>> +        }
>> +        if (backing_len != chip_len) {
>> +            error_setg(errp, "device requires %" PRIu32 " bytes, "
>> +                       "block backend '%s' provides %" PRIu64 " bytes",
>> +                       chip_len, blk_name(pfl->blk), backing_len);
>> +            return;
>> +        }
>> +
>>          /* read the initial flash content */
>>          ret = blk_pread(pfl->blk, 0, pfl->storage, chip_len);
>>          if (ret < 0) {
>> -            vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl));
>> -            error_setg(errp, "failed to read the initial flash content");
>> +            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
>> +            error_setg(errp, "can't read initial flash content"
>> +                       " from block backend '%s'",
>> +                       blk_name(pfl->blk));
>>              return;
>>          }
>>      }
>
> Otherwise all good... not that a r-b means anything from me ;-)

v8 coming up...  thanks!

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 03/08/19 07:24, Markus Armbruster wrote:
>> From: Alex Bennée <alex.bennee@linaro.org>
>> 
>> We reject undersized backends with a rather enigmatic "failed to read
>> the initial flash content" error.
>> 
>> We happily accept oversized images, ignoring their tail.  Throwing
>> away parts of firmware that way is pretty much certain to end in an
>> even more enigmatic failure to boot.
>> 
>> Require the backend's size to match the device's size exactly.  Report
>> mismatch as "device requires N bytes, block backend 'NAME' provides M
>> bytes".
>> 
>> Improve the error for actual read failures to "can't read initial
>> flash content from block backend 'NAME'.
>> 
>> We disabled code to limit device sizes to 8, 16, 32 or 64MiB more than
>> a decade ago in commit 95d1f3edd5e and c8b153d7949, v0.9.1.  Bury.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/block/pflash_cfi01.c | 31 ++++++++++++++++++++++---------
>>  hw/block/pflash_cfi02.c | 31 +++++++++++++++++++++++--------
>>  2 files changed, 45 insertions(+), 17 deletions(-)
>> 
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 9d1c356eb6..2e3a05c0b3 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -730,13 +730,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>>      }
>>      device_len = sector_len_per_device * blocks_per_device;
>>  
>> -    /* XXX: to be fixed */
>> -#if 0
>> -    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
>> -        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
>> -        return NULL;
>> -#endif
>> -
>>      memory_region_init_rom_device(
>>          &pfl->mem, OBJECT(dev),
>>          &pflash_cfi01_ops,
>> @@ -763,12 +756,32 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>>      }
>>  
>>      if (pfl->blk) {
>> +        /*
>> +         * Validate the backing store is the right size for pflash
>> +         * devices. If the user supplies a larger file we ignore the
>> +         * tail.
>> +         */
>> +        int64_t backing_len = blk_getlength(pfl->blk);
>> +
>> +        if (backing_len < 0) {
>> +            error_setg(errp, "can't get size of block backend '%s'",
>> +                       blk_name(pfl->blk));
>> +            return;
>> +        }
>> +        if (backing_len != total_len) {
>> +            error_setg(errp, "device requires %" PRIu64 " bytes, "
>> +                       "block backend '%s' provides %" PRIu64 " bytes",
>> +                       total_len, blk_name(pfl->blk), backing_len);
>> +            return;
>> +        }
>> +
>>          /* read the initial flash content */
>>          ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
>> -
>>          if (ret < 0) {
>>              vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
>> -            error_setg(errp, "failed to read the initial flash content");
>> +            error_setg(errp, "can't read initial flash content"
>> +                       " from block backend '%s'",
>> +                       blk_name(pfl->blk));
>>              return;
>>          }
>>      }
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index 33779ce807..45b88d65d8 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -550,12 +550,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>>      }
>>  
>>      chip_len = pfl->sector_len * pfl->nb_blocs;
>> -    /* XXX: to be fixed */
>> -#if 0
>> -    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
>> -        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
>> -        return NULL;
>> -#endif
>>  
>>      memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl), pfl->be ?
>>                                    &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
>> @@ -581,11 +575,32 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>>      }
>>  
>>      if (pfl->blk) {
>> +        /*
>> +         * Validate the backing store is the right size for pflash
>> +         * devices. If the user supplies a larger file we ignore the
>> +         * tail.
>> +         */
>> +        int64_t backing_len = blk_getlength(pfl->blk);
>> +
>> +        if (backing_len < 0) {
>> +            error_setg(errp, "can't get size of block backend '%s'",
>> +                       blk_name(pfl->blk));
>> +            return;
>> +        }
>> +        if (backing_len != chip_len) {
>> +            error_setg(errp, "device requires %" PRIu32 " bytes, "
>> +                       "block backend '%s' provides %" PRIu64 " bytes",
>> +                       chip_len, blk_name(pfl->blk), backing_len);
>> +            return;
>> +        }
>> +
>>          /* read the initial flash content */
>>          ret = blk_pread(pfl->blk, 0, pfl->storage, chip_len);
>>          if (ret < 0) {
>> -            vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl));
>> -            error_setg(errp, "failed to read the initial flash content");
>> +            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
>> +            error_setg(errp, "can't read initial flash content"
>> +                       " from block backend '%s'",
>> +                       blk_name(pfl->blk));
>>              return;
>>          }
>>      }
>> 
>
> This one has got to be one of the longest bike-shedding sessions! :)
>
> I'm fine with this patch, but I could suggest two improvements.
>
> (1) When blk_getlength() fails, we could format the negative error code
> returned by it into the error message.

I can do that.

> (2) We could extract the common code to a new function in
> "hw/block/block.c". (It says "Common code for block device models" on
> the tin.)

There's so much common code in these two files even before this patch...

> I realize the vmstate_unregister_ram() rollbacks are not identical
> between cfi01 and cfi02. However, that difference only draws my
> attention to the fact that the unregister calls are not made on the
> other error branches in this patch anyway!

You're right, that needs fixing.

> So we should have a common function like this:
>
> void pflash_cfi_read_initial_content(BlockBackend *blk, uint64_t size,
>                                      void *buf, Error **errp)
> {
>     int64_t backing_len = blk_getlength(blk);
>     int ret;
>
>     if (backing_len < 0) {
>         error_setg(...);
>         return;
>     }
>
>     if (backing_len != size) {
>         error_setg(...);
>         return;
>     }
>
>     ret = blk_pread(blk, 0, buf, size);
>     if (ret < 0) {
>         error_setg(...);
>     }
> }
>
> And at the call sites, we can do
>
>     if (pfl->blk) {
>         Error *local_err = NULL;
>
>         pflash_cfi_read_initial_content(pfl->blk,
>                                         total_len OR chip_len,
>                                         pfl->storage,
>                                         &local_err);
>         if (local_err) {
>             error_propagate(errp, local_err);
>             vmstate_unregister_ram(...); // as appropriate for cfiXX
>             return;
>         }
>     }

I'll give it a try for v8.  Thanks!

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Kevin Wolf]

Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben:
> Laszlo Ersek <lersek@redhat.com> writes:
> > This one has got to be one of the longest bike-shedding sessions! :)
> >
> > I'm fine with this patch, but I could suggest two improvements.
> >
> > (1) When blk_getlength() fails, we could format the negative error code
> > returned by it into the error message.
> 
> I can do that.

By using error_setg_errno(), I assume. Not throwing away error details
is always good.

> > (2) We could extract the common code to a new function in
> > "hw/block/block.c". (It says "Common code for block device models" on
> > the tin.)
> 
> There's so much common code in these two files even before this patch...

My understanding is that hw/block/block.c contains code that is
potentially useful to all kinds of block devices, not random code that
two specific similar devices happen to share.

If we want to deduplicate some code in the flash devices, without any
expectation that other devices will use it at some point, I'd rather
create a new source file hw/block/pflash_common.c or something like
that.

Kevin

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Laszlo Ersek]

On 03/08/19 14:35, Kevin Wolf wrote:
> Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben:
>> Laszlo Ersek <lersek@redhat.com> writes:
>>> This one has got to be one of the longest bike-shedding sessions! :)
>>>
>>> I'm fine with this patch, but I could suggest two improvements.
>>>
>>> (1) When blk_getlength() fails, we could format the negative error code
>>> returned by it into the error message.
>>
>> I can do that.
> 
> By using error_setg_errno(), I assume. Not throwing away error details
> is always good.
> 
>>> (2) We could extract the common code to a new function in
>>> "hw/block/block.c". (It says "Common code for block device models" on
>>> the tin.)
>>
>> There's so much common code in these two files even before this patch...
> 
> My understanding is that hw/block/block.c contains code that is
> potentially useful to all kinds of block devices, not random code that
> two specific similar devices happen to share.

Ah, OK.

> If we want to deduplicate some code in the flash devices, without any
> expectation that other devices will use it at some point, I'd rather
> create a new source file hw/block/pflash_common.c or something like
> that.

Sure, that makes a lot of sense then.

Cheers
Laszlo

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Philippe Mathieu-Daudé]

On 3/8/19 2:35 PM, Kevin Wolf wrote:
> Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben:
>> Laszlo Ersek <lersek@redhat.com> writes:
>>> This one has got to be one of the longest bike-shedding sessions! :)
>>>
>>> I'm fine with this patch, but I could suggest two improvements.
>>>
>>> (1) When blk_getlength() fails, we could format the negative error code
>>> returned by it into the error message.
>>
>> I can do that.
> 
> By using error_setg_errno(), I assume. Not throwing away error details
> is always good.
> 
>>> (2) We could extract the common code to a new function in
>>> "hw/block/block.c". (It says "Common code for block device models" on
>>> the tin.)
>>
>> There's so much common code in these two files even before this patch...
> 
> My understanding is that hw/block/block.c contains code that is
> potentially useful to all kinds of block devices, not random code that
> two specific similar devices happen to share.
> 
> If we want to deduplicate some code in the flash devices, without any
> expectation that other devices will use it at some point, I'd rather
> create a new source file hw/block/pflash_common.c or something like
> that.

Agreed, I like that suggestion too.
Actually we already have been talking about a such
cleanup/refactor/rewrite with Markus, but this will wait the next QEMU
dev cycle.

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben:
>> Laszlo Ersek <lersek@redhat.com> writes:
>> > This one has got to be one of the longest bike-shedding sessions! :)
>> >
>> > I'm fine with this patch, but I could suggest two improvements.
>> >
>> > (1) When blk_getlength() fails, we could format the negative error code
>> > returned by it into the error message.
>> 
>> I can do that.
>
> By using error_setg_errno(), I assume. Not throwing away error details
> is always good.
>
>> > (2) We could extract the common code to a new function in
>> > "hw/block/block.c". (It says "Common code for block device models" on
>> > the tin.)
>> 
>> There's so much common code in these two files even before this patch...
>
> My understanding is that hw/block/block.c contains code that is
> potentially useful to all kinds of block devices, not random code that
> two specific similar devices happen to share.
>
> If we want to deduplicate some code in the flash devices, without any
> expectation that other devices will use it at some point, I'd rather
> create a new source file hw/block/pflash_common.c or something like
> that.

Yes.

The helper I came up with (appended) isn't really specific to flash
devices.  Would it be okay for hw/block/block.c even though only the two
flash devices use it for now?


bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
                                 Error **errp)
{
    int64_t blk_len;
    int ret;

    blk_len = blk_getlength(blk);
    if (blk_len < 0) {
        error_setg_errno(errp, -blk_len,
                         "can't get size of block backend '%s'",
                         blk_name(blk));
        return false;
    }
    if (blk_len != size) {
        error_setg(errp, "device requires %" PRIu64 " bytes, "
                   "block backend '%s' provides %" PRIu64 " bytes",
                   size, blk_name(blk), blk_len);
        return false;
    }

    /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */
    assert(size <= BDRV_REQUEST_MAX_BYTES);
    ret = blk_pread(blk, 0, buf, size);
    if (ret < 0) {
        error_setg_errno(errp, -ret, "can't read block backend '%s'",
                         blk_name(blk));
        return false;
    }
    return true;
}

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Philippe Mathieu-Daudé]

On 3/8/19 12:47 PM, Laszlo Ersek wrote:
> On 03/08/19 07:24, Markus Armbruster wrote:
>> From: Alex Bennée <alex.bennee@linaro.org>
>>
>> We reject undersized backends with a rather enigmatic "failed to read
>> the initial flash content" error.
>>
>> We happily accept oversized images, ignoring their tail.  Throwing
>> away parts of firmware that way is pretty much certain to end in an
>> even more enigmatic failure to boot.
>>
>> Require the backend's size to match the device's size exactly.  Report
>> mismatch as "device requires N bytes, block backend 'NAME' provides M
>> bytes".
>>
>> Improve the error for actual read failures to "can't read initial
>> flash content from block backend 'NAME'.
>>
>> We disabled code to limit device sizes to 8, 16, 32 or 64MiB more than
>> a decade ago in commit 95d1f3edd5e and c8b153d7949, v0.9.1.  Bury.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/block/pflash_cfi01.c | 31 ++++++++++++++++++++++---------
>>  hw/block/pflash_cfi02.c | 31 +++++++++++++++++++++++--------
>>  2 files changed, 45 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 9d1c356eb6..2e3a05c0b3 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -730,13 +730,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>>      }
>>      device_len = sector_len_per_device * blocks_per_device;
>>  
>> -    /* XXX: to be fixed */
>> -#if 0
>> -    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
>> -        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
>> -        return NULL;
>> -#endif
>> -
>>      memory_region_init_rom_device(
>>          &pfl->mem, OBJECT(dev),
>>          &pflash_cfi01_ops,
>> @@ -763,12 +756,32 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>>      }
>>  
>>      if (pfl->blk) {
>> +        /*
>> +         * Validate the backing store is the right size for pflash
>> +         * devices. If the user supplies a larger file we ignore the
>> +         * tail.
>> +         */
>> +        int64_t backing_len = blk_getlength(pfl->blk);
>> +
>> +        if (backing_len < 0) {
>> +            error_setg(errp, "can't get size of block backend '%s'",
>> +                       blk_name(pfl->blk));
>> +            return;
>> +        }
>> +        if (backing_len != total_len) {
>> +            error_setg(errp, "device requires %" PRIu64 " bytes, "
>> +                       "block backend '%s' provides %" PRIu64 " bytes",
>> +                       total_len, blk_name(pfl->blk), backing_len);
>> +            return;
>> +        }
>> +
>>          /* read the initial flash content */
>>          ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
>> -
>>          if (ret < 0) {
>>              vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
>> -            error_setg(errp, "failed to read the initial flash content");
>> +            error_setg(errp, "can't read initial flash content"
>> +                       " from block backend '%s'",
>> +                       blk_name(pfl->blk));
>>              return;
>>          }
>>      }
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index 33779ce807..45b88d65d8 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -550,12 +550,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>>      }
>>  
>>      chip_len = pfl->sector_len * pfl->nb_blocs;
>> -    /* XXX: to be fixed */
>> -#if 0
>> -    if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
>> -        total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
>> -        return NULL;
>> -#endif
>>  
>>      memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl), pfl->be ?
>>                                    &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
>> @@ -581,11 +575,32 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>>      }
>>  
>>      if (pfl->blk) {
>> +        /*
>> +         * Validate the backing store is the right size for pflash
>> +         * devices. If the user supplies a larger file we ignore the
>> +         * tail.
>> +         */
>> +        int64_t backing_len = blk_getlength(pfl->blk);
>> +
>> +        if (backing_len < 0) {
>> +            error_setg(errp, "can't get size of block backend '%s'",
>> +                       blk_name(pfl->blk));
>> +            return;
>> +        }
>> +        if (backing_len != chip_len) {
>> +            error_setg(errp, "device requires %" PRIu32 " bytes, "
>> +                       "block backend '%s' provides %" PRIu64 " bytes",
>> +                       chip_len, blk_name(pfl->blk), backing_len);
>> +            return;
>> +        }
>> +
>>          /* read the initial flash content */
>>          ret = blk_pread(pfl->blk, 0, pfl->storage, chip_len);
>>          if (ret < 0) {
>> -            vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl));
>> -            error_setg(errp, "failed to read the initial flash content");
>> +            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
>> +            error_setg(errp, "can't read initial flash content"
>> +                       " from block backend '%s'",
>> +                       blk_name(pfl->blk));
>>              return;
>>          }
>>      }
>>
> 
> This one has got to be one of the longest bike-shedding sessions! :)
> 
> I'm fine with this patch, but I could suggest two improvements.
> 
> (1) When blk_getlength() fails, we could format the negative error code
> returned by it into the error message.

This one is a worthwhile a v8.

> (2) We could extract the common code to a new function in
> "hw/block/block.c". (It says "Common code for block device models" on
> the tin.)

Good idea, but I'd postpone that for the next dev cycle.

> I realize the vmstate_unregister_ram() rollbacks are not identical
> between cfi01 and cfi02. However, that difference only draws my
> attention to the fact that the unregister calls are not made on the
> other error branches in this patch anyway!
> 
> So we should have a common function like this:
> 
> void pflash_cfi_read_initial_content(BlockBackend *blk, uint64_t size,
>                                      void *buf, Error **errp)
> {
>     int64_t backing_len = blk_getlength(blk);
>     int ret;
> 
>     if (backing_len < 0) {
>         error_setg(...);
>         return;
>     }
> 
>     if (backing_len != size) {
>         error_setg(...);
>         return;
>     }
> 
>     ret = blk_pread(blk, 0, buf, size);
>     if (ret < 0) {
>         error_setg(...);
>     }
> }
> 
> And at the call sites, we can do
> 
>     if (pfl->blk) {
>         Error *local_err = NULL;
> 
>         pflash_cfi_read_initial_content(pfl->blk,
>                                         total_len OR chip_len,
>                                         pfl->storage,
>                                         &local_err);
>         if (local_err) {
>             error_propagate(errp, local_err);
>             vmstate_unregister_ram(...); // as appropriate for cfiXX
>             return;
>         }
>     }
> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Kevin Wolf]

Am 08.03.2019 um 15:29 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben:
> >> Laszlo Ersek <lersek@redhat.com> writes:
> >> > This one has got to be one of the longest bike-shedding sessions! :)
> >> >
> >> > I'm fine with this patch, but I could suggest two improvements.
> >> >
> >> > (1) When blk_getlength() fails, we could format the negative error code
> >> > returned by it into the error message.
> >> 
> >> I can do that.
> >
> > By using error_setg_errno(), I assume. Not throwing away error details
> > is always good.
> >
> >> > (2) We could extract the common code to a new function in
> >> > "hw/block/block.c". (It says "Common code for block device models" on
> >> > the tin.)
> >> 
> >> There's so much common code in these two files even before this patch...
> >
> > My understanding is that hw/block/block.c contains code that is
> > potentially useful to all kinds of block devices, not random code that
> > two specific similar devices happen to share.
> >
> > If we want to deduplicate some code in the flash devices, without any
> > expectation that other devices will use it at some point, I'd rather
> > create a new source file hw/block/pflash_common.c or something like
> > that.
> 
> Yes.
> 
> The helper I came up with (appended) isn't really specific to flash
> devices.  Would it be okay for hw/block/block.c even though only the two
> flash devices use it for now?

Hm, it feels more like a helper for devices that can't decide whether
they want to be a block device or not. Or that actually don't want to be
a block device, but use a BlockBackend anyway. Reading in the whole
image isn't something that a normal block device would do.

But yes, it doesn't have flash-specific knowledge, even though I hope
that it's functionality that will remain very specific to these two
devices.

So it's your call, I don't have a strong opinion either way.

> 
> bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>                                  Error **errp)
> {
>     int64_t blk_len;
>     int ret;
> 
>     blk_len = blk_getlength(blk);
>     if (blk_len < 0) {
>         error_setg_errno(errp, -blk_len,
>                          "can't get size of block backend '%s'",
>                          blk_name(blk));
>         return false;
>     }
>     if (blk_len != size) {
>         error_setg(errp, "device requires %" PRIu64 " bytes, "
>                    "block backend '%s' provides %" PRIu64 " bytes",
>                    size, blk_name(blk), blk_len);

Should size use HWADDR_PRIu?

I'm not sure if printing the BlockBackend name is a good idea because
hopefully one day the BlockBackend will be anonymous even for the flash
devices.

>         return false;
>     }
> 
>     /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */
>     assert(size <= BDRV_REQUEST_MAX_BYTES);

I don't think we'd ever want to read in more than 2 GB into a memory
buffer. Before we even get close to this point, the devices should be
reworked to be more like an actual block device and read only what is
actually accessed.

>     ret = blk_pread(blk, 0, buf, size);
>     if (ret < 0) {
>         error_setg_errno(errp, -ret, "can't read block backend '%s'",
>                          blk_name(blk));
>         return false;
>     }
>     return true;
> }

Kevin

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Philippe Mathieu-Daudé]

On 3/8/19 4:40 PM, Kevin Wolf wrote:
> Am 08.03.2019 um 15:29 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben:
>>>> Laszlo Ersek <lersek@redhat.com> writes:
>>>>> This one has got to be one of the longest bike-shedding sessions! :)
>>>>>
>>>>> I'm fine with this patch, but I could suggest two improvements.
>>>>>
>>>>> (1) When blk_getlength() fails, we could format the negative error code
>>>>> returned by it into the error message.
>>>>
>>>> I can do that.
>>>
>>> By using error_setg_errno(), I assume. Not throwing away error details
>>> is always good.
>>>
>>>>> (2) We could extract the common code to a new function in
>>>>> "hw/block/block.c". (It says "Common code for block device models" on
>>>>> the tin.)
>>>>
>>>> There's so much common code in these two files even before this patch...
>>>
>>> My understanding is that hw/block/block.c contains code that is
>>> potentially useful to all kinds of block devices, not random code that
>>> two specific similar devices happen to share.
>>>
>>> If we want to deduplicate some code in the flash devices, without any
>>> expectation that other devices will use it at some point, I'd rather
>>> create a new source file hw/block/pflash_common.c or something like
>>> that.
>>
>> Yes.
>>
>> The helper I came up with (appended) isn't really specific to flash
>> devices.  Would it be okay for hw/block/block.c even though only the two
>> flash devices use it for now?
> 
> Hm, it feels more like a helper for devices that can't decide whether
> they want to be a block device or not. Or that actually don't want to be
> a block device, but use a BlockBackend anyway. Reading in the whole
> image isn't something that a normal block device would do.
> 
> But yes, it doesn't have flash-specific knowledge, even though I hope
> that it's functionality that will remain very specific to these two
> devices.
> 
> So it's your call, I don't have a strong opinion either way.
> 
>>
>> bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>>                                  Error **errp)
>> {
>>     int64_t blk_len;
>>     int ret;
>>
>>     blk_len = blk_getlength(blk);
>>     if (blk_len < 0) {
>>         error_setg_errno(errp, -blk_len,
>>                          "can't get size of block backend '%s'",
>>                          blk_name(blk));
>>         return false;
>>     }
>>     if (blk_len != size) {
>>         error_setg(errp, "device requires %" PRIu64 " bytes, "
>>                    "block backend '%s' provides %" PRIu64 " bytes",
>>                    size, blk_name(blk), blk_len);
> 
> Should size use HWADDR_PRIu?
> 
> I'm not sure if printing the BlockBackend name is a good idea because
> hopefully one day the BlockBackend will be anonymous even for the flash
> devices.
> 
>>         return false;
>>     }
>>
>>     /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */
>>     assert(size <= BDRV_REQUEST_MAX_BYTES);
> 
> I don't think we'd ever want to read in more than 2 GB into a memory
> buffer. Before we even get close to this point, the devices should be
> reworked to be more like an actual block device and read only what is
> actually accessed.

[bikeshedding again]

So you eventually found the root problem of this device. It tries to use
files as 'raw' format block. Since the pflash devices provide write
access, we create a block device in memory.
We should:
- use raw files as ROM (without using a pflash device)
- enforce the pflash device to use block formatted files
- provide a tool to help converting raw file to pflash block, or improve
  the device documentation.

>>     ret = blk_pread(blk, 0, buf, size);
>>     if (ret < 0) {
>>         error_setg_errno(errp, -ret, "can't read block backend '%s'",
>>                          blk_name(blk));
>>         return false;
>>     }
>>     return true;
>> }
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 08.03.2019 um 15:29 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben:
>> >> Laszlo Ersek <lersek@redhat.com> writes:
>> >> > This one has got to be one of the longest bike-shedding sessions! :)
>> >> >
>> >> > I'm fine with this patch, but I could suggest two improvements.
>> >> >
>> >> > (1) When blk_getlength() fails, we could format the negative error code
>> >> > returned by it into the error message.
>> >> 
>> >> I can do that.
>> >
>> > By using error_setg_errno(), I assume. Not throwing away error details
>> > is always good.
>> >
>> >> > (2) We could extract the common code to a new function in
>> >> > "hw/block/block.c". (It says "Common code for block device models" on
>> >> > the tin.)
>> >> 
>> >> There's so much common code in these two files even before this patch...
>> >
>> > My understanding is that hw/block/block.c contains code that is
>> > potentially useful to all kinds of block devices, not random code that
>> > two specific similar devices happen to share.
>> >
>> > If we want to deduplicate some code in the flash devices, without any
>> > expectation that other devices will use it at some point, I'd rather
>> > create a new source file hw/block/pflash_common.c or something like
>> > that.
>> 
>> Yes.
>> 
>> The helper I came up with (appended) isn't really specific to flash
>> devices.  Would it be okay for hw/block/block.c even though only the two
>> flash devices use it for now?
>
> Hm, it feels more like a helper for devices that can't decide whether
> they want to be a block device or not. Or that actually don't want to be
> a block device, but use a BlockBackend anyway.

I think the latter's precisely what the two pflash devices are.

>                                                Reading in the whole
> image isn't something that a normal block device would do.

No argument.

> But yes, it doesn't have flash-specific knowledge, even though I hope
> that it's functionality that will remain very specific to these two
> devices.
>
> So it's your call, I don't have a strong opinion either way.

Understood.

>> bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>>                                  Error **errp)
>> {
>>     int64_t blk_len;
>>     int ret;
>> 
>>     blk_len = blk_getlength(blk);
>>     if (blk_len < 0) {
>>         error_setg_errno(errp, -blk_len,
>>                          "can't get size of block backend '%s'",
>>                          blk_name(blk));
>>         return false;
>>     }
>>     if (blk_len != size) {
>>         error_setg(errp, "device requires %" PRIu64 " bytes, "
>>                    "block backend '%s' provides %" PRIu64 " bytes",
>>                    size, blk_name(blk), blk_len);
>
> Should size use HWADDR_PRIu?

Yes.

> I'm not sure if printing the BlockBackend name is a good idea because
> hopefully one day the BlockBackend will be anonymous even for the flash
> devices.

Hmm.  Tell me what else I can use to identify the troublemaker to the
user.

For the second error, I could sidestep the issue and point to frontend
instead (oh, now I have an even thornier issue).  The first and third
error are *backend* errors, only related to the frontend since the
frontend happens to use them.  I could perhaps identify them as "backend
of frontend F".  If we want to do that going forward, we need suitable
helpers, so we don't reinvent this wheel in every block frontend.

>>         return false;
>>     }
>> 
>>     /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */
>>     assert(size <= BDRV_REQUEST_MAX_BYTES);
>
> I don't think we'd ever want to read in more than 2 GB into a memory
> buffer. Before we even get close to this point, the devices should be
> reworked to be more like an actual block device and read only what is
> actually accessed.

I'll steal from that to turn the comment into a proper excuse for my
lazy assertion.  Thanks!

>>     ret = blk_pread(blk, 0, buf, size);
>>     if (ret < 0) {
>>         error_setg_errno(errp, -ret, "can't read block backend '%s'",
>>                          blk_name(blk));
>>         return false;
>>     }
>>     return true;
>> }
>
> Kevin

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Kevin Wolf]

Am 08.03.2019 um 18:03 hat Markus Armbruster geschrieben:
> >> bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
> >>                                  Error **errp)
> >> {
> >>     int64_t blk_len;
> >>     int ret;
> >> 
> >>     blk_len = blk_getlength(blk);
> >>     if (blk_len < 0) {
> >>         error_setg_errno(errp, -blk_len,
> >>                          "can't get size of block backend '%s'",
> >>                          blk_name(blk));
> >>         return false;
> >>     }
> >>     if (blk_len != size) {
> >>         error_setg(errp, "device requires %" PRIu64 " bytes, "
> >>                    "block backend '%s' provides %" PRIu64 " bytes",
> >>                    size, blk_name(blk), blk_len);
> >
> > Should size use HWADDR_PRIu?
> 
> Yes.
> 
> > I'm not sure if printing the BlockBackend name is a good idea because
> > hopefully one day the BlockBackend will be anonymous even for the flash
> > devices.
> 
> Hmm.  Tell me what else I can use to identify the troublemaker to the
> user.

My hope was that a caller of this would prefix the right context. For
example, if the device were created by -device, the error message would
be prefixed with the whole "-device driver=pflash...:" string, which
gives enough context to the user.

Machine code that instantiates the device based on -drive should
probably do something similar.

Kevin

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Philippe Mathieu-Daudé]

On 3/8/19 4:40 PM, Kevin Wolf wrote:
> Am 08.03.2019 um 15:29 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben:
>>>> Laszlo Ersek <lersek@redhat.com> writes:
>>>>> This one has got to be one of the longest bike-shedding sessions! :)
>>>>>
>>>>> I'm fine with this patch, but I could suggest two improvements.
>>>>>
>>>>> (1) When blk_getlength() fails, we could format the negative error code
>>>>> returned by it into the error message.
>>>>
>>>> I can do that.
>>>
>>> By using error_setg_errno(), I assume. Not throwing away error details
>>> is always good.
>>>
>>>>> (2) We could extract the common code to a new function in
>>>>> "hw/block/block.c". (It says "Common code for block device models" on
>>>>> the tin.)
>>>>
>>>> There's so much common code in these two files even before this patch...
>>>
>>> My understanding is that hw/block/block.c contains code that is
>>> potentially useful to all kinds of block devices, not random code that
>>> two specific similar devices happen to share.
>>>
>>> If we want to deduplicate some code in the flash devices, without any
>>> expectation that other devices will use it at some point, I'd rather
>>> create a new source file hw/block/pflash_common.c or something like
>>> that.
>>
>> Yes.
>>
>> The helper I came up with (appended) isn't really specific to flash
>> devices.  Would it be okay for hw/block/block.c even though only the two
>> flash devices use it for now?
> 
> Hm, it feels more like a helper for devices that can't decide whether
> they want to be a block device or not. Or that actually don't want to be
> a block device, but use a BlockBackend anyway. Reading in the whole
> image isn't something that a normal block device would do.
> 
> But yes, it doesn't have flash-specific knowledge, even though I hope
> that it's functionality that will remain very specific to these two
> devices.
> 
> So it's your call, I don't have a strong opinion either way.
> 
>>
>> bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>>                                  Error **errp)
>> {
>>     int64_t blk_len;
>>     int ret;
>>
>>     blk_len = blk_getlength(blk);
>>     if (blk_len < 0) {
>>         error_setg_errno(errp, -blk_len,
>>                          "can't get size of block backend '%s'",
>>                          blk_name(blk));
>>         return false;
>>     }
>>     if (blk_len != size) {
>>         error_setg(errp, "device requires %" PRIu64 " bytes, "
>>                    "block backend '%s' provides %" PRIu64 " bytes",
>>                    size, blk_name(blk), blk_len);
> 
> Should size use HWADDR_PRIu?
> 
> I'm not sure if printing the BlockBackend name is a good idea because
> hopefully one day the BlockBackend will be anonymous even for the flash
> devices.
> 
>>         return false;
>>     }
>>
>>     /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */
>>     assert(size <= BDRV_REQUEST_MAX_BYTES);
> 
> I don't think we'd ever want to read in more than 2 GB into a memory
> buffer. Before we even get close to this point, the devices should be
> reworked to be more like an actual block device and read only what is
> actually accessed.

The biggest NOR available in the market is 256 MiB (bigger size is
barely ChipSelect MMIO-addressable).

Maybe you can use:

#define NOR_FLASH_MAX_BYTES (256 * MiB)

and refuse bigger flashes.

We could also check what is the widest chip-select range addressable
by all the supported architectures. I don't think it's worth it.

> 
>>     ret = blk_pread(blk, 0, buf, size);

OK, this function is named blk_check_size_and_read_all. Here we
read_all. Refactoring this device we should be able to read at
most sizeof(the biggest sector).

But this implies some serious work.

>>     if (ret < 0) {
>>         error_setg_errno(errp, -ret, "can't read block backend '%s'",
>>                          blk_name(blk));
>>         return false;
>>     }
>>     return true;
>> }
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 3/8/19 4:40 PM, Kevin Wolf wrote:
>> Am 08.03.2019 um 15:29 hat Markus Armbruster geschrieben:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>>> Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben:
>>>>> Laszlo Ersek <lersek@redhat.com> writes:
>>>>>> This one has got to be one of the longest bike-shedding sessions! :)
>>>>>>
>>>>>> I'm fine with this patch, but I could suggest two improvements.
>>>>>>
>>>>>> (1) When blk_getlength() fails, we could format the negative error code
>>>>>> returned by it into the error message.
>>>>>
>>>>> I can do that.
>>>>
>>>> By using error_setg_errno(), I assume. Not throwing away error details
>>>> is always good.
>>>>
>>>>>> (2) We could extract the common code to a new function in
>>>>>> "hw/block/block.c". (It says "Common code for block device models" on
>>>>>> the tin.)
>>>>>
>>>>> There's so much common code in these two files even before this patch...
>>>>
>>>> My understanding is that hw/block/block.c contains code that is
>>>> potentially useful to all kinds of block devices, not random code that
>>>> two specific similar devices happen to share.
>>>>
>>>> If we want to deduplicate some code in the flash devices, without any
>>>> expectation that other devices will use it at some point, I'd rather
>>>> create a new source file hw/block/pflash_common.c or something like
>>>> that.
>>>
>>> Yes.
>>>
>>> The helper I came up with (appended) isn't really specific to flash
>>> devices.  Would it be okay for hw/block/block.c even though only the two
>>> flash devices use it for now?
>> 
>> Hm, it feels more like a helper for devices that can't decide whether
>> they want to be a block device or not. Or that actually don't want to be
>> a block device, but use a BlockBackend anyway. Reading in the whole
>> image isn't something that a normal block device would do.
>> 
>> But yes, it doesn't have flash-specific knowledge, even though I hope
>> that it's functionality that will remain very specific to these two
>> devices.
>> 
>> So it's your call, I don't have a strong opinion either way.
>> 
>>>
>>> bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>>>                                  Error **errp)
>>> {
>>>     int64_t blk_len;
>>>     int ret;
>>>
>>>     blk_len = blk_getlength(blk);
>>>     if (blk_len < 0) {
>>>         error_setg_errno(errp, -blk_len,
>>>                          "can't get size of block backend '%s'",
>>>                          blk_name(blk));
>>>         return false;
>>>     }
>>>     if (blk_len != size) {
>>>         error_setg(errp, "device requires %" PRIu64 " bytes, "
>>>                    "block backend '%s' provides %" PRIu64 " bytes",
>>>                    size, blk_name(blk), blk_len);
>> 
>> Should size use HWADDR_PRIu?
>> 
>> I'm not sure if printing the BlockBackend name is a good idea because
>> hopefully one day the BlockBackend will be anonymous even for the flash
>> devices.
>> 
>>>         return false;
>>>     }
>>>
>>>     /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */
>>>     assert(size <= BDRV_REQUEST_MAX_BYTES);
>> 
>> I don't think we'd ever want to read in more than 2 GB into a memory
>> buffer. Before we even get close to this point, the devices should be
>> reworked to be more like an actual block device and read only what is
>> actually accessed.
>
> The biggest NOR available in the market is 256 MiB (bigger size is
> barely ChipSelect MMIO-addressable).
>
> Maybe you can use:
>
> #define NOR_FLASH_MAX_BYTES (256 * MiB)
>
> and refuse bigger flashes.

The comment next to the definition of property "width" in pflash_cfi01.c
suggests the device model can emulate a bunch of flash chips wired
together:

    /* width here is the overall width of this QEMU device in bytes.
     * The QEMU device may be emulating a number of flash devices
     * wired up in parallel; the width of each individual flash
     * device should be specified via device-width. If the individual
     * devices have a maximum width which is greater than the width
     * they are being used for, this maximum width should be set via
     * max-device-width (which otherwise defaults to device-width).
     * So for instance a 32-bit wide QEMU flash device made from four
     * 16-bit flash devices used in 8-bit wide mode would be configured
     * with width = 4, device-width = 1, max-device-width = 2.
     *
     * If device-width is not specified we default to backwards
     * compatible behaviour which is a bad emulation of two
     * 16 bit devices making up a 32 bit wide QEMU device. This
     * is deprecated for new uses of this device.
     */

> We could also check what is the widest chip-select range addressable
> by all the supported architectures. I don't think it's worth it.
>
>> 
>>>     ret = blk_pread(blk, 0, buf, size);
>
> OK, this function is named blk_check_size_and_read_all. Here we
> read_all. Refactoring this device we should be able to read at
> most sizeof(the biggest sector).
>
> But this implies some serious work.

Here's another thing to consider: shadowing in RAM.  Attractive when you
got the RAM, and it's faster than (parallel) flash.  If you shadow
anyway, you can just as well use serial flash, and throw in compression.
Now, emulated pflash can execute code just as fast es emulated RAM.  The
question is what kind of hardware the firmware expects.  Even if it
supports a variety of hardware, it may still have preferences.

>>>     if (ret < 0) {
>>>         error_setg_errno(errp, -ret, "can't read block backend '%s'",
>>>                          blk_name(blk));
>>>         return false;
>>>     }
>>>     return true;
>>> }
>> 
>> Kevin
>>