From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5009E81EB9 for ; Wed, 23 Nov 2016 18:01:41 -0800 (PST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga103.jf.intel.com with ESMTP; 23 Nov 2016 18:01:26 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,689,1473145200"; d="scan'208";a="1072809117" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga001.fm.intel.com with ESMTP; 23 Nov 2016 18:01:25 -0800 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 23 Nov 2016 18:01:25 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX151.amr.corp.intel.com (10.18.125.4) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 23 Nov 2016 18:01:25 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.142]) with mapi id 14.03.0248.002; Thu, 24 Nov 2016 10:01:21 +0800 From: "Fan, Jeff" To: Laszlo Ersek , "Yao, Jiewen" , "edk2-devel@ml01.01.org" CC: "Kinney, Michael D" Thread-Topic: [PATCH V3] UefiCpuPkg/PiSmmCpu: Correct exception message. Thread-Index: AQHSRZWwd77/fqPcEkevdWIefkx7D6DmPogAgAEjYsA= Date: Thu, 24 Nov 2016 02:01:20 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4A2E67DB@shsmsx102.ccr.corp.intel.com> References: <1479911218-34804-1-git-send-email-jiewen.yao@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMDNjZmM5YjAtNDI4My00MDQ1LWFhM2QtYWI5NWIyNjE0ZDQzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6ImhVbkpBUW5LQXlqcVFwcUJxclZHTURVaTFreVwvUm1BU1FCTHdjaEpaZXRZPSJ9 x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH V3] UefiCpuPkg/PiSmmCpu: Correct exception message. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Nov 2016 02:01:41 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Jeff Fan with Laszlo's comment. -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com]=20 Sent: Thursday, November 24, 2016 12:38 AM To: Yao, Jiewen; edk2-devel@ml01.01.org Cc: Fan, Jeff; Kinney, Michael D Subject: Re: [PATCH V3] UefiCpuPkg/PiSmmCpu: Correct exception message. On 11/23/16 15:26, Jiewen Yao wrote: > This patch fixes the first part of > https://bugzilla.tianocore.org/show_bug.cgi?id=3D242 >=20 > Previously, when SMM exception happens, "stack overflow" is misreported. > This patch checked the PF address to see it is stack overflow, or it=20 > is caused by SMM page protection. >=20 > It dumps exception data, PF address and the module trigger the issue. >=20 > Cc: Laszlo Ersek > Cc: Jeff Fan > Cc: Michael D Kinney > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 38 ++++++++++++++++++-- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 9 +++++ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 37 ++++++++++++++++--- > 3 files changed, 77 insertions(+), 7 deletions(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c=20 > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > index 5033bc5..39e6c9a 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > @@ -91,6 +91,8 @@ SmiPFHandler ( > ) > { > UINTN PFAddress; > + UINTN GuardPageAddress; > + UINTN CpuIndex; > =20 > ASSERT (InterruptType =3D=3D EXCEPT_IA32_PAGE_FAULT); > =20 > @@ -98,10 +100,40 @@ SmiPFHandler ( > =20 > PFAddress =3D AsmReadCr2 (); > =20 > - if ((FeaturePcdGet (PcdCpuSmmStackGuard)) && > - (PFAddress >=3D mCpuHotPlugData.SmrrBase) && > + // > + // If a page fault occurs in SMRAM range, it might be in a SMM=20 > + stack guard page, // or SMM page protection violation. > + // > + if ((PFAddress >=3D mCpuHotPlugData.SmrrBase) && > (PFAddress < (mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize)= )) { > - DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n")); > + CpuIndex =3D GetCpuIndex (); > + GuardPageAddress =3D (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex = * mSmmStackSize); > + if ((FeaturePcdGet (PcdCpuSmmStackGuard)) && > + (PFAddress >=3D GuardPageAddress) && > + (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) { > + DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n")); > + } else { > + DEBUG ((DEBUG_ERROR, "SMM exception data - 0x%x(", SystemContext.S= ystemContextIa32->ExceptionData)); > + DEBUG ((DEBUG_ERROR, "I:%x, R:%x, U:%x, W:%X, P:%x", > + (SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID)= !=3D 0, > + (SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_RSV= D) !=3D 0, > + (SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_US)= !=3D 0, > + (SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_WR)= !=3D 0, > + (SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_P) = !=3D 0 > + )); > + DEBUG ((DEBUG_ERROR, ")\n",=20 > + SystemContext.SystemContextIa32->ExceptionData)); The last argument in this DEBUG call can be removed. It causes no bugs (it = is simply ignored), but it would be nice to remove it. No need to repost just for this. > + if ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_I= D) !=3D 0) { > + DEBUG ((DEBUG_ERROR, "SMM exception at execution (0x%x)\n", PFAd= dress)); > + DEBUG_CODE ( > + DumpModuleInfoByIp (*(UINTN *)(UINTN)SystemContext.SystemConte= xtIa32->Esp); > + ); > + } else { > + DEBUG ((DEBUG_ERROR, "SMM exception at access (0x%x)\n", PFAddre= ss)); > + DEBUG_CODE ( > + DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Ei= p); > + ); > + } > + } > CpuDeadLoop (); > } > =20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h=20 > b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h > index b6fb5cf..04a3dfb 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h > @@ -105,6 +105,15 @@ InitPaging ( > VOID > ); > =20 > +/** > + Get CPU Index from APIC ID. > + > +**/ > +UINTN > +GetCpuIndex ( > + VOID > + ); > + > // > // The flag indicates if execute-disable is supported by processor. > // > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c=20 > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 531e188..94f2e03 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -804,6 +804,8 @@ SmiPFHandler ( > ) > { > UINTN PFAddress; > + UINTN GuardPageAddress; > + UINTN CpuIndex; > =20 > ASSERT (InterruptType =3D=3D EXCEPT_IA32_PAGE_FAULT); > =20 > @@ -817,12 +819,39 @@ SmiPFHandler ( > } > =20 > // > - // If a page fault occurs in SMRAM range, it should be in a SMM stack = guard page. > + // If a page fault occurs in SMRAM range, it might be in a SMM=20 > + stack guard page, // or SMM page protection violation. > // > - if ((FeaturePcdGet (PcdCpuSmmStackGuard)) && > - (PFAddress >=3D mCpuHotPlugData.SmrrBase) && > + if ((PFAddress >=3D mCpuHotPlugData.SmrrBase) && > (PFAddress < (mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize)= )) { > - DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n")); > + CpuIndex =3D GetCpuIndex (); > + GuardPageAddress =3D (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex = * mSmmStackSize); > + if ((FeaturePcdGet (PcdCpuSmmStackGuard)) && > + (PFAddress >=3D GuardPageAddress) && > + (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) { > + DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n")); > + } else { > + DEBUG ((DEBUG_ERROR, "SMM exception data - 0x%lx(", SystemContext.= SystemContextX64->ExceptionData)); > + DEBUG ((DEBUG_ERROR, "I:%x, R:%x, U:%x, W:%X, P:%x", > + (SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_ID) = !=3D 0, > + (SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_RSVD= ) !=3D 0, > + (SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_US) = !=3D 0, > + (SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_WR) = !=3D 0, > + (SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_P) != =3D 0 > + )); > + DEBUG ((DEBUG_ERROR, ")\n",=20 > + SystemContext.SystemContextX64->ExceptionData)); Same comment as above: the last argument is superfluous. > + if ((SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_ID= ) !=3D 0) { > + DEBUG ((DEBUG_ERROR, "SMM exception at execution (0x%lx)\n", PFA= ddress)); > + DEBUG_CODE ( > + DumpModuleInfoByIp (*(UINTN *)(UINTN)SystemContext.SystemConte= xtX64->Rsp); > + ); > + } else { > + DEBUG ((DEBUG_ERROR, "SMM exception at access (0x%lx)\n", PFAddr= ess)); > + DEBUG_CODE ( > + DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip= ); > + ); > + } > + } > CpuDeadLoop (); > } > =20 >=20 Another thing I noticed: in the following format string, which is used in t= wo places: "I:%x, R:%x, U:%x, W:%X, P:%x" ^ | the %X that stands after W is upper-case, while the rest is lower-case. It is entirely correct of course, but you might want to lower-case it for c= onsistency, before pushing the patch. Reviewed-by: Laszlo Ersek Tested-by: Laszlo Ersek Very nice patch, thank you for it! Laszlo