[Patch] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console

[Patch] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console

[Michael Kinney]

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]
-- 
2.6.3.windows.1

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

[Laszlo Ersek]

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

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

[Kinney, Michael D]

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

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

[Laszlo Ersek]

On 01/09/17 19:07, Kinney, Michael D wrote:
> 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?

It would be possible, yes.

The UART settings currently come from "gUart" [BdsPlatform.h]; we could
replace those constants with FixedPcdGet() macro invocations (so they
remain usable in initializers). Furthermore, the terminal type is
hard-coded in "gPcAnsiTerminal".

... This code predates my involvement with OVMF (it has undergone some
changes / reorganizations, but the behavior has remained the same). I've
always thought it's been good enough (which is why I never touched it):
the defaults only make a difference when you create a new virtual
machine (or at least you forcibly empty your variable store and then
re-launch the VM). At that point, the ConIn / ConOut / ErrOut variables
are empty / nonexistent, and will be populated accordingly. But, at
subsequent boots, those variables will exist, so the

  (VarConout == NULL || VarConin == NULL)

check in PlatformInitializeConsole() will not fire, and previous
settings will take effect. Plus, these settings can be customized from
the setup utility. So what I usually do with a new VM (the first time I
use its virtual serial console) is, I flip the terminal type from
PC-ANSI to VT100 manually at the first boot, and then that setting sticks.

In ArmVirtPkg/Library/PlatformBootManagerLib, we have a more-or-less
from-the-scratch implementation for setting up the console variables,
and there we do use the PCDs. (For the default terminal type, we use an
ArmVirtPkg-specific buffer PCD called "PcdTerminalTypeGuidBuffer", which
is further controlled by the TTY_TERMINAL build flag.)

Consuming such PCDs in "OvmfPkg/Library/PlatformBootManagerLib" could
make sense, yes, but I never found the use case compelling enough to
spend time on patching the code. (This is not to discourage a patch, of
course.)

Thanks!
Laszlo

> 
> 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