From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 4D7BE8039D for ; Fri, 24 Mar 2017 00:38:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1490341089; x=1521877089; h=mime-version:content-transfer-encoding:to:message-id: from:in-reply-to:cc:references:subject:date; bh=eNfLgdiQuCOpyLnHYF8h7jK42M7xmZSzkSucxQbx4a8=; b=ILitUZW2heKVp0Rs62bf87HTfczmvR2t5WROoeny6TCeRKVtNe6uHQVG cmcG6Mg4QuqiJSgYCT/kk2SWCOC4kA==; Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Mar 2017 00:38:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,213,1486454400"; d="scan'208";a="838052392" Received: from junghyun-mobl.amr.corp.intel.com (HELO localhost) ([10.252.131.12]) by FMSMGA003.fm.intel.com with ESMTP; 24 Mar 2017 00:38:08 -0700 MIME-Version: 1.0 To: Laszlo Ersek Message-ID: <149034108785.2439.14733776608486243050@jljusten-skl> From: Jordan Justen In-Reply-To: Cc: edk2-devel-01 References: <20170314233246.17864-1-lersek@redhat.com> <148969026858.27582.5519307275216644796@jljusten-skl.jf.intel.com> <319ff8f1-4e99-f977-5c2e-75509a222706@redhat.com> User-Agent: alot/0.5.1 Date: Fri, 24 Mar 2017 00:38:08 -0700 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 07:38:09 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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 remov= ed. > >>> > >>> The benefits of the patch set are automatically enabled for SMM_REQUI= RE > >>> builds. > >>> > >>> For non-SMM builds (which are the default), another build flag is bei= ng > >>> 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 f= rom > >>> the Variable PEIM and the dependent UEFI memmap defragmentation, > >>> MEM_VARSTORE_EMU_ENABLE has to be *disabled*. There are two reasons f= or > >>> this: > >>> > >>> - The PEI phase FTW and Variable modules need immediate (at-startup) > >>> access to the variable store, but the reserved memory based emulati= on > >>> 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 some= how. > > = > > (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 reserv= ed > >>> 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 (=3D=3D implement a long-term maximum search) over several = cold > > boots (=3D 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=3DFALSE > > = > > 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 cours= e.) > = > 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 mem= map > >>> 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 comm= it > >>> b963ec494c48 ("OvmfPkg: QemuFlashFvbServicesRuntimeDxe: adhere to -D > >>> SMM_REQUIRE", 2015-11-30). > >>> > >>> * -D MEM_VARSTORE_EMU_ENABLE=3DFALSE: eliminates the reserved memory / > >>> NvVars based variable emulation, turns pflash into a hard requireme= nt, > >>> 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 varsto= re > >>> (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 s= et; > >>> 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 PcdMemVarstoreEmuEnab= le > >>> 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.in= f | 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/FlashNvS= torageAddressLib.inf > >>> create mode 100644 OvmfPkg/Library/FlashNvStorageAddressLib/FlashNvS= torageAddressLib.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 > > = >=20