From: "Ni, Ray" <ray.ni@intel.com>
To: "Rusocki, Krzysztof" <krzysztof.rusocki@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Nikodem, Damian" <damian.nikodem@intel.com>,
"Dong, Eric" <eric.dong@intel.com>,
"Wang, Jian J" <jian.j.wang@intel.com>,
Laszlo Ersek <lersek@redhat.com>
Subject: Re: UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to handle current page fault
Date: Tue, 6 Aug 2019 01:56:20 +0000 [thread overview]
Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C2726A4@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20190805215732.22052-1-krzysztof.rusocki@intel.com>
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
next prev parent reply other threads:[~2019-08-06 1:56 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2019-08-06 2:02 ` Wang, Jian J
2019-08-07 16:43 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=734D49CCEBEEF84792F5B80ED585239D5C2726A4@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox