Re: [edk2] [PATCH v3 1/2] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Laszlo Ersek]

On 02/22/18 20:57, Ard Biesheuvel wrote:
> On 22 February 2018 at 19:40, Kinney, Michael D
>  wrote:
>>
>> Ard,
>>
>> In DebugAssert(), if you have deadloop or BP enabled for
>> the ASSERT(), then it would be good to have the message
>> available in the local variable Buffer.
>>
>> Perhaps only the call to SerialPortWrite() should be
>> filtered.
>>
> 
> Good point!

I agree!

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

Re: [edk2] [PATCH v3 1/2] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Ard Biesheuvel]

On 22 February 2018 at 19:40, Kinney, Michael D
 wrote:
>
> Ard,
>
> In DebugAssert(), if you have deadloop or BP enabled for
> the ASSERT(), then it would be good to have the message
> available in the local variable Buffer.
>
> Perhaps only the call to SerialPortWrite() should be
> filtered.
>

Good point!


>
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Thursday, February 22, 2018 10:15 AM
>> To: edk2-devel@lists.01.org
>> Cc: leif.lindh...@linaro.org; ler...@redhat.com; Gao,
>> Liming ; Kinney, Michael D
>> ; af...@apple.com; Zeng,
>> Star ; Ni, Ruiyu
>> ; Ard Biesheuvel
>> 
>> Subject: [PATCH v3 1/2] MdePkg: introduce
>> DxeRuntimeDebugLibSerialPort
>>
>> Introduce a variant of BaseDebugLibSerialPort that
>> behaves correctly with
>> regards to the use of the serial port after
>> ExitBootServices(). At boot
>> time, all DEBUG() prints and ASSERT() invocations are
>> executed normally.
>> At runtime, DEBUG() prints are dropped entirely, and
>> ASSERT()s omit the
>> serial output as well, and only perform the configured
>> post-ASSERT()
>> action, i.e., issue a CPU breakpoint or enter a
>> deadloop.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel
>> 
>> ---
>>  MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
>> | 346 
>>
>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDe
>> bugLibSerialPort.inf |  55 
>>
>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDe
>> bugLibSerialPort.uni |  21 ++
>>  3 files changed, 422 insertions(+)
>>
>> diff --git
>> a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
>> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
>> new file mode 100644
>> index ..a987159d8fc8
>> --- /dev/null
>> +++
>> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
>> @@ -0,0 +1,346 @@
>> +/** @file
>> +  DXE runtime Debug library instance based on Serial
>> Port library.
>> +  It takes care not to call into SerialPortLib after
>> ExitBootServices() has
>> +  been called, to prevent touching hardware that is no
>> longer owned by the
>> +  firmware.
>> +
>> +  Copyright (c) 2006 - 2011, Intel Corporation. All
>> rights reserved.
>> +  Copyright (c) 2018, Linaro, Ltd. All rights
>> reserved.
>> +
>> +  This program and the accompanying materials
>> +  are licensed and made available under the terms and
>> conditions of the BSD License
>> +  which accompanies this distribution.  The full text
>> of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php.
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON
>> AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>> EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +STATIC EFI_EVENT  mEfiExitBootServicesEvent;
>> +STATIC BOOLEANmEfiAtRuntime;
>> +
>> +//
>> +// Define the maximum debug and assert message length
>> that this library supports
>> +//
>> +#define MAX_DEBUG_MESSAGE_LENGTH  0x100
>> +
>> +/**
>> +  Set AtRuntime flag as TRUE after ExitBootServices.
>> +
>> +  @param[in]  Event   The Event that is being
>> processed.
>> +  @param[in]  Context The Event Context.
>> +
>> +**/
>> +STATIC
>> +VOID
>> +EFIAPI
>> +ExitBootServicesEvent (
>> +  IN EFI_EVENTEvent,
>> +  IN VOID *Context
>> +  )
>> +{
>> +  mEfiAtRuntime = TRUE;
>> +}
>> +
>> +/**
>> +  The constructor function to initialize the Serial
>> Port library and
>> +  register a callback for the ExitBootServices event.
>> +
>> +  @param[in]  ImageHandle   The firmware allocated
>> handle for the EFI image.
>> +  @param[in]  SystemTable   A pointer to the EFI System
>> Table.
>> +
>> +  @retval EFI_SUCCESS   The operation completed
>> successfully.
>> +  @retval other Either the serial port failed
>> to initialize or the
>> +ExitBootServices event callback
>> registration failed.
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +DxeRuntimeDebugLibSerialPortConstructor (
>> +  IN EFI_HANDLEImageHandle,
>> +  IN EFI_SYSTEM_TABLE  *SystemTable
>> +  )
>> +{
>> +  EFI_STATUSStatus;
>> +
>> +  Status = SerialPortInitialize ();
>> +  if (EFI_ERROR (Status)) {
>> +return Status;
>> +  }
>> +
>> +  return SystemTable->BootServices->CreateEventEx
>> (EVT_NOTIFY_SIGNAL,
>> +  TPL_NOTIFY,
>> ExitBootServicesEvent, NULL,
>> +
>> ,
>> +
>> );
>> +}
>> +
>> +/**
>> +  If a runtime driver exits with an error, it must call
>> this routine
>> +  to free the allocated resource before the exiting.
>> +
>> +  @param[in]  ImageHandle   The firmware allocated
>> handle for the EFI image.
>> +  

Re: [edk2] [PATCH v3 1/2] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Kinney, Michael D]

Ard,

In DebugAssert(), if you have deadloop or BP enabled for
the ASSERT(), then it would be good to have the message
available in the local variable Buffer.

Perhaps only the call to SerialPortWrite() should be
filtered.

Mike

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Thursday, February 22, 2018 10:15 AM
> To: edk2-devel@lists.01.org
> Cc: leif.lindh...@linaro.org; ler...@redhat.com; Gao,
> Liming ; Kinney, Michael D
> ; af...@apple.com; Zeng,
> Star ; Ni, Ruiyu
> ; Ard Biesheuvel
> 
> Subject: [PATCH v3 1/2] MdePkg: introduce
> DxeRuntimeDebugLibSerialPort
> 
> Introduce a variant of BaseDebugLibSerialPort that
> behaves correctly with
> regards to the use of the serial port after
> ExitBootServices(). At boot
> time, all DEBUG() prints and ASSERT() invocations are
> executed normally.
> At runtime, DEBUG() prints are dropped entirely, and
> ASSERT()s omit the
> serial output as well, and only perform the configured
> post-ASSERT()
> action, i.e., issue a CPU breakpoint or enter a
> deadloop.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel
> 
> ---
>  MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
> | 346 
> 
> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDe
> bugLibSerialPort.inf |  55 
> 
> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDe
> bugLibSerialPort.uni |  21 ++
>  3 files changed, 422 insertions(+)
> 
> diff --git
> a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
> new file mode 100644
> index ..a987159d8fc8
> --- /dev/null
> +++
> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
> @@ -0,0 +1,346 @@
> +/** @file
> +  DXE runtime Debug library instance based on Serial
> Port library.
> +  It takes care not to call into SerialPortLib after
> ExitBootServices() has
> +  been called, to prevent touching hardware that is no
> longer owned by the
> +  firmware.
> +
> +  Copyright (c) 2006 - 2011, Intel Corporation. All
> rights reserved.
> +  Copyright (c) 2018, Linaro, Ltd. All rights
> reserved.
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and
> conditions of the BSD License
> +  which accompanies this distribution.  The full text
> of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php.
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON
> AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +STATIC EFI_EVENT  mEfiExitBootServicesEvent;
> +STATIC BOOLEANmEfiAtRuntime;
> +
> +//
> +// Define the maximum debug and assert message length
> that this library supports
> +//
> +#define MAX_DEBUG_MESSAGE_LENGTH  0x100
> +
> +/**
> +  Set AtRuntime flag as TRUE after ExitBootServices.
> +
> +  @param[in]  Event   The Event that is being
> processed.
> +  @param[in]  Context The Event Context.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +ExitBootServicesEvent (
> +  IN EFI_EVENTEvent,
> +  IN VOID *Context
> +  )
> +{
> +  mEfiAtRuntime = TRUE;
> +}
> +
> +/**
> +  The constructor function to initialize the Serial
> Port library and
> +  register a callback for the ExitBootServices event.
> +
> +  @param[in]  ImageHandle   The firmware allocated
> handle for the EFI image.
> +  @param[in]  SystemTable   A pointer to the EFI System
> Table.
> +
> +  @retval EFI_SUCCESS   The operation completed
> successfully.
> +  @retval other Either the serial port failed
> to initialize or the
> +ExitBootServices event callback
> registration failed.
> +**/
> +EFI_STATUS
> +EFIAPI
> +DxeRuntimeDebugLibSerialPortConstructor (
> +  IN EFI_HANDLEImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUSStatus;
> +
> +  Status = SerialPortInitialize ();
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  return SystemTable->BootServices->CreateEventEx
> (EVT_NOTIFY_SIGNAL,
> +  TPL_NOTIFY,
> ExitBootServicesEvent, NULL,
> +
> ,
> +
> );
> +}
> +
> +/**
> +  If a runtime driver exits with an error, it must call
> this routine
> +  to free the allocated resource before the exiting.
> +
> +  @param[in]  ImageHandle   The firmware allocated
> handle for the EFI image.
> +  @param[in]  SystemTable   A pointer to the EFI System
> Table.
> +
> +  @retval EFI_SUCCESS   The Runtime Driver Lib
> shutdown successfully.
> +  @retval EFI_UNSUPPORTED   Runtime Driver lib was
> not initialized.
> +**/
> +EFI_STATUS
> +EFIAPI
> 

Re: [edk2] [PATCH v3 1/2] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Laszlo Ersek]

On 02/22/18 19:15, Ard Biesheuvel wrote:
> Introduce a variant of BaseDebugLibSerialPort that behaves correctly with
> regards to the use of the serial port after ExitBootServices(). At boot
> time, all DEBUG() prints and ASSERT() invocations are executed normally.
> At runtime, DEBUG() prints are dropped entirely, and ASSERT()s omit the
> serial output as well, and only perform the configured post-ASSERT()
> action, i.e., issue a CPU breakpoint or enter a deadloop.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c   
> | 346 
>  MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf 
> |  55 
>  MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni 
> |  21 ++
>  3 files changed, 422 insertions(+)

I compared this to v2, and separately to BaseDebugLibSerialPort too. It
looks good to me.

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel