Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformPei: consider AP stacks for pei memory cap

["Laszlo Ersek" <lersek@redhat.com>]

On 1/31/24 12:59, Gerd Hoffmann wrote:
> Needed to avoid running out of memory when booting
> with a large (~2048) number of vcpus.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

This idea seems justified; it resembles commit 45d870815156 ("OvmfPkg/PlatformPei: rebase and resize the permanent PEI memory for S3", 2016-07-15), which also operates with the (PcdCpuMaxLogicalProcessorNumber * PcdCpuApStackSize) product. OK.

> 
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 493cb1fbeb01..338798b54171 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -187,6 +187,8 @@ GetPeiMemoryCap (
>    UINT32   Pml4Entries;
>    UINT32   PdpEntries;
>    UINTN    TotalPages;
> +  UINTN    ApStacks;
> +  UINTN    MemoryCap;
>  
>    //
>    // If DXE is 32-bit, then just return the traditional 64 MB cap.
> @@ -234,12 +236,18 @@ GetPeiMemoryCap (
>                 (PdpEntries + 1) * Pml4Entries + 1;
>    ASSERT (TotalPages <= 0x40201);
>  
> +  ApStacks  = PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber * PcdGet32 (PcdCpuApStackSize);

Doing this here is safe; we have the following call tree:

  InitializePlatform()                    [OvmfPkg/PlatformPei/Platform.c]
    MaxCpuCountInitialization()           [OvmfPkg/PlatformPei/Platform.c]
      PlatformMaxCpuCountInitialization() [OvmfPkg/Library/PlatformInitLib/Platform.c]
    PublishPeiMemory()                    [OvmfPkg/PlatformPei/MemDetect.c]
      GetPeiMemoryCap()                   [OvmfPkg/PlatformPei/MemDetect.c]

Meaning that "PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber" will be set here. OK.

> +  MemoryCap = EFI_PAGES_TO_SIZE (TotalPages) + ApStacks + SIZE_64MB;

(1) The comment at the bottom is now both misplaced, and out of date.  First, we perform the addition here, not down there. Second, we now have an extra addend, which may even dominate the cap.

> +  if (MemoryCap > MAX_UINT32) {
> +    MemoryCap = MAX_UINT32;
> +  }
> +

(2) This doesn't look good, for two reasons.

First, if UINTN is UINT32, then we're too late to check. (If it's intentional that the code be dead on IA32, then that should be documented.)

Second, while returning MAX_UINT32 from this function is safe, it's also obscure. After the call site, we've got

    MemoryBase = PlatformInfoHob->S3Supported && PlatformInfoHob->SmmSmramRequire ?
                 PcdGet32 (PcdOvmfDecompressionScratchEnd) :
                 PcdGet32 (PcdOvmfDxeMemFvBase) + PcdGet32 (PcdOvmfDxeMemFvSize);
    MemorySize = LowerMemorySize - MemoryBase;
    if (MemorySize > PeiMemoryCap) {
      MemoryBase = LowerMemorySize - PeiMemoryCap;
      MemorySize = PeiMemoryCap;
    }

If PeiMemoryCap is huge (e.g., it approximates 4GB), then (MemorySize > PeiMemoryCap) will evaluate to FALSE, and we'll just give most of the low RAM to PEI. This effectively means that permanent PEI RAM is "uncapped" (while remaining 32-bit only, of course). Whether that will cause a boot failure later on, or not, remains to be seen, but as far as this code is affected, it's not a problem if GetPeiMemoryCap() returns MAX_UINT32. *However*, this argument should be captured in a comment.

>    //
>    // Add 64 MB for miscellaneous allocations. Note that for
>    // PhysMemAddressWidth values close to 36, the cap will actually be
>    // dominated by this increment.
>    //
> -  return (UINT32)(EFI_PAGES_TO_SIZE (TotalPages) + SIZE_64MB);
> +  return (UINT32)(MemoryCap);
>  }
>  
>  /**

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114902): https://edk2.groups.io/g/devel/message/114902
Mute This Topic: https://groups.io/mt/104073297/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-