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

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

Laszlo,

Thanks for the feedback.  I have send v2 and v3.  Please ignore v2.
I missed the STD_ERROR flag in v2 and fixed in v3.

On a related topic, if you look at:

  QuarkPlatformPkg/Library/PlatformBootManagerLib/PlatformData.c

You will see in the function InitializePlatformBootManagerLib()
Updates the device path global variables based on UART and terminal
PCD settings.  

Do you think this applies to OVMF?

Thanks,

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, January 9, 2017 4: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: [edk2] [Patch] OvmgPkg/PlatformBootManagerLib: Add Debug Agent
> console
> 
> 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