Re: [Patch V4 05/15] UefiCpuPkg: Use CpuPageTableLib to convert SMM paging attribute.

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

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 3deb1ffd67..a25a96f68c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1,7 +1,7 @@
>  /** @file
>  Page Fault (#PF) handler for X64 processors
> 
> -Copyright (c) 2009 - 2022, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.<BR>
>  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -353,7 +353,12 @@ SmmInitPageTable (
>    m1GPageTableSupport           = Is1GPageSupport ();
>    m5LevelPagingNeeded           = Is5LevelPagingNeeded ();
>    mPhysicalAddressBits          = CalculateMaximumSupportAddress ();
> -  PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded,
> 1);
> +  if (m5LevelPagingNeeded) {
> +    mPagingMode = m1GPageTableSupport ? Paging5Level1GB : Paging5Level;
> +    PatchInstructionX86 (gPatch5LevelPagingNeeded, TRUE, 1);

1. this change assumes the default value in assembly is 0 while old logic doesn't
have such assumption. Can you patch the instruction no matter m5LevelPagingNeeded?


> +      DEBUG_CODE (
> +        if (((Attributes & EFI_MEMORY_RO) == 0) || (((Attributes &
> EFI_MEMORY_XP) == 0) && (mXdSupported))) {
> +        //
> +        // When mapping a range to present and EFI_MEMORY_RO or
> EFI_MEMORY_XP is not specificed,
> +        // check if [BaseAddress, BaseAddress + Length] contains present range.
> +        // Existing Present range in [BaseAddress, BaseAddress + Length] is set to
> NX disable and ReadOnly.
> +        //
> +        Count  = 0;
> +        Map    = NULL;
> +        Status = PageTableParse (PageTableBase, mPagingMode, NULL, &Count);
> +        while (Status == RETURN_BUFFER_TOO_SMALL) {
> +          if (Map != NULL) {
> +            FreePool (Map);
> +          }
> 
> -      if (IsModified != NULL) {
> -        *IsModified = TRUE;
> +          Map = AllocatePool (Count * sizeof (IA32_MAP_ENTRY));
> +          ASSERT (Map != NULL);
> +          Status = PageTableParse (PageTableBase, mPagingMode, Map, &Count);
> +        }
> +
> +        ASSERT_RETURN_ERROR (Status);
> +
> +        for (Index = 0; Index < Count; Index++) {
> +          if ((BaseAddress < Map[Index].LinearAddress +
> +               Map[Index].Length) && (BaseAddress + Length >
> Map[Index].LinearAddress))
> +          {
> +            DEBUG ((DEBUG_ERROR, "SMM ConvertMemoryPageAttributes:
> Existing Present range in [0x%lx, 0x%lx] is set to NX disable and ReadOnly\n",
> BaseAddress, BaseAddress + Length));
> +            break;
> +          }
> +        }
> +
> +        FreePool (Map);
>        }

2. What's the purpose of the above DEBUG_CODE()?
Because when mapping a range of memory from not-present to present,
the function clears all other attributes but only set the "present" bit.
If part of the range is "present" already, the function might reset
its other attributes. This is not expected by caller.
So, you want to notify caller?

Can you split this logic to a separate commit?
If the sub-range's attributes match to what you are going to set
for the entire range, caller can ignore such error message, right?



> 
> -      //
> -      // Just split current page
> -      // Convert success in next around
> -      //
> +        );
>      }
>    }
> 
> +  if (PagingAttrMask.Uint64 == 0) {
> +    return RETURN_SUCCESS;
> +  }
> +
> +  PageTableBufferSize = 0;
> +  Status              = PageTableMap (&PageTableBase, PagingMode, NULL,
> &PageTableBufferSize, BaseAddress, Length, &PagingAttribute,
> &PagingAttrMask, IsModified);
> +
> +  if (Status == RETURN_BUFFER_TOO_SMALL) {
> +    PageTableBuffer = AllocatePageTableMemory (EFI_SIZE_TO_PAGES
> (PageTableBufferSize));
> +    ASSERT (PageTableBuffer != NULL);
> +    Status = PageTableMap (&PageTableBase, PagingMode, PageTableBuffer,
> &PageTableBufferSize, BaseAddress, Length, &PagingAttribute,
> &PagingAttrMask, IsModified);
> +  }
> +
> +  if (Status == RETURN_INVALID_PARAMETER) {
> +    //
> +    // The only reason that PageTableMap returns
> RETURN_INVALID_PARAMETER here is to modify other attributes
> +    // of a non-present range but remains the non-present range still as non-
> present.
> +    //
> +    DEBUG ((DEBUG_ERROR, "SMM ConvertMemoryPageAttributes: Non-
> present range in [0x%lx, 0x%lx] needs to be removed\n", BaseAddress,
> BaseAddress + Length));

3. Don't quite understand. Can you describe in a clearer way?