* [Patch v3] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console
@ 2017-01-09 18:02 Michael Kinney
2017-01-09 19:33 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Michael Kinney @ 2017-01-09 18:02 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Laszlo Ersek, Jeff Fan
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.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
.../Library/PlatformBootManagerLib/BdsPlatform.h | 13 ++++-
.../PlatformBootManagerLib.inf | 3 +-
.../Library/PlatformBootManagerLib/PlatformData.c | 55 ++++++++++++++++++++--
3 files changed, 64 insertions(+), 7 deletions(-)
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;
+
+
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) },
+ { NULL, 0 }
};
//
--
2.6.3.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Patch v3] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console
2017-01-09 18:02 [Patch v3] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console Michael Kinney
@ 2017-01-09 19:33 ` Laszlo Ersek
2017-01-09 19:43 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2017-01-09 19:33 UTC (permalink / raw)
To: Michael Kinney, edk2-devel; +Cc: Jordan Justen, Jeff Fan
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.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jeff Fan <jeff.fan@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney <michael.d.kinney@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 <lersek@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?
Thank you,
Laszlo
> + { NULL, 0 }
> };
>
> //
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch v3] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console
2017-01-09 19:33 ` Laszlo Ersek
@ 2017-01-09 19:43 ` Laszlo Ersek
2017-01-10 20:56 ` Kinney, Michael D
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2017-01-09 19:43 UTC (permalink / raw)
To: Michael Kinney, edk2-devel; +Cc: Jordan Justen, Jeff Fan
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.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Jeff Fan <jeff.fan@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Michael Kinney <michael.d.kinney@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 <lersek@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 }
>> };
>>
>> //
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch v3] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console
2017-01-09 19:43 ` Laszlo Ersek
@ 2017-01-10 20:56 ` Kinney, Michael D
0 siblings, 0 replies; 4+ messages in thread
From: Kinney, Michael D @ 2017-01-10 20:56 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@ml01.01.org, Kinney, Michael D
Cc: Justen, Jordan L, Fan, Jeff
Laszlo,
Committed with your feedback applied and rb at:
https://github.com/tianocore/edk2/commit/f4d575b51bbdb61e9f634dcabb84149ca836cffa
Mike
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, January 9, 2017 11:44 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@ml01.01.org
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Fan, Jeff <jeff.fan@intel.com>
> Subject: Re: [Patch v3] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console
>
> 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.justen@intel.com>
> >> Cc: Laszlo Ersek <lersek@redhat.com>
> >> Cc: Jeff Fan <jeff.fan@intel.com>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Michael Kinney <michael.d.kinney@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 <lersek@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 }
> >> };
> >>
> >> //
> >>
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-10 20:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-09 18:02 [Patch v3] OvmgPkg/PlatformBootManagerLib: Add Debug Agent console Michael Kinney
2017-01-09 19:33 ` Laszlo Ersek
2017-01-09 19:43 ` Laszlo Ersek
2017-01-10 20:56 ` Kinney, Michael D
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox