Re: [PATCH 1/3] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Feb 20, 2018 at 08:20:48AM -0800, Andrew Fish wrote:
> > On Feb 20, 2018, at 6:16 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > 
> > On Tue, Feb 20, 2018 at 11:05:22AM +0000, Ard Biesheuvel wrote:
> >> +/**
> >> +  Prints an assert message containing a filename, line number, and description.
> >> +  This may be followed by a breakpoint or a dead loop.
> >> +
> >> +  Print a message of the form "ASSERT <FileName>(<LineNumber>): <Description>\n"
> >> +  to the debug output device.  If DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED bit
> >> +  of PcdDebugProperyMask is set then CpuBreakpoint() is called. Otherwise, if
> >> +  DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED bit of PcdDebugProperyMask is set then
> >> +  CpuDeadLoop() is called.  If neither of these bits are set, then this function
> >> +  returns immediately after the message is printed to the debug output device.
> >> +  DebugAssert() must actively prevent recursion.  If DebugAssert() is called
> >> +  while processing another DebugAssert(), then DebugAssert() must return
> >> +  immediately.
> >> +
> >> +  If FileName is NULL, then a <FileName> string of "(NULL) Filename" is printed.
> >> +  If Description is NULL, then a <Description> string of "(NULL) Description" is
> >> +  printed.
> >> +
> >> +  @param  FileName     The pointer to the name of the source file that generated
> >> +                       the assert condition.
> >> +  @param  LineNumber   The line number in the source file that generated the
> >> +                       assert condition
> >> +  @param  Description  The pointer to the description of the assert condition.
> >> +
> >> +**/
> >> +VOID
> >> +EFIAPI
> >> +DebugAssert (
> >> +  IN CONST CHAR8  *FileName,
> >> +  IN UINTN        LineNumber,
> >> +  IN CONST CHAR8  *Description
> >> +  )
> >> +{
> >> +  CHAR8  Buffer[MAX_DEBUG_MESSAGE_LENGTH];
> >> +
> >> +  if (!mEfiAtRuntime) {
> >> +    //
> >> +    // Generate the ASSERT() message in Ascii format
> >> +    //
> >> +    AsciiSPrint (Buffer, sizeof (Buffer), "ASSERT [%a] %a(%d): %a\n",
> >> +      gEfiCallerBaseName, FileName, LineNumber, Description);
> >> +
> >> +    //
> >> +    // Send the print string to the Console Output device
> >> +    //
> >> +    SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
> >> +  }
> >> +
> >> +  //
> >> +  // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings
> >> +  //
> >> +  if ((FixedPcdGet8 (PcdDebugPropertyMask) &
> >> +       DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
> >> +    CpuBreakpoint ();
> >> +  } else if ((FixedPcdGet8 (PcdDebugPropertyMask) &
> >> +              DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) {
> >> +    CpuDeadLoop ();
> >> +  }
> > 
> > Hmm ... I know this does not fundamentally change the behaviour of the
> > existing implementation, but if we're looking to improve runtime
> > behaviour, we've just gone from generating a runtime fault to silently
> > freezing (if BREAKPOINT_ENABLED or DEADLOOP_ENABLED).
> > 
> > What do breakpoint/deadloop mean in a runtime context anyway - do we
> > not need to halt _all_ running cores?
> > 
> > I don't see an obvious "right way" solution here, and this only
> > affects DEBUG builds.
> 
> Leif,
> 
> It is not related to DEBUG builds, it is related to PCD configuration. 

Sorry, I was oversimplifying based on most RELEASE builds tending to
have DebugAssertEnabled disabled through PcdDebugPropertyMask.
Looking through edk2 platforms, that shows to be incorrect even among
most open platform ports, so thanks for pointing this out.

> > Possible ways of handling this that I can think
> > of include:
> > - Don't respect BREAKPOINT/DEADLOOP if at runtime.
> > - Respect BREAKPOINT/DEADLOOP and disable all cores.
> > - Take ownership back of the system and re-enable 1:1 mapping so
> >  messages can be printed.
> 
> There is not much risk of losing user data if you "panic" EFI, that
> is not true if you are going to "panic" the OS. I've seen more bugs
> at runtime from confusion about what is legal to do at runtime,
> vs. actual bugs. You can always just return device error on failure,
> and if the RT Services hangs you can core dump the OS. Given the OS
> provided the virtual mapping it is likely the stuck EFI could would
> be in the stack trace of the panic.

Oh, indeed. My concern is regarding the fact that this library (with
either of those options set) would halt the executing processor (and
no others) without any output whatsoever.

/
    Leif