Thanks for your reply. Comments below:
> -----Original Message-----
> From: Paulo Alcantara [mailto:pca...@zytor.com]
> Sent: Friday, November 17, 2017 6:13 AM
> To: Yao, Jiewen <jiewen....@intel.com>
> Cc: Paulo Alcantara <pca...@zytor.com>; edk2-devel@lists.01.org; Laszlo Ersek
> <ler...@redhat.com>; Dong, Eric <eric.d...@intel.com>
> Subject: RE: [edk2] [RFC v2 1/3] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add
> stack trace support
>
> Hi Jiewen,
>
> On Wed, November 15, 2017 11:57 pm, Yao, Jiewen wrote:
> > Hi Paulo
> > Thanks to bring this cool feature.
> >
> > I have same feeling as Jeff Fan. It is great!
> >
> > I have some questions, hope you can clarify.
> > 1) Would you please let us know which tool change is validated? Such as
> > MSVC, or GCC?
>
> GCC only. This should not work on MSVC because it relies on frame pointers.
[Jiewen] Not work is OK.
But my question is more about: Have you tested? Or have you tried?
Do you see basic exception information can be dumped without any problem for
MSVC?
Or do you see system crash immediately for MSVC?
> > 2) Would you please let us know which phase is validated? Such as PEI
> > PreMemory, PEI PostMemory, DXE, SMM?
>
> DXE. I'm currently testing it by placing a random "CpuBreakpoint ()" in
> PartitionDxe driver.
>
[Jiewen] Ummm.
Based upon the fact that the code you update may be used in all those phases.
I recommend to validate them in the final patch.
> > 3) Would you please let us know if there is any special condition
> > validated? Such as code is in an interrupt handler, SMM initialization
> > code, thunk code during mode switch, etc.
> > I ask this because I have seen lots of discussion on sanity check, to
> > avoid double exception.
>
> At the moment I'm only ensuring proper aligned RSP and ESP values. But we
> still need to validate the RIP values, etc. as Andrew and Brian suggested.
[Jiewen] OK. I look forward to seeing your patch to validate more.
Same as #2. I suggest you validate all those corner cases in the final patch.
> > 4) We supported some security feature to create a hole in normal memory
> > map, such as PageGuard, StackGuard, SMM communication protection, etc.
> > Accessing those memory directly will cause #PF immediately.
> > Would you please let us know how we detect that case, to avoid RIP or RSP
> > fail to the non-present page?
>
> Sorry. I have no idea :-( I'd hope to get some help from you guys.
[Jiewen] One possible solution is to scan page table, before access this
address, to make sure you are accessing present page.
You can also filer invalid address directly by checking CPUID physical address
bit before that. The invalid address will cause #GP instead of #PF.
All I all, I would like to clarify the goal for exception lib: whatever
exception condition the BIOS meets, the exception handler can always dump the
*basic* exception information and halt.
The missing of advanced symbol info is acceptable. But system crash, when we
want to dump the advanced info, is not the best choice.
> > 5) May I know why we check RIP < ImageBase? Is that legal or illegal if
> > RIP > ImageBase+ImageSize, but RIP in another PE/COFF image?
>
> That check was a wrong assumption that I had in the beginning. RIP may
> point to either lower or higher addresses where the other PE/COFF images
> are located. Fixed it in v3.
[Jiewen] Great. Thanks.
> Sorry for the delay in the response. I'm only able to work on this in my
> free time.
>
> Thanks!
> Paulo
>
> >> + //
> >> + // Check if RIP is within another PE/COFF image base address
> >> + //
> >> + if (Rip < ImageBase) {
> >> + //
> >> + // Search for the respective PE/COFF image based on RIP
> >> + //
> >
> > Thank you
> > Yao Jiewen
> >
> >
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >> Paulo
> >> Alcantara
> >> Sent: Thursday, November 16, 2017 9:18 AM
> >> To: edk2-devel@lists.01.org
> >> Cc: Laszlo Ersek <ler...@redhat.com>; Dong, Eric <eric.d...@intel.com>
> >> Subject: [edk2] [RFC v2 1/3] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add
> >> stack
> >> trace support
> >>
> >> This patch adds stack trace support during a X64 CPU exception.
> >>
> >> It will dump out back trace, stack contents as well as image module
> >> names that were part of the call stack.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Cc: Eric Dong <eric.d...@intel.com>
> >> Cc: Laszlo Ersek <ler...@redhat.com>
> >> Signed-off-by: Paulo Alcantara <pca...@zytor.com>
> >> ---
> >> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c |
> >> 369
> >> +++++++++++++++++++-
> >> 1 file changed, 367 insertions(+), 2 deletions(-)
> >>
> >> diff --git
> >> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
> >> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
> >> index 65f0cff680..11cd7c9e1c 100644
> >> ---
> >> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
> >> +++
> >> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
> >> @@ -14,6 +14,11 @@
> >>
> >> #include "CpuExceptionCommon.h"
> >>
> >> +//
> >> +// Unknown PDB file name
> >> +//
> >> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8
> >> *mUnknownPdbFileName = "????";
> >> +
> >> /**
> >> Return address map of exception handler template so that C code can
> >> generate
> >> exception tables.
> >> @@ -242,6 +247,350 @@ DumpCpuContext (
> >> );
> >> }
> >>
> >> +/**
> >> + Get absolute path and file name of PDB file in PE/COFF image.
> >> +
> >> + @param[in] ImageBase Base address of PE/COFF image.
> >> + @param[out] PdbAbsoluteFilePath Absolute path of PDB file.
> >> + @param[out] PdbFileName File name of PDB file.
> >> +**/
> >> +STATIC
> >> +VOID
> >> +GetPdbFileName (
> >> + IN UINTN ImageBase,
> >> + OUT CHAR8 **PdbAbsoluteFilePath,
> >> + OUT CHAR8 **PdbFileName
> >> + )
> >> +{
> >> + VOID *PdbPointer;
> >> + CHAR8 *Str;
> >> +
> >> + //
> >> + // Get PDB file name from PE/COFF image
> >> + //
> >> + PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)ImageBase);
> >> + if (PdbPointer == NULL) {
> >> + //
> >> + // No PDB file name found. Set it to an unknown file name.
> >> + //
> >> + *PdbFileName = (CHAR8 *)mUnknownPdbFileName;
> >> + if (PdbAbsoluteFilePath != NULL) {
> >> + *PdbAbsoluteFilePath = NULL;
> >> + }
> >> + } else {
> >> + //
> >> + // Get file name portion out of PDB file in PE/COFF image
> >> + //
> >> + Str = (CHAR8 *)((UINTN)PdbPointer +
> >> + AsciiStrLen ((CHAR8 *)PdbPointer) - sizeof *Str);
> >> + for (; *Str != '/' && *Str != '\\'; Str--) {
> >> + ;
> >> + }
> >> +
> >> + //
> >> + // Set PDB file name (also skip trailing path separator: '/' or
> >> '\\')
> >> + //
> >> + *PdbFileName = Str + 1;
> >> +
> >> + if (PdbAbsoluteFilePath != NULL) {
> >> + //
> >> + // Set absolute file path of PDB file
> >> + //
> >> + *PdbAbsoluteFilePath = PdbPointer;
> >> + }
> >> + }
> >> +}
> >> +
> >> +/**
> >> + Dump stack contents.
> >> +
> >> + @param[in] CurrentRsp Current stack pointer address.
> >> + @param[in] UnwondStacksCount Count of unwond stack frames.
> >> +**/
> >> +STATIC
> >> +VOID
> >> +DumpStackContents (
> >> + IN UINT64 CurrentRsp,
> >> + IN INTN UnwondStacksCount
> >> + )
> >> +{
> >> + //
> >> + // Check for proper stack pointer alignment
> >> + //
> >> + if (((UINTN)CurrentRsp & (CPU_STACK_ALIGNMENT - 1)) != 0) {
> >> + InternalPrintMessage ("!!!! Unaligned stack pointer. !!!!\n");
> >> + return;
> >> + }
> >> +
> >> + //
> >> + // Dump out stack contents
> >> + //
> >> + InternalPrintMessage ("\nStack dump:\n");
> >> + while (UnwondStacksCount-- > 0) {
> >> + InternalPrintMessage (
> >> + "0x%016lx: %016lx %016lx\n",
> >> + CurrentRsp,
> >> + *(UINT64 *)CurrentRsp,
> >> + *(UINT64 *)((UINTN)CurrentRsp + 8)
> >> + );
> >> +
> >> + //
> >> + // Point to next stack
> >> + //
> >> + CurrentRsp += CPU_STACK_ALIGNMENT;
> >> + }
> >> +}
> >> +
> >> +/**
> >> + Dump all image module names from call stack.
> >> +
> >> + @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT.
> >> +**/
> >> +STATIC
> >> +VOID
> >> +DumpImageModuleNames (
> >> + IN EFI_SYSTEM_CONTEXT SystemContext
> >> + )
> >> +{
> >> + EFI_STATUS Status;
> >> + UINT64 Rip;
> >> + UINTN ImageBase;
> >> + VOID *EntryPoint;
> >> + CHAR8 *PdbAbsoluteFilePath;
> >> + CHAR8 *PdbFileName;
> >> + UINT64 Rbp;
> >> +
> >> + //
> >> + // Set current RIP address
> >> + //
> >> + Rip = SystemContext.SystemContextX64->Rip;
> >> +
> >> + //
> >> + // Set current frame pointer address
> >> + //
> >> + Rbp = SystemContext.SystemContextX64->Rbp;
> >> +
> >> + //
> >> + // Check for proper frame pointer alignment
> >> + //
> >> + if (((UINTN)Rbp & (CPU_STACK_ALIGNMENT - 1)) != 0) {
> >> + InternalPrintMessage ("!!!! Unaligned frame pointer. !!!!\n");
> >> + return;
> >> + }
> >> +
> >> + //
> >> + // Get initial PE/COFF image base address from current RIP
> >> + //
> >> + ImageBase = PeCoffSearchImageBase (Rip);
> >> + if (ImageBase == 0) {
> >> + InternalPrintMessage ("!!!! Could not find image module names.
> >> !!!!");
> >> + return;
> >> + }
> >> +
> >> + //
> >> + // Get initial PE/COFF image's entry point
> >> + //
> >> + Status = PeCoffLoaderGetEntryPoint ((VOID *)ImageBase, &EntryPoint);
> >> + if (EFI_ERROR (Status)) {
> >> + EntryPoint = NULL;
> >> + }
> >> +
> >> + //
> >> + // Get file name and absolute path of initial PDB file
> >> + //
> >> + GetPdbFileName (ImageBase, &PdbAbsoluteFilePath, &PdbFileName);
> >> +
> >> + //
> >> + // Print out initial image module name (if any)
> >> + //
> >> + if (PdbAbsoluteFilePath != NULL) {
> >> + InternalPrintMessage (
> >> + "\n%a (ImageBase=0x%016lx, EntryPoint=0x%016lx):\n",
> >> + PdbFileName,
> >> + ImageBase,
> >> + (UINTN)EntryPoint
> >> + );
> >> + InternalPrintMessage ("%a\n", PdbAbsoluteFilePath);
> >> + }
> >> +
> >> + //
> >> + // Walk through call stack and find next module names
> >> + //
> >> + for (;;) {
> >> + //
> >> + // Set RIP with return address from current stack frame
> >> + //
> >> + Rip = *(UINT64 *)((UINTN)Rbp + 8);
> >> +
> >> + //
> >> + // If RIP is zero, then stop unwinding the stack
> >> + //
> >> + if (Rip == 0) {
> >> + break;
> >> + }
> >> +
> >> + //
> >> + // Check if RIP is within another PE/COFF image base address
> >> + //
> >> + if (Rip < ImageBase) {
> >> + //
> >> + // Search for the respective PE/COFF image based on RIP
> >> + //
> >> + ImageBase = PeCoffSearchImageBase (Rip);
> >> + if (ImageBase == 0) {
> >> + //
> >> + // Stop stack trace
> >> + //
> >> + break;
> >> + }
> >> +
> >> + //
> >> + // Get PE/COFF image's entry point
> >> + //
> >> + Status = PeCoffLoaderGetEntryPoint ((VOID *)ImageBase,
> >> &EntryPoint);
> >> + if (EFI_ERROR (Status)) {
> >> + EntryPoint = NULL;
> >> + }
> >> +
> >> + //
> >> + // Get file name and absolute path of PDB file
> >> + //
> >> + GetPdbFileName (ImageBase, &PdbAbsoluteFilePath,
> &PdbFileName);
> >> +
> >> + //
> >> + // Print out image module name (if any)
> >> + //
> >> + if (PdbAbsoluteFilePath != NULL) {
> >> + InternalPrintMessage (
> >> + "%a (ImageBase=0x%016lx, EntryPoint=0x%016lx):\n",
> >> + PdbFileName,
> >> + ImageBase,
> >> + (UINTN)EntryPoint
> >> + );
> >> + InternalPrintMessage ("%a\n", PdbAbsoluteFilePath);
> >> + }
> >> + }
> >> +
> >> + //
> >> + // Unwind the stack
> >> + //
> >> + Rbp = *(UINT64 *)(UINTN)Rbp;
> >> + }
> >> +}
> >> +
> >> +/**
> >> + Dump stack trace.
> >> +
> >> + @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT.
> >> + @param[out] UnwondStacksCount Count of unwond stack frames.
> >> +**/
> >> +STATIC
> >> +VOID
> >> +DumpStackTrace (
> >> + IN EFI_SYSTEM_CONTEXT SystemContext,
> >> + OUT INTN *UnwondStacksCount
> >> + )
> >> +{
> >> + UINT64 Rip;
> >> + UINT64 Rbp;
> >> + UINTN ImageBase;
> >> + CHAR8 *PdbFileName;
> >> +
> >> + //
> >> + // Set current RIP address
> >> + //
> >> + Rip = SystemContext.SystemContextX64->Rip;
> >> +
> >> + //
> >> + // Set current frame pointer address
> >> + //
> >> + Rbp = SystemContext.SystemContextX64->Rbp;
> >> +
> >> + //
> >> + // Get initial PE/COFF image base address from current RIP
> >> + //
> >> + ImageBase = PeCoffSearchImageBase (Rip);
> >> + if (ImageBase == 0) {
> >> + InternalPrintMessage ("!!!! Could not find backtrace information.
> >> !!!!");
> >> + return;
> >> + }
> >> +
> >> + //
> >> + // Get PDB file name from initial PE/COFF image
> >> + //
> >> + GetPdbFileName (ImageBase, NULL, &PdbFileName);
> >> +
> >> + //
> >> + // Initialize count of unwond stacks
> >> + //
> >> + *UnwondStacksCount = 1;
> >> +
> >> + //
> >> + // Print out back trace
> >> + //
> >> + InternalPrintMessage ("\nCall trace:\n");
> >> +
> >> + for (;;) {
> >> + //
> >> + // Print stack frame in the following format:
> >> + //
> >> + // # <RIP> @ <ImageBase>+<RelOffset> (RBP) in [<ModuleName>
> | ????]
> >> + //
> >> + InternalPrintMessage (
> >> + "%d 0x%016lx @ 0x%016lx+0x%x (0x%016lx) in %a\n",
> >> + *UnwondStacksCount - 1,
> >> + Rip,
> >> + ImageBase,
> >> + Rip - ImageBase - 1,
> >> + Rbp,
> >> + PdbFileName
> >> + );
> >> +
> >> + //
> >> + // Set RIP with return address from current stack frame
> >> + //
> >> + Rip = *(UINT64 *)((UINTN)Rbp + 8);
> >> +
> >> + //
> >> + // If RIP is zero, then stop unwinding the stack
> >> + //
> >> + if (Rip == 0) {
> >> + break;
> >> + }
> >> +
> >> + //
> >> + // Check if RIP is within another PE/COFF image base address
> >> + //
> >> + if (Rip < ImageBase) {
> >> + //
> >> + // Search for the respective PE/COFF image based on RIP
> >> + //
> >> + ImageBase = PeCoffSearchImageBase (Rip);
> >> + if (ImageBase == 0) {
> >> + //
> >> + // Stop stack trace
> >> + //
> >> + break;
> >> + }
> >> +
> >> + //
> >> + // Get PDB file name
> >> + //
> >> + GetPdbFileName (ImageBase, NULL, &PdbFileName);
> >> + }
> >> +
> >> + //
> >> + // Unwind the stack
> >> + //
> >> + Rbp = *(UINT64 *)(UINTN)Rbp;
> >> +
> >> + //
> >> + // Increment count of unwond stacks
> >> + //
> >> + (*UnwondStacksCount)++;
> >> + }
> >> +}
> >> +
> >> /**
> >> Display CPU information.
> >>
> >> @@ -254,9 +603,25 @@ DumpImageAndCpuContent (
> >> IN EFI_SYSTEM_CONTEXT SystemContext
> >> )
> >> {
> >> + INTN UnwondStacksCount;
> >> +
> >> + //
> >> + // Dump CPU context
> >> + //
> >> DumpCpuContext (ExceptionType, SystemContext);
> >> +
> >> + //
> >> + // Dump stack trace
> >> + //
> >> + DumpStackTrace (SystemContext, &UnwondStacksCount);
> >> +
> >> + //
> >> + // Dump image module names
> >> + //
> >> + DumpImageModuleNames (SystemContext);
> >> +
> >> //
> >> - // Dump module image base and module entry point by RIP
> >> + // Dump stack contents
> >> //
> >> - DumpModuleImageInfo (SystemContext.SystemContextX64->Rip);
> >> + DumpStackContents (SystemContext.SystemContextX64->Rsp,
> >> UnwondStacksCount);
> >> }
> >> --
> >> 2.14.3
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> >
> >
>
>
> --
> Paulo Alcantara, HP Inc.
> Speaking for myself only.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
