From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: jian.j.wang@intel.com) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by groups.io with SMTP; Mon, 05 Aug 2019 19:02:50 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Aug 2019 19:02:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,352,1559545200"; d="scan'208";a="373267066" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga005.fm.intel.com with ESMTP; 05 Aug 2019 19:02:50 -0700 Received: from fmsmsx605.amr.corp.intel.com (10.18.126.85) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 5 Aug 2019 19:02:48 -0700 Received: from fmsmsx605.amr.corp.intel.com (10.18.126.85) by fmsmsx605.amr.corp.intel.com (10.18.126.85) 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 19:02:48 -0700 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by fmsmsx605.amr.corp.intel.com (10.18.126.85) 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 19:02:47 -0700 Received: from shsmsx107.ccr.corp.intel.com ([169.254.9.65]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.215]) with mapi id 14.03.0439.000; Tue, 6 Aug 2019 10:02:45 +0800 From: "Wang, Jian J" To: "Rusocki, Krzysztof" , "devel@edk2.groups.io" CC: "Nikodem, Damian" , "Dong, Eric" , "Ni, Ray" , 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: AQHVS9jmOxAcnSx0c0uCU6RJBpFNR6btXliw Date: Tue, 6 Aug 2019 02:02:45 +0000 Message-ID: References: <20190805215732.22052-1-krzysztof.rusocki@intel.com> In-Reply-To: <20190805215732.22052-1-krzysztof.rusocki@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNWIxZDYwZmEtMTVkYi00ZGNlLTkyYWEtMTViYTdmNThiNWU5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiZkF5YnVSdUUwRkczMGlnU2tOR3F2UzZiNEZ6MEJSMTBvNUtFems5WXRxRGYyU2taMzVlVytvVlRoXC93QWVoSnkifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jian.j.wang@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Jian J Wang > -----Original Message----- > From: Rusocki, Krzysztof > Sent: Tuesday, August 06, 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 > page 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 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; > + 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 @@ 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 !=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 @@ 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] =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