Re: [PATCH v2] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory

[Laszlo Ersek <lersek@redhat.com>]

On 03/03/18 10:03, Jian J Wang wrote:
>> v2 changes:
>> a. Reserve memory of mReservedApLoopFunc and mReservedTopOfApStack
>>    separately to avoid making mReservedTopOfApStack executable.
> 
> if PcdDxeNxMemoryProtectionPolicy is enabled for EfiReservedMemoryType
> of memory, #PF will be triggered for each APs after ExitBootServices
> in SCRT test. The root cause is that AP wakeup code executed at that
> time is stored in memory of type EfiReservedMemoryType (referenced by
> global mReservedApLoopFunc), which is marked as non-executable.
> 
> This patch fixes this issue by setting memory of mReservedApLoopFunc to
> be executable immediately after allocation.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 38 +++++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index fd2317924f..e7ed21c6cd 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -388,9 +388,9 @@ InitMpGlobalData (
>    // Allocating it in advance since memory services are not available in
>    // Exit Boot Services callback function.
>    //
> -  ApSafeBufferSize  = CpuMpData->AddressMap.RelocateApLoopFuncSize;
> -  ApSafeBufferSize += CpuMpData->CpuCount * AP_SAFE_STACK_SIZE;
> -
> +  ApSafeBufferSize  = EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (
> +                        CpuMpData->AddressMap.RelocateApLoopFuncSize
> +                        ));
>    Address = BASE_4GB - 1;
>    Status  = gBS->AllocatePages (
>                     AllocateMaxAddress,
> @@ -399,9 +399,39 @@ InitMpGlobalData (
>                     &Address
>                     );
>    ASSERT_EFI_ERROR (Status);
> +
>    mReservedApLoopFunc = (VOID *) (UINTN) Address;
>    ASSERT (mReservedApLoopFunc != NULL);
> -  mReservedTopOfApStack = (UINTN) Address + EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (ApSafeBufferSize));
> +
> +  //
> +  // Make sure that the buffer memory is executable if NX protection is enabled
> +  // for EfiReservedMemoryType.
> +  // 
> +  // TODO: Check EFI_MEMORY_XP bit set or not once it's available in DXE GCD
> +  //       service.
> +  //
> +  Status = gDS->GetMemorySpaceDescriptor (Address, &MemDesc);
> +  if (!EFI_ERROR (Status)) {
> +    gDS->SetMemorySpaceAttributes (
> +           Address,
> +           ApSafeBufferSize,
> +           MemDesc.Attributes & (~EFI_MEMORY_XP)
> +           );
> +  }
> +
> +  ApSafeBufferSize = EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (
> +                       CpuMpData->CpuCount * AP_SAFE_STACK_SIZE
> +                       ));
> +  Address = BASE_4GB - 1;
> +  Status  = gBS->AllocatePages (
> +                   AllocateMaxAddress,
> +                   EfiReservedMemoryType,
> +                   EFI_SIZE_TO_PAGES (ApSafeBufferSize),
> +                   &Address
> +                   );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  mReservedTopOfApStack = (UINTN) Address + ApSafeBufferSize;
>    ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0);
>    CopyMem (
>      mReservedApLoopFunc,
> 

I didn't verify whether or not the AP wakeup code relies on the stack
following directly the AP loop func. Since you must have tested this
patch successfully, I guess there is no such dependency (it would be
wrong anyway, IMO).

Furthermore, the comment is helpful -- we don't call PcdGet, but the
comment seems helpful in finding the PCD after all. (The commit message
does name the PCD, and we can run "git blame" on the comment.)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo