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

[Daniel P . Berrangé]

On Tue, Nov 08, 2022 at 03:12:42PM +0100, Philippe Mathieu-Daudé wrote:
> On 7/11/22 18:34, Daniel P. Berrangé wrote:
> > On Mon, Nov 07, 2022 at 06:32:01PM +0100, Andrew Jones wrote:
> > > On Mon, Nov 07, 2022 at 04:19:10PM +0000, Daniel P. Berrangé wrote:
> > > > On Mon, Nov 07, 2022 at 03:50:44PM +0000, Alex Bennée wrote:
> > > > > 
> > > > > Sunil V L <sunilvl@ventanamicro.com> writes:
> > > > > 
> > > > > > On Mon, Nov 07, 2022 at 01:06:38PM +0000, 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. 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"
> > > > > > > > 
> > > > > > > > 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...
> > > > > > > 
> > > > > > 
> > > > > > Hi Peter,
> > > > > > 
> > > > > > x86 appears to support variable flash but arm doesn't. What is
> > > > > > the reason for not supporting variable size flash in arm?
> > > > > 
> > > > > If I recall from the last time we went around this is was the question
> > > > > of what you should pad it with.
> > > > 
> > > > Padding is a very good thing from the POV of upgrades. Firmware has 
> > > > shown
> > > > a tendancy to change (grow) over time, and the size has an impact of the
> > > > guest ABI/live migration state.
> > > > 
> > > > To be able to live migrate, or save/restore to/from files, then the 
> > > > machine
> > > > firmware size needs to be sufficient to cope with future size changes of
> > > > the firmware. The best way to deal with this is to not use the firmware
> > > > binaries' minimum compiled size, but instead to pad it upto a higher
> > > > boundary.
> > > > 
> > > > Enforcing such padding is a decent way to prevent users from 
> > > > inadvertantly
> > > > painting themselves into a corner with a very specific firmware binary
> > > > size at initial boot.
> > > 
> > > Padding is a good idea, but too much causes other problems. When building
> > > lightweight VMs which may pull the firmware image from a network,
> > > AArch64 VMs require 64MB of mostly zeros to be transferred first, which
> > > can become a substantial amount of the overall boot time[*]. Being able to
> > > create images smaller than the total flash device size, but still add some
> > > pad for later growth, seems like the happy-medium to shoot for.
> > 
> > QEMU configures the firmware using -blockdev, so can use any file
> > format that QEMU supports at the block layer.  IOW, you can store
> > the firmware in a qcow2 file and thus you will never fetch any
> > of the padding zeros to be transferred.  That said I'm not sure
> > that libvirt supports anything other than a raw file today.
> 
> Drew might be referring to:
> 
> https://lore.kernel.org/qemu-devel/20210810134050.396747-1-david.edmondson@oracle.com/
> 
>  > Currently ARM UEFI images are typically built as 2MB/768kB flash
>  > images for code and variables respectively. These images are both
>  > then padded out to 64MB before being loaded by QEMU.
>  >
>  > Because the images are 64MB each, QEMU allocates 128MB of memory to
>  > read them, and then proceeds to read all 128MB from disk (dirtying
>  > the memory). Of this 128MB less than 3MB is useful - the rest is
>  > zero padding.
>  >
>  > On a machine with 100 VMs this wastes over 12GB of memory.
> 
> See previous attempts:
> - Huawei
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg607292.html
> - Tencent
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg742066.html
> - Oracle
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg760065.html
> - Red Hat
> https://www.mail-archive.com/qemu-block@nongnu.org/msg81714.html

I've not looked at the patch series above in detail, but I'm thinking
that even if the file is 64 MB in size, it should never read all 64 MB
of data. The block layer APIs have ability to detect and report holes,
so it ought to be possible to only read the data which is non-zero,
and thus avoid dirtying more than 3 MB of useful

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|