Re: [Patch V5 19/22] UefiCpuPkg: Combine branch for non-present and leaf ParentEntry

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

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Friday, March 24, 2023 2:00 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V5 19/22] UefiCpuPkg: Combine branch for non-present and
> leaf ParentEntry
> 
> Combine 'if' condition branch for non-present and leaf Parent
> Entry in PageTableLibMapInLevel. Most steps of these two condition
> are the same. This commit doesn't change any functionality.
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Tested-by: Gerd Hoffmann <kraxel@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 85
> ++++++++++++++++++++++++++++++++------------------------------------------
> -----------
>  1 file changed, 32 insertions(+), 53 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index ad1e263084..2430f1b37c 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -351,68 +351,45 @@ PageTableLibMapInLevel (
>    // ParentPagingEntry ONLY is deferenced for checking Present and
> MustBeOne bits
>    // when Modify is FALSE.
>    //
> -
> -  if (ParentPagingEntry->Pce.Present == 0) {
> -    //
> -    // [LinearAddress, LinearAddress + Length] contains non-present range.
> -    //
> -    Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute, Mask);
> -    if (RETURN_ERROR (Status)) {
> -      return Status;
> -    }
> -
> -    //
> -    // Check the attribute in ParentPagingEntry is equal to attribute calculated
> by input Attribue and Mask.
> -    //
> -    PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute
> (&ParentPagingEntry->PleB, ParentAttribute);
> -    if ((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
> -        == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))
> -    {
> -      return RETURN_SUCCESS;
> -    }
> -
> +  if ((ParentPagingEntry->Pce.Present == 0) || IsPle (ParentPagingEntry,
> Level + 1)) {
>      //
> -    // The parent entry is CR3 or PML5E/PML4E/PDPTE/PDE.
> +    // When ParentPagingEntry is non-present, parent entry is CR3 or
> PML5E/PML4E/PDPTE/PDE.
>      // It does NOT point to an existing page directory.
> +    // When ParentPagingEntry is present, parent entry is leaf PDPTE_1G or
> PDE_2M. Split to 2M or 4K pages.
> +    // Note: it's impossible the parent entry is a PTE_4K.
>      //
> -    ASSERT (Buffer == NULL || *BufferSize >= SIZE_4KB);
> -    CreateNew    = TRUE;
> -    *BufferSize -= SIZE_4KB;
> -
> -    if (Modify) {
> -      ParentPagingEntry->Uintn = (UINTN)Buffer + *BufferSize;
> -      ZeroMem ((VOID *)ParentPagingEntry->Uintn, SIZE_4KB);
> -      //
> -      // Set default attribute bits for PML5E/PML4E/PDPTE/PDE.
> -      //
> -      PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute,
> &AllOneMask);
> -    } else {
> +    PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute
> (&ParentPagingEntry->PleB, ParentAttribute);
> +    if (ParentPagingEntry->Pce.Present == 0) {
>        //
> -      // Just make sure Present and MustBeZero (PageSize) bits are accurate.
> +      // [LinearAddress, LinearAddress + Length] contains non-present range.
>        //
> +      Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute,
> Mask);
> +      if (RETURN_ERROR (Status)) {
> +        return Status;
> +      }
> +
>        OneOfPagingEntry.Pnle.Uint64 = 0;
> +    } else {
> +      PageTableLibSetPle (Level, &OneOfPagingEntry, 0, &PleBAttribute,
> &AllOneMask);
>      }
> -  } else if (IsPle (ParentPagingEntry, Level + 1)) {
> -    //
> -    // The parent entry is a PDPTE_1G or PDE_2M. Split to 2M or 4K pages.
> -    // Note: it's impossible the parent entry is a PTE_4K.
> -    //
> +
>      //
> -    // Use NOP attributes as the attribute of grand-parents because CPU will
> consider
> -    // the actual attributes of grand-parents when determing the memory
> type.
> +    // Check if the attribute, the physical address calculated by
> ParentPagingEntry is equal to
> +    // the attribute, the physical address calculated by input Attribue and
> Mask.
>      //
> -    PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute
> (&ParentPagingEntry->PleB, ParentAttribute);
>      if ((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
>          == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))
>      {
> -      //
> -      // This function is called when the memory length is less than the region
> length of the parent level.
> -      // No need to split the page when the attributes equal.
> -      //
>        if ((Mask->Bits.PageTableBaseAddressLow == 0) && (Mask-
> >Bits.PageTableBaseAddressHigh == 0)) {
>          return RETURN_SUCCESS;
>        }
> 
> +      //
> +      // Non-present entry won't reach there since:
> +      // 1.When map non-present entry to present, the attribute must be
> different.
> +      // 2.When still map non-present entry to non-present,
> PageTableBaseAddressLow and High in Mask must be 0.
> +      //
> +      ASSERT (ParentPagingEntry->Pce.Present == 1);
>        PhysicalAddrInEntry =
> IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&PleBAttribute) +
> (UINT64)PagingEntryIndex * RegionLength;
>        PhysicalAddrInAttr  =
> (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset)
> & (~RegionMask);
>        if (PhysicalAddrInEntry == PhysicalAddrInAttr) {
> @@ -423,17 +400,19 @@ PageTableLibMapInLevel (
>      ASSERT (Buffer == NULL || *BufferSize >= SIZE_4KB);
>      CreateNew    = TRUE;
>      *BufferSize -= SIZE_4KB;
> -    PageTableLibSetPle (Level, &OneOfPagingEntry, 0, &PleBAttribute,
> &AllOneMask);
> +
>      if (Modify) {
> -      //
> -      // Create 512 child-level entries that map to 2M/4K.
> -      //
>        PagingEntry = (IA32_PAGING_ENTRY *)((UINTN)Buffer + *BufferSize);
>        ZeroMem (PagingEntry, SIZE_4KB);
> 
> -      for (SubOffset = 0, Index = 0; Index < 512; Index++) {
> -        PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset;
> -        SubOffset                += RegionLength;
> +      if (ParentPagingEntry->Pce.Present) {
> +        //
> +        // Create 512 child-level entries that map to 2M/4K.
> +        //
> +        for (SubOffset = 0, Index = 0; Index < 512; Index++) {
> +          PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset;
> +          SubOffset                += RegionLength;
> +        }
>        }
> 
>        //
> --
> 2.31.1.windows.1