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

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 7 Nov 2022 at 14:08, Sunil V L <sunilvl@ventanamicro.com> wrote:
>>
>> 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...
>> >
>
>> x86 appears to support variable flash but arm doesn't. What is
>> the reason for not supporting variable size flash in arm?
>
> Mostly, that that's the straightforward thing to code.
> Partly, that it avoids weird cases (eg you can have a backing
> file that's an odd number of bytes but you can't have a
> flash that size).
>
> If x86 has a standard way of handling this then I'm all
> in favour of being consistent across platforms. What's
> the x86 board doing there?

I'm hardly the expert here, but I messed with it at some time... I guess
I can try to answer.

It's complicated.  More often than not, long development history + need
for backward compatibility = complicated.  Which makes it (in my
opinion) not a useful example to follow.

We use either ROM or flash device(s) to hold the BIOS.

The flash option exists since v1.1.

The user interface for picking one or the other, and configure contents
has a long and complicated history.  I'll spare you the details.

ROM contents comes from an image file.  Configurable with -bios.
Default depends on the machine type.

ROM size is the image file size.  It's mapped so it ends right before
address 2^32.

It's mapped a second time so it ends right before 2^20.  If the image
file is larger than 128KiB, only the last 128KiB are mapped there.

Flash contents comes from a block backend.  Configurable with machine
properties "pflash0" and "pflash1" (or legacy -drive if=pflash).  There
is no default.  If you don't configure flash contents, you get ROM
instead.

Flash device size is the block backend size.  Must be a multiple of
4KiB.

If "pflash0" is configured, we create a flash device so it ends right
before address 2^32.  If "pflash1" is also configured, we create a
second flash device so it ends right before the first one (no gap).
Combined size must not exceed a limit that depends on the machine type.

Aside: why two flash devices?  A physical machine has just one...  Well,
we need a read-only part for the code, and a read-write part for
persistent configuration ("varstore" in UEFI parlance).  Physical flash
devices let you write-protect partially, but our device models don't
implement that, it's all or nothing.  So we use two.  Kludge.

Again, there's a second mapping that ends right before 2^20, limited to
128KiB.


In my opinion, setting flash or ROM size to the size of the block
backend or image file is a bad idea.  It tangles up configuration of
frontend and backend.  We used to do this a lot (e.g. -drive).
Untangling (e.g. -device and -blockdev) was a lot of work.  With the
tangle kept around for compatibility.

Doug McIlroy's quip applies: "KISS became MICAHI: make it complicated
and hide it."

A physical machine has a fixed flash memory size.

A virtual machine models a physical machine.  It has a fixed flash
memory size, too.

If we want a machine type to model multiple variations of a physical
machine, say multiple flash sizes, the configuration knob really belongs
to the machine type.  Hiding it somewhere on the file system instead is
a bad idea.