[Qemu-devel] [RFC PATCH v6 0/4] hw/block: better reporting on pflash backing file mismatch

[Qemu-devel] [RFC PATCH v6 0/4] hw/block: better reporting on pflash backing file mismatch

[Markus Armbruster]

This is Alex's "[PATCH v5] hw/block: better reporting on pflash
backing file mismatch" with the padding split off into its own patch,
and both resulting patches applied to pflash_cfi02.c.  I downgraded it
to RFC for two reasons:

1. I think the padding patches should not be applied.  I'll reply to
PATCH 2 to explain why.

2. I left the update of pflash_cfi02.c as separate fixup patches, so
we can easily ignore them while we discuss 1.

v6:
* Split padding into its own patch.
* Delete "It should be padded to a multiple of the flash block size."
  comment.  Happy to put it back if you can enlighten me about its
  purpose.
* Improve both commit messages.
* New fixup patches to pflash_cfi02.c.

Alex Bennée (2):
  hw/block: better reporting on pflash backing file mismatch
  hw/block: Pad undersized read-only images with 0xFF

Markus Armbruster (2):
  fixup! hw/block: better reporting on pflash backing file mismatch
  fixup! hw/block: Pad undersized read-only images with 0xFF

 hw/block/pflash_cfi01.c | 41 ++++++++++++++++++++++++++++++++++-------
 hw/block/pflash_cfi02.c | 40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 68 insertions(+), 13 deletions(-)

-- 
2.17.2

[Qemu-devel] [RFC PATCH v6 1/4] hw/block: better reporting on pflash backing file mismatch

[Markus Armbruster]

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

It looks like there was going to be code to check we had some sort of
alignment so let's replace it with an actual check.  Reject undersized
images with "device needs N bytes, backing file provides only M
bytes".  This is a bit more useful than the enigmatic "failed to read
the initial flash content" when we attempt to read the number of bytes
the device should have.  Continue to accept oversized images, but warn
"device needs N bytes, rest ignored".

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/block/pflash_cfi01.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 9d1c356eb6..75ce8ef489 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -45,6 +45,7 @@
 #include "qemu/bitops.h"
 #include "qemu/host-utils.h"
 #include "qemu/log.h"
+#include "qemu/error-report.h"
 #include "hw/sysbus.h"
 #include "sysemu/sysemu.h"
 #include "trace.h"
@@ -730,13 +731,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,6 +757,27 @@ 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, "unable to check size of backing file");
+            return;
+        }
+
+        if (backing_len < total_len) {
+            error_setg(errp, "device needs %" PRIu64 " bytes, "
+                       "backing file provides only %" PRIu64 " bytes",
+                       total_len, backing_len);
+            return;
+        } else if (backing_len > total_len) {
+            warn_report("device needs %" PRIu64 " bytes, rest ignored",
+                        total_len);
+        }
+
         /* read the initial flash content */
         ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
 
-- 
2.17.2

Re: [Qemu-devel] [RFC PATCH v6 1/4] hw/block: better reporting on pflash backing file mismatch

[Alex Bennée]

Markus Armbruster <armbru@redhat.com> writes:

> From: Alex Bennée <alex.bennee@linaro.org>
>
> It looks like there was going to be code to check we had some sort of
> alignment so let's replace it with an actual check.  Reject undersized
> images with "device needs N bytes, backing file provides only M
> bytes".  This is a bit more useful than the enigmatic "failed to read
> the initial flash content" when we attempt to read the number of bytes
> the device should have.  Continue to accept oversized images, but warn
> "device needs N bytes, rest ignored".
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 9d1c356eb6..75ce8ef489 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -45,6 +45,7 @@
>  #include "qemu/bitops.h"
>  #include "qemu/host-utils.h"
>  #include "qemu/log.h"
> +#include "qemu/error-report.h"
>  #include "hw/sysbus.h"
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
> @@ -730,13 +731,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,6 +757,27 @@ 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, "unable to check size of backing file");
> +            return;
> +        }
> +
> +        if (backing_len < total_len) {
> +            error_setg(errp, "device needs %" PRIu64 " bytes, "
> +                       "backing file provides only %" PRIu64 " bytes",
> +                       total_len, backing_len);
> +            return;
> +        } else if (backing_len > total_len) {
> +            warn_report("device needs %" PRIu64 " bytes, rest ignored",
> +                        total_len);
> +        }

As discussed elsewhere I'm happy for this just to error out with a
useful error message.

> +
>          /* read the initial flash content */
>          ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);


--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH v6 1/4] hw/block: better reporting on pflash backing file mismatch

[Markus Armbruster]

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

> Markus Armbruster <armbru@redhat.com> writes:
>
>> From: Alex Bennée <alex.bennee@linaro.org>
>>
>> It looks like there was going to be code to check we had some sort of
>> alignment so let's replace it with an actual check.  Reject undersized
>> images with "device needs N bytes, backing file provides only M
>> bytes".  This is a bit more useful than the enigmatic "failed to read
>> the initial flash content" when we attempt to read the number of bytes
>> the device should have.  Continue to accept oversized images, but warn
>> "device needs N bytes, rest ignored".
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/block/pflash_cfi01.c | 29 ++++++++++++++++++++++-------
>>  1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 9d1c356eb6..75ce8ef489 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -45,6 +45,7 @@
>>  #include "qemu/bitops.h"
>>  #include "qemu/host-utils.h"
>>  #include "qemu/log.h"
>> +#include "qemu/error-report.h"
>>  #include "hw/sysbus.h"
>>  #include "sysemu/sysemu.h"
>>  #include "trace.h"
>> @@ -730,13 +731,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,6 +757,27 @@ 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, "unable to check size of backing file");
>> +            return;
>> +        }
>> +
>> +        if (backing_len < total_len) {
>> +            error_setg(errp, "device needs %" PRIu64 " bytes, "
>> +                       "backing file provides only %" PRIu64 " bytes",
>> +                       total_len, backing_len);
>> +            return;
>> +        } else if (backing_len > total_len) {
>> +            warn_report("device needs %" PRIu64 " bytes, rest ignored",
>> +                        total_len);
>> +        }
>
> As discussed elsewhere I'm happy for this just to error out with a
> useful error message.

I'll do that in the next spin.  Thanks!

>> +
>>          /* read the initial flash content */
>>          ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
>
>
> --
> Alex Bennée

[Qemu-devel] [RFC PATCH v6 2/4] hw/block: Pad undersized read-only images with 0xFF

[Markus Armbruster]

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

We reject undersized images.  As of the previous commit, even with a
decent error message.  Still, this is a potentially confusing
stumbling block when you move from using -bios to using -drive
if=pflash,file=blob,format=raw,readonly for loading your firmware
code. To mitigate that we automatically pad in the read-only case and
warn the user when we have performed magic to enable things to Just
Work (tm).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/block/pflash_cfi01.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 75ce8ef489..00980316dc 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -759,8 +759,9 @@ 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.
+         * devices. If the device is read-only we can elide the check
+         * and just pad the region first. If the user supplies a
+         * larger file we ignore the tail.
          */
         int64_t backing_len = blk_getlength(pfl->blk);
         if (backing_len < 0) {
@@ -769,10 +770,21 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         }
 
         if (backing_len < total_len) {
-            error_setg(errp, "device needs %" PRIu64 " bytes, "
-                       "backing file provides only %" PRIu64 " bytes",
-                       total_len, backing_len);
-            return;
+            if (pfl->ro) {
+                size_t pad_bytes = total_len - backing_len;
+                /* pad with NOR erase pattern */
+                memset((uint8_t *)pfl->storage + backing_len,
+                       0xff, pad_bytes);
+                warn_report("device needs %" PRIu64
+                            " bytes, padded with %zu 0xff bytes",
+                            total_len, pad_bytes);
+                total_len = backing_len;
+            } else {
+                error_setg(errp, "device needs %" PRIu64 " bytes, "
+                           "backing file provides only %" PRIu64 " bytes",
+                           total_len, backing_len);
+                return;
+            }
         } else if (backing_len > total_len) {
             warn_report("device needs %" PRIu64 " bytes, rest ignored",
                         total_len);
-- 
2.17.2

Re: [Qemu-devel] [RFC PATCH v6 2/4] hw/block: Pad undersized read-only images with 0xFF

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> From: Alex Bennée <alex.bennee@linaro.org>
>
> We reject undersized images.  As of the previous commit, even with a
> decent error message.  Still, this is a potentially confusing
> stumbling block when you move from using -bios to using -drive
> if=pflash,file=blob,format=raw,readonly for loading your firmware
> code. To mitigate that we automatically pad in the read-only case and
> warn the user when we have performed magic to enable things to Just
> Work (tm).
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

I think this convenience feature is a bad idea, and this patch should
not be applied.  Here are my reasons.

1. As I explained in my disccussion of v5[*], there is no single true
   way to pad.  This patch pads with 0xFF.  When working with physical
   devices, you'd sometimes pad that way, but at other times, you'd pad
   differently.

2. Except this patch doesn't *actually* pad with 0xFF.  The block layer
   silently pads with zeros up to the next multiple of 512.  Evidence:

    $ yes | dd of=4090b.img bs=1 count=4090
    4090+0 records in
    4090+0 records out
    4090 bytes (4.1 kB, 4.0 KiB) copied, 0.0186459 s, 219 kB/s
    $ qemu-io -f raw -c 'read -v 4000 96' 4090b.img
    00000fa0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
    00000fb0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
    00000fc0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
    00000fd0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
    00000fe0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
    00000ff0:  79 0a 79 0a 79 0a 79 0a 79 0a 00 00 00 00 00 00  y.y.y.y.y.......
    read 96/96 bytes at offset 4000
    96 bytes, 1 ops; 0.0001 sec (694.444 KiB/sec and 7407.4074 ops/sec)

   This patch then pads some more with 0xFF to the flash memory size.

   Because of that the way the magic padding works makes no sense, to be
   frank.  Going back to v3's zero padding would at least be explainable
   without blushing.

   I consider the block layer's padding a misfeature here, but right now
   we got to play the hand we've been dealt.

3. Convenience magic has bitten us in the posterior so many times.  Just
   say no unless there's a really compelling use case.  Where's the use
   case for this one?  We've rejected undersized images for ages, and
   nobody complained.  Why add convenience magic now?


[*] Message-ID: <87r2bl5oah.fsf@dusky.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2019-03/msg01115.html

Re: [Qemu-devel] [RFC PATCH v6 2/4] hw/block: Pad undersized read-only images with 0xFF

[Kevin Wolf]

Am 07.03.2019 um 11:05 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > From: Alex Bennée <alex.bennee@linaro.org>
> >
> > We reject undersized images.  As of the previous commit, even with a
> > decent error message.  Still, this is a potentially confusing
> > stumbling block when you move from using -bios to using -drive
> > if=pflash,file=blob,format=raw,readonly for loading your firmware
> > code. To mitigate that we automatically pad in the read-only case and
> > warn the user when we have performed magic to enable things to Just
> > Work (tm).
> >
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> I think this convenience feature is a bad idea, and this patch should
> not be applied.  Here are my reasons.
> 
> 1. As I explained in my disccussion of v5[*], there is no single true
>    way to pad.  This patch pads with 0xFF.  When working with physical
>    devices, you'd sometimes pad that way, but at other times, you'd pad
>    differently.
> 
> 2. Except this patch doesn't *actually* pad with 0xFF.  The block layer
>    silently pads with zeros up to the next multiple of 512.  Evidence:
> 
>     $ yes | dd of=4090b.img bs=1 count=4090
>     4090+0 records in
>     4090+0 records out
>     4090 bytes (4.1 kB, 4.0 KiB) copied, 0.0186459 s, 219 kB/s
>     $ qemu-io -f raw -c 'read -v 4000 96' 4090b.img
>     00000fa0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fb0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fc0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fd0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fe0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000ff0:  79 0a 79 0a 79 0a 79 0a 79 0a 00 00 00 00 00 00  y.y.y.y.y.......
>     read 96/96 bytes at offset 4000
>     96 bytes, 1 ops; 0.0001 sec (694.444 KiB/sec and 7407.4074 ops/sec)
> 
>    This patch then pads some more with 0xFF to the flash memory size.
> 
>    Because of that the way the magic padding works makes no sense, to be
>    frank.  Going back to v3's zero padding would at least be explainable
>    without blushing.
> 
>    I consider the block layer's padding a misfeature here, but right now
>    we got to play the hand we've been dealt.

That will be solved as soon as the block layer is consistently converted
to byte granularity. We've already converted a lot, but bdrv_getlength()
is still sector (512 bytes) granularity.

You could argue that file-posix should just reject files that are not
sector aligned, but that's probably not right either because image
formats don't necessarily have that alignment for their files.

Maybe disk device should reject being attached to nodes that aren't a
multiple of their physical and logical sector size. A 512-byte aligned
image is probably suitable for most disks, but might not be for a virtual
native 4k disk.

> 3. Convenience magic has bitten us in the posterior so many times.  Just
>    say no unless there's a really compelling use case.  Where's the use
>    case for this one?  We've rejected undersized images for ages, and
>    nobody complained.  Why add convenience magic now?

I agree.

Kevin

Re: [Qemu-devel] [RFC PATCH v6 2/4] hw/block: Pad undersized read-only images with 0xFF

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2353 bytes --]

On 3/7/19 8:55 AM, Kevin Wolf wrote:

>>     00000ff0:  79 0a 79 0a 79 0a 79 0a 79 0a 00 00 00 00 00 00  y.y.y.y.y.......
>>     read 96/96 bytes at offset 4000
>>     96 bytes, 1 ops; 0.0001 sec (694.444 KiB/sec and 7407.4074 ops/sec)
>>
>>    This patch then pads some more with 0xFF to the flash memory size.
>>
>>    Because of that the way the magic padding works makes no sense, to be
>>    frank.  Going back to v3's zero padding would at least be explainable
>>    without blushing.
>>
>>    I consider the block layer's padding a misfeature here, but right now
>>    we got to play the hand we've been dealt.
> 
> That will be solved as soon as the block layer is consistently converted
> to byte granularity. We've already converted a lot, but bdrv_getlength()
> is still sector (512 bytes) granularity.
> 
> You could argue that file-posix should just reject files that are not
> sector aligned, but that's probably not right either because image
> formats don't necessarily have that alignment for their files.
> 
> Maybe disk device should reject being attached to nodes that aren't a
> multiple of their physical and logical sector size. A 512-byte aligned
> image is probably suitable for most disks, but might not be for a virtual
> native 4k disk.

nbdkit has a filter that allows padding any non-sector-aligned image out
to the next sector alignment, where reads from the tail see zero, writes
_of zero_ to the tail succeed, but writes of non-zero fail (I don't know
if it prefers EIO or ENOSPC, but that's secondary).  I have an open bug
against the nbd block driver where we can trigger assertions on
unaligned file sizes; and it would definitely be nice to fix it globally
in the block layer rather than copy-and-paste piecemeal in every
affected driver.

I may still fix NBD for 4.0 during soft freeze to avoid the assert, but
that fix will be a bare-bones bandaid (enough to avoid the assertion
failures, and no more).  But yes, I definitely agree that a 4.1 project
of making bdrv_getlength() be byte-aware coupled with block-layer smarts
about handling the tail when widening from a smaller alignment to a
larger is going to make a lot of things nicer.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC PATCH v6 2/4] hw/block: Pad undersized read-only images with 0xFF

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 07.03.2019 um 11:05 hat Markus Armbruster geschrieben:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > From: Alex Bennée <alex.bennee@linaro.org>
>> >
>> > We reject undersized images.  As of the previous commit, even with a
>> > decent error message.  Still, this is a potentially confusing
>> > stumbling block when you move from using -bios to using -drive
>> > if=pflash,file=blob,format=raw,readonly for loading your firmware
>> > code. To mitigate that we automatically pad in the read-only case and
>> > warn the user when we have performed magic to enable things to Just
>> > Work (tm).
>> >
>> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> 
>> I think this convenience feature is a bad idea, and this patch should
>> not be applied.  Here are my reasons.
>> 
>> 1. As I explained in my disccussion of v5[*], there is no single true
>>    way to pad.  This patch pads with 0xFF.  When working with physical
>>    devices, you'd sometimes pad that way, but at other times, you'd pad
>>    differently.
>> 
>> 2. Except this patch doesn't *actually* pad with 0xFF.  The block layer
>>    silently pads with zeros up to the next multiple of 512.  Evidence:
>> 
>>     $ yes | dd of=4090b.img bs=1 count=4090
>>     4090+0 records in
>>     4090+0 records out
>>     4090 bytes (4.1 kB, 4.0 KiB) copied, 0.0186459 s, 219 kB/s
>>     $ qemu-io -f raw -c 'read -v 4000 96' 4090b.img
>>     00000fa0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>>     00000fb0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>>     00000fc0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>>     00000fd0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>>     00000fe0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>>     00000ff0:  79 0a 79 0a 79 0a 79 0a 79 0a 00 00 00 00 00 00  y.y.y.y.y.......
>>     read 96/96 bytes at offset 4000
>>     96 bytes, 1 ops; 0.0001 sec (694.444 KiB/sec and 7407.4074 ops/sec)

By the way, when you write to the padding, the block layer grows the
file to the next multiple of 512.

>> 
>>    This patch then pads some more with 0xFF to the flash memory size.
>> 
>>    Because of that the way the magic padding works makes no sense, to be
>>    frank.  Going back to v3's zero padding would at least be explainable
>>    without blushing.
>> 
>>    I consider the block layer's padding a misfeature here, but right now
>>    we got to play the hand we've been dealt.
>
> That will be solved as soon as the block layer is consistently converted
> to byte granularity. We've already converted a lot, but bdrv_getlength()
> is still sector (512 bytes) granularity.
>
> You could argue that file-posix should just reject files that are not
> sector aligned, but that's probably not right either because image
> formats don't necessarily have that alignment for their files.

Yes, it could well turn out to be overly restrictive.

> Maybe disk device should reject being attached to nodes that aren't a
> multiple of their physical and logical sector size. A 512-byte aligned
> image is probably suitable for most disks, but might not be for a virtual
> native 4k disk.

Makes sense to me.

Could perhaps be done in or near blkconf_blocksizes().

>> 3. Convenience magic has bitten us in the posterior so many times.  Just
>>    say no unless there's a really compelling use case.  Where's the use
>>    case for this one?  We've rejected undersized images for ages, and
>>    nobody complained.  Why add convenience magic now?
>
> I agree.

Okay.  Let's take just the error reporting improvment then (PATCH 1 and
its fixup).  Since we all seem to agree on upgrading the warning to an
error, do that.  Don't take the convenience feature now (PATCH 2 and its
fixup).  We can always revisit it later.

This should give us a good chance at getting it done in time for the
soft freeze.

I'll prepare v7.

Thanks!

Re: [Qemu-devel] [RFC PATCH v6 2/4] hw/block: Pad undersized read-only images with 0xFF

[Laszlo Ersek]

On 03/07/19 11:05, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> From: Alex Bennée <alex.bennee@linaro.org>
>>
>> We reject undersized images.  As of the previous commit, even with a
>> decent error message.  Still, this is a potentially confusing
>> stumbling block when you move from using -bios to using -drive
>> if=pflash,file=blob,format=raw,readonly for loading your firmware
>> code. To mitigate that we automatically pad in the read-only case and
>> warn the user when we have performed magic to enable things to Just
>> Work (tm).
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> I think this convenience feature is a bad idea, and this patch should
> not be applied.

I'm fine with that.

Thanks,
Laszlo


>  Here are my reasons.
> 
> 1. As I explained in my disccussion of v5[*], there is no single true
>    way to pad.  This patch pads with 0xFF.  When working with physical
>    devices, you'd sometimes pad that way, but at other times, you'd pad
>    differently.
> 
> 2. Except this patch doesn't *actually* pad with 0xFF.  The block layer
>    silently pads with zeros up to the next multiple of 512.  Evidence:
> 
>     $ yes | dd of=4090b.img bs=1 count=4090
>     4090+0 records in
>     4090+0 records out
>     4090 bytes (4.1 kB, 4.0 KiB) copied, 0.0186459 s, 219 kB/s
>     $ qemu-io -f raw -c 'read -v 4000 96' 4090b.img
>     00000fa0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fb0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fc0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fd0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000fe0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  y.y.y.y.y.y.y.y.
>     00000ff0:  79 0a 79 0a 79 0a 79 0a 79 0a 00 00 00 00 00 00  y.y.y.y.y.......
>     read 96/96 bytes at offset 4000
>     96 bytes, 1 ops; 0.0001 sec (694.444 KiB/sec and 7407.4074 ops/sec)
> 
>    This patch then pads some more with 0xFF to the flash memory size.
> 
>    Because of that the way the magic padding works makes no sense, to be
>    frank.  Going back to v3's zero padding would at least be explainable
>    without blushing.
> 
>    I consider the block layer's padding a misfeature here, but right now
>    we got to play the hand we've been dealt.
> 
> 3. Convenience magic has bitten us in the posterior so many times.  Just
>    say no unless there's a really compelling use case.  Where's the use
>    case for this one?  We've rejected undersized images for ages, and
>    nobody complained.  Why add convenience magic now?
> 
> 
> [*] Message-ID: <87r2bl5oah.fsf@dusky.pond.sub.org>
> https://lists.nongnu.org/archive/html/qemu-devel/2019-03/msg01115.html
> 

[Qemu-devel] [RFC PATCH v6 3/4] fixup! hw/block: better reporting on pflash backing file mismatch

[Markus Armbruster]

---
 hw/block/pflash_cfi02.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 33779ce807..d30a351472 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -39,6 +39,7 @@
 #include "hw/hw.h"
 #include "hw/block/flash.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu/timer.h"
 #include "sysemu/block-backend.h"
 #include "qemu/host-utils.h"
@@ -550,12 +551,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,6 +576,27 @@ 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, "unable to check size of backing file");
+            return;
+        }
+
+        if (backing_len < chip_len) {
+            error_setg(errp, "device needs %" PRIu32 " bytes, "
+                       "backing file provides only %" PRIu64 " bytes",
+                       chip_len, backing_len);
+            return;
+        } else if (backing_len > chip_len) {
+            warn_report("device needs %" PRIu32 " bytes, rest ignored",
+                        chip_len);
+        }
+
         /* read the initial flash content */
         ret = blk_pread(pfl->blk, 0, pfl->storage, chip_len);
         if (ret < 0) {
-- 
2.17.2

[Qemu-devel] [RFC PATCH v6 4/4] fixup! hw/block: Pad undersized read-only images with 0xFF

[Markus Armbruster]

---
 hw/block/pflash_cfi02.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index d30a351472..db1c39499c 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -578,8 +578,9 @@ 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.
+         * devices. If the device is read-only we can elide the check
+         * and just pad the region first. If the user supplies a
+         * larger file we ignore the tail.
          */
         int64_t backing_len = blk_getlength(pfl->blk);
         if (backing_len < 0) {
@@ -588,9 +589,20 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
         }
 
         if (backing_len < chip_len) {
-            error_setg(errp, "device needs %" PRIu32 " bytes, "
-                       "backing file provides only %" PRIu64 " bytes",
-                       chip_len, backing_len);
+            if (pfl->ro) {
+                size_t pad_bytes = chip_len - backing_len;
+                /* pad with NOR erase pattern */
+                memset((uint8_t *)pfl->storage + backing_len,
+                       0xff, pad_bytes);
+                warn_report("device needs %" PRIu32
+                            " bytes, padded with %zu 0xff bytes",
+                            chip_len, pad_bytes);
+                chip_len = backing_len;
+            } else {
+                error_setg(errp, "device needs %" PRIu32 " bytes, "
+                           "backing file provides only %" PRIu64 " bytes",
+                           chip_len, backing_len);
+            }
             return;
         } else if (backing_len > chip_len) {
             warn_report("device needs %" PRIu32 " bytes, rest ignored",
-- 
2.17.2

Re: [Qemu-devel] [RFC PATCH v6 0/4] hw/block: better reporting on pflash backing file mismatch

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190307093723.655-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20190307093723.655-1-armbru@redhat.com
Subject: [Qemu-devel] [RFC PATCH v6 0/4] hw/block: better reporting on pflash backing file mismatch

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190307093723.655-1-armbru@redhat.com -> patchew/20190307093723.655-1-armbru@redhat.com
Switched to a new branch 'test'
48c4518e34 fixup! hw/block: Pad undersized read-only images with 0xFF
b542c515b5 fixup! hw/block: better reporting on pflash backing file mismatch
936dbecaad hw/block: Pad undersized read-only images with 0xFF
0a12006a6e hw/block: better reporting on pflash backing file mismatch

=== OUTPUT BEGIN ===
1/4 Checking commit 0a12006a6e45 (hw/block: better reporting on pflash backing file mismatch)
2/4 Checking commit 936dbecaade4 (hw/block: Pad undersized read-only images with 0xFF)
3/4 Checking commit b542c515b53b (fixup! hw/block: better reporting on pflash backing file mismatch)
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 46 lines checked

Patch 3/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/4 Checking commit 48c4518e348b (fixup! hw/block: Pad undersized read-only images with 0xFF)
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 34 lines checked

Patch 4/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190307093723.655-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com