* [Patch] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console @ 2017-01-09 5:31 Michael Kinney 2017-01-09 12:43 ` Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Michael Kinney @ 2017-01-09 5:31 UTC (permalink / raw) To: edk2-devel; +Cc: Jordan Justen, Laszlo Ersek, Jeff Fan 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Patch] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console 2017-01-09 5:31 [Patch] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console Michael Kinney @ 2017-01-09 12:43 ` Laszlo Ersek 2017-01-09 18:07 ` Kinney, Michael D 0 siblings, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2017-01-09 12:43 UTC (permalink / raw) To: Michael Kinney, edk2-devel; +Cc: Jordan Justen, Jeff Fan 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console 2017-01-09 12:43 ` Laszlo Ersek @ 2017-01-09 18:07 ` Kinney, Michael D 2017-01-09 19:10 ` Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Kinney, Michael D @ 2017-01-09 18:07 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@ml01.01.org, Kinney, Michael D Cc: Justen, Jordan L, Fan, Jeff 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console 2017-01-09 18:07 ` Kinney, Michael D @ 2017-01-09 19:10 ` Laszlo Ersek 0 siblings, 0 replies; 4+ messages in thread From: Laszlo Ersek @ 2017-01-09 19:10 UTC (permalink / raw) To: Kinney, Michael D, edk2-devel@ml01.01.org; +Cc: Justen, Jordan L, Fan, Jeff 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-09 19:10 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-09 5:31 [Patch] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console Michael Kinney 2017-01-09 12:43 ` Laszlo Ersek 2017-01-09 18:07 ` Kinney, Michael D 2017-01-09 19:10 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox