* UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to handle current page fault
@ 2019-08-05 21:57 Krzysztof Rusocki
2019-08-06 1:56 ` Ni, Ray
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Krzysztof Rusocki @ 2019-08-05 21:57 UTC (permalink / raw)
To: devel
Cc: Damian Nikodem, Eric Dong, Ray Ni, Jian J Wang, Laszlo Ersek,
Krzysztof Rusocki
From: Damian Nikodem <damian.nikodem@intel.com>
Reclaim may free page table pages that are required to handle current page
fault. This causes a page leak, and, after sufficent number of specific
page fault+reclaim pairs, we run out of reclaimable pages and hit:
ASSERT (MinAcc != (UINT64)-1);
To remedy, prevent pages essential to handling current page fault:
(1) from being considered as reclaim candidates (first reclaim phase)
(2) from being freed as part of "branch cleanup" (second reclaim phase)
Signed-off-by: Damian Nikodem <damian.nikodem@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Krzysztof Rusocki <krzysztof.rusocki@intel.com>
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index a3b62f7787..f11323f439 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -543,6 +543,11 @@ ReclaimPages (
UINT64 *ReleasePageAddress;
IA32_CR4 Cr4;
BOOLEAN Enable5LevelPaging;
+ UINT64 PFAddress;
+ UINT64 PFAddressPml5Index;
+ UINT64 PFAddressPml4Index;
+ UINT64 PFAddressPdptIndex;
+ UINT64 PFAddressPdtIndex;
Pml4 = NULL;
Pdpt = NULL;
@@ -554,6 +559,11 @@ ReclaimPages (
MinPdt = (UINTN)-1;
Acc = 0;
ReleasePageAddress = 0;
+ PFAddress = AsmReadCr2 ();
+ PFAddressPml5Index = BitFieldRead64 (PFAddress, 48, 48 + 8);
+ PFAddressPml4Index = BitFieldRead64 (PFAddress, 39, 39 + 8);
+ PFAddressPdptIndex = BitFieldRead64 (PFAddress, 30, 30 + 8);
+ PFAddressPdtIndex = BitFieldRead64 (PFAddress, 21, 21 + 8);
Cr4.UintN = AsmReadCr4 ();
Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
@@ -628,40 +638,46 @@ ReclaimPages (
// we will find the entry has the smallest access record value
//
PDPTEIgnore = TRUE;
- Acc = GetAndUpdateAccNum (Pdt + PdtIndex);
+ if (PdtIndex != PFAddressPdtIndex || PdptIndex != PFAddressPdptIndex ||
+ Pml4Index != PFAddressPml4Index || Pml5Index != PFAddressPml5Index) {
+ Acc = GetAndUpdateAccNum (Pdt + PdtIndex);
+ if (Acc < MinAcc) {
+ //
+ // If the PD entry has the smallest access record value,
+ // save the Page address to be released
+ //
+ MinAcc = Acc;
+ MinPml5 = Pml5Index;
+ MinPml4 = Pml4Index;
+ MinPdpt = PdptIndex;
+ MinPdt = PdtIndex;
+ ReleasePageAddress = Pdt + PdtIndex;
+ }
+ }
+ }
+ }
+ if (!PDPTEIgnore) {
+ //
+ // If this PDPT entry has no PDT entries pointer to 4 KByte pages,
+ // it should only has the entries point to 2 MByte Pages
+ //
+ if (PdptIndex != PFAddressPdptIndex || Pml4Index != PFAddressPml4Index ||
+ Pml5Index != PFAddressPml5Index) {
+ Acc = GetAndUpdateAccNum (Pdpt + PdptIndex);
if (Acc < MinAcc) {
//
- // If the PD entry has the smallest access record value,
+ // If the PDPT entry has the smallest access record value,
// save the Page address to be released
//
MinAcc = Acc;
MinPml5 = Pml5Index;
MinPml4 = Pml4Index;
MinPdpt = PdptIndex;
- MinPdt = PdtIndex;
- ReleasePageAddress = Pdt + PdtIndex;
+ MinPdt = (UINTN)-1;
+ ReleasePageAddress = Pdpt + PdptIndex;
}
}
}
- if (!PDPTEIgnore) {
- //
- // If this PDPT entry has no PDT entries pointer to 4 KByte pages,
- // it should only has the entries point to 2 MByte Pages
- //
- Acc = GetAndUpdateAccNum (Pdpt + PdptIndex);
- if (Acc < MinAcc) {
- //
- // If the PDPT entry has the smallest access record value,
- // save the Page address to be released
- //
- MinAcc = Acc;
- MinPml5 = Pml5Index;
- MinPml4 = Pml4Index;
- MinPdpt = PdptIndex;
- MinPdt = (UINTN)-1;
- ReleasePageAddress = Pdpt + PdptIndex;
- }
- }
}
}
if (!PML4EIgnore) {
@@ -669,18 +685,20 @@ ReclaimPages (
// If PML4 entry has no the PDPT entry pointer to 2 MByte pages,
// it should only has the entries point to 1 GByte Pages
//
- Acc = GetAndUpdateAccNum (Pml4 + Pml4Index);
- if (Acc < MinAcc) {
- //
- // If the PML4 entry has the smallest access record value,
- // save the Page address to be released
- //
- MinAcc = Acc;
- MinPml5 = Pml5Index;
- MinPml4 = Pml4Index;
- MinPdpt = (UINTN)-1;
- MinPdt = (UINTN)-1;
- ReleasePageAddress = Pml4 + Pml4Index;
+ if (Pml4Index != PFAddressPml4Index || Pml5Index != PFAddressPml5Index) {
+ Acc = GetAndUpdateAccNum (Pml4 + Pml4Index);
+ if (Acc < MinAcc) {
+ //
+ // If the PML4 entry has the smallest access record value,
+ // save the Page address to be released
+ //
+ MinAcc = Acc;
+ MinPml5 = Pml5Index;
+ MinPml4 = Pml4Index;
+ MinPdpt = (UINTN)-1;
+ MinPdt = (UINTN)-1;
+ ReleasePageAddress = Pml4 + Pml4Index;
+ }
}
}
}
@@ -708,7 +726,8 @@ ReclaimPages (
Pml4 = (UINT64 *) (UINTN) (Pml5[MinPml5] & gPhyMask);
Pdpt = (UINT64*)(UINTN)(Pml4[MinPml4] & ~mAddressEncMask & gPhyMask);
SubEntriesNum = GetSubEntriesNum(Pdpt + MinPdpt);
- if (SubEntriesNum == 0) {
+ if (SubEntriesNum == 0 &&
+ (MinPdpt != PFAddressPdptIndex || MinPml4 != PFAddressPml4Index || MinPml5 != PFAddressPml5Index)) {
//
// Release the empty Page Directory table if there was no more 4 KByte Page Table entry
// clear the Page directory entry
@@ -724,7 +743,7 @@ ReclaimPages (
//
// Update the sub-entries filed in PDPT entry and exit
//
- SetSubEntriesNum (Pdpt + MinPdpt, SubEntriesNum - 1);
+ SetSubEntriesNum (Pdpt + MinPdpt, (SubEntriesNum - 1) & 0x1FF);
break;
}
if (MinPdpt != (UINTN)-1) {
@@ -732,7 +751,7 @@ ReclaimPages (
// One 2MB Page Table is released or Page Directory table is released, check the PML4 entry
//
SubEntriesNum = GetSubEntriesNum (Pml4 + MinPml4);
- if (SubEntriesNum == 0) {
+ if (SubEntriesNum == 0 && (MinPml4 != PFAddressPml4Index || MinPml5 != PFAddressPml5Index)) {
//
// Release the empty PML4 table if there was no more 1G KByte Page Table entry
// clear the Page directory entry
@@ -745,7 +764,7 @@ ReclaimPages (
//
// Update the sub-entries filed in PML4 entry and exit
//
- SetSubEntriesNum (Pml4 + MinPml4, SubEntriesNum - 1);
+ SetSubEntriesNum (Pml4 + MinPml4, (SubEntriesNum - 1) & 0x1FF);
break;
}
//
@@ -918,7 +937,7 @@ SmiDefaultPFHandler (
PageTable[PTIndex] = ((PFAddress | mAddressEncMask) & gPhyMask & ~((1ull << EndBit) - 1)) |
PageAttribute | IA32_PG_A | PAGE_ATTRIBUTE_BITS;
if (UpperEntry != NULL) {
- SetSubEntriesNum (UpperEntry, GetSubEntriesNum (UpperEntry) + 1);
+ SetSubEntriesNum (UpperEntry, (GetSubEntriesNum (UpperEntry) + 1) & 0x1FF);
}
//
// Get the next page address if we need to create more page tables
--------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to handle current page fault
2019-08-05 21:57 UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to handle current page fault Krzysztof Rusocki
@ 2019-08-06 1:56 ` Ni, Ray
2019-08-06 2:02 ` Wang, Jian J
2019-08-07 16:43 ` Laszlo Ersek
2 siblings, 0 replies; 4+ messages in thread
From: Ni, Ray @ 2019-08-06 1:56 UTC (permalink / raw)
To: Rusocki, Krzysztof, devel@edk2.groups.io
Cc: Nikodem, Damian, Dong, Eric, Wang, Jian J, Laszlo Ersek
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Rusocki, Krzysztof
> Sent: Tuesday, August 6, 2019 5:58 AM
> To: devel@edk2.groups.io
> Cc: Nikodem, Damian <damian.nikodem@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; Rusocki,
> Krzysztof <krzysztof.rusocki@intel.com>
> Subject: UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that
> are required to handle current page fault
>
> From: Damian Nikodem <damian.nikodem@intel.com>
>
> Reclaim may free page table pages that are required to handle current page
> fault. This causes a page leak, and, after sufficent number of specific page
> fault+reclaim pairs, we run out of reclaimable pages and hit:
>
> ASSERT (MinAcc != (UINT64)-1);
>
> To remedy, prevent pages essential to handling current page fault:
> (1) from being considered as reclaim candidates (first reclaim phase)
> (2) from being freed as part of "branch cleanup" (second reclaim phase)
>
> Signed-off-by: Damian Nikodem <damian.nikodem@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Krzysztof Rusocki <krzysztof.rusocki@intel.com>
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index a3b62f7787..f11323f439 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -543,6 +543,11 @@ ReclaimPages (
> UINT64 *ReleasePageAddress;
> IA32_CR4 Cr4;
> BOOLEAN Enable5LevelPaging;
> + UINT64 PFAddress;
> + UINT64 PFAddressPml5Index;
> + UINT64 PFAddressPml4Index;
> + UINT64 PFAddressPdptIndex;
> + UINT64 PFAddressPdtIndex;
>
> Pml4 = NULL;
> Pdpt = NULL;
> @@ -554,6 +559,11 @@ ReclaimPages (
> MinPdt = (UINTN)-1;
> Acc = 0;
> ReleasePageAddress = 0;
> + PFAddress = AsmReadCr2 ();
> + PFAddressPml5Index = BitFieldRead64 (PFAddress, 48, 48 + 8);
> + PFAddressPml4Index = BitFieldRead64 (PFAddress, 39, 39 + 8);
> + PFAddressPdptIndex = BitFieldRead64 (PFAddress, 30, 30 + 8);
> + PFAddressPdtIndex = BitFieldRead64 (PFAddress, 21, 21 + 8);
>
> Cr4.UintN = AsmReadCr4 ();
> Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1); @@ -628,40 +638,46
> @@ ReclaimPages (
> // we will find the entry has the smallest access record value
> //
> PDPTEIgnore = TRUE;
> - Acc = GetAndUpdateAccNum (Pdt + PdtIndex);
> + if (PdtIndex != PFAddressPdtIndex || PdptIndex !=
> PFAddressPdptIndex ||
> + Pml4Index != PFAddressPml4Index || Pml5Index !=
> PFAddressPml5Index) {
> + Acc = GetAndUpdateAccNum (Pdt + PdtIndex);
> + if (Acc < MinAcc) {
> + //
> + // If the PD entry has the smallest access record value,
> + // save the Page address to be released
> + //
> + MinAcc = Acc;
> + MinPml5 = Pml5Index;
> + MinPml4 = Pml4Index;
> + MinPdpt = PdptIndex;
> + MinPdt = PdtIndex;
> + ReleasePageAddress = Pdt + PdtIndex;
> + }
> + }
> + }
> + }
> + if (!PDPTEIgnore) {
> + //
> + // If this PDPT entry has no PDT entries pointer to 4 KByte pages,
> + // it should only has the entries point to 2 MByte Pages
> + //
> + if (PdptIndex != PFAddressPdptIndex || Pml4Index !=
> PFAddressPml4Index ||
> + Pml5Index != PFAddressPml5Index) {
> + Acc = GetAndUpdateAccNum (Pdpt + PdptIndex);
> if (Acc < MinAcc) {
> //
> - // If the PD entry has the smallest access record value,
> + // If the PDPT entry has the smallest access record
> + value,
> // save the Page address to be released
> //
> MinAcc = Acc;
> MinPml5 = Pml5Index;
> MinPml4 = Pml4Index;
> MinPdpt = PdptIndex;
> - MinPdt = PdtIndex;
> - ReleasePageAddress = Pdt + PdtIndex;
> + MinPdt = (UINTN)-1;
> + ReleasePageAddress = Pdpt + PdptIndex;
> }
> }
> }
> - if (!PDPTEIgnore) {
> - //
> - // If this PDPT entry has no PDT entries pointer to 4 KByte pages,
> - // it should only has the entries point to 2 MByte Pages
> - //
> - Acc = GetAndUpdateAccNum (Pdpt + PdptIndex);
> - if (Acc < MinAcc) {
> - //
> - // If the PDPT entry has the smallest access record value,
> - // save the Page address to be released
> - //
> - MinAcc = Acc;
> - MinPml5 = Pml5Index;
> - MinPml4 = Pml4Index;
> - MinPdpt = PdptIndex;
> - MinPdt = (UINTN)-1;
> - ReleasePageAddress = Pdpt + PdptIndex;
> - }
> - }
> }
> }
> if (!PML4EIgnore) {
> @@ -669,18 +685,20 @@ ReclaimPages (
> // If PML4 entry has no the PDPT entry pointer to 2 MByte pages,
> // it should only has the entries point to 1 GByte Pages
> //
> - Acc = GetAndUpdateAccNum (Pml4 + Pml4Index);
> - if (Acc < MinAcc) {
> - //
> - // If the PML4 entry has the smallest access record value,
> - // save the Page address to be released
> - //
> - MinAcc = Acc;
> - MinPml5 = Pml5Index;
> - MinPml4 = Pml4Index;
> - MinPdpt = (UINTN)-1;
> - MinPdt = (UINTN)-1;
> - ReleasePageAddress = Pml4 + Pml4Index;
> + if (Pml4Index != PFAddressPml4Index || Pml5Index !=
> PFAddressPml5Index) {
> + Acc = GetAndUpdateAccNum (Pml4 + Pml4Index);
> + if (Acc < MinAcc) {
> + //
> + // If the PML4 entry has the smallest access record value,
> + // save the Page address to be released
> + //
> + MinAcc = Acc;
> + MinPml5 = Pml5Index;
> + MinPml4 = Pml4Index;
> + MinPdpt = (UINTN)-1;
> + MinPdt = (UINTN)-1;
> + ReleasePageAddress = Pml4 + Pml4Index;
> + }
> }
> }
> }
> @@ -708,7 +726,8 @@ ReclaimPages (
> Pml4 = (UINT64 *) (UINTN) (Pml5[MinPml5] & gPhyMask);
> Pdpt = (UINT64*)(UINTN)(Pml4[MinPml4] & ~mAddressEncMask &
> gPhyMask);
> SubEntriesNum = GetSubEntriesNum(Pdpt + MinPdpt);
> - if (SubEntriesNum == 0) {
> + if (SubEntriesNum == 0 &&
> + (MinPdpt != PFAddressPdptIndex || MinPml4 !=
> + PFAddressPml4Index || MinPml5 != PFAddressPml5Index)) {
> //
> // Release the empty Page Directory table if there was no more 4 KByte
> Page Table entry
> // clear the Page directory entry @@ -724,7 +743,7 @@ ReclaimPages (
> //
> // Update the sub-entries filed in PDPT entry and exit
> //
> - SetSubEntriesNum (Pdpt + MinPdpt, SubEntriesNum - 1);
> + SetSubEntriesNum (Pdpt + MinPdpt, (SubEntriesNum - 1) & 0x1FF);
> break;
> }
> if (MinPdpt != (UINTN)-1) {
> @@ -732,7 +751,7 @@ ReclaimPages (
> // One 2MB Page Table is released or Page Directory table is released,
> check the PML4 entry
> //
> SubEntriesNum = GetSubEntriesNum (Pml4 + MinPml4);
> - if (SubEntriesNum == 0) {
> + if (SubEntriesNum == 0 && (MinPml4 != PFAddressPml4Index ||
> + MinPml5 != PFAddressPml5Index)) {
> //
> // Release the empty PML4 table if there was no more 1G KByte Page
> Table entry
> // clear the Page directory entry @@ -745,7 +764,7 @@ ReclaimPages (
> //
> // Update the sub-entries filed in PML4 entry and exit
> //
> - SetSubEntriesNum (Pml4 + MinPml4, SubEntriesNum - 1);
> + SetSubEntriesNum (Pml4 + MinPml4, (SubEntriesNum - 1) & 0x1FF);
> break;
> }
> //
> @@ -918,7 +937,7 @@ SmiDefaultPFHandler (
> PageTable[PTIndex] = ((PFAddress | mAddressEncMask) & gPhyMask &
> ~((1ull << EndBit) - 1)) |
> PageAttribute | IA32_PG_A | PAGE_ATTRIBUTE_BITS;
> if (UpperEntry != NULL) {
> - SetSubEntriesNum (UpperEntry, GetSubEntriesNum (UpperEntry) + 1);
> + SetSubEntriesNum (UpperEntry, (GetSubEntriesNum (UpperEntry) + 1)
> + & 0x1FF);
> }
> //
> // Get the next page address if we need to create more page tables
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to handle current page fault
2019-08-05 21:57 UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to handle current page fault Krzysztof Rusocki
2019-08-06 1:56 ` Ni, Ray
@ 2019-08-06 2:02 ` Wang, Jian J
2019-08-07 16:43 ` Laszlo Ersek
2 siblings, 0 replies; 4+ messages in thread
From: Wang, Jian J @ 2019-08-06 2:02 UTC (permalink / raw)
To: Rusocki, Krzysztof, devel@edk2.groups.io
Cc: Nikodem, Damian, Dong, Eric, Ni, Ray, Laszlo Ersek
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
> -----Original Message-----
> From: Rusocki, Krzysztof
> Sent: Tuesday, August 06, 2019 5:58 AM
> To: devel@edk2.groups.io
> Cc: Nikodem, Damian <damian.nikodem@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; Rusocki,
> Krzysztof <krzysztof.rusocki@intel.com>
> Subject: UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that
> are required to handle current page fault
>
> From: Damian Nikodem <damian.nikodem@intel.com>
>
> Reclaim may free page table pages that are required to handle current page
> fault. This causes a page leak, and, after sufficent number of specific
> page fault+reclaim pairs, we run out of reclaimable pages and hit:
>
> ASSERT (MinAcc != (UINT64)-1);
>
> To remedy, prevent pages essential to handling current page fault:
> (1) from being considered as reclaim candidates (first reclaim phase)
> (2) from being freed as part of "branch cleanup" (second reclaim phase)
>
> Signed-off-by: Damian Nikodem <damian.nikodem@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Krzysztof Rusocki <krzysztof.rusocki@intel.com>
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index a3b62f7787..f11323f439 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -543,6 +543,11 @@ ReclaimPages (
> UINT64 *ReleasePageAddress;
> IA32_CR4 Cr4;
> BOOLEAN Enable5LevelPaging;
> + UINT64 PFAddress;
> + UINT64 PFAddressPml5Index;
> + UINT64 PFAddressPml4Index;
> + UINT64 PFAddressPdptIndex;
> + UINT64 PFAddressPdtIndex;
>
> Pml4 = NULL;
> Pdpt = NULL;
> @@ -554,6 +559,11 @@ ReclaimPages (
> MinPdt = (UINTN)-1;
> Acc = 0;
> ReleasePageAddress = 0;
> + PFAddress = AsmReadCr2 ();
> + PFAddressPml5Index = BitFieldRead64 (PFAddress, 48, 48 + 8);
> + PFAddressPml4Index = BitFieldRead64 (PFAddress, 39, 39 + 8);
> + PFAddressPdptIndex = BitFieldRead64 (PFAddress, 30, 30 + 8);
> + PFAddressPdtIndex = BitFieldRead64 (PFAddress, 21, 21 + 8);
>
> Cr4.UintN = AsmReadCr4 ();
> Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> @@ -628,40 +638,46 @@ ReclaimPages (
> // we will find the entry has the smallest access record value
> //
> PDPTEIgnore = TRUE;
> - Acc = GetAndUpdateAccNum (Pdt + PdtIndex);
> + if (PdtIndex != PFAddressPdtIndex || PdptIndex !=
> PFAddressPdptIndex ||
> + Pml4Index != PFAddressPml4Index || Pml5Index !=
> PFAddressPml5Index) {
> + Acc = GetAndUpdateAccNum (Pdt + PdtIndex);
> + if (Acc < MinAcc) {
> + //
> + // If the PD entry has the smallest access record value,
> + // save the Page address to be released
> + //
> + MinAcc = Acc;
> + MinPml5 = Pml5Index;
> + MinPml4 = Pml4Index;
> + MinPdpt = PdptIndex;
> + MinPdt = PdtIndex;
> + ReleasePageAddress = Pdt + PdtIndex;
> + }
> + }
> + }
> + }
> + if (!PDPTEIgnore) {
> + //
> + // If this PDPT entry has no PDT entries pointer to 4 KByte pages,
> + // it should only has the entries point to 2 MByte Pages
> + //
> + if (PdptIndex != PFAddressPdptIndex || Pml4Index !=
> PFAddressPml4Index ||
> + Pml5Index != PFAddressPml5Index) {
> + Acc = GetAndUpdateAccNum (Pdpt + PdptIndex);
> if (Acc < MinAcc) {
> //
> - // If the PD entry has the smallest access record value,
> + // If the PDPT entry has the smallest access record value,
> // save the Page address to be released
> //
> MinAcc = Acc;
> MinPml5 = Pml5Index;
> MinPml4 = Pml4Index;
> MinPdpt = PdptIndex;
> - MinPdt = PdtIndex;
> - ReleasePageAddress = Pdt + PdtIndex;
> + MinPdt = (UINTN)-1;
> + ReleasePageAddress = Pdpt + PdptIndex;
> }
> }
> }
> - if (!PDPTEIgnore) {
> - //
> - // If this PDPT entry has no PDT entries pointer to 4 KByte pages,
> - // it should only has the entries point to 2 MByte Pages
> - //
> - Acc = GetAndUpdateAccNum (Pdpt + PdptIndex);
> - if (Acc < MinAcc) {
> - //
> - // If the PDPT entry has the smallest access record value,
> - // save the Page address to be released
> - //
> - MinAcc = Acc;
> - MinPml5 = Pml5Index;
> - MinPml4 = Pml4Index;
> - MinPdpt = PdptIndex;
> - MinPdt = (UINTN)-1;
> - ReleasePageAddress = Pdpt + PdptIndex;
> - }
> - }
> }
> }
> if (!PML4EIgnore) {
> @@ -669,18 +685,20 @@ ReclaimPages (
> // If PML4 entry has no the PDPT entry pointer to 2 MByte pages,
> // it should only has the entries point to 1 GByte Pages
> //
> - Acc = GetAndUpdateAccNum (Pml4 + Pml4Index);
> - if (Acc < MinAcc) {
> - //
> - // If the PML4 entry has the smallest access record value,
> - // save the Page address to be released
> - //
> - MinAcc = Acc;
> - MinPml5 = Pml5Index;
> - MinPml4 = Pml4Index;
> - MinPdpt = (UINTN)-1;
> - MinPdt = (UINTN)-1;
> - ReleasePageAddress = Pml4 + Pml4Index;
> + if (Pml4Index != PFAddressPml4Index || Pml5Index !=
> PFAddressPml5Index) {
> + Acc = GetAndUpdateAccNum (Pml4 + Pml4Index);
> + if (Acc < MinAcc) {
> + //
> + // If the PML4 entry has the smallest access record value,
> + // save the Page address to be released
> + //
> + MinAcc = Acc;
> + MinPml5 = Pml5Index;
> + MinPml4 = Pml4Index;
> + MinPdpt = (UINTN)-1;
> + MinPdt = (UINTN)-1;
> + ReleasePageAddress = Pml4 + Pml4Index;
> + }
> }
> }
> }
> @@ -708,7 +726,8 @@ ReclaimPages (
> Pml4 = (UINT64 *) (UINTN) (Pml5[MinPml5] & gPhyMask);
> Pdpt = (UINT64*)(UINTN)(Pml4[MinPml4] & ~mAddressEncMask &
> gPhyMask);
> SubEntriesNum = GetSubEntriesNum(Pdpt + MinPdpt);
> - if (SubEntriesNum == 0) {
> + if (SubEntriesNum == 0 &&
> + (MinPdpt != PFAddressPdptIndex || MinPml4 !=
> PFAddressPml4Index || MinPml5 != PFAddressPml5Index)) {
> //
> // Release the empty Page Directory table if there was no more 4
> KByte Page Table entry
> // clear the Page directory entry
> @@ -724,7 +743,7 @@ ReclaimPages (
> //
> // Update the sub-entries filed in PDPT entry and exit
> //
> - SetSubEntriesNum (Pdpt + MinPdpt, SubEntriesNum - 1);
> + SetSubEntriesNum (Pdpt + MinPdpt, (SubEntriesNum - 1) & 0x1FF);
> break;
> }
> if (MinPdpt != (UINTN)-1) {
> @@ -732,7 +751,7 @@ ReclaimPages (
> // One 2MB Page Table is released or Page Directory table is released,
> check the PML4 entry
> //
> SubEntriesNum = GetSubEntriesNum (Pml4 + MinPml4);
> - if (SubEntriesNum == 0) {
> + if (SubEntriesNum == 0 && (MinPml4 != PFAddressPml4Index ||
> MinPml5 != PFAddressPml5Index)) {
> //
> // Release the empty PML4 table if there was no more 1G KByte Page
> Table entry
> // clear the Page directory entry
> @@ -745,7 +764,7 @@ ReclaimPages (
> //
> // Update the sub-entries filed in PML4 entry and exit
> //
> - SetSubEntriesNum (Pml4 + MinPml4, SubEntriesNum - 1);
> + SetSubEntriesNum (Pml4 + MinPml4, (SubEntriesNum - 1) & 0x1FF);
> break;
> }
> //
> @@ -918,7 +937,7 @@ SmiDefaultPFHandler (
> PageTable[PTIndex] = ((PFAddress | mAddressEncMask) & gPhyMask &
> ~((1ull << EndBit) - 1)) |
> PageAttribute | IA32_PG_A | PAGE_ATTRIBUTE_BITS;
> if (UpperEntry != NULL) {
> - SetSubEntriesNum (UpperEntry, GetSubEntriesNum (UpperEntry) + 1);
> + SetSubEntriesNum (UpperEntry, (GetSubEntriesNum (UpperEntry) + 1)
> & 0x1FF);
> }
> //
> // Get the next page address if we need to create more page tables
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to handle current page fault
2019-08-05 21:57 UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to handle current page fault Krzysztof Rusocki
2019-08-06 1:56 ` Ni, Ray
2019-08-06 2:02 ` Wang, Jian J
@ 2019-08-07 16:43 ` Laszlo Ersek
2 siblings, 0 replies; 4+ messages in thread
From: Laszlo Ersek @ 2019-08-07 16:43 UTC (permalink / raw)
To: Krzysztof Rusocki, devel; +Cc: Damian Nikodem, Eric Dong, Ray Ni, Jian J Wang
On 08/05/19 23:57, Krzysztof Rusocki wrote:
> From: Damian Nikodem <damian.nikodem@intel.com>
>
> Reclaim may free page table pages that are required to handle current page
> fault. This causes a page leak, and, after sufficent number of specific
> page fault+reclaim pairs, we run out of reclaimable pages and hit:
>
> ASSERT (MinAcc != (UINT64)-1);
>
> To remedy, prevent pages essential to handling current page fault:
> (1) from being considered as reclaim candidates (first reclaim phase)
> (2) from being freed as part of "branch cleanup" (second reclaim phase)
>
> Signed-off-by: Damian Nikodem <damian.nikodem@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Krzysztof Rusocki <krzysztof.rusocki@intel.com>
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index a3b62f7787..f11323f439 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -543,6 +543,11 @@ ReclaimPages (
> UINT64 *ReleasePageAddress;
> IA32_CR4 Cr4;
> BOOLEAN Enable5LevelPaging;
> + UINT64 PFAddress;
> + UINT64 PFAddressPml5Index;
> + UINT64 PFAddressPml4Index;
> + UINT64 PFAddressPdptIndex;
> + UINT64 PFAddressPdtIndex;
>
> Pml4 = NULL;
> Pdpt = NULL;
> @@ -554,6 +559,11 @@ ReclaimPages (
> MinPdt = (UINTN)-1;
> Acc = 0;
> ReleasePageAddress = 0;
> + PFAddress = AsmReadCr2 ();
> + PFAddressPml5Index = BitFieldRead64 (PFAddress, 48, 48 + 8);
> + PFAddressPml4Index = BitFieldRead64 (PFAddress, 39, 39 + 8);
> + PFAddressPdptIndex = BitFieldRead64 (PFAddress, 30, 30 + 8);
> + PFAddressPdtIndex = BitFieldRead64 (PFAddress, 21, 21 + 8);
>
> Cr4.UintN = AsmReadCr4 ();
> Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> @@ -628,40 +638,46 @@ ReclaimPages (
> // we will find the entry has the smallest access record value
> //
> PDPTEIgnore = TRUE;
> - Acc = GetAndUpdateAccNum (Pdt + PdtIndex);
> + if (PdtIndex != PFAddressPdtIndex || PdptIndex != PFAddressPdptIndex ||
> + Pml4Index != PFAddressPml4Index || Pml5Index != PFAddressPml5Index) {
> + Acc = GetAndUpdateAccNum (Pdt + PdtIndex);
> + if (Acc < MinAcc) {
> + //
> + // If the PD entry has the smallest access record value,
> + // save the Page address to be released
> + //
> + MinAcc = Acc;
> + MinPml5 = Pml5Index;
> + MinPml4 = Pml4Index;
> + MinPdpt = PdptIndex;
> + MinPdt = PdtIndex;
> + ReleasePageAddress = Pdt + PdtIndex;
> + }
> + }
> + }
> + }
> + if (!PDPTEIgnore) {
> + //
> + // If this PDPT entry has no PDT entries pointer to 4 KByte pages,
> + // it should only has the entries point to 2 MByte Pages
> + //
> + if (PdptIndex != PFAddressPdptIndex || Pml4Index != PFAddressPml4Index ||
> + Pml5Index != PFAddressPml5Index) {
> + Acc = GetAndUpdateAccNum (Pdpt + PdptIndex);
> if (Acc < MinAcc) {
> //
> - // If the PD entry has the smallest access record value,
> + // If the PDPT entry has the smallest access record value,
> // save the Page address to be released
> //
> MinAcc = Acc;
> MinPml5 = Pml5Index;
> MinPml4 = Pml4Index;
> MinPdpt = PdptIndex;
> - MinPdt = PdtIndex;
> - ReleasePageAddress = Pdt + PdtIndex;
> + MinPdt = (UINTN)-1;
> + ReleasePageAddress = Pdpt + PdptIndex;
> }
> }
> }
> - if (!PDPTEIgnore) {
> - //
> - // If this PDPT entry has no PDT entries pointer to 4 KByte pages,
> - // it should only has the entries point to 2 MByte Pages
> - //
> - Acc = GetAndUpdateAccNum (Pdpt + PdptIndex);
> - if (Acc < MinAcc) {
> - //
> - // If the PDPT entry has the smallest access record value,
> - // save the Page address to be released
> - //
> - MinAcc = Acc;
> - MinPml5 = Pml5Index;
> - MinPml4 = Pml4Index;
> - MinPdpt = PdptIndex;
> - MinPdt = (UINTN)-1;
> - ReleasePageAddress = Pdpt + PdptIndex;
> - }
> - }
> }
> }
> if (!PML4EIgnore) {
> @@ -669,18 +685,20 @@ ReclaimPages (
> // If PML4 entry has no the PDPT entry pointer to 2 MByte pages,
> // it should only has the entries point to 1 GByte Pages
> //
> - Acc = GetAndUpdateAccNum (Pml4 + Pml4Index);
> - if (Acc < MinAcc) {
> - //
> - // If the PML4 entry has the smallest access record value,
> - // save the Page address to be released
> - //
> - MinAcc = Acc;
> - MinPml5 = Pml5Index;
> - MinPml4 = Pml4Index;
> - MinPdpt = (UINTN)-1;
> - MinPdt = (UINTN)-1;
> - ReleasePageAddress = Pml4 + Pml4Index;
> + if (Pml4Index != PFAddressPml4Index || Pml5Index != PFAddressPml5Index) {
> + Acc = GetAndUpdateAccNum (Pml4 + Pml4Index);
> + if (Acc < MinAcc) {
> + //
> + // If the PML4 entry has the smallest access record value,
> + // save the Page address to be released
> + //
> + MinAcc = Acc;
> + MinPml5 = Pml5Index;
> + MinPml4 = Pml4Index;
> + MinPdpt = (UINTN)-1;
> + MinPdt = (UINTN)-1;
> + ReleasePageAddress = Pml4 + Pml4Index;
> + }
> }
> }
> }
> @@ -708,7 +726,8 @@ ReclaimPages (
> Pml4 = (UINT64 *) (UINTN) (Pml5[MinPml5] & gPhyMask);
> Pdpt = (UINT64*)(UINTN)(Pml4[MinPml4] & ~mAddressEncMask & gPhyMask);
> SubEntriesNum = GetSubEntriesNum(Pdpt + MinPdpt);
> - if (SubEntriesNum == 0) {
> + if (SubEntriesNum == 0 &&
> + (MinPdpt != PFAddressPdptIndex || MinPml4 != PFAddressPml4Index || MinPml5 != PFAddressPml5Index)) {
> //
> // Release the empty Page Directory table if there was no more 4 KByte Page Table entry
> // clear the Page directory entry
> @@ -724,7 +743,7 @@ ReclaimPages (
> //
> // Update the sub-entries filed in PDPT entry and exit
> //
> - SetSubEntriesNum (Pdpt + MinPdpt, SubEntriesNum - 1);
> + SetSubEntriesNum (Pdpt + MinPdpt, (SubEntriesNum - 1) & 0x1FF);
> break;
> }
> if (MinPdpt != (UINTN)-1) {
> @@ -732,7 +751,7 @@ ReclaimPages (
> // One 2MB Page Table is released or Page Directory table is released, check the PML4 entry
> //
> SubEntriesNum = GetSubEntriesNum (Pml4 + MinPml4);
> - if (SubEntriesNum == 0) {
> + if (SubEntriesNum == 0 && (MinPml4 != PFAddressPml4Index || MinPml5 != PFAddressPml5Index)) {
> //
> // Release the empty PML4 table if there was no more 1G KByte Page Table entry
> // clear the Page directory entry
> @@ -745,7 +764,7 @@ ReclaimPages (
> //
> // Update the sub-entries filed in PML4 entry and exit
> //
> - SetSubEntriesNum (Pml4 + MinPml4, SubEntriesNum - 1);
> + SetSubEntriesNum (Pml4 + MinPml4, (SubEntriesNum - 1) & 0x1FF);
> break;
> }
> //
> @@ -918,7 +937,7 @@ SmiDefaultPFHandler (
> PageTable[PTIndex] = ((PFAddress | mAddressEncMask) & gPhyMask & ~((1ull << EndBit) - 1)) |
> PageAttribute | IA32_PG_A | PAGE_ATTRIBUTE_BITS;
> if (UpperEntry != NULL) {
> - SetSubEntriesNum (UpperEntry, GetSubEntriesNum (UpperEntry) + 1);
> + SetSubEntriesNum (UpperEntry, (GetSubEntriesNum (UpperEntry) + 1) & 0x1FF);
> }
> //
> // Get the next page address if we need to create more page tables
> --------------------------------------------------------------------
>
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
>
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
> przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
> others is strictly prohibited.
>
got no capacity left for this; please go ahead with the reviews you
already got
thanks
Laszlo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-07 16:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-05 21:57 UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to handle current page fault Krzysztof Rusocki
2019-08-06 1:56 ` Ni, Ray
2019-08-06 2:02 ` Wang, Jian J
2019-08-07 16:43 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox