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

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

On 1/31/24 15:55, Gerd Hoffmann wrote:
>   Hi,
>
>>> +  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.)
>
> IA32 has an early exit in this function.
>
> Looking again I see this applies to 32-bit DXE only, so with mixed
> 32-bit PEI / 64-bit DXE we can still land here.  Hmm, so yes, you
> have a point here.
>
> In practice we don't come even close to MAX_UINT32.  With 4096 vcpus
> the amount of memory needed for the AP stacks sums up to 128 MB.  The
> page table memory wouldn't grow that big too because OVMF will use at
> most 1 TB of address space if gigabyte pages are not available (to
> limit page table memory and boot time needed to set them all up).
>
>> 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;
>>     }
>
> MemorySize is UINT64.  So I guess the easiest would be to just switch
> MemoryCap variable and GetPeiMemoryCap() return value to UINT64 too.
> Avoids all the thinking and arguing whenever UINT32 is safe or not.

I consider that a cop-out; I'd want to see evidence (or deduce the
evidence myself) that UINT64 was safe, too. UINT64 is not "unlimited"
either.

In general, I don't want to do integer arithmetic without knowing the
potential value ranges at all times.

(1) It's great that we're discussing this, BTW -- an integer overflow is
already possible (at least in theory) after the second patch in the
series.

Before the series, TotalPages is at most 0x40201, so
EFI_PAGES_TO_SIZE(TotalPages) is at most ~1GB. We add SIZE_64MB to it;
that still fits in the UINT32 return value.

After the first patch in the series, a new increment is ApStacks; you
mention it never exceeds 128 MB in practice. So that's safe (in
practice).

However, after the second patch in the series, that no longer suffices
(in theory). As I calculated under patch#2, TotalPages can grow up to
and including 0x8040201, which means EFI_PAGES_TO_SIZE(TotalPages) is
~513 GB.

Meaning that,

(1.1) on IA32X64, the following

  MemoryCap = EFI_PAGES_TO_SIZE (TotalPages) + ApStacks + SIZE_64MB;

already truncates (because EFI_PAGES_TO_SIZE takes a UINTN and produces
a UINTN, but UINTN is just UINT32); and that

(1.2) on X64, while "MemoryCap" itself is fine, the final

   return (UINT32)(MemoryCap);

will truncate.

(2) Now, if we consider the 1TB address space limitation from
PlatformAddressWidthFromCpuid()
[OvmfPkg/Library/PlatformInitLib/MemDetect.c] that you highlight:

    if (!Page1GSupport && (PhysBits > 40)) {
      DEBUG ((DEBUG_INFO, "%a: limit PhysBits to 40 (no 1G pages available)\n", __func__));
      PhysBits = 40;
    }

then we get (from the exact calculation):

  Level2Pages = BIT10;
  Level3Pages = BIT1;
  Level4Pages = 1;
  Level5Pages = 1;
  TotalPages  = 1024 + 2 + 1 + 1;

and then EFI_PAGES_TO_SIZE(1028) is just ~ 4 MB. So the UINT32 retval is
safe, with the 1TB address space limitation in place -- but then we
should ASSERT the predicate here that PlatformAddressWidthFromCpuid()
ensures: ((PhysBits <= 40) || Page1GSupport)!

(3) Finally, if Page1GSupport is TRUE, and PhysMemAddressWidth is 57, we
get:

  Level2Pages = 0;
  Level3Pages = BIT18;
  Level4Pages = BIT9;
  Level5Pages = BIT0;

which produces the pre-series value 0x40201 for TotalPages -- so that
again will be safe for the UINT32 retval (the memory allocated for page
tables will be again ~1GB).


OK, summary:

- before calculating "End" and "Level2Pages" in patch#2, we should
  ASSERT ((PhysBits <= 40) || Page1GSupport).

- I guess it is fine to assume that
  (PcdCpuMaxLogicalProcessorNumber*PcdCpuApStackSize) will never
  exceed a few hundred MB. Maybe add a comment?

- I suggest keeping the retval UINT32; there is no reason for widening
  it to UINT64 (thanks to the 1TB address space limitation, without 1G
  pages!)

- The pre-series assertion that (TotalPages <= 0x40201) remains correct,
  after all, using the exact calculation (again due to the 1TB address
  space limitation, without 1G pages).

>
> And maybe add an else branch to log a warning in case
> MemorySize < PeiMemoryCap.

That is a good idea even with the unchanged UINT32 retval type!

Laszlo



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