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

[Laszlo Ersek <lersek@redhat.com>]

On 01/09/17 19:02, 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.h   | 13 ++++-
>  .../PlatformBootManagerLib.inf                     |  3 +-
>  .../Library/PlatformBootManagerLib/PlatformData.c  | 55 ++++++++++++++++++++--
>  3 files changed, 64 insertions(+), 7 deletions(-)


The patch looks good to me:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I just have two trivial improvements in mind:

> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> index 00a1d22..ec58efa 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> @@ -1,7 +1,7 @@
>  /** @file
>    Platform BDS customizations include file.
>  
> -  Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2006 - 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
> @@ -65,6 +65,7 @@ Abstract:
>  #include <Guid/HobList.h>
>  #include <Guid/GlobalVariable.h>
>  #include <Guid/EventGroup.h>
> +#include <Guid/DebugAgentGuid.h>
>  
>  #include <OvmfPlatforms.h>
>  
> @@ -144,6 +145,16 @@ extern VENDOR_DEVICE_PATH         gTerminalTypeDeviceNode;
>      DEVICE_PATH_MESSAGING_PC_ANSI \
>    }
>  
> +#define gEndEntire \
> +  { \
> +    END_DEVICE_PATH_TYPE, \
> +    END_ENTIRE_DEVICE_PATH_SUBTYPE, \
> +    { \
> +      END_DEVICE_PATH_LENGTH, \
> +      0 \
> +    } \
> +  }
> +
>  #define PCI_CLASS_SCC          0x07
>  #define PCI_SUBCLASS_SERIAL    0x00
>  #define PCI_IF_16550           0x02
> 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]
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> index e9737a7..fa5cd7d 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> @@ -2,7 +2,7 @@
>    Defined the platform specific device path which will be used by
>    platform Bbd to perform the platform policy connect.
>  
> -  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
> @@ -15,6 +15,17 @@
>  
>  #include "BdsPlatform.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;

(1) I think this typedef should be wrapped in "#pragma pack (1)".

> +
> +
>  ACPI_HID_DEVICE_PATH       gPnpPs2KeyboardDeviceNode  = gPnpPs2Keyboard;
>  ACPI_HID_DEVICE_PATH       gPnp16550ComPortDeviceNode = gPnp16550ComPort;
>  UART_DEVICE_PATH           gUartDeviceNode            = gUart;
> @@ -24,14 +35,48 @@ VENDOR_DEVICE_PATH         gTerminalTypeDeviceNode    = gPcAnsiTerminal;
>  // Platform specific keyboard device path
>  //
>  
> +
> +//
> +// Debug Agent UART Device Path
> +//
> +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
> +  },
> +  gPcAnsiTerminal,
> +  gEndEntire
> +};
> +
> +
>  //
>  // Predefined platform default console device path
>  //
>  PLATFORM_CONSOLE_CONNECT_ENTRY   gPlatformConsole[] = {
> -  {
> -    NULL,
> -    0
> -  }
> +  { (EFI_DEVICE_PATH_PROTOCOL *) &gDebugAgentUartDevicePath, (CONSOLE_OUT | CONSOLE_IN | STD_ERROR) },

(2) I'd like if we could split this long-ish line, so that the bitmask
starts a separate line.

Do you prefer sending v4, or are you okay if I do these changes before
pushing the patch?

Thank you,
Laszlo

> +  { NULL, 0 }
>  };
>  
>  //
>