comments below
On 01/09/14 01:44, Jordan Justen wrote:
> This 32k section of RAM will be declared to the PEI Core on
> S3 resume to allow memory allocations during S3 resume PEI.
>
> If the boot mode is BOOT_ON_S3_RESUME, then we publish
> the pre-reserved PcdS3AcpiReservedMemory range to PEI.
>
> If the boot mode is not BOOT_ON_S3_RESUME, then we reserve
> this range as ACPI NVS so the OS will not use it.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen <[email protected]>
> ---
> OvmfPkg/OvmfPkg.dec | 1 +
> OvmfPkg/OvmfPkgIa32.fdf | 3 +++
> OvmfPkg/OvmfPkgIa32X64.fdf | 3 +++
> OvmfPkg/OvmfPkgX64.fdf | 3 +++
> OvmfPkg/PlatformPei/MemDetect.c | 36 ++++++++++++++++++++++++++----------
> OvmfPkg/PlatformPei/PlatformPei.inf | 5 ++++-
> 6 files changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 034ccd8..8a52bb1 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -84,6 +84,7 @@
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize|0x0|UINT32|0x12
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|0x0|UINT32|0x13
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize|0x0|UINT32|0x14
> + gUefiOvmfPkgTokenSpaceGuid.PcdS3AcpiReservedMemoryBase|0x0|UINT32|0x17
>
> [PcdsDynamic, PcdsDynamicEx]
> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index ac2c756..2c57376 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -141,6 +141,9 @@
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.P
> 0x010000|0x008000
>
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>
> +0x018000|0x008000
> +gUefiOvmfPkgTokenSpaceGuid.PcdS3AcpiReservedMemoryBase|gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdS3AcpiReservedMemorySize
> +
> 0x020000|0x0E0000
>
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
> FV = PEIFV
Interesting to see that
"IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec" already declares
PcdS3AcpiReservedMemorySize.
The PCD affects
"IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiVariableThunkPlatform.c",
which would allocate a block of this size manually.
As far as I understand, even in v4 of the series, OVMF's copy of
AcpiS3SaveDxe will remove that file, so it should be fine; we can freely
use the PCD for its original purpose without unintended hooks.
[...]
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index dcfe952..bf67d7c 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -99,16 +99,21 @@ PublishPeiMemory (
> UINT64 MemorySize;
> UINT64 LowerMemorySize;
>
> - LowerMemorySize = GetSystemMemorySizeBelow4gb ();
> -
> - //
> - // Determine the range of memory to use during PEI
> - //
> - MemoryBase = PcdGet32 (PcdOvmfDxeMemFvBase) + PcdGet32
> (PcdOvmfDxeMemFvSize);
> - MemorySize = LowerMemorySize - MemoryBase;
> - if (MemorySize > SIZE_64MB) {
> - MemoryBase = LowerMemorySize - SIZE_64MB;
> - MemorySize = SIZE_64MB;
> + if (mBootMode == BOOT_ON_S3_RESUME) {
> + MemoryBase = PcdGet32 (PcdS3AcpiReservedMemoryBase);
> + MemorySize = PcdGet32 (PcdS3AcpiReservedMemorySize);
> + } else {
> + LowerMemorySize = GetSystemMemorySizeBelow4gb ();
> +
> + //
> + // Determine the range of memory to use during PEI
> + //
> + MemoryBase = PcdGet32 (PcdOvmfDxeMemFvBase) + PcdGet32
> (PcdOvmfDxeMemFvSize);
> + MemorySize = LowerMemorySize - MemoryBase;
> + if (MemorySize > SIZE_64MB) {
> + MemoryBase = LowerMemorySize - SIZE_64MB;
> + MemorySize = SIZE_64MB;
> + }
> }
>
> //
> @@ -158,6 +163,17 @@ MemDetect (
> MtrrSetMemoryAttribute (BASE_4GB, UpperMemorySize, CacheWriteBack);
> }
>
> + if (mBootMode != BOOT_ON_S3_RESUME) {
> + //
> + // This is the memory range that will be used for PEI on S3 resume
> + //
> + BuildMemoryAllocationHob (
> + (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdS3AcpiReservedMemoryBase),
> + (UINT64)(UINTN) PcdGet32 (PcdS3AcpiReservedMemorySize),
> + EfiACPIMemoryNVS
> + );
> + }
> +
> return LowerMemorySize;
> }
>
I think the HOB is correct, but the call is made in the wrong place. Xen
never calls MemDetect().
Also I'd prefer to keep all memalloc HOBs in one place, probably
PeiFvInitialization(), so that we can more easily validate that all
ranges are covered. (Anyway I don't insist on this.)
Thus far we have
0x000000|0x006000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
- to be covered in v4 17/26
0x010000|0x008000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
- to be covered in v4 16/26
0x020000|0x0E0000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
- covered in v4 08/16
0x100000|0x700000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
- covered in v4 08/16 (as EfiBootServicesData)
and this patch does cover the new range, just not under Xen. (Is it your
intent perhaps?)
[...]
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
