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
>
> 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.
[...]
> 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?
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
