Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: log pei memory cap details

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

On 1/31/24 13:00, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index e22743b4bfaa..988fcc512362 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -246,6 +246,30 @@ GetPeiMemoryCap (
>      MemoryCap = MAX_UINT32;
>    }
>  
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: page tables: %6lld kb (%d/%d/%d/%d)\n",
> +    __func__,
> +    (UINT64)(EFI_PAGES_TO_SIZE (TotalPages) / 1024),

(1) There is no "ll" size modifier in edk2; <PrintLib.h> is not standard
C :)

Use a single "l" or "L" (doesn't matter which).

(2) "d" is not right for UINT*; we should use "u".

In total, please use %lu or %Lu.

(3) Using the exact inclusive page count limit, from my comments on the
previous patch in the series (0x8040201), the memory usage could be as
high as 537,921,540 KB (decimal). Setting the field width to 6 is not
consistent with that; we should use 9 (if we want padding).

(4) we shouldn't divide in 64-bit "natively", in such code that may be
compiled for 32-bit; either use RShiftU64() or DivU64x32(), from
BaseLib. (This is really hard to remember, unfortunately.)

Ooops, wait, ignore this: you are not dividing in UINT64, but in UINTN.
That's fine! The cast only refers to the quotient.


> +    Level5Pages,
> +    Level4Pages,
> +    Level3Pages,
> +    Level2Pages

(5) In my proposal for the previous patch, I changed the type of these
variables to UINT64 (for simplicity), so they should all be formatted
with %Lu or %lu. (%d is not right in any case; it should be %u even for
UINT32.)

> +    ));
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: ap stacks:   %6lld kb (%d cpus)\n",
> +    __func__,
> +    (UINT64)(ApStacks / 1024),

The division should be OK... ApStacks is UINTN (in fact it is a UINT32
product).

Not sure what to specify as field width... 4GB is 4,194,304 KB, so in
theory, we should use 7 for the field width.

(6) I propose "%7lu".

> +    PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber

(7) UINT32, so should be formatted with %u.

(BTW, our ApStacks calculation includes the BSP as well (!), so it is a
bit of an over-estimate. The name "ApStacks" is slightly misleading.)

> +    ));
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: memory cap:  %6lld kb\n",
> +    __func__,
> +    (UINT64)(MemoryCap / 1024)
> +    ));
> +

(8) At this point, MemoryCap is limited to MAX_UINT32, therefore I again
propose "%7lu".

>    //
>    // Add 64 MB for miscellaneous allocations. Note that for
>    // PhysMemAddressWidth values close to 36, the cap will actually be

Laszlo



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