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 C6E9E8193A for ; Mon, 9 Jan 2017 11:43:44 -0800 (PST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 6241E4DD4C; Mon, 9 Jan 2017 19:43:45 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-74.phx2.redhat.com [10.3.116.74]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v09Jhhhs011586; Mon, 9 Jan 2017 14:43:44 -0500 To: Michael Kinney , edk2-devel@ml01.01.org References: <1483984950-30208-1-git-send-email-michael.d.kinney@intel.com> <6cbaade1-9c9e-6708-f408-a3365f461059@redhat.com> Cc: Jordan Justen , Jeff Fan From: Laszlo Ersek Message-ID: Date: Mon, 9 Jan 2017 20:43:42 +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: <6cbaade1-9c9e-6708-f408-a3365f461059@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 09 Jan 2017 19:43:45 +0000 (UTC) Subject: Re: [Patch v3] 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:43:44 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 01/09/17 20:33, Laszlo Ersek wrote: > On 01/09/17 19:02, 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.h | 13 ++++- >> .../PlatformBootManagerLib.inf | 3 +- >> .../Library/PlatformBootManagerLib/PlatformData.c | 55 ++++++++++++++++++++-- >> 3 files changed, 64 insertions(+), 7 deletions(-) > > > The patch looks good to me: > > Reviewed-by: Laszlo Ersek > > I just have two trivial improvements in mind: > >> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h >> index 00a1d22..ec58efa 100644 >> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h >> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h >> @@ -1,7 +1,7 @@ >> /** @file >> Platform BDS customizations include file. >> >> - Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
>> + Copyright (c) 2006 - 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 >> @@ -65,6 +65,7 @@ Abstract: >> #include >> #include >> #include >> +#include >> >> #include >> >> @@ -144,6 +145,16 @@ extern VENDOR_DEVICE_PATH gTerminalTypeDeviceNode; >> DEVICE_PATH_MESSAGING_PC_ANSI \ >> } >> >> +#define gEndEntire \ >> + { \ >> + END_DEVICE_PATH_TYPE, \ >> + END_ENTIRE_DEVICE_PATH_SUBTYPE, \ >> + { \ >> + END_DEVICE_PATH_LENGTH, \ >> + 0 \ >> + } \ >> + } >> + >> #define PCI_CLASS_SCC 0x07 >> #define PCI_SUBCLASS_SERIAL 0x00 >> #define PCI_IF_16550 0x02 >> 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] >> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c >> index e9737a7..fa5cd7d 100644 >> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c >> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c >> @@ -2,7 +2,7 @@ >> Defined the platform specific device path which will be used by >> platform Bbd to perform the platform policy connect. >> >> - 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 >> @@ -15,6 +15,17 @@ >> >> #include "BdsPlatform.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; > > (1) I think this typedef should be wrapped in "#pragma pack (1)". > >> + >> + >> ACPI_HID_DEVICE_PATH gPnpPs2KeyboardDeviceNode = gPnpPs2Keyboard; >> ACPI_HID_DEVICE_PATH gPnp16550ComPortDeviceNode = gPnp16550ComPort; >> UART_DEVICE_PATH gUartDeviceNode = gUart; >> @@ -24,14 +35,48 @@ VENDOR_DEVICE_PATH gTerminalTypeDeviceNode = gPcAnsiTerminal; >> // Platform specific keyboard device path >> // >> >> + >> +// >> +// Debug Agent UART Device Path >> +// >> +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 >> + }, >> + gPcAnsiTerminal, >> + gEndEntire >> +}; >> + >> + >> // >> // Predefined platform default console device path >> // >> PLATFORM_CONSOLE_CONNECT_ENTRY gPlatformConsole[] = { >> - { >> - NULL, >> - 0 >> - } >> + { (EFI_DEVICE_PATH_PROTOCOL *) &gDebugAgentUartDevicePath, (CONSOLE_OUT | CONSOLE_IN | STD_ERROR) }, > > (2) I'd like if we could split this long-ish line, so that the bitmask > starts a separate line. > > Do you prefer sending v4, or are you okay if I do these changes before > pushing the patch? Ugh, apologies (it's been a while since last year and it looks like I got a bit rusty...) I forgot that: (3) you too can do the changes (if you agree with them) and commit / push the patch at once, with my R-b. So I guess that would be simplest. Thanks! Laszlo > Thank you, > Laszlo > >> + { NULL, 0 } >> }; >> >> // >> >