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.jus...@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
>>>
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
