Re: [PATCH 2/4] UefiCpuPkg: Has APs in 64 bit long-mode before booting to OS.

["Ni, Ray" <ray.ni@intel.com>]

> +
> +    PageTable = CreatePageTable (
> +                  mReservedTopOfApStack,
> +                  ApSafeBufferSize
> +                  );
> +
> +    mApPageTable = PageTable;

1. Can you directly assign the CreatePageTable() return value to mApPageTable?
     So that the local "PageTable" is not needed.

> +
> +    mReservedTopOfApStack += CpuMpData->CpuCount * AP_SAFE_STACK_SIZE;
> +  }
2. Can you confirm if above change to mReservedTopOfApStack is needed for AMD path?

> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/CreatePageTable.c b/UefiCpuPkg/Library/MpInitLib/Ia32/CreatePageTable.c
> new file mode 100644
> index 0000000000..54ec42469e
> --- /dev/null
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/CreatePageTable.c
> @@ -0,0 +1,28 @@
> +/** @file
> +  CreatePageTable
3. File header comments are too simple.



> +
> +  Copyright (c) 2016 - 2022, Intel Corporation. All rights reserved.<BR>

4. Copyright year can be just "2022". No need to have "2016".

> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>

5. Can you try to remove the above two library header inclusion?

> +
> +/**
> +  Create or update page table.

6. The function header comments could be:
    "Create 1:1 mapping page table in reserved memory to map the specified address range."
    Please update in header file as well.

>  /**
>    Initialize global data for MP support.
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm
> index 47fc8e9325..b2d95adf6d 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm
> @@ -514,4 +514,4 @@ DoHltAmd:
>      jmp        DoHltAmd
> 
>  BITS 64
> -AsmRelocateApLoopEndAmd:
> \ No newline at end of file
> +AsmRelocateApLoopEndAmd:

7. The change to add newline character should be in the first patch, not in this patch.