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

[Laszlo Ersek <lersek@redhat.com>]

On 03/24/17 08:38, Jordan Justen wrote:
> 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?

That is my understanding, yes.

Thanks,
Laszlo

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