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 <jordan.l.jus...@intel.com>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Cc: Jeff Fan <jeff....@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Michael Kinney <michael.d.kin...@intel.com>
>> ---
>> .../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 <ler...@redhat.com>
>
> 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 the BSD
>> License
>> which accompanies this distribution. The full text of the license may be
>> found at
>> @@ -65,6 +65,7 @@ Abstract:
>> #include <Guid/HobList.h>
>> #include <Guid/GlobalVariable.h>
>> #include <Guid/EventGroup.h>
>> +#include <Guid/DebugAgentGuid.h>
>>
>> #include <OvmfPlatforms.h>
>>
>> @@ -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.<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]
>> 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 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 }
>> };
>>
>> //
>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
