From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AACA781935 for ; Mon, 9 Jan 2017 11:10:52 -0800 (PST) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 317C07F7C0; Mon, 9 Jan 2017 19:10:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-74.phx2.redhat.com [10.3.116.74]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v09JApQm015254; Mon, 9 Jan 2017 14:10:51 -0500 To: "Kinney, Michael D" , "edk2-devel@ml01.01.org" References: <1483939865-24012-1-git-send-email-michael.d.kinney@intel.com> <51fada6f-2adb-5a7d-2577-7271b14eaa8e@redhat.com> Cc: "Justen, Jordan L" , "Fan, Jeff" From: Laszlo Ersek Message-ID: Date: Mon, 9 Jan 2017 20:10:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 09 Jan 2017 19:10:53 +0000 (UTC) Subject: Re: [Patch] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jan 2017 19:10:52 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 ; edk2-devel@ml01.01.org >> Cc: Justen, Jordan L ; Fan, Jeff >> 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 >>> Cc: Laszlo Ersek >>> Cc: Jeff Fan >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Michael Kinney >>> --- >>> .../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.
>>> + Copyright (c) 2004 - 2017, Intel Corporation. All rights reserved.
>>> 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 >>> #include >>> #include >>> +#include >>> >>> +// >>> +// 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.
>>> +# Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.
>>> # 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