[libvirt] [Qemu-devel] Configuring pflash devices for OVMF firmware

[Peter Maydell]

On Fri, 25 Jan 2019 at 15:11, Markus Armbruster <armbru at redhat.com> wrote:
> (1) cfi.pflash01 isn't available with -device.
>
> (2) "Magic board code picks up the backend [created for -drive
>     if=pflash], creates a frontend (a cfi.pflash01 device), and maps it
>     into the guest's address space."  When we replace if=pflash by
>     if=none, we get to replicate that magic on top of -device.
>
> Issue (1) isn't too hard: we add the device to the dynamic sysbus device
> white-list, move a sysbus_mmio_map() from pflash_cfi01_realize() into
> pflash_cfi01_realize().  The latter requires a new device property to
> configure the base address.  I got a working prototype.  Since this
> makes the device model's name and properties ABI, review would be
> advisable.

Flash devices exist on the board at specific addresses, so they
should in general be created by the board model. Creating them
by the user on the command line is a mess because then the
user has to know the right base address. And then the board
code needs to do something for "if the user didn't create this
then we need to do it", because the flash device should exist
in the model whether the user cared about its contents or not.

Dynamic sysbus is something I'd rather we did less of, not more
of. It's there because it solves an annoying problem (people
want to do device passthrough of hardware that's not on a nice
pluggable probeable bus), but it's really awkward.

> To solve (2), we first have to understand the magic.  Device
> cfi.pflash01 has the following properties:
>
>     num-blocks                  Size of the device in blocks
>     sector-length               Size of a block
>                                 (admire the choice of names)
>     width                       Bank width
>     big-endian                  Endianess (d'oh)
>     id0, id1, id2, id3          Some kind of device ID, guest-visible,
>                                 default to zero, few boards change it

Note that most of this is stuff that the hardware has.
A lot of boards set these to garbage values which happened
to be what the very old implementation of pflash hardcoded,
because most guests don't care. This is strictly speaking
wrong and they should use whatever the hardware really has,
but most of these cases are for old not-very-maintained dev
boards where probably nobody even has the relevant hardware
even if they cared enough to find out what its ID values are.

>     name                        Memory region name
>                                 (why is this even configurable?)

(a) for debug purposes, so a machine can create two flash
devices and give them names which make them easier to tell
apart in monitor "info mem" and similar command output
(b) more  importantly, the memory region name is used as
the migration vmstate ram name, so if you change it you
break migration compat.

>     phys-addr                   Physical base address
>                                 (this is the new device property
>                                 mentioned above)
>     secure                      For restricting access to firmware,
>                                 default off
>     device-width                you don't want to know,
>                                 there is a default, but it's documented
>                                 as "bad, do not use", yet pretty much
>                                 all boards use it

See above about "old not-very-maintained dev boards". A
board which does use this is one that's doing it for
back-compat because nobody's cared to fix and test.

>     max-device-width            defaults to device-width
>                                 not actually set anywhere
>     old-multiple-chip-handling  back-compat gunk for
>                                 machine types 2.8 and older
>
> The magic board code in hw/i386/pc_sysfw.c configures as follows:
>
>     num-blocks                  computed from backend size
>     sector-length               4096
>     width                       1
>     big-endian                  0
>     id0, id1, id2, id3          all 0
>     name                        system.pflash<U>, where U is -drive's
>                                 unit number
>     phys-addr                   computed so
>                                 unit 0 ends right below 0x100000000,
>                                 unit n+1 ends at right below unit n
>
> "secure", "device-width", "max-device-width",
> "old-multiple-chip-handling" are left at the default.
>
> One additional bit of magic is actually in libvirt: it configures
> "secure" by flipping its default with
> -global driver=cfi.pflash01,property=secure,value=on.
>
> Now let's consider how to replicate this magic on top of device.
>
> Perhaps machine-type specific defaults could take care of sector-length,
> width, big-endian, id0, id1, id2, id3.  Leaves num-blocks, name, and
> phys-addr.

You could at least in theory have a machine with several
different flash devices of different make/ID. This is why
they're device properties. I think they're fine the way
they are, ie set by the code that creates the device.

thanks
-- PMM