Re: [Patch] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console

[Laszlo Ersek <lersek@redhat.com>]

On 01/09/17 06:31, Michael Kinney wrote:
> The Debug Agent in the SourceLevelDebugPkg can multiplex
> both source level debug messages and console messages on
> the same UART.  WHen this is done, the Debug Agent owns
> the UART device and an additional device handle with a
> Serial I/O Protocol is produced with a VenHw device path
> node.
> 
> In order for a platform to provide a UART based console
> when the Debug Agent is using the same UART device, the
> PlatformBootManagerLib must consider the SerialI/O
> Protocol produces by the Debug Agent as one of the
> supported consoles.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jeff Fan <jeff.fan@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
> ---
>  .../Library/PlatformBootManagerLib/BdsPlatform.c   | 82 +++++++++++++++++++++-
>  .../PlatformBootManagerLib.inf                     |  3 +-
>  2 files changed, 83 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index cc35630..0d94840 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Platform BDS customizations.
>  
> -  Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2004 - 2017, Intel Corporation. All rights reserved.<BR>
>    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
> @@ -16,7 +16,17 @@
>  #include <Guid/XenInfo.h>
>  #include <Guid/RootBridgesConnectedEventGroup.h>
>  #include <Protocol/FirmwareVolume2.h>
> +#include <Guid/DebugAgentGuid.h>
>  
> +//
> +// Debug Agent UART Device Path structure
> +//
> +typedef struct {
> +  VENDOR_DEVICE_PATH        VendorHardware;
> +  UART_DEVICE_PATH          Uart;
> +  VENDOR_DEVICE_PATH        TerminalType;
> +  EFI_DEVICE_PATH_PROTOCOL  End;
> +} VENDOR_UART_DEVICE_PATH;
>  
>  //
>  // Global data
> @@ -28,6 +38,53 @@ VOID          *mEmuVariableEventReg;
>  EFI_EVENT     mEmuVariableEvent;
>  BOOLEAN       mDetectVgaOnly;
>  UINT16        mHostBridgeDevId;
> +VENDOR_UART_DEVICE_PATH gDebugAgentUartDevicePath = {
> +  {
> +    {
> +      HARDWARE_DEVICE_PATH,
> +      HW_VENDOR_DP,
> +      {
> +        (UINT8) (sizeof (VENDOR_DEVICE_PATH)),
> +        (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
> +      }
> +    },
> +    EFI_DEBUG_AGENT_GUID,
> +  },
> +  {
> +    {
> +      MESSAGING_DEVICE_PATH,
> +      MSG_UART_DP,
> +      {
> +        (UINT8) (sizeof (UART_DEVICE_PATH)),
> +        (UINT8) ((sizeof (UART_DEVICE_PATH)) >> 8)
> +      }
> +    },
> +    0,  // Reserved
> +    0,  // BaudRate - Default
> +    0,  // DataBits - Default
> +    0,  // Parity   - Default
> +    0,  // StopBits - Default
> +  },
> +  {
> +    {
> +      MESSAGING_DEVICE_PATH,
> +      MSG_VENDOR_DP,
> +      {
> +        (UINT8)(sizeof (VENDOR_DEVICE_PATH)),
> +        (UINT8)((sizeof (VENDOR_DEVICE_PATH)) >> 8)
> +      }
> +    },
> +    DEVICE_PATH_MESSAGING_PC_ANSI
> +  },
> +  {
> +    END_DEVICE_PATH_TYPE,
> +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +    {
> +      END_DEVICE_PATH_LENGTH,
> +      0
> +    }
> +  }
> +};
>  
>  //
>  // Table of host IRQs matching PCI IRQs A-D
> @@ -532,6 +589,29 @@ Returns:
>    EfiBootManagerUpdateConsoleVariable (ConIn, DevicePath, NULL);
>    EfiBootManagerUpdateConsoleVariable (ErrOut, DevicePath, NULL);
>  
> +  //
> +  // Register Debug Agent
> +  //
> +  DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)&gDebugAgentUartDevicePath;
> +
> +  //
> +  // Print Device Path
> +  //
> +  DevPathStr = ConvertDevicePathToText (DevicePath, FALSE, FALSE);
> +  if (DevPathStr != NULL) {
> +    DEBUG((
> +      EFI_D_INFO,
> +      "BdsPlatform.c+%d: DevPath: %s\n",
> +      __LINE__,
> +      DevPathStr
> +      ));
> +    FreePool(DevPathStr);
> +  }
> +
> +  EfiBootManagerUpdateConsoleVariable (ConOut, DevicePath, NULL);
> +  EfiBootManagerUpdateConsoleVariable (ConIn, DevicePath, NULL);
> +  EfiBootManagerUpdateConsoleVariable (ErrOut, DevicePath, NULL);
> +
>    return EFI_SUCCESS;
>  }
>  
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index 4a6bece..f9e35c9 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  #  Platform BDS customizations library.
>  #
> -#  Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.<BR>
>  #  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
> @@ -36,6 +36,7 @@
>    MdePkg/MdePkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
> +  SourceLevelDebugPkg/SourceLevelDebugPkg.dec
>    OvmfPkg/OvmfPkg.dec
>  
>  [LibraryClasses]
> 

I'm going to suggest / request almost the same thing here as I did
(independently) last week:

https://lists.01.org/pipermail/edk2-devel/2017-January/006190.html

In other words, given that the complete device path for the DebugAgent
console is constant and known in advance,

(1) please move the definitions of VENDOR_UART_DEVICE_PATH and
"gDebugAgentUartDevicePath" to the file

  OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c

and

(2) rather than inserting additional logic to the
PrepareLpcBridgeDevicePath() callback function, please add the address
of "gDebugAgentUartDevicePath" to "gPlatformConsole" in
"PlatformData.c", with a

  CONSOLE_IN | CONSOLE_OUT | STD_ERROR

bitmask. This will cause the PlatformInitializeConsole() function to
update the console variables appropriately, when necessary.

(The debug output won't contain the textual rendering of the device
path, but that's fine (it's a constant devpath).)

(3) In the "gDebugAgentUartDevicePath" initializer, feel free to reuse
the "gPcAnsiTerminal" macro for shortening the "TerminalType" field's
initializer.

Thanks!
Laszlo