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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Laszlo,

Committed with your feedback applied and rb at:

https://github.com/tianocore/edk2/commit/f4d575b51bbdb61e9f634dcabb84149ca836cffa

Mike


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, January 9, 2017 11:44 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@ml01.01.org
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Fan, Jeff <jeff.fan@intel.com>
> Subject: Re: [Patch v3] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console
> 
> 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 }
> >>  };
> >>
> >>  //
> >>
> >