Re: [edk2-devel] [Patch V4 18/21] UefiCpuPkg: Combine branch for non-present and leaf ParentEntry

2023-03-23 Thread Ni, Ray
> -//
> -// 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;
> +OneOfPagingEntry.Pnle.Uint64 = 0;
> +PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute
> (&ParentPagingEntry->PleB, ParentAttribute);
> 
> -if (Modify) {
> -  ParentPagingEntry->Uintn = (UINTN)Buffer + *BufferSize;
> -  ZeroMem ((VOID *)ParentPagingEntry->Uintn, SIZE_4KB);
> +if (ParentPagingEntry->Pce.Present == 0) {
>//
> -  // Set default attribute bits for PML5E/PML4E/PDPTE/PDE.
> +  // [LinearAddress, LinearAddress + Length] contains non-present range.
>//
> -  PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute,
> &AllOneMask);
> +  Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute,
> Mask);
> +  if (RETURN_ERROR (Status)) {
> +return Status;
> +  }

1. do you think put " OneOfPagingEntry.Pnle.Uint64 = 0;" in the if-body is 
better?
So both if and else initializes OneOfPagingEntry.
Other logic looks good to me.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101689): https://edk2.groups.io/g/devel/message/101689
Mute This Topic: https://groups.io/mt/97796396/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-




[edk2-devel] [Patch V4 18/21] UefiCpuPkg: Combine branch for non-present and leaf ParentEntry

2023-03-23 Thread duntan
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 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 83 
+++
 1 file changed, 31 insertions(+), 52 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 55a756ad90..773948349e 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -348,68 +348,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;
-}
-
+  if ((ParentPagingEntry->Pce.Present == 0) || IsPle (ParentPagingEntry, Level 
+ 1)) {
 //
-// 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;
-}
-
-//
-// 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;
+OneOfPagingEntry.Pnle.Uint64 = 0;
+PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute 
(&ParentPagingEntry->PleB, ParentAttribute);
 
-if (Modify) {
-  ParentPagingEntry->Uintn = (UINTN)Buffer + *BufferSize;
-  ZeroMem ((VOID *)ParentPagingEntry->Uintn, SIZE_4KB);
+if (ParentPagingEntry->Pce.Present == 0) {
   //
-  // Set default attribute bits for PML5E/PML4E/PDPTE/PDE.
+  // [LinearAddress, LinearAddress + Length] contains non-present range.
   //
-  PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, 
&AllOneMask);
+  Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute, Mask);
+  if (RETURN_ERROR (Status)) {
+return Status;
+  }
 } else {
-  //
-  // Just make sure Present and MustBeZero (PageSize) bits are accurate.
-  //
-  OneOfPagingEntry.Pnle.Uint64 = 0;
+  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) + PagingEntryIndex * RegionLength;
   PhysicalAddrInAttr  = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS