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

[Laszlo Ersek <lersek@redhat.com>]

On 01/09/17 20:33, Laszlo Ersek wrote:
> 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?

Ugh, apologies (it's been a while since last year and it looks like I
got a bit rusty...) I forgot that: (3) you too can do the changes (if
you agree with them) and commit / push the patch at once, with my R-b.

So I guess that would be simplest.

Thanks!
Laszlo

> Thank you,
> Laszlo
> 
>> +  { NULL, 0 }
>>  };
>>  
>>  //
>>
>