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