Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/CpuMpPei: Print correct buffer size used for page table

["Michael Kubacki" <mikuback@linux.microsoft.com>]

Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>

A couple comments below.

On 6/8/2023 1:23 PM, Ard Biesheuvel wrote:
> The DEBUG print that outputs the base and size of the page table
> allocation always prints 0x0 for the size, given that BufferSize will be
> updated by PageTableMap () and contain the unused allocation on return.
> 
> So move the DEBUG print right after the allocation.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   UefiCpuPkg/CpuMpPei/CpuPaging.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> index b7ddb0005b6fbcac..175e47ccd737a0c1 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> @@ -396,6 +396,13 @@ EnablePaePageTable (
>       return EFI_OUT_OF_RESOURCES;
> 
>     }
> 
>   
> 
> +  DEBUG ((
> 
> +    DEBUG_INFO,
> 
> +    "EnablePaePageTable: Created PageTable = 0x%x, BufferSize = %x\n",
> 
> +    PageTable,
> 
> +    BufferSize
> 
> +    ));
> 
> +

In the past, a point was made to improve portability between 32-bit and 
64-bit architectures by casting UINTN values to UINT64 and then printing 
them %Lx. If this is a DEBUG only change that might be worth adding as well.

In any case, can you please prefix the print specifier for BufferSize 
with "0x"?

> 
>     Status = PageTableMap (&PageTable, PagingPae, Buffer, &BufferSize, 0, SIZE_4GB, &MapAttribute, &MapMask, NULL);
> 
>     ASSERT_EFI_ERROR (Status);
> 
>     if (EFI_ERROR (Status) || (PageTable == 0)) {
> 
> @@ -417,13 +424,6 @@ EnablePaePageTable (
>     //
> 
>     AsmWriteCr0 (AsmReadCr0 () | BIT31);
> 
>   
> 
> -  DEBUG ((
> 
> -    DEBUG_INFO,
> 
> -    "EnablePaePageTable: Created PageTable = 0x%x, BufferSize = %x\n",
> 
> -    PageTable,
> 
> -    BufferSize
> 
> -    ));
> 
> -
> 
>     return Status;
> 
>   }
> 
>   
>