Re: PATCH: Increase System Firmware Max Size

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 09/11/20 17:06, Dr. David Alan Gilbert wrote:
> > * Laszlo Ersek (lersek@redhat.com) wrote:
> >> On 09/11/20 10:34, Dr. David Alan Gilbert wrote:
> 
> >>> 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!
> 
> Yes, that would be nice. I don't know how it works. Maybe when adding
> the next MemoryRegion there's an error or an assert(). No clue.
> 
> >> 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?
> 
> My point wasn't that the guest would be lost or corrupted, only that it
> couldn't be migrated. We'd say "for this, you have to upgrade QEMU on
> the destination host as well, or use a smaller firmware", and they'd say
> "we don't want either of those things".

My point is that already breaks if you used a smaller firmware on the
source and the current size on the destination.

> >>> 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.
> 
> Right; if we can keep regressions out (not just functional regressions,
> but workflow / use case regressions too), then it's OK to support more
> use cases.
> 
> By workflow / use case regressions I mean that it should not become more
> difficult to maintain OVMF as a result of the patch. (It should not
> imply that now people can stuff even more cruft into OVMF, because "hey
> there's more room now".)
> 
> >> 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.
> 
> Technically, I agree. It's fine to run larger UEFI firmware as long as
> the default size restriction for the default (or traditional) UEFI
> firmware remains the same.
> 
> Turn the size limit into a property, keep the same default value,
> implement the migration aspects; specify *very clearly* in the commit
> message what particular firmware this knob is being introduced for.

I don't think there is a migration aspect here; and this knob is a
general knob - it's just Erich here is the poor unfortunate person
who wanted to tune that knob for the first time.

> ... And I'd still be grumpy, because it increases maintenance burden for
> QEMU (and possibly OVMF too -- "hey we got more room now!"; see above),
> without open source users benefiting from the change. It's not like
> we'll ever be able to run, or read the source code of, that 8MB+
> firmware image, is it?

Sorry, not QEMU's problem; we don't restrict RAM to 640k just because
it should be enough for anyone.

> >>> 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.
> 
> Sorry, I wasn't clear enough.
> 
> I *know* we're going to see *those* "open uses" that you meant.
> 
> Precisely because they are "open uses", they have a chance at justifying
> the churn.
> 
> My intent was to apply your (valid) argument to *this* proposal -- let
> us see the "open uses" for *this* particular proposal.
> 
> Notice, in the thread starter:
> 
> "We have a need for increased firmware size", "our Uefi Firmware",
> "change can be made to open source" --> it's obviously for the sake of a
> proprietary platform firmware. Do you feel comfortable about taking on
> more risks, reviews and maintenance for that?

I'm not seeing any more risks, reviews or maintenance in QEMU due to it.
Indeed replacing an undocumented, unchecked constant comparison with
a proper property seems better.

> (Note that I'm not singling out this particular proprietary guest
> payload. I feel the exact same way when QEMU is being contorted for
> Windows of OSX guests, but at least those guest payloads are universally
> available to users, if the users are willing to comply with the
> corresponding terms and conditions.)
> 
> > 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.
> 
> You as a QEMU maintainer / reviewer, and I as an OVMF maintainer /
> reviewer, are going to pay with our time and effort for a proprietary
> guest oriented change that normal QEMU users won't even be able to run,
> let alone read, modify, distribute.

We have lots of complex hideous changes that I'm never going to use but
seem reasonable;  this is a tiny change that seems perfectly reasonable
both for open and closed firmware.
I realise herding OVMF developers is tricky, but that's not a reason
to nack a reasonable almost trivial change in QEMU.

Dave

> But yes, technically speaking, we can replace the size limit constant
> with a property, I think.
> 
> Laszlo
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK