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

[Peter Maydell <peter.maydell@linaro.org>]

On Wed, 30 Jan 2019 at 07:24, Markus Armbruster <armbru@redhat.com> wrote:
>
> 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...

Usual reason -- we don't change stuff unless we're reasonably
sure it doesn't actually break guests that used to work.

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

The property setup here is basically maintaining compat
with the way the old pre-qdev implementation worked.
If there's a nicer way to do this that doesn't change
the memory region name and break migration compat, that's
great.

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

Yes. vexpress was the board where the broken pflash implementation
was first reported, so we fixed the pflash device and made
vexpress set things correctly. virt also gets a fixed device
because it postdates things being fixed.

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

...vexpress can't be both fixed and not...

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

Well, nobody who does anything with x86 has cared enough to
make the pflash implementation actually correct. The
weird "behave like a buggy implementation" default is there
pretty much exactly because x86 uses it and I didn't
want to change the x86 behaviour.

thanks
-- PMM