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

[Laszlo Ersek <lersek@redhat.com>]

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