public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, kraxel@redhat.com
Cc: Oliver Steffen <osteffen@redhat.com>,
	Jiewen Yao <jiewen.yao@intel.com>,
	Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformPei: consider AP stacks for pei memory cap
Date: Wed, 31 Jan 2024 15:08:54 +0100	[thread overview]
Message-ID: <65a072d8-9f2c-3487-dc26-9a72e9eccc3d@redhat.com> (raw)
In-Reply-To: <20240131120000.358090-2-kraxel@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]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2024-01-31 14:09 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-31 11:59 [edk2-devel] [PATCH 0/3] OvmfPkg/PlatformPei: scaleability fixes for GetPeiMemoryCap() Gerd Hoffmann
2024-01-31 11:59 ` [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformPei: consider AP stacks for pei memory cap Gerd Hoffmann
2024-01-31 14:08   ` Laszlo Ersek [this message]
2024-01-31 14:55     ` Gerd Hoffmann
2024-01-31 19:29       ` Laszlo Ersek
2024-01-31 11:59 ` [edk2-devel] [PATCH 2/3] OvmfPkg/PlatformPei: rewrite page table calculation Gerd Hoffmann
2024-01-31 15:13   ` Laszlo Ersek
2024-01-31 15:21     ` Laszlo Ersek
2024-01-31 16:28     ` Gerd Hoffmann
2024-02-01 21:04       ` Laszlo Ersek
2024-01-31 12:00 ` [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: log pei memory cap details Gerd Hoffmann
2024-01-31 15:38   ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=65a072d8-9f2c-3487-dc26-9a72e9eccc3d@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox