From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DD4D68044B for ; Fri, 24 Mar 2017 05:49:25 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 26D2261B8D; Fri, 24 Mar 2017 12:49:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 26D2261B8D Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 26D2261B8D Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-209.phx2.redhat.com [10.3.116.209]) by smtp.corp.redhat.com (Postfix) with ESMTP id 118897F94D; Fri, 24 Mar 2017 12:49:24 +0000 (UTC) To: Jordan Justen References: <20170314233246.17864-1-lersek@redhat.com> <148969026858.27582.5519307275216644796@jljusten-skl.jf.intel.com> <319ff8f1-4e99-f977-5c2e-75509a222706@redhat.com> <149034108785.2439.14733776608486243050@jljusten-skl> Cc: edk2-devel-01 From: Laszlo Ersek Message-ID: <4b06b195-d8fb-a33e-3492-74fe396ebf6e@redhat.com> Date: Fri, 24 Mar 2017 13:49:22 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <149034108785.2439.14733776608486243050@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Fri, 24 Mar 2017 12:49:26 +0000 (UTC) Subject: Re: [PATCH 00/14] OvmfPkg: add the Variable PEIM, defragment the UEFI memmap X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Mar 2017 12:49:26 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 >>>>> 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 >>>>> . >>>>> >>>>> Repo: https://github.com/lersek/edk2.git >>>>> Branch: memmap_defrag >>>>> >>>>> CC: Jordan Justen >>>>> >>>>> 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 >>> >>