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 CF0BB817C0 for ; Mon, 9 Jan 2017 11:33:23 -0800 (PST) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (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 763D8C04B948; Mon, 9 Jan 2017 19:33:24 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-74.phx2.redhat.com [10.3.116.74]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v09JXMFO019232; Mon, 9 Jan 2017 14:33:23 -0500 To: Michael Kinney , edk2-devel@ml01.01.org References: <1483984950-30208-1-git-send-email-michael.d.kinney@intel.com> Cc: Jordan Justen , Jeff Fan From: Laszlo Ersek Message-ID: <6cbaade1-9c9e-6708-f408-a3365f461059@redhat.com> Date: Mon, 9 Jan 2017 20:33:21 +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: <1483984950-30208-1-git-send-email-michael.d.kinney@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Mon, 09 Jan 2017 19:33:24 +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:33:23 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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? Thank you, Laszlo > + { NULL, 0 } > }; > > // >