Hi, Brian

Yes. This is one regression. Thanks your fix.

Reviewed-by: Jeff Fan <jeff....@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:bjohn...@sgi.com] 
Sent: Thursday, July 09, 2015 3:34 AM
To: edk2-devel@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 <bjohn...@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
    //

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Reply via email to