[edk2] [PATCH] SourceLevelDebugPkg: Fix PEI timer interrupt regression

Fan, Jeff

2015-07-09 02:20:31 UTC

Hi, Brian

Yes. This is one regression. Thanks your fix.

Reviewed-by: Jeff Fan <***@intel.com>

There is another issue on enable CPU interrupt in DEBUG_AGENT_INIT_POSTMEM_SEC case. I will fix it soon.

Thanks!
Jeff
-----Original Message-----
From: Brian J. Johnson [mailto:***@sgi.com]
Sent: Thursday, July 09, 2015 3:34 AM
To: edk2-***@lists.sourceforge.net
Cc: Fan, Jeff
Subject: [PATCH] SourceLevelDebugPkg: Fix PEI timer interrupt regression

Recent changes to debug timer initialization (commit 2befbc82, svn
17572) modified the Sec/Pei InitializeDebugAgent() routine to enable debug timer interrupts. This causes problems in the DEBUG_AGENT_INIT_POSTMEM_SEC case: the callers appear to assume that if they block timer interrupts before the call, interrupts will remain blocked afterwards.

It is not always safe to have interrupts enabled on return from InitializeDebugAgent(). For instance, after calling InitializeDebugAgent(), OvmfPkg's TemporaryRamMigration() moves the stack, heap, and IDT to RAM, then switches to the new stack. Only then does it reenable timer interrupts. Taking an interrupt during this process can corrupt state, causing crashes.

Do not unmask the debug timer interrupt in the DEBUG_AGENT_INIT_POSTMEM_SEC case.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Brian J. Johnson <***@sgi.com>
---

This fix has been built with OvmfPkg/OvmfPkgIa32X64.dsc, using the GCC48 toolchain under Ubuntu. It builds and boots to shell under Qemu/tcg, but I'm not set up to test remote debugging with OVMF. An equivalent fix works well on hardware in a proprietary BIOS, built with the WINDDK compilers.

This is a minimal fix. I'm not sure if timer interrupts should be enabled in any of the other InitializeDebugAgent() code paths... if not, the call to SaveAndSetDebugTimerInterrupt (TRUE) can just be removed.
Someone more familiar with them may want to audit the callers. The
DEBUG_AGENT_INIT_THUNK_PEI_IA32TOX64 case is especially unclear to me; I'm not quite sure what the caller (MdeModulePkg/Universal/CapsulePei)
is up to.

We also call SaveAndSetDebugTimerInterrupt (TRUE) in InitializeDebugAgentPhase2(), where it's necessary to enable interrupts for the DEBUG_AGENT_INIT_PREMEM_SEC case.

Thanks,
Brian

--------------------------------------------------------------------

My statements are my own, are not authorized by SGI, and do not
necessarily represent SGI’s positions.

.../Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c |
6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git
a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
index ea75742..9fbe4ce 100644
---
a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
+++
b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
@@ -612,9 +612,11 @@ InitializeDebugAgent (
}

//
- // Enable Debug Timer interrupt
+ // Enable Debug Timer interrupt. In post-memory SEC, the caller
enables it.
//
- SaveAndSetDebugTimerInterrupt (TRUE);
+ if (InitFlag != DEBUG_AGENT_INIT_POSTMEM_SEC) {
+ SaveAndSetDebugTimerInterrupt (TRUE); }
//
// Enable CPU interrupts so debug timer interrupts can be delivered
//