On Thu, Jan 9, 2014 at 10:57 AM, Laszlo Ersek <[email protected]> wrote: > comments below > > On 01/09/14 01:44, Jordan Justen wrote: >> By splitting the PEI and DXE phases into separate FVs, >> we can only reserve the PEI FV for ACPI S3 support. >> This should save about 7MB. >> >> Unfortunately, this all has to happen in a single commit. >> >> DEC: >> * Remove PcdOvmfMemFv(Base|Size) >> * Add PcdOvmfPeiMemFv(Base|Size) >> * Add PcdOvmfDxeMemFv(Base|Size) >> >> FDF: >> * Add new PEIFV. Move PEI modules here. >> * Remove MAINFV >> * Add PEIFV and DXEFV into FVMAIN_COMPACT >> - They are added as 2 sections of a file, and compressed >> together so they should retain good compression >> * PcdOvmf(Pei|Dxe)MemFv(Base|Size) are set > > I propose to add the following paragraphs to the commit message (can be > done when committing, or not at all): > > Before: > > FD.OVMF > FV.FVMAIN_COMPACT uncompressed > container compressed with LzmaCompress > FV.MAINFV uncompressed > individual PEI modules uncompressed > container compressed with PI_NONE > FV.DXEFV uncompressed > individual DXE modules uncompressed > FV.SECFV uncompressed > > After: > > FD.OVMF > FV.FVMAIN_COMPACT uncompressed > container compressed with LzmaCompress > FV.PEIFV uncompressed > individual PEI modules uncompressed > FV.DXEFV uncompressed > individual DXE modules uncompressed > FV.SECFV uncompressed
Sounds good. >> >> SEC: >> * Find both the PEI and DXE FVs after decompression. >> - Copy them separately to their memory locations. >> >> Platform PEI driver: >> * Fv.c: Publish both FVs as appropriate >> * MemDetect.c: PcdOvmfMemFv(Base|Size) => >> PcdOvmfDxeMemFv(Base|Size) >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jordan Justen <[email protected]> >> --- >> OvmfPkg/OvmfPkg.dec | 6 ++-- >> OvmfPkg/OvmfPkgIa32.fdf | 23 ++++++------ >> OvmfPkg/OvmfPkgIa32X64.fdf | 23 ++++++------ >> OvmfPkg/OvmfPkgX64.fdf | 23 ++++++------ >> OvmfPkg/PlatformPei/Fv.c | 46 ++++++++++++++++-------- >> OvmfPkg/PlatformPei/MemDetect.c | 4 +-- >> OvmfPkg/PlatformPei/PlatformPei.inf | 8 +++-- >> OvmfPkg/Sec/SecMain.c | 70 >> ++++++++++++++++++++++++++----------- >> OvmfPkg/Sec/SecMain.inf | 8 +++-- >> 9 files changed, 130 insertions(+), 81 deletions(-) >> >> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec >> index 0e33b3b..034ccd8 100644 >> --- a/OvmfPkg/OvmfPkg.dec >> +++ b/OvmfPkg/OvmfPkg.dec >> @@ -48,8 +48,10 @@ >> gBlockMmioProtocolGuid = {0x6b558ce3, 0x69e5, 0x4c67, {0xa6, >> 0x34, 0xf7, 0xfe, 0x72, 0xad, 0xbe, 0x84}} >> >> [PcdsFixedAtBuild] >> - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfMemFvBase|0x0|UINT32|0 >> - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfMemFvSize|0x0|UINT32|1 >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|0x0|UINT32|0 >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize|0x0|UINT32|1 >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|0x0|UINT32|0x15 >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize|0x0|UINT32|0x16 >> >> ## This flag is used to control the destination port for >> PlatformDebugLibIoPort >> gUefiOvmfPkgTokenSpaceGuid.PcdDebugIoPort|0x402|UINT16|4 >> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf >> index 210abde..ac2c756 100644 >> --- a/OvmfPkg/OvmfPkgIa32.fdf >> +++ b/OvmfPkg/OvmfPkgIa32.fdf >> @@ -141,9 +141,13 @@ >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.P >> 0x010000|0x008000 >> >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize >> >> -0x020000|0x7E0000 >> -gUefiOvmfPkgTokenSpaceGuid.PcdOvmfMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfMemFvSize >> -FV = MAINFV >> +0x020000|0x0E0000 >> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize >> +FV = PEIFV >> + >> +0x100000|0x700000 >> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize >> +FV = DXEFV >> >> >> ################################################################################ >> > > OK, 896 KB for the PEI code after decompression, and 7 MB for DXE. > >> @@ -170,14 +174,14 @@ READ_LOCK_STATUS = TRUE >> # SEC Phase modules >> # >> # The code in this FV handles the initial firmware startup, and >> -# decompresses the MAINFV which handles the majority of the boot sequence. >> +# decompresses the PEI and DXE FVs which handles the rest of the boot >> sequence. >> # >> INF OvmfPkg/Sec/SecMain.inf >> >> INF RuleOverride=RESET_VECTOR >> UefiCpuPkg/ResetVector/Vtf0/Bin/ResetVector.inf >> >> >> ################################################################################ >> -[FV.MAINFV] >> +[FV.PEIFV] >> BlockSize = 0x10000 >> FvAlignment = 16 >> ERASE_POLARITY = 1 > > There's also a comment block here a bit lower down, saying "Files to be > placed in MAIN FV". I guess you didn't find it because of the extra > space between MAIN and FV. This comment block looks like needing an > update too. Ok, I'll fix that. > [...] > > >> diff --git a/OvmfPkg/PlatformPei/Fv.c b/OvmfPkg/PlatformPei/Fv.c >> index f389e27..fbdb597 100644 >> --- a/OvmfPkg/PlatformPei/Fv.c >> +++ b/OvmfPkg/PlatformPei/Fv.c >> @@ -1,7 +1,7 @@ >> /** @file >> Build FV related hobs for platform. >> >> - Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.<BR> >> + Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> >> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD >> License >> which accompanies this distribution. The full text of the license may be >> found at >> @@ -20,10 +20,8 @@ >> >> >> /** >> - Perform a call-back into the SEC simulator to get address of the Firmware >> Hub >> - >> - @param FfsHeader Ffs Header availible to every PEIM >> - @param PeiServices General purpose services available to every PEIM. >> + Publish PEI & DXE (Decompressed) Memory based FVs to let PEI >> + and DXE know about them. >> >> @retval EFI_SUCCESS Platform PEI FVs were initialized successfully. >> >> @@ -33,26 +31,44 @@ PeiFvInitialization ( >> VOID >> ) >> { >> - DEBUG ((EFI_D_ERROR, "Platform PEI Firmware Volume Initialization\n")); >> + DEBUG ((EFI_D_INFO, "Platform PEI Firmware Volume Initialization\n")); >> >> - DEBUG ( >> - (EFI_D_ERROR, "Firmware Volume HOB: 0x%x 0x%x\n", >> - PcdGet32 (PcdOvmfMemFvBase), >> - PcdGet32 (PcdOvmfMemFvSize) >> - ) >> + // >> + // Create a memory allocation HOB for the PEI FV. >> + // >> + // This is marked as ACPI NVS so it will still be available on S3 resume. >> + // >> + BuildMemoryAllocationHob ( >> + PcdGet32 (PcdOvmfPeiMemFvBase), >> + PcdGet32 (PcdOvmfPeiMemFvSize), >> + EfiACPIMemoryNVS >> ); >> >> - BuildFvHob (PcdGet32 (PcdOvmfMemFvBase), PcdGet32 (PcdOvmfMemFvSize)); >> + // >> + // Let DXE know about the DXE FV >> + // >> + BuildFvHob (PcdGet32 (PcdOvmfDxeMemFvBase), PcdGet32 >> (PcdOvmfDxeMemFvSize)); >> >> // >> - // Create a memory allocation HOB. >> + // Create a memory allocation HOB for the DXE FV. >> // >> BuildMemoryAllocationHob ( >> - PcdGet32 (PcdOvmfMemFvBase), >> - PcdGet32 (PcdOvmfMemFvSize), >> + PcdGet32 (PcdOvmfDxeMemFvBase), >> + PcdGet32 (PcdOvmfDxeMemFvSize), >> EfiBootServicesData >> ); >> >> + // >> + // Let PEI know about the DXE FV so it can find the DXE Core >> + // >> + PeiServicesInstallFvInfoPpi ( >> + NULL, >> + (VOID *)(UINTN) PcdGet32 (PcdOvmfDxeMemFvBase), >> + PcdGet32 (PcdOvmfDxeMemFvSize), >> + NULL, >> + NULL >> + ); >> + > > Hm. I wonder why we didn't need this before. > > - before: BuildFvHob() covering entire FV, no FvInfoPpi > - after: BuildFvHob() covering DXE, FvInfoPpi also covering DXE > > Can you provide the source code or PI spec reference requiring the > FvInfo PPI? I vaguely remembered (and now very superficially "confirmed" > by grepping) that FindNextCoreFvHandle() in > "MdeModulePkg/Core/Pei/FwVol/FwVol.c" calls > PeiServicesInstallFvInfoPpi(). The FV hob should suffice, no? I couldn't find anything spec wise to refer you to... PEI Core picks up the boot firmware volume sent in from SEC and will then find items from that FV. That is why we didn't need the FvInfo previously. For additional FVs, MdeModulePkg/Core/Pei/FwVol/FwVol.c seems to get notified when FvInfo is installed, and then discovers new FVs. If I enable gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport, then the HOB seems to be enough. We don't want to enable this. So, PEI needs the BFV from SEC for the first PEI FV + FvInfo for additional PEI FVs. DXE needs the HOB. This hand-waving "explanation" enough for an r-b? :) -Jordan > The memory allocation HOBs seem OK. > >> return EFI_SUCCESS; >> } >> >> diff --git a/OvmfPkg/PlatformPei/MemDetect.c >> b/OvmfPkg/PlatformPei/MemDetect.c >> index a1de762..bd57b77 100644 >> --- a/OvmfPkg/PlatformPei/MemDetect.c >> +++ b/OvmfPkg/PlatformPei/MemDetect.c >> @@ -1,7 +1,7 @@ >> /**@file >> Memory Detection for Virtual Machines. >> >> - Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.<BR> >> + Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR> >> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD >> License >> which accompanies this distribution. The full text of the license may be >> found at >> @@ -104,7 +104,7 @@ PublishPeiMemory ( >> // >> // Determine the range of memory to use during PEI >> // >> - MemoryBase = PcdGet32 (PcdOvmfMemFvBase) + PcdGet32 (PcdOvmfMemFvSize); >> + MemoryBase = PcdGet32 (PcdOvmfDxeMemFvBase) + PcdGet32 >> (PcdOvmfDxeMemFvSize); >> MemorySize = LowerMemorySize - MemoryBase; >> if (MemorySize > SIZE_64MB) { >> MemoryBase = LowerMemorySize - SIZE_64MB; > > Seems OK. > > [...] > >> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c >> index 902ac85..21c2a36 100644 >> --- a/OvmfPkg/Sec/SecMain.c >> +++ b/OvmfPkg/Sec/SecMain.c >> @@ -319,8 +319,8 @@ FindFfsFileAndSection ( >> >> **/ >> EFI_STATUS >> -DecompressGuidedFv ( >> - IN OUT EFI_FIRMWARE_VOLUME_HEADER **Fv >> +DecompressMemFvs ( >> + IN OUT EFI_FIRMWARE_VOLUME_HEADER **PeiFv >> ) >> { >> EFI_STATUS Status; > > The leading comment should be updated perhaps? On output, *PeiFv points > to the decompressed PEI PV (not main FV). > > >> @@ -331,13 +331,14 @@ DecompressGuidedFv ( >> UINT32 AuthenticationStatus; >> VOID *OutputBuffer; >> VOID *ScratchBuffer; >> - EFI_FIRMWARE_VOLUME_IMAGE_SECTION *NewFvSection; >> - EFI_FIRMWARE_VOLUME_HEADER *NewFv; >> + EFI_FIRMWARE_VOLUME_IMAGE_SECTION *FvSection; >> + EFI_FIRMWARE_VOLUME_HEADER *PeiMemFv; >> + EFI_FIRMWARE_VOLUME_HEADER *DxeMemFv; >> >> - NewFvSection = (EFI_FIRMWARE_VOLUME_IMAGE_SECTION*) NULL; >> + FvSection = (EFI_FIRMWARE_VOLUME_IMAGE_SECTION*) NULL; >> >> Status = FindFfsFileAndSection ( >> - *Fv, >> + *PeiFv, >> EFI_FV_FILETYPE_FIRMWARE_VOLUME_IMAGE, >> EFI_SECTION_GUID_DEFINED, >> (EFI_COMMON_SECTION_HEADER**) &Section >> @@ -358,8 +359,7 @@ DecompressGuidedFv ( >> return Status; >> } >> >> - //PcdGet32 (PcdOvmfMemFvBase), PcdGet32 (PcdOvmfMemFvSize) >> - OutputBuffer = (VOID*) ((UINT8*)(UINTN) PcdGet32 (PcdOvmfMemFvBase) + >> SIZE_1MB); >> + OutputBuffer = (VOID*) ((UINT8*)(UINTN) PcdGet32 (PcdOvmfDxeMemFvBase) + >> SIZE_1MB); >> ScratchBuffer = ALIGN_POINTER ((UINT8*) OutputBuffer + OutputBufferSize, >> SIZE_1MB); >> Status = ExtractGuidedSectionDecode ( >> Section, > > I tried to understand this code before, with the output buffer and the > scratch buffer. See the diagram in > > http://thread.gmane.org/gmane.comp.bios.tianocore.devel/5728/focus=5727 > > After this patch, we place the output buffer at 0x800000 + 0x100000 + > SIZE_1MB, ie. at 10 MB, 1 MB higher than earlier. > > OutputBufferSize ended up 8 MB + 80 bytes when I last checked. Since our > compressed blob includes the same binaries as before, and the cumulative > size of PEIFV and DXEFV after decompression is 8MB-128KB, I think > OutputBufferSize will border on (or be slightly above) 8MB again. > > Which means that the decompression might overwrite memory up to 18MB, > maybe higher. > > Consequently, ScratchBuffer can start at 18MB (same as before), or > possibly 19MB. ScratchBufferSize used to be 64K. We could be overwriting > memory up to 19MB + 64K. > > >> @@ -372,27 +372,57 @@ DecompressGuidedFv ( >> return Status; >> } >> >> - Status = FindFfsSectionInSections ( >> + Status = FindFfsSectionInstance ( >> OutputBuffer, >> OutputBufferSize, >> EFI_SECTION_FIRMWARE_VOLUME_IMAGE, >> - (EFI_COMMON_SECTION_HEADER**) &NewFvSection >> + 0, >> + (EFI_COMMON_SECTION_HEADER**) &FvSection >> ); >> if (EFI_ERROR (Status)) { >> - DEBUG ((EFI_D_ERROR, "Unable to find FV image in extracted data\n")); >> + DEBUG ((EFI_D_ERROR, "Unable to find PEI FV section\n")); >> return Status; >> } >> >> - NewFv = (EFI_FIRMWARE_VOLUME_HEADER*)(UINTN) PcdGet32 (PcdOvmfMemFvBase); >> - CopyMem (NewFv, (VOID*) (NewFvSection + 1), PcdGet32 (PcdOvmfMemFvSize)); >> + ASSERT (SECTION_SIZE (FvSection) == >> + (PcdGet32 (PcdOvmfPeiMemFvSize) + sizeof (*FvSection))); >> + ASSERT (FvSection->Type == EFI_SECTION_FIRMWARE_VOLUME_IMAGE); >> >> - if (NewFv->Signature != EFI_FVH_SIGNATURE) { >> - DEBUG ((EFI_D_ERROR, "Extracted FV at %p does not have FV header >> signature\n", NewFv)); >> + PeiMemFv = (EFI_FIRMWARE_VOLUME_HEADER*)(UINTN) PcdGet32 >> (PcdOvmfPeiMemFvBase); >> + CopyMem (PeiMemFv, (VOID*) (FvSection + 1), PcdGet32 >> (PcdOvmfPeiMemFvSize)); > > Seems OK. The PEI FV ends up at 0x820000..0x900000. OutputBuffer starts > at 0xA00000, so it remains intact. > >> + >> + if (PeiMemFv->Signature != EFI_FVH_SIGNATURE) { >> + DEBUG ((EFI_D_ERROR, "Extracted FV at %p does not have FV header >> signature\n", PeiMemFv)); >> + CpuDeadLoop (); >> + return EFI_VOLUME_CORRUPTED; >> + } >> + >> + Status = FindFfsSectionInstance ( >> + OutputBuffer, >> + OutputBufferSize, >> + EFI_SECTION_FIRMWARE_VOLUME_IMAGE, >> + 1, >> + (EFI_COMMON_SECTION_HEADER**) &FvSection >> + ); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((EFI_D_ERROR, "Unable to find DXE FV section\n")); >> + return Status; >> + } >> + >> + ASSERT (FvSection->Type == EFI_SECTION_FIRMWARE_VOLUME_IMAGE); >> + ASSERT (SECTION_SIZE (FvSection) == >> + (PcdGet32 (PcdOvmfDxeMemFvSize) + sizeof (*FvSection))); >> + >> + DxeMemFv = (EFI_FIRMWARE_VOLUME_HEADER*)(UINTN) PcdGet32 >> (PcdOvmfDxeMemFvBase); >> + CopyMem (DxeMemFv, (VOID*) (FvSection + 1), PcdGet32 >> (PcdOvmfDxeMemFvSize)); > > Copies from somewhere above 0xA00000 to 0x900000, overwriting part of > OutputBuffer too (CopyMem() handles overlapping copies), but that should > be OK because we're done with OutputBuffer. > >> + >> + if (DxeMemFv->Signature != EFI_FVH_SIGNATURE) { >> + DEBUG ((EFI_D_ERROR, "Extracted FV at %p does not have FV header >> signature\n", DxeMemFv)); >> CpuDeadLoop (); >> return EFI_VOLUME_CORRUPTED; >> } >> >> - *Fv = NewFv; >> + *PeiFv = PeiMemFv; >> return EFI_SUCCESS; >> } >> > > OK. > > The memory range 16MB..(19MB + 64K) is never reserved, but as far as I > understand (v4 15/26), we won't decompress anything during resume. We > won't need DXE, and the PEI firmware will remain intact due to covering > it with an ACPI NVS memory allocation HOB. > > Nothing is changed in the PEI firmware volume (the 0x820000..0x900000 > range) during first run, right? I mean no static variables are stored > there, executables are only loaded from there to temporary / permanent > RAM and relocated, right? > > >> @@ -452,17 +482,17 @@ FindPeiCoreImageBaseInFv ( >> **/ >> VOID >> FindPeiCoreImageBase ( >> - IN OUT EFI_FIRMWARE_VOLUME_HEADER **BootFv, >> + IN OUT EFI_FIRMWARE_VOLUME_HEADER **PeiFv, >> OUT EFI_PHYSICAL_ADDRESS *PeiCoreImageBase >> ) >> { >> *PeiCoreImageBase = 0; >> >> - FindMainFv (BootFv); >> + FindMainFv (PeiFv); >> >> - DecompressGuidedFv (BootFv); >> + DecompressMemFvs (PeiFv); >> >> - FindPeiCoreImageBaseInFv (*BootFv, PeiCoreImageBase); >> + FindPeiCoreImageBaseInFv (*PeiFv, PeiCoreImageBase); >> } >> >> /** > > This routine seems to key off from the "BootFv" param of > SecCoreStartupWithStack(), but its output is now PEI specific, so the > renaming should be in order. > > [...] > > 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 ------------------------------------------------------------------------------ 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
