[edk2] [PATCH V4 0/5] Some improvements on serial terminal

Discussion:

Heyi Guo

2015-07-16 08:21:57 UTC

There were some issues when I ran SCT test against Serial IO Protocol
on qemu aarch64, and below patches were made after I went through the
code of TerminalDxe driver and the flow of console devices being
connected.

V2 is based on TTY terminal patches.
V3 updates comment for PcdDefaultTerminalType.
V4 splits the 1st patch.

Thanks.

Cc: Michael D Kinney <***@intel.com>
Cc: Liming Gao <***@intel.com>
Cc: Jordan Justen <***@intel.com>
Cc: Andrew Fish <***@apple.com>
Cc: Laszlo Ersek <***@redhat.com>
Cc: Ard Biesheuvel <***@linaro.org>
Cc: Feng Tian <***@intel.com>
Cc: Ruiyu Ni <***@intel.com>

Heyi Guo (5):
MdePkg: Update comment for PcdDefaultTerminalType
EmulatorPkg: Update comment for PcdDefaultTerminalType
ArmVirtPkg: Make terminal type consistent
MdeModulePkg/TerminalDxe: Set NullRemaining to FALSE by default
MdeModulePkg/TerminalDxe: Some improvements

ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++
EmulatorPkg/EmulatorPkg.dsc | 2 +-
.../Universal/Console/TerminalDxe/Terminal.c | 29 ++++------------------
MdePkg/MdePkg.dec | 3 ++-
4 files changed, 16 insertions(+), 26 deletions(-)

--
2.1.4

Heyi Guo

2015-07-16 08:21:58 UTC

Permalink

Update comment for PcdDefaultTerminalType, as a new terminal type
TTYTERM has been added with type value of 4.

The new type is NOT defined in UEFI SPEC v2.5.

Cc: Michael D Kinney <***@intel.com>
Cc: Liming Gao <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
Reviewed-by: Liming Gao <***@intel.com>
---
MdePkg/MdePkg.dec | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index bda6550..598a6d0 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2012,8 +2012,9 @@
# 1 - VT100<BR>
# 2 - VT100+<BR>
# 3 - UTF8<BR>
+ # 4 - TTYTERM, NOT defined in UEFI SPEC<BR>
# @Prompt Default Terminal Type.
- # @ValidRange 0x80000001 | 0 - 3
+ # @ValidRange 0x80000001 | 0 - 4
gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|0|UINT8|0x00000024

## Error level for hardware recorder.

--
2.1.4

Heyi Guo

2015-07-16 08:21:59 UTC

Permalink

Update comment for PcdDefaultTerminalType, as a new terminal type
TTYTERM has been added with type value of 4.

The new type is NOT defined in UEFI SPEC v2.5.

Cc: Jordan Justen <***@intel.com>
Cc: Andrew Fish <***@apple.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
Reviewed-by: Jordan Justen <***@intel.com>
---
EmulatorPkg/EmulatorPkg.dsc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/EmulatorPkg/EmulatorPkg.dsc b/EmulatorPkg/EmulatorPkg.dsc
index e0c6161..dfcd476 100644
--- a/EmulatorPkg/EmulatorPkg.dsc
+++ b/EmulatorPkg/EmulatorPkg.dsc
@@ -228,7 +228,7 @@
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuModel|L"Intel(R) Processor Model"
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuSpeed|L"3000"

- # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8
+ # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1

[PcdsDynamicDefault.common.DEFAULT]

--
2.1.4

Heyi Guo

2015-07-16 08:22:00 UTC

Permalink

Change default terminal type to be consistent with default
ConIn/ConOut device path, which is now determined by TTY_TERMINAL
flag, TTYTERM or VT100.

I can't say this is a bug, as we can pass the whole console device
path to ConnectController, and TerminalDxe driver will pick up the
terminal in the remaining device path. However, in rare circumstances,
the console devices may be disconnected with the driver, and they will
be ignored by ConPlatformDxe until we pass the device path explicitly
just as BDS.

Changing default terminal type to be the same with console device
path could help serial terminal be reconnected with normal connect
controller operation.

Cc: Laszlo Ersek <***@redhat.com>
Cc: Ard Biesheuvel <***@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
Reviewed-by: Laszlo Ersek <***@redhat.com>
---
ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 0d4f4b0..be2b984 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -140,9 +140,17 @@
!if $(TTY_TERMINAL) == TRUE
gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)"
gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)"
+ ## Terminal Type - TTYTERM, consistent with ConOut/ConIn Device Path.
+ ## 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
+ gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4
!else
gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()"
gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()"
+ ## Terminal Type - VT100, consistent with ConOut/ConIn Device Path.
+ ## When Intel BDS is enabled, the above ConOut/ConIn device path is useless,
+ ## but we still use VT100 terminal type when TTY_TERMINAL is not TRUE.
+ ## 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
+ gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
!endif
gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3

--
2.1.4

Heyi Guo

2015-07-16 08:22:01 UTC

Permalink

This is bug fix for TerminalDxe: NullRemaining should be set to FALSE
by fault and then be set to TRUE conditionally.

Cc: Feng Tian <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
Reviewed-by: Ruiyu Ni <***@intel.com>
Reviewed-by: Feng Tian <***@intel.com>
---
MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index 597b15d..75bfdec 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -568,7 +568,7 @@ TerminalDriverBindingStart (
DefaultNode = NULL;
ConInSelected = FALSE;
ConOutSelected = FALSE;
- NullRemaining = TRUE;
+ NullRemaining = FALSE;
SimTxtInInstalled = FALSE;
SimTxtOutInstalled = FALSE;
FirstEnter = FALSE;

--
2.1.4

Heyi Guo

2015-07-16 08:22:02 UTC

Permalink

1. Get default terminal type from PCD rather than using PCANSI
directly in BuildTeminalDevpath;
2. Only terminal type is needed to create an TerminalDev instance, so
remove the useless code of creating and freeing DefaultNode.
3. Some white space refining.

Cc: Feng Tian <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
Reviewed-by: Ruiyu Ni <***@intel.com>
Reviewed-by: Feng Tian <***@intel.com>
---
.../Universal/Console/TerminalDxe/Terminal.c | 27 ++++------------------
1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index 75bfdec..6fde3b2 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -258,14 +258,13 @@ BuildTerminalDevpath (
EFI_STATUS Status;

TerminalDevicePath = NULL;
- TerminalType = PCANSITYPE;

//
// Use the RemainingDevicePath to determine the terminal type
//
Node = (VENDOR_DEVICE_PATH *) RemainingDevicePath;
if (Node == NULL) {
- TerminalType = PCANSITYPE;
+ TerminalType = PcdGet8 (PcdDefaultTerminalType);

} else if (CompareGuid (&Node->Guid, &gEfiPcAnsiGuid)) {

@@ -545,7 +544,6 @@ TerminalDriverBindingStart (
EFI_SERIAL_IO_PROTOCOL *SerialIo;
EFI_DEVICE_PATH_PROTOCOL *ParentDevicePath;
VENDOR_DEVICE_PATH *Node;
- VENDOR_DEVICE_PATH *DefaultNode;
EFI_SERIAL_IO_MODE *Mode;
UINTN SerialInTimeOut;
TERMINAL_DEV *TerminalDevice;
@@ -565,9 +563,8 @@ TerminalDriverBindingStart (
UINTN ModeCount;

TerminalDevice = NULL;
- DefaultNode = NULL;
- ConInSelected = FALSE;
- ConOutSelected = FALSE;
+ ConInSelected = FALSE;
+ ConOutSelected = FALSE;
NullRemaining = FALSE;
SimTxtInInstalled = FALSE;
SimTxtOutInstalled = FALSE;
@@ -709,23 +706,14 @@ TerminalDriverBindingStart (
}

//
- // If RemainingDevicePath is NULL, then create default device path node
+ // If RemainingDevicePath is NULL, use default terminal type
//
if (RemainingDevicePath == NULL) {
- DefaultNode = AllocateZeroPool (sizeof (VENDOR_DEVICE_PATH));
- if (DefaultNode == NULL) {
- Status = EFI_OUT_OF_RESOURCES;
- goto Error;
- }
-
TerminalType = PcdGet8 (PcdDefaultTerminalType);
//
// Must be between PCANSITYPE (0) and TTYTERMTYPE (4)
//
ASSERT (TerminalType <= TTYTERMTYPE);
-
- CopyMem (&DefaultNode->Guid, gTerminalType[TerminalType], sizeof (EFI_GUID));
- RemainingDevicePath = (EFI_DEVICE_PATH_PROTOCOL *) DefaultNode;
} else if (!IsDevicePathEnd (RemainingDevicePath)) {
//
// If RemainingDevicePath isn't the End of Device Path Node,
@@ -1183,9 +1171,6 @@ TerminalDriverBindingStart (
goto Error;
}
}
- if (DefaultNode != NULL) {
- FreePool (DefaultNode);
- }

return EFI_SUCCESS;

@@ -1254,10 +1239,6 @@ Error:
}
}

- if (DefaultNode != NULL) {
- FreePool (DefaultNode);
- }
-
This->Stop (This, Controller, 0, NULL);

return Status;

--
2.1.4

Ard Biesheuvel

2015-07-16 08:51:04 UTC

Permalink

Post by Heyi Guo
There were some issues when I ran SCT test against Serial IO Protocol
on qemu aarch64, and below patches were made after I went through the
code of TerminalDxe driver and the flow of console devices being
connected.
V2 is based on TTY terminal patches.
V3 updates comment for PcdDefaultTerminalType.
V4 splits the 1st patch.

Thanks Heyi

Committed as SVN r18023 ... r18027

Regards,
Ard.

Post by Heyi Guo
MdePkg: Update comment for PcdDefaultTerminalType
EmulatorPkg: Update comment for PcdDefaultTerminalType
ArmVirtPkg: Make terminal type consistent
MdeModulePkg/TerminalDxe: Set NullRemaining to FALSE by default
MdeModulePkg/TerminalDxe: Some improvements
ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++
EmulatorPkg/EmulatorPkg.dsc | 2 +-
.../Universal/Console/TerminalDxe/Terminal.c | 29 ++++------------------
MdePkg/MdePkg.dec | 3 ++-
4 files changed, 16 insertions(+), 26 deletions(-)
--
2.1.4

Laszlo Ersek

2015-07-16 10:17:09 UTC

Permalink

Post by Ard Biesheuvel

Thanks Heyi
Committed as SVN r18023 ... r18027

Cool, thanks. I should have processed my INBOX before replying to v3...

Thanks!
Laszlo

Post by Ard Biesheuvel
Regards,
Ard.