* Laszlo Ersek (ler...@redhat.com) wrote: > On 09/11/20 17:06, Dr. David Alan Gilbert wrote: > > * Laszlo Ersek (ler...@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 / dgilb...@redhat.com / Manchester, UK
