[Qemu-devel] [PATCH v2] hw/block: report when pflash backing file isn't aligned

[Qemu-devel] [PATCH v2] hw/block: report when pflash backing file isn't aligned

[Alex Bennée]

It looks like there was going to be code to check we had some sort of
alignment so lets replace it with an actual check. 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.

This is a potential confusing stumbling block when you move from using
-bios to using -drive if=pflash,file=blob,format=raw,readonly for
loading your firmware code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use PRIu64 instead of PRId64
  - tweaked message output
---
 hw/block/pflash_cfi01.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index bffb4c40e7..7532c8d8e8 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -722,12 +722,20 @@ 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
+    /*
+     * Validate the backing store is the right size for pflash
+     * devices. It has to be padded to a multiple of the flash block
+     * size.
+     */
+    if (pfl->blk) {
+        uint64_t backing_len = blk_getlength(pfl->blk);
+        if (device_len != backing_len) {
+            error_setg(errp, "device needs %" PRIu64 " bytes, "
+                       "backing file provides only %" PRIu64 " bytes",
+                       device_len, backing_len);
+            return;
+        }
+    }
 
     memory_region_init_rom_device(
         &pfl->mem, OBJECT(dev),
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2] hw/block: report when pflash backing file isn't aligned

[Laszlo Ersek]

On 02/15/19 13:28, Alex Bennée wrote:
> It looks like there was going to be code to check we had some sort of
> alignment so lets replace it with an actual check. 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.
> 
> This is a potential confusing stumbling block when you move from using
> -bios to using -drive if=pflash,file=blob,format=raw,readonly for
> loading your firmware code.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>   - use PRIu64 instead of PRId64
>   - tweaked message output
> ---
>  hw/block/pflash_cfi01.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index bffb4c40e7..7532c8d8e8 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -722,12 +722,20 @@ 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
> +    /*
> +     * Validate the backing store is the right size for pflash
> +     * devices. It has to be padded to a multiple of the flash block
> +     * size.
> +     */
> +    if (pfl->blk) {
> +        uint64_t backing_len = blk_getlength(pfl->blk);
> +        if (device_len != backing_len) {
> +            error_setg(errp, "device needs %" PRIu64 " bytes, "
> +                       "backing file provides only %" PRIu64 " bytes",
> +                       device_len, backing_len);
> +            return;
> +        }
> +    }
>  
>      memory_region_init_rom_device(
>          &pfl->mem, OBJECT(dev),
> 

The word "only" implies that the file is too small. It could be too
large as well (the C expression is right, but the message doesn't
reflect it).

With the word "only" dropped, I think the message looks fine.


Also, now I've checked blk_getlength(). First, it can directly return
(-ENOMEDIUM). Second, it delegates the job to bdrv_getlength(), which
itself can return (-EFBIG). Third, bdrv_nb_sectors(), used internally,
can itself return (-ENOMEDIUM).

For me this is pretty much impossible to follow. Can we:

- use type "int64_t" for "backing_len" in the new code, AND

  - either prove (from the rest of pflash_cfi01_realize()) that
    "backing_len" is nonnegative, and then *assert* it, plus cast
    "backing_len" to uint64_t for the comparison;

  - or check for a negative "backing_len" explicitly, and if that
    happens, fail pflash_cfi01_realize() with an error message that
    reports *that* failure?

Sorry about the pedantry; I've got no clue what's happening in
blk_getlength() for real.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v2] hw/block: report when pflash backing file isn't aligned

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 02/15/19 13:28, Alex Bennée wrote:
>> It looks like there was going to be code to check we had some sort of
>> alignment so lets replace it with an actual check. 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.
>> 
>> This is a potential confusing stumbling block when you move from using
>> -bios to using -drive if=pflash,file=blob,format=raw,readonly for
>> loading your firmware code.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> ---
>> v2
>>   - use PRIu64 instead of PRId64
>>   - tweaked message output
>> ---
>>  hw/block/pflash_cfi01.c | 20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>> 
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index bffb4c40e7..7532c8d8e8 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -722,12 +722,20 @@ 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

pflash_cfi02_realize() has the same XXX.

There's a pair of related XXXs in taihu_405ep_init().  Related because
@total_len is computed like this

    total_len = pfl->sector_len * pfl->nb_blocs;

and the two factors come from callers of pflash_cfi01_realize(),
pflash_cfi02_realize(), or from ve_pflash_cfi01_register(),
xtfpga_flash_init().  Aside: the latter two are slight variations of
pflash_cfi01_realize() which I'm going to clean up.  Some of these use
fixed sizes (good, real machines do that, too).  Some of them compute
them from blk_getlength(), with varying levels of care.

I'm afraid we need to take all that into account.

Let's take a step back and consider sane requirements.

The size of the flash chip is a property of the machine.  It is *fixed*.
Using whatever size the image has is sloppy modelling.

A machine may come in minor variations that aren't worth their own
machine type.  One such variation could be a different flash chip size.
Using the image size to select one from the set of fixed sizes is
tolerable.

Aside: the image size can change between the place where we get it to
pick a flash chip size and realize().  I guess that's a "don't do that
then".

If the image size matches the flash chip's size exactly, all is well.

Should we require the size to match the flash chip's size?

If we accept a smaller image, we want to pad with zeros.  What about
writes beyond the size of the image?  Discard them, or let them extend
the image file?

If we accept a larger image, we want to ignore its tail.

Sorry for turning the tiny issue your patch tries to address into a much
larger one...

>> +    /*
>> +     * Validate the backing store is the right size for pflash
>> +     * devices. It has to be padded to a multiple of the flash block
>> +     * size.
>> +     */
>> +    if (pfl->blk) {
>> +        uint64_t backing_len = blk_getlength(pfl->blk);
>> +        if (device_len != backing_len) {
>> +            error_setg(errp, "device needs %" PRIu64 " bytes, "
>> +                       "backing file provides only %" PRIu64 " bytes",
>> +                       device_len, backing_len);
>> +            return;
>> +        }
>> +    }
>>  
>>      memory_region_init_rom_device(
>>          &pfl->mem, OBJECT(dev),
>> 
>
> The word "only" implies that the file is too small. It could be too
> large as well (the C expression is right, but the message doesn't
> reflect it).
>
> With the word "only" dropped, I think the message looks fine.
>
>
> Also, now I've checked blk_getlength(). First, it can directly return
> (-ENOMEDIUM). Second, it delegates the job to bdrv_getlength(), which
> itself can return (-EFBIG). Third, bdrv_nb_sectors(), used internally,
> can itself return (-ENOMEDIUM).
>
> For me this is pretty much impossible to follow. Can we:
>
> - use type "int64_t" for "backing_len" in the new code, AND
>
>   - either prove (from the rest of pflash_cfi01_realize()) that
>     "backing_len" is nonnegative, and then *assert* it, plus cast
>     "backing_len" to uint64_t for the comparison;
>
>   - or check for a negative "backing_len" explicitly, and if that
>     happens, fail pflash_cfi01_realize() with an error message that
>     reports *that* failure?
>
> Sorry about the pedantry; I've got no clue what's happening in
> blk_getlength() for real.

László is right, blk_getlength() *can* fail.  It doesn't fail often, so
neglecting to check for failure can go undetected for quite a while.

Re: [Qemu-devel] [PATCH v2] hw/block: report when pflash backing file isn't aligned

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Laszlo Ersek <lersek@redhat.com> writes:
>
>> On 02/15/19 13:28, Alex Bennée wrote:
>>> It looks like there was going to be code to check we had some sort of
>>> alignment so lets replace it with an actual check. 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.
>>> 
>>> This is a potential confusing stumbling block when you move from using
>>> -bios to using -drive if=pflash,file=blob,format=raw,readonly for
>>> loading your firmware code.
>>> 
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> 
>>> ---
>>> v2
>>>   - use PRIu64 instead of PRId64
>>>   - tweaked message output
>>> ---
>>>  hw/block/pflash_cfi01.c | 20 ++++++++++++++------
>>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>>> index bffb4c40e7..7532c8d8e8 100644
>>> --- a/hw/block/pflash_cfi01.c
>>> +++ b/hw/block/pflash_cfi01.c
>>> @@ -722,12 +722,20 @@ 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
>
> pflash_cfi02_realize() has the same XXX.
>
> There's a pair of related XXXs in taihu_405ep_init().  Related because
> @total_len is computed like this
>
>     total_len = pfl->sector_len * pfl->nb_blocs;
>
> and the two factors come from callers of pflash_cfi01_realize(),
> pflash_cfi02_realize(), or from ve_pflash_cfi01_register(),
> xtfpga_flash_init().  Aside: the latter two are slight variations of
> pflash_cfi01_realize() which I'm going to clean up.  Some of these use
> fixed sizes (good, real machines do that, too).  Some of them compute
> them from blk_getlength(), with varying levels of care.

Missed one: create_one_flash().

Let's review flash size computation.

Single fixed size per machine type:

    collie_init()               0x02000000 (two times)
    connex_init()               0x01000000
    verdex_init()               0x02000000
    mainstone_common_init()     0x02000000
    sx1_init()                  (16 * 1024 * 1024) for sx1-v1
                                (32 * 1024 * 1024) for sx1
                                ( 8 * 1024 * 1024) for sx1-v1
    versatile_init()            (64 * 1024 * 1024)
    z2_init()                   0x00800000
    milkymist_init()            32 * MiB
    petalogix_ml605_init()      (32 * MiB)
    petalogix_s3adsp1800_init() (16 * MiB)
    mips_malta_init()           (4 * MiB)
    mips_r4k_init()             0x00400000
    virtex_init()               (16 * MiB)
    digic4_add_k8p3215uqb_rom() (4 * 1024 * 1024)
    zynq_init()                 (64 * 1024 * 1024)
    lm32_evr_init()             32 * MiB
    lm32_uclinux_init()         32 * MiB
    taihu_405ep_init()          32 * MiB (but see below)
    r2d_init()                  0x02000000
    ve_pflash_cfi01_register()  (64 * 1024 * 1024)
    xtfpga_flash_init()         0x00400000 for lx60*
                                0x01000000 for lx200*
                                0x01000000 for ml605*
                                0x08000000 for kc705*
    create_one_flash()          0x08000000 / 2 (two times)

Set of fixed sizes:

    musicpal_init()             image size, either 8*1024*1024,
                                16*1024*1024 or 32*1024*1024

Troublemakers:

    pc_system_flash_init()      sum of image sizes, at most 8MiB
                                rejects images whose size is not a
                                multiple of 4KiB
    sam460ex_load_uboot()       image size rounded up to multiple of
                                64KiB
    ref405ep_init()             image size rounded up to multiple of
                                64KiB
    taihu_405ep_init()          image size rounded up to multiple of
                                64KiB

> I'm afraid we need to take all that into account.

We can ignore all but the four troublemakers.

[...]

Re: [Qemu-devel] [PATCH v2] hw/block: report when pflash backing file isn't aligned

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Laszlo Ersek <lersek@redhat.com> writes:
>>
>>> On 02/15/19 13:28, Alex Bennée wrote:
>>>> It looks like there was going to be code to check we had some sort of
>>>> alignment so lets replace it with an actual check. 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.
>>>> 
>>>> This is a potential confusing stumbling block when you move from using
>>>> -bios to using -drive if=pflash,file=blob,format=raw,readonly for
>>>> loading your firmware code.
>>>> 
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> 
>>>> ---
>>>> v2
>>>>   - use PRIu64 instead of PRId64
>>>>   - tweaked message output
>>>> ---
>>>>  hw/block/pflash_cfi01.c | 20 ++++++++++++++------
>>>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>>> 
>>>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>>>> index bffb4c40e7..7532c8d8e8 100644
>>>> --- a/hw/block/pflash_cfi01.c
>>>> +++ b/hw/block/pflash_cfi01.c
>>>> @@ -722,12 +722,20 @@ 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
>>
>> pflash_cfi02_realize() has the same XXX.
>>
>> There's a pair of related XXXs in taihu_405ep_init().  Related because
>> @total_len is computed like this
>>
>>     total_len = pfl->sector_len * pfl->nb_blocs;
>>
>> and the two factors come from callers of pflash_cfi01_realize(),
>> pflash_cfi02_realize(), or from ve_pflash_cfi01_register(),
>> xtfpga_flash_init().  Aside: the latter two are slight variations of
>> pflash_cfi01_realize() which I'm going to clean up.  Some of these use
>> fixed sizes (good, real machines do that, too).  Some of them compute
>> them from blk_getlength(), with varying levels of care.
>
> Missed one: create_one_flash().
>
> Let's review flash size computation.
>
> Single fixed size per machine type:
>
>     collie_init()               0x02000000 (two times)
>     connex_init()               0x01000000
>     verdex_init()               0x02000000
>     mainstone_common_init()     0x02000000
>     sx1_init()                  (16 * 1024 * 1024) for sx1-v1
>                                 (32 * 1024 * 1024) for sx1
>                                 ( 8 * 1024 * 1024) for sx1-v1
>     versatile_init()            (64 * 1024 * 1024)
>     z2_init()                   0x00800000
>     milkymist_init()            32 * MiB
>     petalogix_ml605_init()      (32 * MiB)
>     petalogix_s3adsp1800_init() (16 * MiB)
>     mips_malta_init()           (4 * MiB)
>     mips_r4k_init()             0x00400000
>     virtex_init()               (16 * MiB)
>     digic4_add_k8p3215uqb_rom() (4 * 1024 * 1024)
>     zynq_init()                 (64 * 1024 * 1024)
>     lm32_evr_init()             32 * MiB
>     lm32_uclinux_init()         32 * MiB
>     taihu_405ep_init()          32 * MiB (but see below)
>     r2d_init()                  0x02000000
>     ve_pflash_cfi01_register()  (64 * 1024 * 1024)
>     xtfpga_flash_init()         0x00400000 for lx60*
>                                 0x01000000 for lx200*
>                                 0x01000000 for ml605*
>                                 0x08000000 for kc705*
>     create_one_flash()          0x08000000 / 2 (two times)
>
> Set of fixed sizes:
>
>     musicpal_init()             image size, either 8*1024*1024,
>                                 16*1024*1024 or 32*1024*1024
>
> Troublemakers:
>
>     pc_system_flash_init()      sum of image sizes, at most 8MiB
>                                 rejects images whose size is not a
>                                 multiple of 4KiB

Image size must be one of n * 4KiB where 1 <= n <= 2048.  Can be filed
under "Set of fixed sizes"

>     sam460ex_load_uboot()       image size rounded up to multiple of
>                                 64KiB

Looks broken.

(1) The flash is mapped at address 0xFFF00000.  When no image is
    supplied, it's size is 1MiB (0x100000).  Else, it's size is the size
    of the image rounded up to the next multiple of 64KiB.

    I have no idea what happens when the size exceeds 1MiB.  Does it
    wrap around the 32 bit address space?

(2) Unless the image size is a multiple of 64KiB, pflash_cfi01_realize()
    fails with "failed to read the initial flash content".

>     ref405ep_init()             image size rounded up to multiple of
>                                 64KiB

Looks broken.

(1) The flash is mapped at address 2^32 - (image size).  Its size is the
    size of the image rounded up to the next multiple of 64KiB.

    Unless the rounding is a no-op, the flash extends beyond the end of
    the 32 bit address space.  Probably irrelevant due to (2).

    If the size exceeds 0x80000 Bytes, we overlap first SRAM, then other
    stuff.  No idea how that would play out.
    
    I suspect we should either require the image to be 512KiB, or maybe
    also accept smaller ones and pad them to 512KiB.  The latter would
    be unusual.

(2) Unless the image size is a multiple of 64KiB, pflash_cfi01_realize()
    fails with "failed to read the initial flash content".

>     taihu_405ep_init()          image size rounded up to multiple of
>                                 64KiB

Looks broken.

Boot flash (unit 0) is just like ref405ep_init(), just with different
addresses.

(3) Application flash (unit 1) has a fixed size of 32 MiB.  If the image
    is smaller, pflash_cfi01_realize() fails with "failed to read the
    initial flash content".

I think we should require the image to match the real machine's flash
size.  We could also accept smaller ones and pad them to 1MiB, or larger
ones ignoring the tail, but that would be unusual.

>> I'm afraid we need to take all that into account.
>
> We can ignore all but the four troublemakers.
>
> [...]

I'll post patches.

Re: [Qemu-devel] [PATCH v2] hw/block: report when pflash backing file isn't aligned

[Laszlo Ersek]

On 02/15/19 17:01, Markus Armbruster wrote:

> The size of the flash chip is a property of the machine.  It is *fixed*.

I'll have to disagree on this one; in OVMF's case, you can build OVMF in
1MB, 2MB, and 4MB *cumulative* size (that is, the numbers I give here
each refer to the sum of both pflash chips, namely varstore and executable).

The cumulative size that is picked at OVMF build time influences
(obviously) the amount of crap ^W firmware features that one can squeeze
into the executable image, but it also determines other things. For
example, the 4MB build has a different (incompatible!) internal
structure than the 2MB does. See the small table/comparison in the
following commit message:

  https://github.com/tianocore/edk2/commit/b24fca05751f

In order to provide some numbers that one can observe simply on their
host filesystem, the 2MB cumulative size consists of (128K varstore
chip, 1920KB executable chip); while the 4MB one comprises (528K
varstore chip, 3568KB executable chip).

> Using whatever size the image has is sloppy modelling.

Maybe so, but it's also very convenient, and also quite important, right
now (given the multiple firmware image sizes used in the wild).

> A machine may come in minor variations that aren't worth their own
> machine type.  One such variation could be a different flash chip size.
> Using the image size to select one from the set of fixed sizes is
> tolerable.

The problem is that this requires coordination between QEMU and firmware
development.

(Well, I have to agree that the present patch is *already* that kind of
coordination; my point is that when I introduced the 4MB build for OVMF,
I didn't have to touch QEMU. In retrospect, I'm extremely thankful for
that, as the introduction of the 4MB build was difficult in itself.)

"Using the image size to flexibly dictate the pflash size, in board
code" would be preferable, to "select from a pre-determined set". (A
similarly flexible approach would be if we required the user or mgmt app
to specify base & size as pflash device properties -- see your option #1
elsewhere --, but I digress.)

> Aside: the image size can change between the place where we get it to
> pick a flash chip size and realize().

Haha :)

> I guess that's a "don't do that then".

I think so! :)

> If the image size matches the flash chip's size exactly, all is well.
> 
> Should we require the size to match the flash chip's size?

So, this is hard to answer generally for all targets / boards.

pc_system_flash_init() (on i440fx/q35, i.e. when OVMF is used) will
guarantee this equality -- ignoring the TOCTOU that you point out above,
anyway.

For "virt", the answer carries a lot more weight, because *that* board
code does not size the pflash from the fw image found in the filesystem.
(See create_flash(), and a15memmap[VIRT_FLASH].)

IIUC, this patch suggests, "yes, we should require equality". A no-op
for OVMF (= pc_system_flash_init()), and helpful against user mistakes
with the ArmVirtQemu platform firmware (= create_flash()).

> If we accept a smaller image, we want to pad with zeros.  What about
> writes beyond the size of the image?  Discard them, or let them extend
> the image file?
> 
> If we accept a larger image, we want to ignore its tail.
> 
> Sorry for turning the tiny issue your patch tries to address into a much
> larger one...

I think the following would be useful:

- reject images that are larger than the pflash chip size
- pad smaller images with zero (not on the disk)
- ignore guest writes to padding

Because, in case of the ArmVirtQemu firmware at least, the build process
produces the following two files:

  2.0M QEMU_EFI.fd
  768K QEMU_VARS.fd

And we've always had to manually pad each of these to 64MB, with zeroes,
on-disk, for use with the "virt" board. If QEMU could do that
automatically (in memory), that would be a win, in my book.

If Alex thinks such padding is out of scope for now, I will certainly
not insist; I think the present patch (enforcing equality) is already a
significant usability improvement. I'd just like the error message to be
precise, and the error checking to be (more) complete.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v2] hw/block: report when pflash backing file isn't aligned

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 02/15/19 17:01, Markus Armbruster wrote:
>
>> Let's take a step back and consider sane requirements.
>> 
>> The size of the flash chip is a property of the machine.  It is *fixed*.
>
> I'll have to disagree on this one; in OVMF's case, you can build OVMF in
> 1MB, 2MB, and 4MB *cumulative* size (that is, the numbers I give here
> each refer to the sum of both pflash chips, namely varstore and executable).
>
> The cumulative size that is picked at OVMF build time influences
> (obviously) the amount of crap ^W firmware features that one can squeeze
> into the executable image, but it also determines other things. For
> example, the 4MB build has a different (incompatible!) internal
> structure than the 2MB does. See the small table/comparison in the
> following commit message:
>
>   https://github.com/tianocore/edk2/commit/b24fca05751f
>
> In order to provide some numbers that one can observe simply on their
> host filesystem, the 2MB cumulative size consists of (128K varstore
> chip, 1920KB executable chip); while the 4MB one comprises (528K
> varstore chip, 3568KB executable chip).
>
>> Using whatever size the image has is sloppy modelling.
>
> Maybe so, but it's also very convenient, and also quite important, right
> now (given the multiple firmware image sizes used in the wild).
>
>> A machine may come in minor variations that aren't worth their own
>> machine type.  One such variation could be a different flash chip size.
>> Using the image size to select one from the set of fixed sizes is
>> tolerable.
>
> The problem is that this requires coordination between QEMU and firmware
> development.
>
> (Well, I have to agree that the present patch is *already* that kind of
> coordination;

We've always had that kind of coordination.  It just happens to be less
tight in the case of PC firmware in flash than in most other cases.

>               my point is that when I introduced the 4MB build for OVMF,
> I didn't have to touch QEMU. In retrospect, I'm extremely thankful for
> that, as the introduction of the 4MB build was difficult in itself.)

You don't actually need "flash size is whatever the image size is" for
that.  "Use image size to select one from the set of fixed sizes" should
suffice.

Actually, the PC machines currently comply, just with a rather large
set: { n * 4KiB | 1 <= n <= 2048 }.

I very much doubt PC firmware sizes other than powers of two between
64KiB and 8MiB matter.  Have you ever seen real flash chips with sizes
like 64140KiB?

For traditional (non-flash) firmware, these machines require the image
size to be a multiple of 64KiB.  There is no upper limit.  Images 2GiB
and larger cause integer overflow in old_pc_system_rom_init().

I think both mechanisms should accept the same set of sizes.

> "Using the image size to flexibly dictate the pflash size, in board
> code" would be preferable, to "select from a pre-determined set". (A
> similarly flexible approach would be if we required the user or mgmt app
> to specify base & size as pflash device properties -- see your option #1
> elsewhere --, but I digress.)

Sticking to what real hardware does has had a much better track record
in QEMU than doing things that don't exist in real hardware.

That said, restricting our virtual pflash chips to sizes that can
plausibly occur in real hardware is not a hill I'm prepared to die on.

>> Aside: the image size can change between the place where we get it to
>> pick a flash chip size and realize().
>
> Haha :)
>
>> I guess that's a "don't do that then".
>
> I think so! :)
>
>> If the image size matches the flash chip's size exactly, all is well.
>> 
>> Should we require the size to match the flash chip's size?
>
> So, this is hard to answer generally for all targets / boards.
>
> pc_system_flash_init() (on i440fx/q35, i.e. when OVMF is used) will
> guarantee this equality -- ignoring the TOCTOU that you point out above,
> anyway.
>
> For "virt", the answer carries a lot more weight, because *that* board
> code does not size the pflash from the fw image found in the filesystem.
> (See create_flash(), and a15memmap[VIRT_FLASH].)

The vast majority of boards use *fixed* flash sizes, namely whatever the
physical machine they're modelling has.

We're discussing variable size only because a few boards (four, if I
count correctly) emulate a family of rather diverse machines (such as
PCs), or are perhaps just confused about what they emulate.

> IIUC, this patch suggests, "yes, we should require equality". A no-op
> for OVMF (= pc_system_flash_init()), and helpful against user mistakes
> with the ArmVirtQemu platform firmware (= create_flash()).

Requiring equality makes sense to me.

>> If we accept a smaller image, we want to pad with zeros.  What about
>> writes beyond the size of the image?  Discard them, or let them extend
>> the image file?
>> 
>> If we accept a larger image, we want to ignore its tail.
>> 
>> Sorry for turning the tiny issue your patch tries to address into a much
>> larger one...
>
> I think the following would be useful:
>
> - reject images that are larger than the pflash chip size
> - pad smaller images with zero (not on the disk)
> - ignore guest writes to padding

The "ignore writes" part adds complexity.

> Because, in case of the ArmVirtQemu firmware at least, the build process
> produces the following two files:
>
>   2.0M QEMU_EFI.fd
>   768K QEMU_VARS.fd
>
> And we've always had to manually pad each of these to 64MB, with zeroes,
> on-disk, for use with the "virt" board. If QEMU could do that
> automatically (in memory), that would be a win, in my book.

Some code somewhere has to add padding.  Moving it from A to B is not an
overall win by itself.

To avoid wasting actual disk, try padding with a hole.

> If Alex thinks such padding is out of scope for now, I will certainly
> not insist; I think the present patch (enforcing equality) is already a
> significant usability improvement. I'd just like the error message to be
> precise, and the error checking to be (more) complete.

Re: [Qemu-devel] [PATCH v2] hw/block: report when pflash backing file isn't aligned

[Laszlo Ersek]

On 02/16/19 12:21, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
>> On 02/15/19 17:01, Markus Armbruster wrote:

>>> Using whatever size the image has is sloppy modelling.
>>
>> Maybe so, but it's also very convenient, and also quite important, right
>> now (given the multiple firmware image sizes used in the wild).
>>
>>> A machine may come in minor variations that aren't worth their own
>>> machine type.  One such variation could be a different flash chip size.
>>> Using the image size to select one from the set of fixed sizes is
>>> tolerable.
>>
>> The problem is that this requires coordination between QEMU and firmware
>> development.
>>
>> (Well, I have to agree that the present patch is *already* that kind of
>> coordination;
> 
> We've always had that kind of coordination.  It just happens to be less
> tight in the case of PC firmware in flash than in most other cases.
> 
>>               my point is that when I introduced the 4MB build for OVMF,
>> I didn't have to touch QEMU. In retrospect, I'm extremely thankful for
>> that, as the introduction of the 4MB build was difficult in itself.)
> 
> You don't actually need "flash size is whatever the image size is" for
> that.  "Use image size to select one from the set of fixed sizes" should
> suffice.
> 
> Actually, the PC machines currently comply, just with a rather large
> set: { n * 4KiB | 1 <= n <= 2048 }.
> 
> I very much doubt PC firmware sizes other than powers of two between
> 64KiB and 8MiB matter.  Have you ever seen real flash chips with sizes
> like 64140KiB?

Honestly, I wouldn't know. I haven't seen any physical flash chip (as
in, directing my gaze at it). I also don't know what the usual flash
chip sizes are on physical boards (e.g. I have no clue what my laptop uses).

I think a jump from 4MB to 8MB would be too large. It gives too much of
sudden convenience to firmware developers. I do agree "7MB" looks quite
lame. I really wonder about the flash sizes used on physical UEFI boards.

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH v2] hw/block: report when pflash backing file isn't aligned

[Alex Bennée]

Laszlo Ersek <lersek@redhat.com> writes:

> On 02/15/19 17:01, Markus Armbruster wrote:
>
>> The size of the flash chip is a property of the machine.  It is *fixed*.
>
> I'll have to disagree on this one; in OVMF's case, you can build OVMF in
> 1MB, 2MB, and 4MB *cumulative* size (that is, the numbers I give here
> each refer to the sum of both pflash chips, namely varstore and executable).
>
> The cumulative size that is picked at OVMF build time influences
> (obviously) the amount of crap ^W firmware features that one can squeeze
> into the executable image, but it also determines other things. For
> example, the 4MB build has a different (incompatible!) internal
> structure than the 2MB does. See the small table/comparison in the
> following commit message:
>
>   https://github.com/tianocore/edk2/commit/b24fca05751f
>
> In order to provide some numbers that one can observe simply on their
> host filesystem, the 2MB cumulative size consists of (128K varstore
> chip, 1920KB executable chip); while the 4MB one comprises (528K
> varstore chip, 3568KB executable chip).
>
>> Using whatever size the image has is sloppy modelling.
>
> Maybe so, but it's also very convenient, and also quite important, right
> now (given the multiple firmware image sizes used in the wild).
>
>> A machine may come in minor variations that aren't worth their own
>> machine type.  One such variation could be a different flash chip size.
>> Using the image size to select one from the set of fixed sizes is
>> tolerable.
>
> The problem is that this requires coordination between QEMU and firmware
> development.
>
> (Well, I have to agree that the present patch is *already* that kind of
> coordination; my point is that when I introduced the 4MB build for OVMF,
> I didn't have to touch QEMU. In retrospect, I'm extremely thankful for
> that, as the introduction of the 4MB build was difficult in itself.)
>
> "Using the image size to flexibly dictate the pflash size, in board
> code" would be preferable, to "select from a pre-determined set". (A
> similarly flexible approach would be if we required the user or mgmt app
> to specify base & size as pflash device properties -- see your option #1
> elsewhere --, but I digress.)
>
>> Aside: the image size can change between the place where we get it to
>> pick a flash chip size and realize().
>
> Haha :)
>
>> I guess that's a "don't do that then".
>
> I think so! :)
>
>> If the image size matches the flash chip's size exactly, all is well.
>>
>> Should we require the size to match the flash chip's size?
>
> So, this is hard to answer generally for all targets / boards.
>
> pc_system_flash_init() (on i440fx/q35, i.e. when OVMF is used) will
> guarantee this equality -- ignoring the TOCTOU that you point out above,
> anyway.
>
> For "virt", the answer carries a lot more weight, because *that* board
> code does not size the pflash from the fw image found in the filesystem.
> (See create_flash(), and a15memmap[VIRT_FLASH].)
>
> IIUC, this patch suggests, "yes, we should require equality". A no-op
> for OVMF (= pc_system_flash_init()), and helpful against user mistakes
> with the ArmVirtQemu platform firmware (= create_flash()).
>
>> If we accept a smaller image, we want to pad with zeros.  What about
>> writes beyond the size of the image?  Discard them, or let them extend
>> the image file?
>>
>> If we accept a larger image, we want to ignore its tail.
>>
>> Sorry for turning the tiny issue your patch tries to address into a much
>> larger one...
>
> I think the following would be useful:
>
> - reject images that are larger than the pflash chip size
> - pad smaller images with zero (not on the disk)
> - ignore guest writes to padding
>
> Because, in case of the ArmVirtQemu firmware at least, the build process
> produces the following two files:
>
>   2.0M QEMU_EFI.fd
>   768K QEMU_VARS.fd
>
> And we've always had to manually pad each of these to 64MB, with zeroes,
> on-disk, for use with the "virt" board. If QEMU could do that
> automatically (in memory), that would be a win, in my book.
>
> If Alex thinks such padding is out of scope for now, I will certainly
> not insist; I think the present patch (enforcing equality) is already a
> significant usability improvement. I'd just like the error message to be
> precise, and the error checking to be (more) complete.

I think padding should be in scope for read-only blobs. It does seem
pointless forcing distros to pad blobs that are ultimately never going
to be written to. The only reason I didn't look into it is because I was
unfamiliar with the blk_* api but I guess we can do it all in the pflash
code.

I'll have a look at Markus's clean-ups first and see if I can re-spin
on top of that.

--
Alex Bennée