From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 B0015817B6 for ; Tue, 10 Jan 2017 12:56:23 -0800 (PST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga102.jf.intel.com with ESMTP; 10 Jan 2017 12:56:22 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,344,1477983600"; d="scan'208";a="51737013" Received: from orsmsx107.amr.corp.intel.com ([10.22.240.5]) by fmsmga005.fm.intel.com with ESMTP; 10 Jan 2017 12:56:22 -0800 Received: from orsmsx162.amr.corp.intel.com (10.22.240.85) by ORSMSX107.amr.corp.intel.com (10.22.240.5) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 10 Jan 2017 12:56:21 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.184]) by ORSMSX162.amr.corp.intel.com ([10.22.240.85]) with mapi id 14.03.0248.002; Tue, 10 Jan 2017 12:56:21 -0800 From: "Kinney, Michael D" To: Laszlo Ersek , "edk2-devel@ml01.01.org" , "Kinney, Michael D" CC: "Justen, Jordan L" , "Fan, Jeff" Thread-Topic: [Patch v3] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console Thread-Index: AQHSaq8/IMU0Gibe6k+BpC5yMEaNQqExEhoAgAEgSlA= Date: Tue, 10 Jan 2017 20:56:20 +0000 Message-ID: References: <1483984950-30208-1-git-send-email-michael.d.kinney@intel.com> <6cbaade1-9c9e-6708-f408-a3365f461059@redhat.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZmEzZDg1YjUtMGNmYi00YzI4LWE5ZTMtOTcyZTU1YzgyYjNkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlwveG93WTZcLzRKRUZ3WWwxWlU4M0R5anVlRGlkVHFFNkQwQ1dcL2xJZXd2V3c9In0= x-originating-ip: [10.22.254.140] MIME-Version: 1.0 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: Tue, 10 Jan 2017 20:56:23 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, Committed with your feedback applied and rb at: https://github.com/tianocore/edk2/commit/f4d575b51bbdb61e9f634dcabb84149ca8= 36cffa Mike > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Monday, January 9, 2017 11:44 AM > To: Kinney, Michael D ; edk2-devel@ml01.01.or= g > Cc: Justen, Jordan L ; Fan, Jeff > Subject: Re: [Patch v3] OvmgPkg/PlatformBootManagerLib: Add Debug Agent c= onsole >=20 > 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.<= BR> > >> + Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<= BR> > >> This program and the accompanying materials > >> are licensed and made available under the terms and conditions of t= he 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/PlatformBootManage= rLib.inf > b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > >> index 4a6bece..f9e35c9 100644 > >> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.in= f > >> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.in= f > >> @@ -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.<= 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 t= he 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 =3D gPnpPs2Keyb= oard; > >> ACPI_HID_DEVICE_PATH gPnp16550ComPortDeviceNode =3D gPnp16550Co= mPort; > >> UART_DEVICE_PATH gUartDeviceNode =3D gUart; > >> @@ -24,14 +35,48 @@ VENDOR_DEVICE_PATH gTerminalTypeDeviceNode= =3D > gPcAnsiTerminal; > >> // Platform specific keyboard device path > >> // > >> > >> + > >> +// > >> +// Debug Agent UART Device Path > >> +// > >> +VENDOR_UART_DEVICE_PATH gDebugAgentUartDevicePath =3D { > >> + { > >> + { > >> + 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[] =3D { > >> - { > >> - 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? >=20 > 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. >=20 > So I guess that would be simplest. >=20 > Thanks! > Laszlo >=20 > > Thank you, > > Laszlo > > > >> + { NULL, 0 } > >> }; > >> > >> // > >> > >