comments at bottom On 01/09/14 01:44, Jordan Justen wrote: > We will not be running DXE on S3 resume, so we don't > need to do these initialization items: > * Reserve EMU Variable memory range > * Declare Firmware volumes > * Add memory HOBs > * MiscInitialization: > - Disable A20 Mask > - Build CPU hob > - Set Power Management Base Address register > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jordan Justen <[email protected]> > --- > OvmfPkg/PlatformPei/MemDetect.c | 16 ++++++++++------ > OvmfPkg/PlatformPei/Platform.c | 18 ++++++++++-------- > 2 files changed, 20 insertions(+), 14 deletions(-) > > diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c > index bf67d7c..bd666c5 100644 > --- a/OvmfPkg/PlatformPei/MemDetect.c > +++ b/OvmfPkg/PlatformPei/MemDetect.c > @@ -147,18 +147,22 @@ MemDetect ( > LowerMemorySize = GetSystemMemorySizeBelow4gb (); > UpperMemorySize = GetSystemMemorySizeAbove4gb (); > > - // > - // Create memory HOBs > - // > - AddMemoryRangeHob (BASE_1MB, LowerMemorySize); > - AddMemoryRangeHob (0, BASE_512KB + BASE_128KB); > + if (mBootMode != BOOT_ON_S3_RESUME) { > + // > + // Create memory HOBs > + // > + AddMemoryRangeHob (BASE_1MB, LowerMemorySize); > + AddMemoryRangeHob (0, BASE_512KB + BASE_128KB); > + } > > MtrrSetMemoryAttribute (BASE_1MB, LowerMemorySize - BASE_1MB, > CacheWriteBack); > > MtrrSetMemoryAttribute (0, BASE_512KB + BASE_128KB, CacheWriteBack); > > if (UpperMemorySize != 0) { > - AddUntestedMemoryBaseSizeHob (BASE_4GB, UpperMemorySize); > + if (mBootMode != BOOT_ON_S3_RESUME) { > + AddUntestedMemoryBaseSizeHob (BASE_4GB, UpperMemorySize); > + } > > MtrrSetMemoryAttribute (BASE_4GB, UpperMemorySize, CacheWriteBack); > } > diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c > index 6a40616..b28f6d6 100644 > --- a/OvmfPkg/PlatformPei/Platform.c > +++ b/OvmfPkg/PlatformPei/Platform.c > @@ -443,17 +443,19 @@ InitializePlatform ( > InitializeXen (XenLeaf); > } > > - ReserveEmuVariableNvStore (); > + if (mBootMode != BOOT_ON_S3_RESUME) { > + ReserveEmuVariableNvStore (); > > - PeiFvInitialization (); > + PeiFvInitialization (); > > - if (XenLeaf != 0) { > - XenMemMapInitialization (); > - } else { > - MemMapInitialization (TopOfMemory); > - } > + if (XenLeaf != 0) { > + XenMemMapInitialization (); > + } else { > + MemMapInitialization (TopOfMemory); > + } > > - MiscInitialization (); > + MiscInitialization (); > + } > > return EFI_SUCCESS; > } >
My approach to producing HOBs on the resume path was that I couldn't guarantee that PEI did not look into its "own" HOBs. Since they should have consumed little memory, I rather left them in place. But I'm easy to convince in this regard. What I am worried about is: (a) XenMemMapInitialization() also contains calls to MtrrSetMemoryAttribute(). May not be too important to perform those on S3 resume, I'm not sure, but you're keeping similar calls intact in MemDetect(). (b) MiscInitialization(). The firmware is required to "restore CPU and chipset state". I'm not really sure about the A20 mask. The CPU hob is again the HOB story, so meh. But the PMBA is important to restore. See this recent discussion on qemu-devel: http://lists.nongnu.org/archive/html/qemu-devel/2013-12/msg03550.html In a nutshell, qemu used to restore (actually, keep) a lot of state across suspend/resume, including the PMBA. From qemu commit commit c046e8c4a26c902ca1b4f5bdf668a2da6bc75f54 Author: Michael S. Tsirkin <[email protected]> Date: Wed Sep 11 13:33:31 2013 +0300 piix4: disable io on reset onwards (released in v1.7.0) the PMBA is cleared however. In the discussion Paolo argued that this is the correct behavior, mandated by the PIIX spec. Commit c046e8c4 broke all kinds of guests running on SeaBIOS (because PM configuration was cleared by qemu and not restored by the firmware), http://lists.nongnu.org/archive/html/qemu-devel/2013-12/msg03431.html but I didn't notice anything on top of OVMF, and I decided that this was thanks to MiscInitialization(). As far as I understand, a patch for SeaBIOS was subsequently submitted in order to restore / re-configure the PMBA on S3 resume: http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/7432 If you've tested this series on top of v1.7.0+, then my analysis must have been wrong (and then I don't understand the difference with SeaBIOS), but right now I think that we need the PMBA reinit during resume. Thanks, Laszlo ------------------------------------------------------------------------------ CenturyLink Cloud: The Leader in Enterprise Cloud Services. Learn Why More Businesses Are Choosing CenturyLink Cloud For Critical Workloads, Development Environments & Everything In Between. Get a Quote or Start a Free Trial Today. http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
