Re: [PATCH V2] hw/riscv: virt: Remove size restriction for pflash

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 7/11/22 14:06, Peter Maydell wrote:
>> On Mon, 7 Nov 2022 at 13:03, Sunil V L <sunilvl@ventanamicro.com> wrote:
>>>
>>> The pflash implementation currently assumes fixed size of the
>>> backend storage.

Intentional.

commit 06f1521795207359a395996c253c306f4ab7586e
Author: Markus Armbruster <armbru@redhat.com>
Date:   Tue Mar 19 17:35:50 2019 +0100

    pflash: Require backend size to match device, improve errors
    
    We reject undersized backends with a rather enigmatic "failed to read
    the initial flash content" error.  For instance:
    
        $ qemu-system-ppc64 -S -display none -M sam460ex -drive 
if=pflash,format=raw,file=eins.img
        qemu-system-ppc64: Initialization of device cfi.pflash02 failed: failed 
to read the initial flash content
    
    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 like this:
    
        qemu-system-ppc64: Initialization of device cfi.pflash01 failed: device 
requires 1048576 bytes, block backend provides 512 bytes
    
    Improve the error for actual read failures to "can't read block
    backend".
    
    To avoid duplicating even more code between the two pflash device
    models, do all that in new helper blk_check_size_and_read_all().
    
    The error reporting can still be confusing.  For instance:
    
        qemu-system-ppc64 -S -display none -M taihu -drive 
if=pflash,format=raw,file=eins.img  -drive 
if=pflash,unit=1,format=raw,file=zwei.img
        qemu-system-ppc64: Initialization of device cfi.pflash02 failed: device 
requires 2097152 bytes, block backend provides 512 bytes
    
    Leaves the user guessing which of the two -drive is wrong.  Mention
    the issue in a TODO comment.
    
    Suggested-by: Alex Bennée <alex.bennee@linaro.org>
    Signed-off-by: Markus Armbruster <armbru@redhat.com>
    Message-Id: <20190319163551.32499-2-armbru@redhat.com>
    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
    Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
    Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>>>                  Due to this, the backend storage file needs to be
>>> exactly of size 32M. Otherwise, there will be an error like below.
>>>
>>> "device requires 33554432 bytes, block backend provides 4194304 bytes"

Why is that a problem?  Genuine question!

>>> Fix this issue by using the actual size of the backing store.
>>>
>>> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
>>> ---
>> Do you really want the flash device size presented to the guest
>> to be variable depending on what the user passed as a block backend?
>> I don't think this is how we handle flash devices on other boards...

Flash device is generally a property of the machine type.  Similar to
physical machines.  Not an accident.

> Ideally handling smaller/bigger backend size should be transparent for
> machine frontend, but we never agreed on what are user expectations and
> how to deal with such cases.
>
> Long term I'd go for:
>
> - if flash is read-only
>
>   a/ bigger backend: display a warning and ignore extra backend data.

Truncating images seems unlikely to be useful.

>   b/ smaller backend: assume flash block is in erased state and fill
>      missing gap with -1 (the default erase value), displaying a warning
>      on startup.

Padding has a better chance to work.  But is it worth the trouble?

>
> - if flash is read-write
>
>   a/ bigger backend: display a warning and ignore extra backend data.
>
>   b/ smaller backend: add a property to pflash device to handle missing
>      gap as erased data. If this flag is not set, display a hint and
>      exit with an error.

What happens when the guest writes to the part that isn't backed by the
backend?

Is this worth the trouble?

> In Sunil particular case, I suppose the issue comes from commit
> 334c388f25 ("hw/block/pflash_cfi0{1, 2}: Error out if device length
> isn't a power of two") which I'm going to revert because the code
> base is not ready for such check:
>
> 78b914c5-ce7e-1d4a-0a67-450f286eb869@linaro.org/">https://lore.kernel.org/qemu-devel/78b914c5-ce7e-1d4a-0a67-450f286eb869@linaro.org/
>
> Regards,
>
> Phil.