Re: [Qemu-devel] Configuring pflash devices for OVMF firmware

[Markus Armbruster <armbru@redhat.com>]

Let me reply to the "why is the cfi.pflash01 device so weird" part
first, because that's relatively quick, and because it could easily
distract us from the more important "how do we want to configure OVMF"
part.  I'll reply to that part later.

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

> On Fri, 25 Jan 2019 at 15:11, Markus Armbruster <armbru@redhat.com> wrote:
[...]
>> 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.

Why are we emulating (badly) stuff nobody cares about enough to find out
what exactly we should be emulating, the world wonders...

>>     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

That's what we have qdev IDs for.

> (b) more  importantly, the memory region name is used as
> the migration vmstate ram name, so if you change it you
> break migration compat.

Sounds about as useful as a --break-me-but-subtly option.

>>     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.

Boards that seem to care:

    hw/arm/vexpress.c:    qdev_prop_set_uint8(dev, "device-width", 2);
    hw/arm/virt.c:    qdev_prop_set_uint8(dev, "device-width", 2);

Boards that seem not to care:

    hw/arm/collie.c:    pflash_cfi01_register(SA_CS0, NULL, "collie.fl1", 0x02000000,
    hw/arm/collie.c:    pflash_cfi01_register(SA_CS1, NULL, "collie.fl2", 0x02000000,
    hw/arm/gumstix.c:    if (!pflash_cfi01_register(0x00000000, NULL, "connext.rom", connex_rom,
    hw/arm/gumstix.c:    if (!pflash_cfi01_register(0x00000000, NULL, "verdex.rom", verdex_rom,
    hw/arm/mainstone.c:        if (!pflash_cfi01_register(mainstone_flash_base[i], NULL,
    hw/arm/omap_sx1.c:        if (!pflash_cfi01_register(OMAP_CS0_BASE, NULL,
    hw/arm/omap_sx1.c:        if (!pflash_cfi01_register(OMAP_CS1_BASE, NULL,
    hw/arm/versatilepb.c:    if (!pflash_cfi01_register(VERSATILE_FLASH_ADDR, NULL, "versatile.flash",
    hw/arm/vexpress.c:static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name,
    hw/arm/vexpress.c:    pflash0 = ve_pflash_cfi01_register(map[VE_NORFLASH0], "vexpress.flash0",
    hw/arm/vexpress.c:    if (!ve_pflash_cfi01_register(map[VE_NORFLASH1], "vexpress.flash1",
    hw/arm/z2.c:    if (!pflash_cfi01_register(Z2_FLASH_BASE,
    hw/i386/pc_sysfw.c:        /* pflash_cfi01_register() creates a deep copy of the name */
    hw/i386/pc_sysfw.c:        system_flash = pflash_cfi01_register(phys_addr, NULL /* qdev */, name,
    hw/lm32/milkymist.c:    pflash_cfi01_register(flash_base, NULL, "milkymist.flash", flash_size,
    hw/microblaze/petalogix_ml605_mmu.c:    pflash_cfi01_register(FLASH_BASEADDR,
    hw/microblaze/petalogix_s3adsp1800_mmu.c:    pflash_cfi01_register(FLASH_BASEADDR,
    hw/mips/mips_malta.c:    fl = pflash_cfi01_register(FLASH_ADDRESS, NULL, "mips_malta.bios",
    hw/mips/mips_r4k.c:        if (!pflash_cfi01_register(0x1fc00000, NULL, "mips_r4k.bios", mips_rom,
    hw/ppc/sam460ex.c:    if (!pflash_cfi01_register(base, NULL, "sam460ex.flash", bios_size,
    hw/ppc/virtex_ml507.c:    pflash_cfi01_register(PFLASH_BASEADDR, NULL, "virtex.flash", FLASH_SIZE,

At least PC can't be characterized as "not-very-maintaned dev board".

>>     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.
[...]