* [Patch V6 00/22] Fix issues in CpuPageTableLib @ 2023-03-24 8:51 duntan 2023-03-24 8:51 ` [Patch V6 04/22] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue duntan 2023-03-24 8:51 ` [Patch V6 06/22] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry duntan 0 siblings, 2 replies; 5+ messages in thread From: duntan @ 2023-03-24 8:51 UTC (permalink / raw) To: devel In the V6 atch set In 'Fix the non-1:1 mapping issue', use MultU64x32 to avoid IA32 build failure. In 'Fix issue when splitting leaf entry', add more precise comments to explain why IA32_PE_BASE_ADDRESS_MASK_40 is used. Other patches are Reviewed-by Ray. Dun Tan (20): UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition UefiCpuPkg/CpuPageTableLib: Add check for input Length UefiCpuPkg/CpuPageTableLib:Initialize some LocalVariable at beginning UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue UefiCpuPkg/CpuPageTableLib:Clear PageSize bit(Bit7) for non-leaf UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry UefiCpuPkg/MpInitLib: Add code to initialize MapMask UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr UefiCpuPkg/CpuPageTableLib: Add manual test to check Mask and Attr UefiCpuPkg/CpuPageTableLib:Modify RandomBoolean() in RandomTest UefiCpuPkg/CpuPageTableLib: Add LastMapEntry pointer UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter. UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified UefiCpuPkg/CpuPageTableLib: Add check for page table creation UefiCpuPkg: Combine branch for non-present and leaf ParentEntry UefiCpuPkg/CpuPageTableLib: Enable PAE paging UefiCpuPkg/CpuPageTableLib: Add RandomTest for PAE paging UefiCpuPkg/CpuPageTableLib: Reduce the number of random tests Zhiguang Liu (2): UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf UefiCpuPkg: Modify UnitTest code since tested API is changed UefiCpuPkg/Include/Library/CpuPageTableLib.h | 44 +++++++++++++++++++++++++------------------- UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------- UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 317 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------------------------ UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableParse.c | 25 +++++++++++++++++++++---- UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------- UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c | 338 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------------------- UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c | 22 +++++++++++++++------- UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c | 11 +++++------ 8 files changed, 782 insertions(+), 327 deletions(-) -- 2.31.1.windows.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Patch V6 04/22] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue 2023-03-24 8:51 [Patch V6 00/22] Fix issues in CpuPageTableLib duntan @ 2023-03-24 8:51 ` duntan 2023-03-24 14:03 ` Ni, Ray 2023-03-24 8:51 ` [Patch V6 06/22] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry duntan 1 sibling, 1 reply; 5+ messages in thread From: duntan @ 2023-03-24 8:51 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann In previous code logic, when splitting a leaf parent entry to smaller granularity child page table, if the parent entry Attribute&Mask(without PageTableBaseAddress field) is equal to the input attribute&mask(without PageTableBaseAddress field), the split process won't happen. This may lead to failure in non-1:1 mapping. For example, there is a page table in which [0, 1G] is mapped(Lv4[0] ,Lv3[0,0], a non-leaf level4 entry and a leaf level3 entry). And we want to remap [0, 2M] linear address range to [1G, 1G + 2M] with the same attibute. The expected behaviour should be: split Lv3[0,0] entry into 512 level2 entries and remap the first level2 entry to cover [0, 2M]. But the split won't happen in previous code since PageTableBaseAddress of input Attribute is not checked. So, when checking if a leaf parent entry needs to be splitted, we should also check if PageTableBaseAddress calculated by parent entry is equal to the value caculated by input attribute. 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 | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c index 127b65183f..b94ef07c56 100644 --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c @@ -274,6 +274,8 @@ PageTableLibMapInLevel ( IA32_MAP_ATTRIBUTE ChildMask; IA32_MAP_ATTRIBUTE CurrentMask; IA32_MAP_ATTRIBUTE LocalParentAttribute; + UINT64 PhysicalAddrInEntry; + UINT64 PhysicalAddrInAttr; ASSERT (Level != 0); ASSERT ((Attribute != NULL) && (Mask != NULL)); @@ -341,7 +343,15 @@ PageTableLibMapInLevel ( // 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. // - return RETURN_SUCCESS; + if (Mask->Bits.PageTableBaseAddress == 0) { + return RETURN_SUCCESS; + } + + PhysicalAddrInEntry = IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&PleBAttribute) + MultU64x32 (RegionLength, (UINT32)PagingEntryIndex); + PhysicalAddrInAttr = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) & (~RegionMask); + if (PhysicalAddrInEntry == PhysicalAddrInAttr) { + return RETURN_SUCCESS; + } } ASSERT (Buffer == NULL || *BufferSize >= SIZE_4KB); -- 2.31.1.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch V6 04/22] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue 2023-03-24 8:51 ` [Patch V6 04/22] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue duntan @ 2023-03-24 14:03 ` Ni, Ray 0 siblings, 0 replies; 5+ messages in thread From: Ni, Ray @ 2023-03-24 14:03 UTC (permalink / raw) To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann Reviewed-by: Ray Ni <ray.ni@intel.com> > -----Original Message----- > From: Tan, Dun <dun.tan@intel.com> > Sent: Friday, March 24, 2023 4:52 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 V6 04/22] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 > mapping issue > > In previous code logic, when splitting a leaf parent entry to > smaller granularity child page table, if the parent entry > Attribute&Mask(without PageTableBaseAddress field) is equal to the > input attribute&mask(without PageTableBaseAddress field), the split > process won't happen. This may lead to failure in non-1:1 mapping. > > For example, there is a page table in which [0, 1G] is mapped(Lv4[0] > ,Lv3[0,0], a non-leaf level4 entry and a leaf level3 entry). And we > want to remap [0, 2M] linear address range to [1G, 1G + 2M] with the > same attibute. The expected behaviour should be: split Lv3[0,0] > entry into 512 level2 entries and remap the first level2 entry to > cover [0, 2M]. But the split won't happen in previous code since > PageTableBaseAddress of input Attribute is not checked. > > So, when checking if a leaf parent entry needs to be splitted, we > should also check if PageTableBaseAddress calculated by parent entry > is equal to the value caculated by input attribute. > > 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 | 12 > +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > index 127b65183f..b94ef07c56 100644 > --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > @@ -274,6 +274,8 @@ PageTableLibMapInLevel ( > IA32_MAP_ATTRIBUTE ChildMask; > IA32_MAP_ATTRIBUTE CurrentMask; > IA32_MAP_ATTRIBUTE LocalParentAttribute; > + UINT64 PhysicalAddrInEntry; > + UINT64 PhysicalAddrInAttr; > > ASSERT (Level != 0); > ASSERT ((Attribute != NULL) && (Mask != NULL)); > @@ -341,7 +343,15 @@ PageTableLibMapInLevel ( > // 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. > // > - return RETURN_SUCCESS; > + if (Mask->Bits.PageTableBaseAddress == 0) { > + return RETURN_SUCCESS; > + } > + > + PhysicalAddrInEntry = > IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&PleBAttribute) + > MultU64x32 (RegionLength, (UINT32)PagingEntryIndex); > + PhysicalAddrInAttr = > (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) > & (~RegionMask); > + if (PhysicalAddrInEntry == PhysicalAddrInAttr) { > + return RETURN_SUCCESS; > + } > } > > ASSERT (Buffer == NULL || *BufferSize >= SIZE_4KB); > -- > 2.31.1.windows.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Patch V6 06/22] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry 2023-03-24 8:51 [Patch V6 00/22] Fix issues in CpuPageTableLib duntan 2023-03-24 8:51 ` [Patch V6 04/22] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue duntan @ 2023-03-24 8:51 ` duntan 2023-03-24 14:03 ` Ni, Ray 1 sibling, 1 reply; 5+ messages in thread From: duntan @ 2023-03-24 8:51 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann When splitting leaf parent entry to smaller granularity, create child page table before modifing parent entry. In previous code logic, when splitting a leaf parent entry, parent entry will point to a null 4k memory before child page table is created in this 4k memory. When the page table to be modified is the page table in CR3, if the executed CpuPageTableLib code is in the range mapped by the modified leaf parent entry, then issue will happen. 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 | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c index 57f1db203b..f09bb63ad1 100644 --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c @@ -363,21 +363,24 @@ PageTableLibMapInLevel ( // // Create 512 child-level entries that map to 2M/4K. // - ParentPagingEntry->Uintn = (UINTN)Buffer + *BufferSize; - ZeroMem ((VOID *)ParentPagingEntry->Uintn, SIZE_4KB); + 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; + } // // Set NOP attributes // Note: Should NOT inherit the attributes from the original entry because a zero RW bit // will make the entire region read-only even the child entries set the RW bit. // + // Non-leaf entry doesn't have PAT bit. So use ~IA32_PE_BASE_ADDRESS_MASK_40 is to make sure PAT bit + // (bit12) in original big-leaf entry is not assigned to PageTableBaseAddress field of non-leaf entry. + // PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, &AllOneMask); - - PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle); - for (SubOffset = 0, Index = 0; Index < 512; Index++) { - PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset; - SubOffset += RegionLength; - } + ParentPagingEntry->Uint64 = ((UINTN)(VOID *)PagingEntry) | (ParentPagingEntry->Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40)); } } else { // -- 2.31.1.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch V6 06/22] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry 2023-03-24 8:51 ` [Patch V6 06/22] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry duntan @ 2023-03-24 14:03 ` Ni, Ray 0 siblings, 0 replies; 5+ messages in thread From: Ni, Ray @ 2023-03-24 14:03 UTC (permalink / raw) To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann Reviewed-by: Ray Ni <ray.ni@intel.com> > -----Original Message----- > From: Tan, Dun <dun.tan@intel.com> > Sent: Friday, March 24, 2023 4:52 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 V6 06/22] UefiCpuPkg/CpuPageTableLib: Fix issue when > splitting leaf entry > > When splitting leaf parent entry to smaller granularity, create > child page table before modifing parent entry. In previous code > logic, when splitting a leaf parent entry, parent entry will > point to a null 4k memory before child page table is created in > this 4k memory. When the page table to be modified is the page > table in CR3, if the executed CpuPageTableLib code is in the > range mapped by the modified leaf parent entry, then issue will > happen. > > 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 | 19 > +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > index 57f1db203b..f09bb63ad1 100644 > --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > @@ -363,21 +363,24 @@ PageTableLibMapInLevel ( > // > // Create 512 child-level entries that map to 2M/4K. > // > - ParentPagingEntry->Uintn = (UINTN)Buffer + *BufferSize; > - ZeroMem ((VOID *)ParentPagingEntry->Uintn, SIZE_4KB); > + 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; > + } > > // > // Set NOP attributes > // Note: Should NOT inherit the attributes from the original entry because > a zero RW bit > // will make the entire region read-only even the child entries set the > RW bit. > // > + // Non-leaf entry doesn't have PAT bit. So use > ~IA32_PE_BASE_ADDRESS_MASK_40 is to make sure PAT bit > + // (bit12) in original big-leaf entry is not assigned to > PageTableBaseAddress field of non-leaf entry. > + // > PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, > &AllOneMask); > - > - PagingEntry = (IA32_PAGING_ENTRY > *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry- > >Pnle); > - for (SubOffset = 0, Index = 0; Index < 512; Index++) { > - PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset; > - SubOffset += RegionLength; > - } > + ParentPagingEntry->Uint64 = ((UINTN)(VOID *)PagingEntry) | > (ParentPagingEntry->Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40)); > } > } else { > // > -- > 2.31.1.windows.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-24 14:03 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-24 8:51 [Patch V6 00/22] Fix issues in CpuPageTableLib duntan 2023-03-24 8:51 ` [Patch V6 04/22] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue duntan 2023-03-24 14:03 ` Ni, Ray 2023-03-24 8:51 ` [Patch V6 06/22] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry duntan 2023-03-24 14:03 ` Ni, Ray
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox