[edk2] [PATCH V3 1/5] Add "TtyTerm" terminal type to TerminalDxe

Treat ASCII 0x7F as backspace, rather than delete, for TTY terminals. This
better matches the default Linux terminal settings that are used when connecting
to a simulated platform using xterm or a similar terminal program.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Roy Franz <***@linaro.org>
Reviewed-by: Ard Biesheuvel <***@linaro.org>
---
MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
index 17a1244..227df85 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
@@ -1561,8 +1561,14 @@ UnicodeToEfiKey (
}

if (UnicodeChar == DEL) {
- Key.ScanCode = SCAN_DELETE;
- Key.UnicodeChar = 0;
+ if (TerminalDevice->TerminalType == TTYTERMTYPE) {
+ Key.ScanCode = SCAN_NULL;
+ Key.UnicodeChar = CHAR_BACKSPACE;
+ }
+ else {
+ Key.ScanCode = SCAN_DELETE;
+ Key.UnicodeChar = 0;
+ }
} else {
Key.ScanCode = SCAN_NULL;
Key.UnicodeChar = UnicodeChar;

--
2.1.4

Roy Franz

2015-07-08 00:44:16 UTC

Accept the VT220 escape code [3~ as backspace for TtyTerm terminals. This is
sent by many Linux terminals by default. Also accept VT220 function keys
F1-F12, and VT100 F1-F4 keys as these are commonly sent by Linux terminals.
The VT220 escape codes are longer, and variable length so a new state is added
to the state machine along with a variable to construct the multibyte escape
sequence.
There are currently no ambiguous escape sequence prefixes accepted, so the TTY
terminal accepts escape sequences for a variety of terminals. The goal is to
'just work' with as many terminals as possible, rather than properly emulating
any specific terminal. Backspace, Del, and F10 have been tested on xterm,
rxvt, tmux, and screen.
Note: The existing vt100 function key handling does not match the vt100
documentation that I found, so I added the TTY terminal handling
of VT100 F1-F4 (really PF1-PF4 on vt100) separately. The vt100
has no F5-F10 keys, so I don't know what the current vt100 code
is based on.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Roy Franz <***@linaro.org>
---
MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c | 6 ++
MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h | 3 +
MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c | 93 +++++++++++++++++++-
3 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index babb097..597b15d 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -81,6 +81,12 @@ TERMINAL_DEV mTerminalDevTemplate = {
NULL, // TwoSecondTimeOut
INPUT_STATE_DEFAULT,
RESET_STATE_DEFAULT,
+ {
+ 0,
+ 0,
+ 0
+ },
+ 0,
FALSE,
{ // SimpleTextInputEx
TerminalConInResetEx,
diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
index 03542a4..269d2ae 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
@@ -99,6 +99,8 @@ typedef struct {
EFI_EVENT TwoSecondTimeOut;
UINT32 InputState;
UINT32 ResetState;
+ UINT16 TtyEscapeStr[3];
+ INTN TtyEscapeIndex;

//
// Esc could not be output to the screen by user,
@@ -118,6 +120,7 @@ typedef struct {
#define INPUT_STATE_LEFTOPENBRACKET 0x04
#define INPUT_STATE_O 0x08
#define INPUT_STATE_2 0x10
+#define INPUT_STATE_LEFTOPENBRACKET_2 0x20

#define RESET_STATE_DEFAULT 0x00
#define RESET_STATE_ESC_R 0x01
diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
index 227df85..fbaf33b 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
@@ -1223,7 +1223,8 @@ UnicodeToEfiKey (
continue;
}

- if (UnicodeChar == 'O' && TerminalDevice->TerminalType == VT100TYPE) {
+ if (UnicodeChar == 'O' && (TerminalDevice->TerminalType == VT100TYPE ||
+ TerminalDevice->TerminalType == TTYTERMTYPE)) {
TerminalDevice->InputState |= INPUT_STATE_O;
TerminalDevice->ResetState = RESET_STATE_DEFAULT;
continue;
@@ -1371,6 +1372,22 @@ UnicodeToEfiKey (
default :
break;
}
+ } else if (TerminalDevice->TerminalType == TTYTERMTYPE) {
+ /* Also accept VT100 escape codes for F1-F4 for TTY term */
+ switch (UnicodeChar) {
+ case 'P':
+ Key.ScanCode = SCAN_F1;
+ break;
+ case 'Q':
+ Key.ScanCode = SCAN_F2;
+ break;
+ case 'R':
+ Key.ScanCode = SCAN_F3;
+ break;
+ case 'S':
+ Key.ScanCode = SCAN_F4;
+ break;
+ }
}

if (Key.ScanCode != SCAN_NULL) {
@@ -1514,6 +1531,21 @@ UnicodeToEfiKey (
}
}

+ /*
+ * The VT220 escape codes that the TTY terminal accepts all have
+ * numeric codes, and there are no ambiguous prefixes shared with
+ * other terminal types.
+ */
+ if (TerminalDevice->TerminalType == TTYTERMTYPE &&
+ Key.ScanCode == SCAN_NULL &&
+ UnicodeChar >= '0' &&
+ UnicodeChar <= '9') {
+ TerminalDevice->TtyEscapeStr[0] = UnicodeChar;
+ TerminalDevice->TtyEscapeIndex = 1;
+ TerminalDevice->InputState |= INPUT_STATE_LEFTOPENBRACKET_2;
+ continue;
+ }
+
if (Key.ScanCode != SCAN_NULL) {
Key.UnicodeChar = 0;
EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
@@ -1527,6 +1559,65 @@ UnicodeToEfiKey (
break;

+ case INPUT_STATE_ESC | INPUT_STATE_LEFTOPENBRACKET | INPUT_STATE_LEFTOPENBRACKET_2:
+ /*
+ * Here we handle the VT220 escape codes that we accept. This
+ * state is only used by the TTY terminal type.
+ */
+ Key.ScanCode = SCAN_NULL;
+ if (TerminalDevice->TerminalType == TTYTERMTYPE) {
+
+ if (UnicodeChar == '~' && TerminalDevice->TtyEscapeIndex <= 2) {
+ UINTN EscCode;
+ TerminalDevice->TtyEscapeStr[TerminalDevice->TtyEscapeIndex] = 0; /* Terminate string */
+ EscCode = StrDecimalToUintn(TerminalDevice->TtyEscapeStr);
+ switch (EscCode) {
+ case 3:
+ Key.ScanCode = SCAN_DELETE;
+ break;
+ case 11:
+ case 12:
+ case 13:
+ case 14:
+ case 15:
+ Key.ScanCode = SCAN_F1 + EscCode - 11;
+ break;
+ case 17:
+ case 18:
+ case 19:
+ case 20:
+ case 21:
+ Key.ScanCode = SCAN_F6 + EscCode - 17;
+ break;
+ case 23:
+ case 24:
+ Key.ScanCode = SCAN_F11 + EscCode - 23;
+ break;
+ default:
+ break;
+ }
+ } else if (TerminalDevice->TtyEscapeIndex == 1){
+ /* 2 character escape code */
+ TerminalDevice->TtyEscapeStr[TerminalDevice->TtyEscapeIndex++] = UnicodeChar;
+ continue;
+ }
+ else {
+ DEBUG ((EFI_D_ERROR, "Unexpected state in escape2\n"));
+ }
+ }
+ TerminalDevice->ResetState = RESET_STATE_DEFAULT;
+
+ if (Key.ScanCode != SCAN_NULL) {
+ Key.UnicodeChar = 0;
+ EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
+ TerminalDevice->InputState = INPUT_STATE_DEFAULT;
+ UnicodeToEfiKeyFlushState (TerminalDevice);
+ continue;
+ }
+
+ UnicodeToEfiKeyFlushState (TerminalDevice);
+ break;
+
default:
//
// Invalid state. This should never happen.

--
2.1.4

Roy Franz

2015-07-08 00:44:17 UTC

From: Laszlo Ersek <***@redhat.com>

Add a fixed pointer PCD to allow build-time selection of VT100 or TTY terminal
type. The default remains VT100 emulation.
Add support for building the ARM QEMU platforms with the TTY terminal
with the "-D TTY_TERMINAL" build option.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <***@redhat.com>
[Roy Franz: minor edits: add TtyTerminal GUID, rename LINUX_TERMINAL to TTY_TERMINAL]
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Roy Franz <***@linaro.org>
Reviewed-by: Ard Biesheuvel <***@linaro.org>
---
ArmVirtPkg/ArmVirt.dsc.inc | 6 ++++++
ArmVirtPkg/ArmVirtPkg.dec | 7 +++++++
ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 12 ++++++++----
ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf | 4 ++++
4 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 7ec0de4..2feebd3 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -15,6 +15,7 @@

[Defines]
DEFINE DEBUG_PRINT_ERROR_LEVEL = 0x8000004F
+ DEFINE TTY_TERMINAL = FALSE

[LibraryClasses.common]
!if $(TARGET) == RELEASE
@@ -359,6 +360,11 @@
gEfiSecurityPkgTokenSpaceGuid.PcdRemovableMediaImageVerificationPolicy|0x04
!endif

+!if $(TTY_TERMINAL) == TRUE
+ # Set terminal type to TtyTerm, the value encoded is EFI_TTY_TERM_GUID
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91, 0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51, 0xef, 0x94}
+!endif
+
[Components.common]
#
# Networking stack
diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 7bbd9ff..9833c5a 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -49,6 +49,13 @@
#
gArmVirtTokenSpaceGuid.PcdDeviceTreeAllocationPadding|256|UINT32|0x00000002

+ #
+ # Binary representation of the GUID that determines the terminal type. The
+ # size must be exactly 16 bytes. The default value corresponds to
+ # EFI_VT_100_GUID.
+ #
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
+
[PcdsDynamic, PcdsFixedAtBuild]
#
# ARM PSCI function invocations can be done either through hypervisor
diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
index 13830cb..b242a29 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
@@ -35,7 +35,7 @@
typedef struct {
VENDOR_DEVICE_PATH SerialDxe;
UART_DEVICE_PATH Uart;
- VENDOR_DEFINED_DEVICE_PATH Vt100;
+ VENDOR_DEFINED_DEVICE_PATH TermType;
EFI_DEVICE_PATH_PROTOCOL End;
} PLATFORM_SERIAL_CONSOLE;
#pragma pack ()
@@ -67,14 +67,16 @@ STATIC PLATFORM_SERIAL_CONSOLE mSerialConsole = {
},

//
- // VENDOR_DEFINED_DEVICE_PATH Vt100
+ // VENDOR_DEFINED_DEVICE_PATH TermType
//
{
{
MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,
DP_NODE_LEN (VENDOR_DEFINED_DEVICE_PATH)
- },
- EFI_VT_100_GUID
+ }
+ //
+ // Guid to be filled in dynamically
+ //
},

//
@@ -421,6 +423,8 @@ PlatformBdsPolicyBehavior (
//
// Add the hardcoded serial console device path to ConIn, ConOut, ErrOut.
//
+ CopyGuid (&mSerialConsole.TermType.Guid,
+ PcdGetPtr (PcdTerminalTypeGuidBuffer));
BdsLibUpdateConsoleVariable (L"ConIn",
(EFI_DEVICE_PATH_PROTOCOL *)&mSerialConsole, NULL);
BdsLibUpdateConsoleVariable (L"ConOut",
diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
index d998216..9a3cfcd 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
@@ -39,6 +39,7 @@
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec

[LibraryClasses]
BaseLib
@@ -61,6 +62,9 @@
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits

+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer
+
[Guids]
gEfiFileInfoGuid
gEfiFileSystemInfoGuid

--
2.1.4

Heyi Guo

2015-07-08 02:46:09 UTC

Hi Roy,

I have one question; please see my comments below.

Post by Roy Franz
Add a fixed pointer PCD to allow build-time selection of VT100 or TTY terminal
type. The default remains VT100 emulation.
Add support for building the ARM QEMU platforms with the TTY terminal
with the "-D TTY_TERMINAL" build option.
Contributed-under: TianoCore Contribution Agreement 1.0
[Roy Franz: minor edits: add TtyTerminal GUID, rename LINUX_TERMINAL to TTY_TERMINAL]
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmVirtPkg/ArmVirt.dsc.inc | 6 ++++++
ArmVirtPkg/ArmVirtPkg.dec | 7 +++++++
ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 12 ++++++++----
ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf | 4 ++++
4 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 7ec0de4..2feebd3 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -15,6 +15,7 @@
[Defines]
DEFINE DEBUG_PRINT_ERROR_LEVEL = 0x8000004F
+ DEFINE TTY_TERMINAL = FALSE

I see in ArmVirtQemu.dsc file (patch 5#), we are using #ifdef
TTY_TERMINAL. In my opinion the above statement will cause #ifdef
TTY_TERMINAL to be always true.

Post by Roy Franz
[LibraryClasses.common]
!if $(TARGET) == RELEASE
@@ -359,6 +360,11 @@
gEfiSecurityPkgTokenSpaceGuid.PcdRemovableMediaImageVerificationPolicy|0x04
!endif
+!if $(TTY_TERMINAL) == TRUE

And I don't understand why we use "!if" here and "!ifdef" in dsc file.

I should have tested the patch directly, but I couldn't apply the mail
patches with "git am" right now (git 2.1.4 on Debian; really appreciate
if someone could tell me the reason :-) ).

Heyi Guo

Post by Roy Franz
+ # Set terminal type to TtyTerm, the value encoded is EFI_TTY_TERM_GUID
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91, 0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51, 0xef, 0x94}
+!endif
+
[Components.common]
#
# Networking stack
diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 7bbd9ff..9833c5a 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -49,6 +49,13 @@
#
gArmVirtTokenSpaceGuid.PcdDeviceTreeAllocationPadding|256|UINT32|0x00000002
+ #
+ # Binary representation of the GUID that determines the terminal type. The
+ # size must be exactly 16 bytes. The default value corresponds to
+ # EFI_VT_100_GUID.
+ #
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
+
[PcdsDynamic, PcdsFixedAtBuild]
#
# ARM PSCI function invocations can be done either through hypervisor
diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
index 13830cb..b242a29 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
@@ -35,7 +35,7 @@
typedef struct {
VENDOR_DEVICE_PATH SerialDxe;
UART_DEVICE_PATH Uart;
- VENDOR_DEFINED_DEVICE_PATH Vt100;
+ VENDOR_DEFINED_DEVICE_PATH TermType;
EFI_DEVICE_PATH_PROTOCOL End;
} PLATFORM_SERIAL_CONSOLE;
#pragma pack ()
@@ -67,14 +67,16 @@ STATIC PLATFORM_SERIAL_CONSOLE mSerialConsole = {
},
//
- // VENDOR_DEFINED_DEVICE_PATH Vt100
+ // VENDOR_DEFINED_DEVICE_PATH TermType
//
{
{
MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,
DP_NODE_LEN (VENDOR_DEFINED_DEVICE_PATH)
- },
- EFI_VT_100_GUID
+ }
+ //
+ // Guid to be filled in dynamically
+ //
},
//
@@ -421,6 +423,8 @@ PlatformBdsPolicyBehavior (
//
// Add the hardcoded serial console device path to ConIn, ConOut, ErrOut.
//
+ CopyGuid (&mSerialConsole.TermType.Guid,
+ PcdGetPtr (PcdTerminalTypeGuidBuffer));
BdsLibUpdateConsoleVariable (L"ConIn",
(EFI_DEVICE_PATH_PROTOCOL *)&mSerialConsole, NULL);
BdsLibUpdateConsoleVariable (L"ConOut",
diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
index d998216..9a3cfcd 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
@@ -39,6 +39,7 @@
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
[LibraryClasses]
BaseLib
@@ -61,6 +62,9 @@
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer
+
[Guids]
gEfiFileInfoGuid
gEfiFileSystemInfoGuid

Laszlo Ersek

2015-07-08 09:46:35 UTC

Post by Heyi Guo
Hi Roy,
I have one question; please see my comments below.

I see in ArmVirtQemu.dsc file (patch 5#), we are using #ifdef
TTY_TERMINAL. In my opinion the above statement will cause #ifdef
TTY_TERMINAL to be always true.

I agree. That's my only comment for patch #5 too. Patch #5 should be
consistent with this one here, and use

!if $(TTY_TERMINAL) == TRUE

And I don't understand why we use "!if" here and "!ifdef" in dsc file.

BTW, we have a good reason for this kind of conditional. We use

$(WHATEVER_ENABLE) == TRUE

because that allows

-D WHATEVER_ENABLE=FALSE

on a command line to *override* an earlier

-D WHATEVER_ENABLE[=TRUE]

on the same command line. The "build" utility has no -U option (for
undefining a macro).

Assume you have a build script that hardcodes

-D WHATEVER_ENABLE

because that's how you build the tree most of the time. However,
occasionally you'd like to build without that feature, and you'd like to
override the macro. If the script just appends "$@" to the build command
line, *and* the DSC files use the above kind of conditional, then you
can simply pass -D WHATEVER_ENABLE=FALSE to override the macro (but
still use the rest of your build script).

... Yes, we should have done the same with INTEL_BDS too, and we didn't.

I guess I didn't catch that because I *never* build without INTEL_BDS.

... Well, that, and because the patch that added INTEL_BDS (SVN r16208)
was never reviewed on the list.

Anyway, I digress.

Post by Heyi Guo
I should have tested the patch directly, but I couldn't apply the mail
patches with "git am" right now (git 2.1.4 on Debian; really appreciate
if someone could tell me the reason :-) ).

Sure. The edk2 project uses CRLF line endings. For that to work in the
first place (without git yelling at you all the time), you need to add
the following git config settings:

[core]
whitespace = cr-at-eol

Then, to make git-am "compatible", you further need:

[am]
keepcr = true

This will *almost* make things work, but it will still break down when
you have a patch that adds or removes files *and* has crossed MTA
boundaries. In this case, you'll have

+++ /dev/null\r

or

--- /dev/null\r

hunk headers in the patch. The am.keepcr setting (which is otherwise
necessary for the *source code* CRLFs) will prevent git-am from
stripping the \r even from the /dev/null hunk headers, and that will
trip up git-am. So you need to pre-process the /dev/null hunk headers
manually, stripping *only those* \r characters, *if* you have a patch
that adds or removes files *and* has crossed MTA boundaries. (If the
patch doesn't cross an MTA, ie. it is straight out of git format-patch,
then /dev/null will have no \r.)

Summary (I'm sure you'll appreciate a summary): if you'd like to apply
edk2 patches from the mailing list, with git-am, do the following:
(1) set "core.whitespace" to "cr-at-eol",
(2) set "am.keepcr" to "true",
(3) save the patch emails *intact* into local files (eg. *.eml)
(4) preprocess those local files with:

sed -r -i 's,^((---|\+\+\+) /dev/null)\r$,\1,' *.eml

(5) apply them with git-am.

HTH
Laszlo

Post by Heyi Guo
Heyi Guo

Post by Roy Franz
+ # Set terminal type to TtyTerm, the value encoded is EFI_TTY_TERM_GUID
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91,
0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51,
0xef, 0x94}
+!endif
+
[Components.common]
#
# Networking stack
diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 7bbd9ff..9833c5a 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -49,6 +49,13 @@
#
gArmVirtTokenSpaceGuid.PcdDeviceTreeAllocationPadding|256|UINT32|0x00000002
+ #
+ # Binary representation of the GUID that determines the terminal type. The
+ # size must be exactly 16 bytes. The default value corresponds to
+ # EFI_VT_100_GUID.
+ #
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6,
0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F,
0xC1, 0x4D}|VOID*|0x00000007
+
[PcdsDynamic, PcdsFixedAtBuild]
#
# ARM PSCI function invocations can be done either through hypervisor
diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
index 13830cb..b242a29 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
@@ -35,7 +35,7 @@
typedef struct {
VENDOR_DEVICE_PATH SerialDxe;
UART_DEVICE_PATH Uart;
- VENDOR_DEFINED_DEVICE_PATH Vt100;
+ VENDOR_DEFINED_DEVICE_PATH TermType;
EFI_DEVICE_PATH_PROTOCOL End;
} PLATFORM_SERIAL_CONSOLE;
#pragma pack ()
@@ -67,14 +67,16 @@ STATIC PLATFORM_SERIAL_CONSOLE mSerialConsole = {
},
//
- // VENDOR_DEFINED_DEVICE_PATH Vt100
+ // VENDOR_DEFINED_DEVICE_PATH TermType
//
{
{
MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,
DP_NODE_LEN (VENDOR_DEFINED_DEVICE_PATH)
- },
- EFI_VT_100_GUID
+ }
+ //
+ // Guid to be filled in dynamically
+ //
},
//
@@ -421,6 +423,8 @@ PlatformBdsPolicyBehavior (
//
// Add the hardcoded serial console device path to ConIn, ConOut, ErrOut.
//
+ CopyGuid (&mSerialConsole.TermType.Guid,
+ PcdGetPtr (PcdTerminalTypeGuidBuffer));
BdsLibUpdateConsoleVariable (L"ConIn",
(EFI_DEVICE_PATH_PROTOCOL *)&mSerialConsole, NULL);
BdsLibUpdateConsoleVariable (L"ConOut",
diff --git
a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
index d998216..9a3cfcd 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
@@ -39,6 +39,7 @@
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
[LibraryClasses]
BaseLib
@@ -61,6 +62,9 @@
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer
+
[Guids]
gEfiFileInfoGuid
gEfiFileSystemInfoGuid

Roy Franz

2015-07-08 18:49:12 UTC

Post by Heyi Guo
Hi Roy,
I have one question; please see my comments below.

I see in ArmVirtQemu.dsc file (patch 5#), we are using #ifdef
TTY_TERMINAL. In my opinion the above statement will cause #ifdef
TTY_TERMINAL to be always true.

I agree. That's my only comment for patch #5 too. Patch #5 should be
consistent with this one here, and use
!if $(TTY_TERMINAL) == TRUE

And I don't understand why we use "!if" here and "!ifdef" in dsc file.

BTW, we have a good reason for this kind of conditional. We use
$(WHATEVER_ENABLE) == TRUE
because that allows
-D WHATEVER_ENABLE=FALSE
on a command line to *override* an earlier
-D WHATEVER_ENABLE[=TRUE]
on the same command line. The "build" utility has no -U option (for
undefining a macro).
Assume you have a build script that hardcodes
-D WHATEVER_ENABLE
because that's how you build the tree most of the time. However,
occasionally you'd like to build without that feature, and you'd like to
line, *and* the DSC files use the above kind of conditional, then you
can simply pass -D WHATEVER_ENABLE=FALSE to override the macro (but
still use the rest of your build script).
... Yes, we should have done the same with INTEL_BDS too, and we didn't.
I guess I didn't catch that because I *never* build without INTEL_BDS.

And I followed the INTEL_BDS pattern in that file :)

I have fixed this (and the INTEL_BDS define as well), and I'll send
this off shortly.

I think I have addressed all comments on the other patches.

Thanks,
Roy

Post by Laszlo Ersek
... Well, that, and because the patch that added INTEL_BDS (SVN r16208)
was never reviewed on the list.
Anyway, I digress.

Sure. The edk2 project uses CRLF line endings. For that to work in the
first place (without git yelling at you all the time), you need to add
[core]
whitespace = cr-at-eol
[am]
keepcr = true
This will *almost* make things work, but it will still break down when
you have a patch that adds or removes files *and* has crossed MTA
boundaries. In this case, you'll have
+++ /dev/null\r
or
--- /dev/null\r
hunk headers in the patch. The am.keepcr setting (which is otherwise
necessary for the *source code* CRLFs) will prevent git-am from
stripping the \r even from the /dev/null hunk headers, and that will
trip up git-am. So you need to pre-process the /dev/null hunk headers
manually, stripping *only those* \r characters, *if* you have a patch
that adds or removes files *and* has crossed MTA boundaries. (If the
patch doesn't cross an MTA, ie. it is straight out of git format-patch,
then /dev/null will have no \r.)
Summary (I'm sure you'll appreciate a summary): if you'd like to apply
(1) set "core.whitespace" to "cr-at-eol",
(2) set "am.keepcr" to "true",
(3) save the patch emails *intact* into local files (eg. *.eml)
sed -r -i 's,^((---|\+\+\+) /dev/null)\r$,\1,' *.eml
(5) apply them with git-am.
HTH
Laszlo

Post by Heyi Guo
Heyi Guo

Post by Roy Franz
+ # Set terminal type to TtyTerm, the value encoded is EFI_TTY_TERM_GUID
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91,
0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51,
0xef, 0x94}
+!endif
+
[Components.common]
#
# Networking stack
diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 7bbd9ff..9833c5a 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -49,6 +49,13 @@
#
gArmVirtTokenSpaceGuid.PcdDeviceTreeAllocationPadding|256|UINT32|0x00000002
+ #
+ # Binary representation of the GUID that determines the terminal type. The
+ # size must be exactly 16 bytes. The default value corresponds to
+ # EFI_VT_100_GUID.
+ #
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6,
0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F,
0xC1, 0x4D}|VOID*|0x00000007
+
[PcdsDynamic, PcdsFixedAtBuild]
#
# ARM PSCI function invocations can be done either through hypervisor
diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
index 13830cb..b242a29 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
@@ -35,7 +35,7 @@
typedef struct {
VENDOR_DEVICE_PATH SerialDxe;
UART_DEVICE_PATH Uart;
- VENDOR_DEFINED_DEVICE_PATH Vt100;
+ VENDOR_DEFINED_DEVICE_PATH TermType;
EFI_DEVICE_PATH_PROTOCOL End;
} PLATFORM_SERIAL_CONSOLE;
#pragma pack ()
@@ -67,14 +67,16 @@ STATIC PLATFORM_SERIAL_CONSOLE mSerialConsole = {
},
//
- // VENDOR_DEFINED_DEVICE_PATH Vt100
+ // VENDOR_DEFINED_DEVICE_PATH TermType
//
{
{
MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,
DP_NODE_LEN (VENDOR_DEFINED_DEVICE_PATH)
- },
- EFI_VT_100_GUID
+ }
+ //
+ // Guid to be filled in dynamically
+ //
},
//
@@ -421,6 +423,8 @@ PlatformBdsPolicyBehavior (
//
// Add the hardcoded serial console device path to ConIn, ConOut, ErrOut.
//
+ CopyGuid (&mSerialConsole.TermType.Guid,
+ PcdGetPtr (PcdTerminalTypeGuidBuffer));
BdsLibUpdateConsoleVariable (L"ConIn",
(EFI_DEVICE_PATH_PROTOCOL *)&mSerialConsole, NULL);
BdsLibUpdateConsoleVariable (L"ConOut",
diff --git
a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
index d998216..9a3cfcd 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
@@ -39,6 +39,7 @@
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
[LibraryClasses]
BaseLib
@@ -61,6 +62,9 @@
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer
+
[Guids]
gEfiFileInfoGuid
gEfiFileSystemInfoGuid

Ryan Harkin

2015-07-08 20:36:59 UTC

Hi Laszlo,

Post by Heyi Guo
Hi Roy,
I have one question; please see my comments below.

I see in ArmVirtQemu.dsc file (patch 5#), we are using #ifdef
TTY_TERMINAL. In my opinion the above statement will cause #ifdef
TTY_TERMINAL to be always true.

I agree. That's my only comment for patch #5 too. Patch #5 should be
consistent with this one here, and use
!if $(TTY_TERMINAL) == TRUE

Post by Roy Franz
[LibraryClasses.common]
!if $(TARGET) == RELEASE
@@ -359,6 +360,11 @@

gEfiSecurityPkgTokenSpaceGuid.PcdRemovableMediaImageVerificationPolicy|0x04

Post by Roy Franz
!endif
+!if $(TTY_TERMINAL) == TRUE

And I don't understand why we use "!if" here and "!ifdef" in dsc file.

BTW, we have a good reason for this kind of conditional. We use
$(WHATEVER_ENABLE) == TRUE
because that allows
-D WHATEVER_ENABLE=FALSE
on a command line to *override* an earlier
-D WHATEVER_ENABLE[=TRUE]
on the same command line. The "build" utility has no -U option (for
undefining a macro).
Assume you have a build script that hardcodes
-D WHATEVER_ENABLE
because that's how you build the tree most of the time. However,
occasionally you'd like to build without that feature, and you'd like to
line, *and* the DSC files use the above kind of conditional, then you
can simply pass -D WHATEVER_ENABLE=FALSE to override the macro (but
still use the rest of your build script).
... Yes, we should have done the same with INTEL_BDS too, and we didn't.
I guess I didn't catch that because I *never* build without INTEL_BDS.
... Well, that, and because the patch that added INTEL_BDS (SVN r16208)
was never reviewed on the list.
Anyway, I digress.

I have an alias in my ~/.gitconfig file:

[alias]
amm=am --ignore-whitespace --ignore-space-change

Then I use "git amm" to apply patches and that seems to work most of the
time.

But I suspect your method is doing something smarter and more complex
whereas mine is more brute force.

Post by Laszlo Ersek
HTH
Laszlo

Post by Heyi Guo
Heyi Guo

Post by Roy Franz
+ # Set terminal type to TtyTerm, the value encoded is

EFI_TTY_TERM_GUID

Post by Roy Franz
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91,
0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51,
0xef, 0x94}
+!endif
+
[Components.common]
#
# Networking stack
diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 7bbd9ff..9833c5a 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -49,6 +49,13 @@
#

gArmVirtTokenSpaceGuid.PcdDeviceTreeAllocationPadding|256|UINT32|0x00000002

Post by Roy Franz
+ #
+ # Binary representation of the GUID that determines the terminal type. The
+ # size must be exactly 16 bytes. The default value corresponds to
+ # EFI_VT_100_GUID.
+ #
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6,
0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F,
0xC1, 0x4D}|VOID*|0x00000007
+
[PcdsDynamic, PcdsFixedAtBuild]
#
# ARM PSCI function invocations can be done either through

hypervisor

Post by Roy Franz
diff --git a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
index 13830cb..b242a29 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
@@ -35,7 +35,7 @@
typedef struct {
VENDOR_DEVICE_PATH SerialDxe;
UART_DEVICE_PATH Uart;
- VENDOR_DEFINED_DEVICE_PATH Vt100;
+ VENDOR_DEFINED_DEVICE_PATH TermType;
EFI_DEVICE_PATH_PROTOCOL End;
} PLATFORM_SERIAL_CONSOLE;
#pragma pack ()
@@ -67,14 +67,16 @@ STATIC PLATFORM_SERIAL_CONSOLE mSerialConsole = {
},
//
- // VENDOR_DEFINED_DEVICE_PATH Vt100
+ // VENDOR_DEFINED_DEVICE_PATH TermType
//
{
{
MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,
DP_NODE_LEN (VENDOR_DEFINED_DEVICE_PATH)
- },
- EFI_VT_100_GUID
+ }
+ //
+ // Guid to be filled in dynamically
+ //
},
//
@@ -421,6 +423,8 @@ PlatformBdsPolicyBehavior (
//
// Add the hardcoded serial console device path to ConIn, ConOut, ErrOut.
//
+ CopyGuid (&mSerialConsole.TermType.Guid,
+ PcdGetPtr (PcdTerminalTypeGuidBuffer));
BdsLibUpdateConsoleVariable (L"ConIn",
(EFI_DEVICE_PATH_PROTOCOL *)&mSerialConsole, NULL);
BdsLibUpdateConsoleVariable (L"ConOut",
diff --git
a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
index d998216..9a3cfcd 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
@@ -39,6 +39,7 @@
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
[LibraryClasses]
BaseLib
@@ -61,6 +62,9 @@
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer
+
[Guids]
gEfiFileInfoGuid
gEfiFileSystemInfoGuid

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Laszlo Ersek

2015-07-08 21:03:15 UTC

Post by Ryan Harkin
[alias]
amm=am --ignore-whitespace --ignore-space-change
Then I use "git amm" to apply patches and that seems to work most of the
time.
But I suspect your method is doing something smarter and more complex
whereas mine is more brute force.

Right. Your command is perfect for ad-hoc testing, but it ignores more
whitespace than what would be acceptable for actually committing a patch
to SVN. For example, it would enable source files to become mixed LF and
CRLF, I think. (Because:
- git-am would strip all \r characters from the patch email,
- ignore the resultant LF <-> CRLF mismatch in the hunks' contexts,
- and add the new lines with LF only.)

BTW, Paolo had written code for git at some point that fixed this issue
within git:

http://thread.gmane.org/gmane.comp.version-control.git/260215

His idea was to armor patch emails against line terminator changes
incurred by MTAs. (Which is what makes the "sed" command necessary in my
method, for the /dev/null hunk headers.)

Although the quoted-printable and base64 transfer encodings, used for
such protection, would have resulted in human-*un*readable raw email
files, Paolo suggested a one liner back-converter for those who like to
save patch emails to regular files, for comparing them between each
other. The results would have been very useful: "git-am" would just
work, and people comparing raw patches could rely on a one-liner in
their workflow.

Alas, I'm unsure about the fate of Paolo's work. I think he didn't have
time to work on further versions of the patch set, and I had neither the
time nor the expertise.

Thanks!
Laszlo

Paolo Bonzini

2015-07-09 06:45:36 UTC

This was included in git 2.3.0. :)

Paolo

Post by Laszlo Ersek
His idea was to armor patch emails against line terminator changes
incurred by MTAs. (Which is what makes the "sed" command necessary in my
method, for the /dev/null hunk headers.)
Although the quoted-printable and base64 transfer encodings, used for
such protection, would have resulted in human-*un*readable raw email
files, Paolo suggested a one liner back-converter for those who like to
save patch emails to regular files, for comparing them between each
other. The results would have been very useful: "git-am" would just
work, and people comparing raw patches could rely on a one-liner in
their workflow.
Alas, I'm unsure about the fate of Paolo's work. I think he didn't have
time to work on further versions of the patch set, and I had neither the
time nor the expertise.
Thanks!
Laszlo

Laszlo Ersek

2015-07-09 06:50:55 UTC

Post by Paolo Bonzini

This was included in git 2.3.0. :)

Yay! \o/

Thank you! :)
Laszlo

Post by Paolo Bonzini
Paolo

Roy Franz

2015-07-08 00:44:18 UTC