* [PATCH] UefiCpuPkg/PiSmmCpu: Correct exception message.
@ 2016-11-22 11:47 Jiewen Yao
2016-11-22 21:17 ` Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Jiewen Yao @ 2016-11-22 11:47 UTC (permalink / raw)
To: edk2-devel; +Cc: Laszlo Ersek, Jeff Fan, Michael D Kinney
This patch fixes the first part of
https://bugzilla.tianocore.org/show_bug.cgi?id=242
Previously, when SMM exception happens, "stack overflow" is misreported.
This patch checked the PF address to see it is stack overflow, or
it is caused by SMM page protection.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 28 +++++++++++++++++---
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 9 +++++++
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 27 ++++++++++++++++---
3 files changed, 57 insertions(+), 7 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index 5033bc5..feca142 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -91,6 +91,8 @@ SmiPFHandler (
)
{
UINTN PFAddress;
+ UINTN GuardPageAddress;
+ UINTN CpuIndex;
ASSERT (InterruptType == EXCEPT_IA32_PAGE_FAULT);
@@ -98,10 +100,30 @@ SmiPFHandler (
PFAddress = AsmReadCr2 ();
- if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
- (PFAddress >= mCpuHotPlugData.SmrrBase) &&
+ //
+ // If a page fault occurs in SMRAM range, it might be in a SMM stack guard page,
+ // or SMM page protection violation.
+ //
+ if ((PFAddress >= mCpuHotPlugData.SmrrBase) &&
(PFAddress < (mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize))) {
- DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
+ CpuIndex = GetCpuIndex ();
+ GuardPageAddress = (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex * mSmmStackSize);
+ if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
+ (PFAddress >= GuardPageAddress) &&
+ (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) {
+ DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
+ }
+ if ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != 0) {
+ DEBUG ((DEBUG_ERROR, "SMM exception at execution (0x%lx)\n", PFAddress));
+ DEBUG_CODE (
+ DumpModuleInfoByIp (*(UINTN *)(UINTN)SystemContext.SystemContextIa32->Esp);
+ );
+ } else {
+ DEBUG ((DEBUG_ERROR, "SMM exception at write (0x%lx)\n", PFAddress));
+ DEBUG_CODE (
+ DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Eip);
+ );
+ }
CpuDeadLoop ();
}
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h 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
);
+/**
+ 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 b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 531e188..ec8eab7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -804,6 +804,8 @@ SmiPFHandler (
)
{
UINTN PFAddress;
+ UINTN GuardPageAddress;
+ UINTN CpuIndex;
ASSERT (InterruptType == EXCEPT_IA32_PAGE_FAULT);
@@ -817,12 +819,29 @@ SmiPFHandler (
}
//
- // 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 stack guard page,
+ // or SMM page protection violation.
//
- if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
- (PFAddress >= mCpuHotPlugData.SmrrBase) &&
+ if ((PFAddress >= mCpuHotPlugData.SmrrBase) &&
(PFAddress < (mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize))) {
- DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
+ CpuIndex = GetCpuIndex ();
+ GuardPageAddress = (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex * mSmmStackSize);
+ if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
+ (PFAddress >= GuardPageAddress) &&
+ (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) {
+ DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
+ }
+ if ((SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_ID) != 0) {
+ DEBUG ((DEBUG_ERROR, "SMM exception at execution (0x%lx)\n", PFAddress));
+ DEBUG_CODE (
+ DumpModuleInfoByIp (*(UINTN *)(UINTN)SystemContext.SystemContextX64->Rsp);
+ );
+ } else {
+ DEBUG ((DEBUG_ERROR, "SMM exception at write (0x%lx)\n", PFAddress));
+ DEBUG_CODE (
+ DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
+ );
+ }
CpuDeadLoop ();
}
--
2.7.4.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpu: Correct exception message.
2016-11-22 11:47 [PATCH] UefiCpuPkg/PiSmmCpu: Correct exception message Jiewen Yao
@ 2016-11-22 21:17 ` Laszlo Ersek
2016-11-23 0:38 ` Yao, Jiewen
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2016-11-22 21:17 UTC (permalink / raw)
To: Jiewen Yao, edk2-devel; +Cc: Michael D Kinney, Jeff Fan
On 11/22/16 12:47, Jiewen Yao wrote:
> This patch fixes the first part of
> https://bugzilla.tianocore.org/show_bug.cgi?id=242
>
> Previously, when SMM exception happens, "stack overflow" is misreported.
> This patch checked the PF address to see it is stack overflow, or
> it is caused by SMM page protection.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jeff Fan <jeff.fan@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 28 +++++++++++++++++---
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 9 +++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 27 ++++++++++++++++---
> 3 files changed, 57 insertions(+), 7 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 5033bc5..feca142 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -91,6 +91,8 @@ SmiPFHandler (
> )
> {
> UINTN PFAddress;
> + UINTN GuardPageAddress;
> + UINTN CpuIndex;
>
> ASSERT (InterruptType == EXCEPT_IA32_PAGE_FAULT);
>
> @@ -98,10 +100,30 @@ SmiPFHandler (
>
> PFAddress = AsmReadCr2 ();
>
> - if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> - (PFAddress >= mCpuHotPlugData.SmrrBase) &&
> + //
> + // If a page fault occurs in SMRAM range, it might be in a SMM stack guard page,
> + // or SMM page protection violation.
> + //
> + if ((PFAddress >= mCpuHotPlugData.SmrrBase) &&
> (PFAddress < (mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize))) {
> - DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> + CpuIndex = GetCpuIndex ();
> + GuardPageAddress = (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex * mSmmStackSize);
> + if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> + (PFAddress >= GuardPageAddress) &&
> + (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) {
> + DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> + }
> + if ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != 0) {
> + DEBUG ((DEBUG_ERROR, "SMM exception at execution (0x%lx)\n", PFAddress));
> + DEBUG_CODE (
> + DumpModuleInfoByIp (*(UINTN *)(UINTN)SystemContext.SystemContextIa32->Esp);
> + );
> + } else {
> + DEBUG ((DEBUG_ERROR, "SMM exception at write (0x%lx)\n", PFAddress));
> + DEBUG_CODE (
> + DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Eip);
> + );
> + }
> CpuDeadLoop ();
> }
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h 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
> );
>
> +/**
> + 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 b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 531e188..ec8eab7 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -804,6 +804,8 @@ SmiPFHandler (
> )
> {
> UINTN PFAddress;
> + UINTN GuardPageAddress;
> + UINTN CpuIndex;
>
> ASSERT (InterruptType == EXCEPT_IA32_PAGE_FAULT);
>
> @@ -817,12 +819,29 @@ SmiPFHandler (
> }
>
> //
> - // 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 stack guard page,
> + // or SMM page protection violation.
> //
> - if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> - (PFAddress >= mCpuHotPlugData.SmrrBase) &&
> + if ((PFAddress >= mCpuHotPlugData.SmrrBase) &&
> (PFAddress < (mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize))) {
> - DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> + CpuIndex = GetCpuIndex ();
> + GuardPageAddress = (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex * mSmmStackSize);
> + if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> + (PFAddress >= GuardPageAddress) &&
> + (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) {
> + DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> + }
> + if ((SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_ID) != 0) {
> + DEBUG ((DEBUG_ERROR, "SMM exception at execution (0x%lx)\n", PFAddress));
> + DEBUG_CODE (
> + DumpModuleInfoByIp (*(UINTN *)(UINTN)SystemContext.SystemContextX64->Rsp);
> + );
> + } else {
> + DEBUG ((DEBUG_ERROR, "SMM exception at write (0x%lx)\n", PFAddress));
> + DEBUG_CODE (
> + DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
> + );
> + }
> CpuDeadLoop ();
> }
>
>
(1) The "PFAddress" variable is UINTN in both variants. Printing UINTN
with %lx is incorrect in the Ia32 case, because %lx takes an UINT64.
I suggest the following pattern for printing UINTN values portably:
DEBUG ((level, "%lx", (UINT64)Value));
That is, always use %lx and add an explicit cast. The cast is a no-op on
X64, and does the right conversion on Ia32. The %lx conversion
specification matches the result in both cases.
(2) I tested the X64 stack overflow branch as follows:
I temporarily reverted (on top of your present patch) commit
0d0c245dfb147 ("OvmfPkg: set SMM stack size to 16KB"), and then ran the
certificate enrollment application that originally triggered the stack
overflow.
This is the debug output I got:
> SMM stack overflow!
> SMM exception at write (0x7FF9CFEC)
> It is invoked from the instruction before IP(0x7FFE4CA3) in module
(.../Build/Ovmf3264/NOOPT_GCC48/X64/MdeModulePkg/Core/PiSmmCore/PiSmmCore/DEBUG/PiSmmCore.dll)
Shouldn't you change the IA32_PF_EC_ID check into an "else if"? Because
I think once you determine the stack overflow, we shouldn't look for any
other kind of exception.
(3) I tested the Ia32 execution / write fault branch as follows:
I temporarily reverted (on top of your present patch) commit
750ec4cabd07 ("UefiCpuPkg/PiSmmCpu: Check XdSupport before set NX.").
Then, under the circumstances I reported in
<https://lists.01.org/pipermail/edk2-devel/2016-November/004929.html>, I
get:
> ConvertPageEntryAttribute 0x7FEA4067->0x800000007FEA4067
> SMM exception at write (0x7FEA4890)
> It is invoked from the instruction before IP(0x7FFB879A) in module
(.../Build/OvmfIa32/NOOPT_GCC48/IA32/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm/DEBUG/PiSmmCpuDxeSmm.dll)
Here the page fault is explained by the fact that we set the unsupported
NX bit in the PTE that maps the page, and then we try to read from the
page (not write to it). If I remember correctly at least.
Is it possible to distinguish "read" from "write" in the fault symptoms?
If it is, then I suggest to customize the error message. If it is not
possible, then I suggest to replace the word "write" with "access".
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpu: Correct exception message.
2016-11-22 21:17 ` Laszlo Ersek
@ 2016-11-23 0:38 ` Yao, Jiewen
0 siblings, 0 replies; 3+ messages in thread
From: Yao, Jiewen @ 2016-11-23 0:38 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@ml01.01.org; +Cc: Kinney, Michael D, Fan, Jeff
Yes. You are right.
I agree all. Will send out V2 patch.
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, November 23, 2016 5:17 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@ml01.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpu: Correct exception message.
On 11/22/16 12:47, Jiewen Yao wrote:
> This patch fixes the first part of
> https://bugzilla.tianocore.org/show_bug.cgi?id=242
>
> Previously, when SMM exception happens, "stack overflow" is misreported.
> This patch checked the PF address to see it is stack overflow, or
> it is caused by SMM page protection.
>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Cc: Jeff Fan <jeff.fan@intel.com<mailto:jeff.fan@intel.com>>
> Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 28 +++++++++++++++++---
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 9 +++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 27 ++++++++++++++++---
> 3 files changed, 57 insertions(+), 7 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 5033bc5..feca142 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -91,6 +91,8 @@ SmiPFHandler (
> )
> {
> UINTN PFAddress;
> + UINTN GuardPageAddress;
> + UINTN CpuIndex;
>
> ASSERT (InterruptType == EXCEPT_IA32_PAGE_FAULT);
>
> @@ -98,10 +100,30 @@ SmiPFHandler (
>
> PFAddress = AsmReadCr2 ();
>
> - if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> - (PFAddress >= mCpuHotPlugData.SmrrBase) &&
> + //
> + // If a page fault occurs in SMRAM range, it might be in a SMM stack guard page,
> + // or SMM page protection violation.
> + //
> + if ((PFAddress >= mCpuHotPlugData.SmrrBase) &&
> (PFAddress < (mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize))) {
> - DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> + CpuIndex = GetCpuIndex ();
> + GuardPageAddress = (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex * mSmmStackSize);
> + if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> + (PFAddress >= GuardPageAddress) &&
> + (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) {
> + DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> + }
> + if ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != 0) {
> + DEBUG ((DEBUG_ERROR, "SMM exception at execution (0x%lx)\n", PFAddress));
> + DEBUG_CODE (
> + DumpModuleInfoByIp (*(UINTN *)(UINTN)SystemContext.SystemContextIa32->Esp);
> + );
> + } else {
> + DEBUG ((DEBUG_ERROR, "SMM exception at write (0x%lx)\n", PFAddress));
> + DEBUG_CODE (
> + DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Eip);
> + );
> + }
> CpuDeadLoop ();
> }
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h 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
> );
>
> +/**
> + 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 b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 531e188..ec8eab7 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -804,6 +804,8 @@ SmiPFHandler (
> )
> {
> UINTN PFAddress;
> + UINTN GuardPageAddress;
> + UINTN CpuIndex;
>
> ASSERT (InterruptType == EXCEPT_IA32_PAGE_FAULT);
>
> @@ -817,12 +819,29 @@ SmiPFHandler (
> }
>
> //
> - // 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 stack guard page,
> + // or SMM page protection violation.
> //
> - if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> - (PFAddress >= mCpuHotPlugData.SmrrBase) &&
> + if ((PFAddress >= mCpuHotPlugData.SmrrBase) &&
> (PFAddress < (mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize))) {
> - DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> + CpuIndex = GetCpuIndex ();
> + GuardPageAddress = (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex * mSmmStackSize);
> + if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
> + (PFAddress >= GuardPageAddress) &&
> + (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) {
> + DEBUG ((DEBUG_ERROR, "SMM stack overflow!\n"));
> + }
> + if ((SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_ID) != 0) {
> + DEBUG ((DEBUG_ERROR, "SMM exception at execution (0x%lx)\n", PFAddress));
> + DEBUG_CODE (
> + DumpModuleInfoByIp (*(UINTN *)(UINTN)SystemContext.SystemContextX64->Rsp);
> + );
> + } else {
> + DEBUG ((DEBUG_ERROR, "SMM exception at write (0x%lx)\n", PFAddress));
> + DEBUG_CODE (
> + DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
> + );
> + }
> CpuDeadLoop ();
> }
>
>
(1) The "PFAddress" variable is UINTN in both variants. Printing UINTN
with %lx is incorrect in the Ia32 case, because %lx takes an UINT64.
I suggest the following pattern for printing UINTN values portably:
DEBUG ((level, "%lx", (UINT64)Value));
That is, always use %lx and add an explicit cast. The cast is a no-op on
X64, and does the right conversion on Ia32. The %lx conversion
specification matches the result in both cases.
(2) I tested the X64 stack overflow branch as follows:
I temporarily reverted (on top of your present patch) commit
0d0c245dfb147 ("OvmfPkg: set SMM stack size to 16KB"), and then ran the
certificate enrollment application that originally triggered the stack
overflow.
This is the debug output I got:
> SMM stack overflow!
> SMM exception at write (0x7FF9CFEC)
> It is invoked from the instruction before IP(0x7FFE4CA3) in module
(.../Build/Ovmf3264/NOOPT_GCC48/X64/MdeModulePkg/Core/PiSmmCore/PiSmmCore/DEBUG/PiSmmCore.dll)
Shouldn't you change the IA32_PF_EC_ID check into an "else if"? Because
I think once you determine the stack overflow, we shouldn't look for any
other kind of exception.
(3) I tested the Ia32 execution / write fault branch as follows:
I temporarily reverted (on top of your present patch) commit
750ec4cabd07 ("UefiCpuPkg/PiSmmCpu: Check XdSupport before set NX.").
Then, under the circumstances I reported in
<https://lists.01.org/pipermail/edk2-devel/2016-November/004929.html>, I
get:
> ConvertPageEntryAttribute 0x7FEA4067->0x800000007FEA4067
> SMM exception at write (0x7FEA4890)
> It is invoked from the instruction before IP(0x7FFB879A) in module
(.../Build/OvmfIa32/NOOPT_GCC48/IA32/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm/DEBUG/PiSmmCpuDxeSmm.dll)
Here the page fault is explained by the fact that we set the unsupported
NX bit in the PTE that maps the page, and then we try to read from the
page (not write to it). If I remember correctly at least.
Is it possible to distinguish "read" from "write" in the fault symptoms?
If it is, then I suggest to customize the error message. If it is not
possible, then I suggest to replace the word "write" with "access".
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-11-23 0:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-22 11:47 [PATCH] UefiCpuPkg/PiSmmCpu: Correct exception message Jiewen Yao
2016-11-22 21:17 ` Laszlo Ersek
2016-11-23 0:38 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox