From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: ray.ni@intel.com) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by groups.io with SMTP; Mon, 05 Aug 2019 18:56:41 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Aug 2019 18:56:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,352,1559545200"; d="scan'208";a="179015619" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga006.jf.intel.com with ESMTP; 05 Aug 2019 18:56:40 -0700 Received: from fmsmsx607.amr.corp.intel.com (10.18.126.87) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 5 Aug 2019 18:56:40 -0700 Received: from fmsmsx607.amr.corp.intel.com (10.18.126.87) by fmsmsx607.amr.corp.intel.com (10.18.126.87) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 5 Aug 2019 18:56:23 -0700 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by fmsmsx607.amr.corp.intel.com (10.18.126.87) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Mon, 5 Aug 2019 18:56:22 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.112]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.249]) with mapi id 14.03.0439.000; Tue, 6 Aug 2019 09:56:21 +0800 From: "Ni, Ray" To: "Rusocki, Krzysztof" , "devel@edk2.groups.io" CC: "Nikodem, Damian" , "Dong, Eric" , "Wang, Jian J" , Laszlo Ersek Subject: Re: UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to handle current page fault Thread-Topic: UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that are required to handle current page fault Thread-Index: AQHVS9jmjcq3bp3ui0W3fcOn1+qDUqbtXCbg Date: Tue, 6 Aug 2019 01:56:20 +0000 Deferred-Delivery: Tue, 6 Aug 2019 01:55:00 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C2726A4@SHSMSX104.ccr.corp.intel.com> References: <20190805215732.22052-1-krzysztof.rusocki@intel.com> In-Reply-To: <20190805215732.22052-1-krzysztof.rusocki@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Ray Ni > -----Original Message----- > From: Rusocki, Krzysztof > Sent: Tuesday, August 6, 2019 5:58 AM > To: devel@edk2.groups.io > Cc: Nikodem, Damian ; Dong, Eric > ; Ni, Ray ; Wang, Jian J > ; Laszlo Ersek ; Rusocki, > Krzysztof > Subject: UefiCpuPkg/PiSmmCpuDxeSmm: don't free page table pages that > are required to handle current page fault >=20 > From: Damian Nikodem >=20 > Reclaim may free page table pages that are required to handle current pag= e > fault. This causes a page leak, and, after sufficent number of specific p= age > fault+reclaim pairs, we run out of reclaimable pages and hit: >=20 > ASSERT (MinAcc !=3D (UINT64)-1); >=20 > 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) >=20 > Signed-off-by: Damian Nikodem > Cc: Eric Dong > Cc: Ray Ni > Cc: Jian J Wang > Cc: Laszlo Ersek > Cc: Krzysztof Rusocki >=20 > 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; >=20 > Pml4 =3D NULL; > Pdpt =3D NULL; > @@ -554,6 +559,11 @@ ReclaimPages ( > MinPdt =3D (UINTN)-1; > Acc =3D 0; > ReleasePageAddress =3D 0; > + PFAddress =3D AsmReadCr2 (); > + PFAddressPml5Index =3D BitFieldRead64 (PFAddress, 48, 48 + 8); > + PFAddressPml4Index =3D BitFieldRead64 (PFAddress, 39, 39 + 8); > + PFAddressPdptIndex =3D BitFieldRead64 (PFAddress, 30, 30 + 8); > + PFAddressPdtIndex =3D BitFieldRead64 (PFAddress, 21, 21 + 8); >=20 > Cr4.UintN =3D AsmReadCr4 (); > Enable5LevelPaging =3D (BOOLEAN) (Cr4.Bits.LA57 =3D=3D 1); @@ -628,40 = +638,46 > @@ ReclaimPages ( > // we will find the entry has the smallest access record v= alue > // > PDPTEIgnore =3D TRUE; > - Acc =3D GetAndUpdateAccNum (Pdt + PdtIndex); > + if (PdtIndex !=3D PFAddressPdtIndex || PdptIndex !=3D > PFAddressPdptIndex || > + Pml4Index !=3D PFAddressPml4Index || Pml5Index !=3D > PFAddressPml5Index) { > + Acc =3D GetAndUpdateAccNum (Pdt + PdtIndex); > + if (Acc < MinAcc) { > + // > + // If the PD entry has the smallest access record valu= e, > + // save the Page address to be released > + // > + MinAcc =3D Acc; > + MinPml5 =3D Pml5Index; > + MinPml4 =3D Pml4Index; > + MinPdpt =3D PdptIndex; > + MinPdt =3D PdtIndex; > + ReleasePageAddress =3D 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 !=3D PFAddressPdptIndex || Pml4Index !=3D > PFAddressPml4Index || > + Pml5Index !=3D PFAddressPml5Index) { > + Acc =3D 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 =3D Acc; > MinPml5 =3D Pml5Index; > MinPml4 =3D Pml4Index; > MinPdpt =3D PdptIndex; > - MinPdt =3D PdtIndex; > - ReleasePageAddress =3D Pdt + PdtIndex; > + MinPdt =3D (UINTN)-1; > + ReleasePageAddress =3D 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 =3D GetAndUpdateAccNum (Pdpt + PdptIndex); > - if (Acc < MinAcc) { > - // > - // If the PDPT entry has the smallest access record value, > - // save the Page address to be released > - // > - MinAcc =3D Acc; > - MinPml5 =3D Pml5Index; > - MinPml4 =3D Pml4Index; > - MinPdpt =3D PdptIndex; > - MinPdt =3D (UINTN)-1; > - ReleasePageAddress =3D 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 =3D GetAndUpdateAccNum (Pml4 + Pml4Index); > - if (Acc < MinAcc) { > - // > - // If the PML4 entry has the smallest access record value, > - // save the Page address to be released > - // > - MinAcc =3D Acc; > - MinPml5 =3D Pml5Index; > - MinPml4 =3D Pml4Index; > - MinPdpt =3D (UINTN)-1; > - MinPdt =3D (UINTN)-1; > - ReleasePageAddress =3D Pml4 + Pml4Index; > + if (Pml4Index !=3D PFAddressPml4Index || Pml5Index !=3D > PFAddressPml5Index) { > + Acc =3D GetAndUpdateAccNum (Pml4 + Pml4Index); > + if (Acc < MinAcc) { > + // > + // If the PML4 entry has the smallest access record value, > + // save the Page address to be released > + // > + MinAcc =3D Acc; > + MinPml5 =3D Pml5Index; > + MinPml4 =3D Pml4Index; > + MinPdpt =3D (UINTN)-1; > + MinPdt =3D (UINTN)-1; > + ReleasePageAddress =3D Pml4 + Pml4Index; > + } > } > } > } > @@ -708,7 +726,8 @@ ReclaimPages ( > Pml4 =3D (UINT64 *) (UINTN) (Pml5[MinPml5] & gPhyMask); > Pdpt =3D (UINT64*)(UINTN)(Pml4[MinPml4] & ~mAddressEncMask & > gPhyMask); > SubEntriesNum =3D GetSubEntriesNum(Pdpt + MinPdpt); > - if (SubEntriesNum =3D=3D 0) { > + if (SubEntriesNum =3D=3D 0 && > + (MinPdpt !=3D PFAddressPdptIndex || MinPml4 !=3D > + PFAddressPml4Index || MinPml5 !=3D 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 @@ ReclaimPag= es ( > // > // Update the sub-entries filed in PDPT entry and exit > // > - SetSubEntriesNum (Pdpt + MinPdpt, SubEntriesNum - 1); > + SetSubEntriesNum (Pdpt + MinPdpt, (SubEntriesNum - 1) & 0x1FF); > break; > } > if (MinPdpt !=3D (UINTN)-1) { > @@ -732,7 +751,7 @@ ReclaimPages ( > // One 2MB Page Table is released or Page Directory table is relea= sed, > check the PML4 entry > // > SubEntriesNum =3D GetSubEntriesNum (Pml4 + MinPml4); > - if (SubEntriesNum =3D=3D 0) { > + if (SubEntriesNum =3D=3D 0 && (MinPml4 !=3D PFAddressPml4Index || > + MinPml5 !=3D PFAddressPml5Index)) { > // > // Release the empty PML4 table if there was no more 1G KByte Pa= ge > Table entry > // clear the Page directory entry @@ -745,7 +764,7 @@ ReclaimPag= es ( > // > // 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] =3D ((PFAddress | mAddressEncMask) & gPhyMask & > ~((1ull << EndBit) - 1)) | > PageAttribute | IA32_PG_A | PAGE_ATTRIBUTE_BITS= ; > if (UpperEntry !=3D 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