Commit 4bf95a9f361e ("MdeModulePkg/ResetSystemRuntimeDxe: Add more debug
message") broke the DEBUG build for systems using a MMIO mapped UART for
DEBUG output. In other words, it broke the build for all ARM and AARCH64
systems, given that port I/O does not exist on those architectures.

Instead of patching it up locally, let's fix this issue once and for all,
by creating a UART DebugLib implementation for DXE_RUNTIME_DRIVER modules
that does the right thing by default.

v4:
- add Laszlo's R-b
- keep ASSERT() message in local buffer even it is not printed to the serial
  port, to allow it to be accessed via the debugger

v3:
- revert PCD changes, as they are unnecessary for the PCDs in question (since
  they are fixed or patchable only)
- drop redundant mEfiAtRuntime checks around ASSERT()s
- revert whitespace cleanup, i.e., move back to BaseDebugLibSerialPort origin
  for most of the file

v2:
- incorporate Laszlo's feedback into patch #1
- add Laszlo's R-b to patch #2
- dropped patch #3 that blacklists the original BASE implementation for
  DXE_RUNTIME_DRIVER modules

Note that I retained the deadloop/breakpoint code for ASSERT()s at runtime.
I think it is perfectly reasonable behavior for a DEBUG build at runtime,
even if other cores may be up and running as well: the purpose of these
facilities is to allow a debugger to attach to the CPU to figure out what
has happened, and both deadloops and breakpoints can achieve that just
fine even at runtime.


Ard Biesheuvel (2):
  MdePkg: introduce DxeRuntimeDebugLibSerialPort
  ArmVirtPkg: switch to DXE runtime version of DebugLib where
    appropriate

 ArmVirtPkg/ArmVirt.dsc.inc                                                   |   3 +
 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c                       | 346 ++++++++++++++++++++
 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf |  55 ++++
 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni |  21 ++
 4 files changed, 425 insertions(+)
 create mode 100644 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
 create mode 100644 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
 create mode 100644 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni

-- 
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 22 February 2018 at 19:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Commit 4bf95a9f361e ("MdeModulePkg/ResetSystemRuntimeDxe: Add more debug
> message") broke the DEBUG build for systems using a MMIO mapped UART for
> DEBUG output. In other words, it broke the build for all ARM and AARCH64
> systems, given that port I/O does not exist on those architectures.
>
> Instead of patching it up locally, let's fix this issue once and for all,
> by creating a UART DebugLib implementation for DXE_RUNTIME_DRIVER modules
> that does the right thing by default.
>
> v4:
> - add Laszlo's R-b
> - keep ASSERT() message in local buffer even it is not printed to the serial
>   port, to allow it to be accessed via the debugger
>

Mike,

Given that all ARM and AARCH64 DEBUG builds are still broken, may we
please have your R-b on this patch so we can proceed to start fixing
things?

Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Ard,

Just a few comments:
* Please pre-initialize global mEfiAtRuntime = FALSE;
* The UNI file in the patch is identical to BaseDebugLibSerialPort.  
  Please update UNI file header and strings to describe this as the
  runtime version of the DebugLib as described in the .inf and .C files.
* gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue should be
  SOMETIMES_CONSUMES in the INF file.

With those changes:

Reviewed-by: Michael D Kinney <Michael.d.kinney@intel.com>

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, February 23, 2018 6:10 AM
> To: edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Laszlo
> Ersek <lersek@redhat.com>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; afish@apple.com; Zeng,
> Star <star.zeng@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [PATCH v4 0/2] Create UART DebugLib
> implementation for runtime drivers
> 
> On 22 February 2018 at 19:56, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > Commit 4bf95a9f361e
> ("MdeModulePkg/ResetSystemRuntimeDxe: Add more debug
> > message") broke the DEBUG build for systems using a
> MMIO mapped UART for
> > DEBUG output. In other words, it broke the build for
> all ARM and AARCH64
> > systems, given that port I/O does not exist on those
> architectures.
> >
> > Instead of patching it up locally, let's fix this
> issue once and for all,
> > by creating a UART DebugLib implementation for
> DXE_RUNTIME_DRIVER modules
> > that does the right thing by default.
> >
> > v4:
> > - add Laszlo's R-b
> > - keep ASSERT() message in local buffer even it is
> not printed to the serial
> >   port, to allow it to be accessed via the debugger
> >
> 
> Mike,
> 
> Given that all ARM and AARCH64 DEBUG builds are still
> broken, may we
> please have your R-b on this patch so we can proceed to
> start fixing
> things?
> 
> Thanks,
> Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 23 February 2018 at 22:36, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
> Ard,
>
> Just a few comments:
> * Please pre-initialize global mEfiAtRuntime = FALSE;
> * The UNI file in the patch is identical to BaseDebugLibSerialPort.
>   Please update UNI file header and strings to describe this as the
>   runtime version of the DebugLib as described in the .inf and .C files.
> * gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue should be
>   SOMETIMES_CONSUMES in the INF file.
>
> With those changes:
>
> Reviewed-by: Michael D Kinney <Michael.d.kinney@intel.com>
>

Thanks all

Pushed as 8bdb0221152c..ebfca258f5d7
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel