Re: PATCH: Increase System Firmware Max Size

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

From:	McMillan, Erich
Subject:	Re: PATCH: Increase System Firmware Max Size
Date:	Fri, 11 Sep 2020 15:22:57 +0000