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

[Laszlo Ersek <lersek@redhat.com>]

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?

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
>