Re: PATCH: Increase System Firmware Max Size

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 09/11/20 10:34, Dr. David Alan Gilbert wrote:
> > * Laszlo Ersek (lersek@redhat.com) wrote:
> >> +Markus, Dave, Phil
> >>
> >> On 09/11/20 03:45, McMillan, Erich wrote:
> >>> Hi all,
> >>>
> >>> (this is my first Qemu patch submission, please let me know if my 
> >>> formatting/content needs to be fixed).
> >>> We have a need for increased firmware size, currently we are building 
> >>> Qemu with the following change to test our Uefi Firmware and it works 
> >>> well for us. Hope that this change can be made to open source. Thank you.
> >>> -------
> >>> Increase allowed system firmware size to 16M per comment suggesting up to 
> >>> 18M is permissible.
> >>>
> >>>  Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
> >>>
> >>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> >>> index 
> >>> b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca
> >>>  100644
> >>> --- a/hw/i386/pc_sysfw.c
> >>> +++ b/hw/i386/pc_sysfw.c
> >>> @@ -46,7 +46,7 @@
> >>>   * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 
> >>> 8MB in
> >>>   * size.
> >>>   */
> >>> -#define FLASH_SIZE_LIMIT (8 * MiB)
> >>> +#define FLASH_SIZE_LIMIT (16 * MiB)
> >>>
> >>>  #define FLASH_SECTOR_SIZE 4096
> >>> -------
> >>>
> >>>
> >>
> >> (1) This is not a trivial change, so qemu-trivial: please ignore.
> >>
> >> (2) The comment has not been updated.
> >>
> >> (3) I'm almost certain that *if* we want to change this, it needs to be
> >> turned into a machine type (or some device model) property, for
> >> migration compatibility.
> > 
> > I'm missing what this constant exists for - why is the
> > check there at all  Is there something else that lives below this
> > address that we have to protect?
> 
> Yes, some MMIOs that I'm at least aware of are IO_APIC_DEFAULT_ADDRESS
> (0xfec00000), TPM_PPI_ADDR_BASE (0xFED45000), APIC_DEFAULT_ADDRESS
> (0xfee00000).
> 
> They are not directly adjacent with pflash; nor should they be.

Hmm those need explicitly checks adding somewhere against
FLASH_SIZE_LIMIT!

> On one hand, the current FLASH_SIZE_LIMIT is meant to be  sufficient for
> a long time ("should be enough for anyone").
> 
> On the other hand, if we have a really strong reason to increase the
> size limit, the current value is supposed to give us a safety margin, so
> that we can satisfy the immediate need at that point *first*, and start
> looking into (likely more intrusive) physical address map changes, to
> restore the safety margin.
> 
> > My reading of the code is that increasing that constant doesn't change
> > the guests view at all, as long as the guest was given the same flash
> > files - so if the guests view doesn't change, then why would we tie
> > it to the machine type?
> 
> If you increase the size limit (without tieing it to a machine type),
> then, with an upgraded QEMU and the same (old) machine type, you can
> start a guest with a larger-than-earlier (cumulative) flash size. Then,
> when you try to migrate this to an old QEMU (but same machine type),
> it's a bad surprise. I understand that backwards migration is not
> universally supported (or expected), but I don't want this problem to
> land on my desk *ever*.

I support backwards migration; but that migration wouldn't work anyway -
wouldn't that fail nicely with a mismatched RAMBlock size?

> Furthermore, un-enumerable / platform-MMIO devices tend to pop up time
> after time. TPM_PPI_ADDR_BASE (0xFED45000) is a somewhat recent
> addition, for example. It's not like we're never going to need more
> address space up there.
> 
> > 
> >> (4) I feel we need much more justification for this change than just
> >> "our firmware is larger than upstream OVMF".
> >>
> >> In the upstream 4MB unified OVMF build, there's *plenty* of free room.
> >> Of course that's not to say that we're willing to *squander* that space
> >> -- whenever we include anything new in the upstream OVMF platform(s),
> >> there must be a very good reason for it --, just that, given the
> >> upstream OVMF status, the proposed pflash increase in QEMU is staggering.
> >>
> >> Considering upstream OVMF and QEMU, it should take *years* to even get
> >> close to filling the 4MB unified flash image of OVMF (hint: link-time
> >> optimization, LZMA compression), let alone to exhaust the twice as large
> >> (8MB) QEMU allowance.
> >>
> >> Unless you are committed to open source your guest firmware too (maybe
> >> as part of upstream edk2, maybe elsewhere), I seriously doubt we should
> >> accommodate this use case in *upstream* QEMU. It complicates things
> >> (minimally with regard to migration), and currently I don't see the
> >> benefit to the upstream community.
> >>
> >> Clearly, for building your firmware image, you must have minimally
> >> modified the DSC and FDF files in OVMF too, or created an entirely
> >> separate firmware platform -- DSC and FDF both.
> >>
> >> If you are using your downstream OVMF build as a testbed for your
> >> proprietary physical platform firmware, that's generally a use case that
> >> we're mildly interested in not breaking in upstream OvmfPkg. But in
> >> order to make me care for real (as an OvmfPkg co-maintainer), you'd need
> >> to upstream your OVMF platform to edk2 (we already have Xen and --
> >> recently added -- bhyve firmware platforms under OvmfPkg, not just
> >> QEMU). You'd also have to offer long-term reviewership and testing for
> >> that platform in edk2 (like the Xen and bhyve stake-holders do). Then we
> >> could consider additional complexity in QEMU for booting that firmware
> >> platform.
> > 
> > Our UEFI firmware is pretty sparse;
> 
> Yes, in part because I strive to keep it that way.

But that's your choice, on our firmware implementation; that's not a
requirement of QEMU or q35.

> I fight to keep out
> all cruft that I can (at least by conditionalizing it) so that there is
> room for the stuff that I cannot keep out. (And I always strive to set
> expectations that flipping all possible build knobs in OVMF to "on" may
> very well cause an "out of space" error.)
> 
> - I've made sure that PVSCSI_ENABLE and MPT_SCSI_ENABLE be stand-alone
> config knobs. The use case behind them is valid, the drivers are open
> source, but the use case is still niche, so they must be easy to keep out.
> 
> - I've made sure LSI_SCSI_ENABLE is a stand-alone config knob too (and
> it even defaults to FALSE). The QEMU device that the driver drives is
> obsolete / deprecated.
> 
> - If VMBus drivers are ever going to be contributed, they'll need a
> config knob.
> 
> - Current Xen code in OVMF is supposed to be separated completely to the
> new, dedicated XenPVH platform
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2122>.
> 
> - Bhyve support is a separate platform build.
> 
> - Capsule updates are not supported by OVMF, and if they will ever be,
> they're going to have to be a separate firmware platform. Datacenter
> virtualization has no use for capsule updates.
> 
> - The next big thing I expect to have to keep out of OVMF is Redfish
> <https://en.wikipedia.org/wiki/Redfish_(specification)>. Libvirt,
> OpenStack, Cockpit, Kubernetes already handle that area *underneath* the
> guest, I believe. (It's OK to use OVMF for developing / testing Redfish;
> it's not OK to expect that the current OVMF firmware platform contain
> everything that it contains now *plus* Redfish.)
> 
> Sparse is *good*.
> 
> > it doesn't have any pretty graphics
> > or snazzy stuff,
> 
> Which is arguably completely superfluous on every possible platform one
> can imagine. On the other hand, if you want a real serial port on
> workstation class hardware, you may have to order a separate part (if
> you are lucky and you *can* order one). Serial-over-LAN is a complete
> non-replacement.
> 
> The reason (should I say: excuse) for the firmware to exist is to (a)
> boot operating systems, (b) abstract away some ugly platform-specific
> hardware for OS runtime (by providing ACPI and SMBIOS descriptions, and
> a *small* set of runtime services). We can maybe add (c) "root of trust".
> 
> In practice, physical firmware is becoming the hw vendor's OS before
> (and under) your OS, one you cannot replace, and one whose updates can
> brick your hardware. Permitting the same feature creep on virtual
> platforms is wrong.

On the firmware you develop that choice is fine; but there's no reason
to force that restriction onto others.

> > or have to survive configuring lots of hardware; also
> > I'm aware of other companies who are looking at putting big blobs
> > of stuff in firmware for open uses;
> 
> Key term being "open uses". Let us see them.

Well yes, I think we know who we're speaking about here and we're
working on it.

> > so I don't see a problem with
> > changing this limit from the QEMU side of things.
> 
> I do. Software and data always expand to consume all available space,
> and it's going to cause a conflict between UEFI features and platform
> MMIO sooner or later. Then I'll get the privilege of shuffling around
> the crap in OVMF (all of which is "indispensable" or course).
> 
> If we ever go down this route, it needs to benefit open source directly
> and significantly.

Being able to use QEMU to let vendors debug their platform firmware is a
perfectly reasonable use of QEMU; maybe not of your OVMF build - we
need to keep the restrictions on the two separate.

Dave

> Laszlo
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK