Okay, I'll split this patch into 2 patches in next version
Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray...@intel.com>
Sent: Wednesday, March 15, 2023 9:28 AM
To: Tan, Dun <dun....@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.d...@intel.com>; Kumar, Rahul R <rahul.r.ku...@intel.com>;
Gerd Hoffmann <kra...@redhat.com>
Subject: RE: [Patch V2 03/14] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1
mapping issue
Dun,
Can you split this patch to 2 patches?
One to move some local variable initialization to the beginning of the function.
The other to fix the bug. So the bug fix changes look smaller.
Thanks,
Ray
> -----Original Message-----
> From: Tan, Dun <dun....@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>;
> Kumar, Rahul R <rahul.r.ku...@intel.com>; Gerd Hoffmann
> <kra...@redhat.com>
> Subject: [Patch V2 03/14] 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....@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> ---
> UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 27
> +++++++++++++++++----------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 4c9d70fa0a..ee27238edb 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -258,6 +258,7 @@ PageTableLibMapInLevel (
> UINTN BitStart;
> UINTN Index;
> IA32_PAGING_ENTRY *PagingEntry;
> + UINTN PagingEntryIndex;
> IA32_PAGING_ENTRY *CurrentPagingEntry;
> UINT64 RegionLength;
> UINT64 SubLength;
> @@ -288,6 +289,13 @@ PageTableLibMapInLevel (
> LocalParentAttribute.Uint64 = ParentAttribute->Uint64;
> ParentAttribute = &LocalParentAttribute;
>
> + //
> + // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1
> + << 21)
> or 4K (1 << 12).
> + //
> + BitStart = 12 + (Level - 1) * 9;
> + PagingEntryIndex = (UINTN)BitFieldRead64 (LinearAddress + Offset,
> BitStart, BitStart + 9 - 1);
> + RegionLength = REGION_LENGTH (Level);
> +
> //
> // ParentPagingEntry ONLY is deferenced for checking Present and
> MustBeOne bits
> // when Modify is FALSE.
> @@ -325,8 +333,11 @@ PageTableLibMapInLevel (
> // the actual attributes of grand-parents when determing the
> memory type.
> //
> 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)))
> + if ((((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
> + == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))) &&
> + ( (Mask->Bits.PageTableBaseAddress == 0)
> + || ((IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS
> (&PleBAttribute) + PagingEntryIndex * RegionLength)
> + == (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS
> + (Attribute)
> + Offset))))
> {
> //
> // This function is called when the memory length is less than
> the region length of the parent level.
> @@ -353,8 +364,7 @@ PageTableLibMapInLevel (
> //
> PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute,
> &AllOneMask);
>
> - RegionLength = REGION_LENGTH (Level);
> - PagingEntry = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> + 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;
> @@ -425,14 +435,11 @@ PageTableLibMapInLevel (
> }
>
> //
> - // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1
> << 21) or 4K (1 << 12).
> // RegionStart: points to the linear address that's aligned on
> RegionLength and lower than (LinearAddress + Offset).
> //
> - BitStart = 12 + (Level - 1) * 9;
> - Index = (UINTN)BitFieldRead64 (LinearAddress + Offset, BitStart,
> BitStart + 9 - 1);
> - RegionLength = LShiftU64 (1, BitStart);
> - RegionMask = RegionLength - 1;
> - RegionStart = (LinearAddress + Offset) & ~RegionMask;
> + Index = PagingEntryIndex;
> + RegionMask = RegionLength - 1;
> + RegionStart = (LinearAddress + Offset) & ~RegionMask;
>
> ParentAttribute->Uint64 = PageTableLibGetPnleMapAttribute
> (&ParentPagingEntry->Pnle, ParentAttribute);
>
> --
> 2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101189): https://edk2.groups.io/g/devel/message/101189
Mute This Topic: https://groups.io/mt/97469471/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
