Re: [PATCH 00/14] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap

[Jordan Justen <jordan.l.justen@intel.com>]

On 2017-03-22 09:58:24, Laszlo Ersek wrote:
> Jordan,
> 
> On 03/16/17 22:22, Laszlo Ersek wrote:
> > On 03/16/17 19:51, Jordan Justen wrote:
> >> On 2017-03-14 16:32:32, Laszlo Ersek wrote:
> >>> This series implements the ideas discussed in the thread
> >>>
> >>>   [edk2] memory type information HOB / UEFI memmap defrag
> >>>   https://lists.01.org/pipermail/edk2-devel/2017-February/007576.html
> >>>
> >>> I'm seeing good results, with approx. 20-24 UEFI memmap entries removed.
> >>>
> >>> The benefits of the patch set are automatically enabled for SMM_REQUIRE
> >>> builds.
> >>>
> >>> For non-SMM builds (which are the default), another build flag is being
> >>> introduced, namely MEM_VARSTORE_EMU_ENABLE. This build flag controls the
> >>> reserved memory / NvVars file based variable emulation. It remains
> >>> enabled by default (keeping the current status quo). For benefiting from
> >>> the Variable PEIM and the dependent UEFI memmap defragmentation,
> >>> MEM_VARSTORE_EMU_ENABLE has to be *disabled*. There are two reasons for
> >>> this:
> >>>
> >>> - The PEI phase FTW and Variable modules need immediate (at-startup)
> >>>   access to the variable store, but the reserved memory based emulation
> >>>   depends on dynamic allocation, plus moving the dynamic pflash
> >>>   detection to PEI is out of question.
> >>>
> >>> - Even more importantly, reading actual variable contents from the
> >>>   non-pflash varstore would require, in PEI, similar hackery that
> >>>   currently happens in BDS -- that's not going to happen.
> >>
> >> What prevents us from enabling the PEI variable stack running using
> >> the memory based "emu fvb"?
> > 
> > (1) PlatformPei would have to dynamically detect whether the pflash
> > address range behaves as flash or ROM. Same as in the current runtime
> > DXE driver, QemuFlashFvbServicesRuntimeDxe. Code duplication or quite a
> > bit of code movement, for factoring it out and sharing it.
> > 
> > (2) If it behaves as ROM, the memory block has to be allocated (like
> > now). If we're not just rebooting within the VM, that block of memory is
> > empty. The FTW and variable PEIMs will look for headers in there, so it
> > must be formatted.
> > 
> > (3) The address of the dynamic allocation has to be propagated to the
> > FTW and variable PEIMs. This introduces an ordering constraint between
> > PlatformPei and the FTW & variable PEIMs that we'd have to satisfy somehow.
> > 
> > (4) After all of the above, no variables would be found in the allocated
> > / formatted memory block, unless we were just rebooting within the VM.
> > (I assume we wouldn't want to reload \NvVars from FAT in PEI...)
> > Considering the current use case, all fresh VM startups would produce
> > the default memory type info HOB, and wouldn't remedy the memory map
> > fragmentation. And in the longer term, considering S4, when the VM goes
> > down for S4 / hibernation, the VM actually goes away (QEMU exits), and
> > S4 resume would likely not work with a different / fragmented memmap (or
> > it would be obscurely different if you warm-rebooted once before).
> > 
> > It is technically solvable, but the benefit is very little; it would
> > just perpetuate the current broken emulation.
> > 
> >>> In fact one of the longer term goals with MEM_VARSTORE_EMU_ENABLE is to
> >>> identify what we'll rip out once we finally decide to drop the reserved
> >>> memory / NvVars file based emulation.
> >>
> >> I think what we could consider ripping out is the disk save feature.
> >> Since QEMU/KVM (at least in the past) seem to preserve RAM contents
> >> across reboot, I think the memory based buffers should continue to be
> >> a fallback for when users don't get pflash setup correctly.
> > 
> > (a) The memory preservation works across reboot, yes, but it is not
> > useful for the memory type information HOB, and consequently (in the
> > longer term) for S4. The memory type information HOB should carry
> > information (== implement a long-term maximum search) over several cold
> > boots (= different QEMU instances) of the same virtual machine.
> > 
> > At every normal boot, the emulated varstore will be empty (and
> > well-formed only because we'd have to do that manually too, see (2)
> > above), and any access to it within PEI will be useless.
> > 
> > (b) If users don't set up pflash correctly, the default build will
> > continue to work for them without any changes at all. This covers Xen as
> > well.
> > 
> > The strict pflash requirements are only live if users build OVMF with
> > either one of the following non-default switches:
> > 
> >   -D SMM_REQUIRE
> > 
> > or
> > 
> >   -D MEM_VARSTORE_EMU_ENABLE=FALSE
> > 
> > Both of these imply "I know what I'm doing".
> > 
> > I see no benefit in enabling the FTW & Variable PEIMs without a
> > persistent flash varstore. I do see it require a whole lot of work.
> > 
> > If someone wants the FTW & Variable PEIMs real hard under the above (IMO
> > useless) circumstances, they're welcome to generalize the code / make it
> > dynamic on top of this series.
> > 
> >> That said, I'm not sure we should decide to rip out the disk save
> >> feature just yet, unless you think it can simplify things greatly.
> > 
> > The disk save (\NvVars) feature is irrelevant for this series. That
> > feature is based off the "PcdOvmfFlashVariablesEnable" PCD, which is set
> > by QemuFlashFvbServicesRuntimeDxe, and consumed by Platform BDS.
> > 
> > When OVMF is built with either of the above build switches,
> > PcdOvmfFlashVariablesEnable is guaranteed to end up as TRUE, so the
> > \NvVars code is always (dynamically) disabled in those builds.
> > 
> > I don't want to prevent users from continuing to *use* the emulated
> > varstore in the default build. However, that emulation is obscurely
> > broken, and has been so for 7 years. QEMU 1.6 (with pflash) has been out
> > for the second half of that time. I certainly want to stop *developing*
> > features for the emulated variable store. (If someone does want to, on
> > top of this first-line enablement, I won't stand in their way, of course.)
> 
> ping -- what do you think of my arguments above, please?

I guess I'd like to see what it would take to enable it for the EMU
FVB path. I hope I can find some time to look at it this weekend.

Regarding depending more on flash by default, is it correct that Xen
is the only significant thing preventing it? I mean, looking at it
from the qemu/kvm side of things, I would say we could discuss just
building that way by default at this point. I still think we would
want a failure path friendlier than a blind crash, which still leaves
some dependence on EMU FVB so we could boot far enough for a user
visible crash message.

-Jordan

> While this series is not urgent for me at the moment, flushing it (as
> in, committing it) would certainly ease my load.
> 
> If you disagree with the above points, I'll have to postpone this work
> indefinitely. (The downstream churn has arrived.)
> 
> Thanks,
> Laszlo
> 
> >>> A summary of build modes:
> >>>
> >>> * default build: works as before, with the Variable PEIM and UEFI memmap
> >>>   defragmentation remaining unavailable.
> >>>
> >>> * -D SMM_REQUIRE: works as before, with the Variable PEIM and UEFI
> >>>   memmap defragmentation enabled.
> >>>
> >>>   As a reminder, this build is inherently incompatible with QEMU's
> >>>   "-bios" and "-L" parameters, which will trigger the ASSERT() in
> >>>   QemuFlashInitialize()
> >>>   [OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c], added in commit
> >>>   b963ec494c48 ("OvmfPkg: QemuFlashFvbServicesRuntimeDxe: adhere to -D
> >>>   SMM_REQUIRE", 2015-11-30).
> >>>
> >>> * -D MEM_VARSTORE_EMU_ENABLE=FALSE: eliminates the reserved memory /
> >>>   NvVars based variable emulation, turns pflash into a hard requirement,
> >>>   and enables the Variable PEIM and the UEFI memmap defragmentation.
> >>>
> >>>   If such a build is executed with -bios or -L, then the FTW and
> >>>   Variable PEIMs will temporarily see a well-formed, but empty varstore
> >>>   (straight from "OvmfPkg/VarStore.fdf.inc"), and then
> >>>   QemuFlashFvbServicesRuntimeDxe will trip a new ASSERT(), in patch #6,
> >>>   that is similar to the one added in commit b963ec494c48 (see above).
> >>>
> >>> Anatomy of the series:
> >>>
> >>> * Patches 01 through 04 clean up some innocent warts I noticed while
> >>>   working on the feature.
> >>>
> >>> * Patches 05 through 07 introduce the new build option and its basic
> >>>   effect, the disabling of reserved memory based variable emulation.
> >>>
> >>> * Patches 08 through 12 include the FTW and Variable PEIMs.
> >>>
> >>> * Patch 13 enables UEFI memmap defragmentation.
> >>>
> >>> * Patch 14 updates the README file.
> >>>
> >>> The variable PEIM was independently requested in
> >>> <https://bugzilla.tianocore.org/show_bug.cgi?id=386> earlier, but
> >>> without a good upstreamable use case, I disagreed with its inclusion.
> >>> The memmap defragmentation is a good use case however.
> >>>
> >>> Also, I didn't lie about "downstream pressure" in my previous patch set;
> >>> I wrote this one at Sunday/Monday night (and now it's Tues/Wed night).
> >>> So one could consider this personal diligence. :)
> >>>
> >>> The series conforms to the multi-line function call syntax outlined in
> >>> <https://bugzilla.tianocore.org/show_bug.cgi?id=425>.
> >>>
> >>> Repo:   https://github.com/lersek/edk2.git
> >>> Branch: memmap_defrag
> >>>
> >>> CC: Jordan Justen <jordan.l.justen@intel.com>
> >>>
> >>> Thanks
> >>> Laszlo
> >>>
> >>> Laszlo Ersek (14):
> >>>   OvmfPkg/EmuVariableFvbRuntimeDxe: always format an auth varstore
> >>>     header
> >>>   OvmfPkg: remove gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable
> >>>   OvmfPkg/PlatformPei: remove unused PcdVariableStoreSize dependency
> >>>   OvmfPkg/PlatformPei: don't allocate reserved mem varstore if
> >>>     SMM_REQUIRE
> >>>   OvmfPkg: introduce PcdMemVarstoreEmuEnable feature flag
> >>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: check PcdMemVarstoreEmuEnable
> >>>   OvmfPkg: conditionally disable reserved memory varstore emulation at
> >>>     build
> >>>   OvmfPkg: resolve PcdLib for all PEIMs individually
> >>>   OvmfPkg: resolve PcdLib for PEIMs to PeiPcdLib by default
> >>>   OvmfPkg: introduce FlashNvStorageAddressLib
> >>>   OvmfPkg: include FaultTolerantWritePei and VariablePei
> >>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: don't set flash PCDs if SMM or
> >>>     no-emu
> >>>   OvmfPkg/PlatformPei: remedy UEFI memory map fragmentation
> >>>   OvmfPkg/README: document MEM_VARSTORE_EMU_ENABLE and memmap defrag
> >>>
> >>>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c                                |  79 +---------
> >>>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf                              |   3 -
> >>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c   |  53 +++++++
> >>>  OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf |  48 +++++++
> >>>  OvmfPkg/OvmfPkg.dec                                                   |   7 +-
> >>>  OvmfPkg/OvmfPkgIa32.dsc                                               |  43 +++---
> >>>  OvmfPkg/OvmfPkgIa32.fdf                                               |   8 +-
> >>>  OvmfPkg/OvmfPkgIa32X64.dsc                                            |  43 +++---
> >>>  OvmfPkg/OvmfPkgIa32X64.fdf                                            |   8 +-
> >>>  OvmfPkg/OvmfPkgX64.dsc                                                |  43 +++---
> >>>  OvmfPkg/OvmfPkgX64.fdf                                                |   8 +-
> >>>  OvmfPkg/PlatformPei/MemTypeInfo.c                                     | 151 ++++++++++++++++++++
> >>>  OvmfPkg/PlatformPei/Platform.c                                        |  28 +---
> >>>  OvmfPkg/PlatformPei/Platform.h                                        |   5 +
> >>>  OvmfPkg/PlatformPei/PlatformPei.inf                                   |   4 +-
> >>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf      |   1 +
> >>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf             |   1 +
> >>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c               |  58 +++++---
> >>>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c                    |   1 +
> >>>  OvmfPkg/README                                                        |  10 ++
> >>>  20 files changed, 425 insertions(+), 177 deletions(-)
> >>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.inf
> >>>  create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvStorageAddressLib.c
> >>>  create mode 100644 OvmfPkg/PlatformPei/MemTypeInfo.c
> >>>
> >>> -- 
> >>> 2.9.3
> >>>
> > 
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> > 
>