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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Markus Armbruster <address@hidden> writes:
>
>> Laszlo Ersek <address@hidden> 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 <address@hidden>
>>>> 
>>>> ---
>>>> 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.