public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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

  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