[edk2] [PATCH 0/2] move PcdDefaultTerminalType to MdeModulePkg

Discussion:

Ard Biesheuvel

2015-07-14 10:36:18 UTC

Now that MdeModulePkg defines a TTYTERM terminal type that MdePkg
knows nothing about, we should move PcdDefaultTerminalType to
MdeModulePkg as well, and allow it to assume the value #4 which
corresponds with TTYTERM.

Ard Biesheuvel (2):
MdeModulePkg|EmulatorPkg: add definition for PcdDefaultTerminalType
MdePkg: remove PcdDefaultTerminalType

EmulatorPkg/EmulatorPkg.dsc | 4 ++--
MdeModulePkg/MdeModulePkg.dec | 10 ++++++++++
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf | 2 +-
MdePkg/MdePkg.dec | 9 ---------
4 files changed, 13 insertions(+), 12 deletions(-)

--
1.9.1

Ard Biesheuvel

2015-07-14 10:36:19 UTC

Permalink

Since PcdDefaultTerminalType may now assume values that are not
defined in MdePkg (i.e., TTYTERM), introduce a new version in
MdeModulePkg instead. Also add TTYTERM as #4.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
EmulatorPkg/EmulatorPkg.dsc | 4 ++--
MdeModulePkg/MdeModulePkg.dec | 10 ++++++++++
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf | 2 +-
3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/EmulatorPkg/EmulatorPkg.dsc b/EmulatorPkg/EmulatorPkg.dsc
index e0c616144df6..c8e9877b85dc 100644
--- a/EmulatorPkg/EmulatorPkg.dsc
+++ b/EmulatorPkg/EmulatorPkg.dsc
@@ -228,8 +228,8 @@ [PcdsFixedAtBuild]
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuModel|L"Intel(R) Processor Model"
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuSpeed|L"3000"

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

[PcdsDynamicDefault.common.DEFAULT]
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|0
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 623b07f9c6a2..76c92d725380 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1333,6 +1333,16 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
# @Prompt Default Creator Revision for ACPI table creation.
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision|0x01000013|UINT32|0x30001038

+ ## Indicates the usable type of terminal. 
+ # 0 - PCANSI 
+ # 1 - VT100 
+ # 2 - VT100+ 
+ # 3 - UTF8 
+ # 4 - TTYTERM 
+ # @Prompt Default Terminal Type.
+ # @ValidRange 0x80000001 | 0 - 4
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDefaultTerminalType|0|UINT8|0x0000006f
+
[PcdsPatchableInModule]
## Specify memory size with page number for PEI code when
# Loading Module at Fixed Address feature is enabled.
diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
index 078029679823..c1ad5779ac29 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
@@ -86,7 +86,7 @@ [Protocols]
gEfiSimpleTextOutProtocolGuid ## BY_START

[Pcd]
- gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType ## SOMETIMES_CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDefaultTerminalType ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable ## CONSUMES

# [Event]

--
1.9.1

Laszlo Ersek

2015-07-14 10:58:48 UTC

Permalink

Post by Ard Biesheuvel
Since PcdDefaultTerminalType may now assume values that are not
defined in MdePkg (i.e., TTYTERM), introduce a new version in
MdeModulePkg instead. Also add TTYTERM as #4.
Contributed-under: TianoCore Contribution Agreement 1.0
---
EmulatorPkg/EmulatorPkg.dsc | 4 ++--
MdeModulePkg/MdeModulePkg.dec | 10 ++++++++++
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf | 2 +-
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/EmulatorPkg/EmulatorPkg.dsc b/EmulatorPkg/EmulatorPkg.dsc
index e0c616144df6..c8e9877b85dc 100644
--- a/EmulatorPkg/EmulatorPkg.dsc
+++ b/EmulatorPkg/EmulatorPkg.dsc
@@ -228,8 +228,8 @@ [PcdsFixedAtBuild]
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuModel|L"Intel(R) Processor Model"
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuSpeed|L"3000"
- # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8
- gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
+ # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDefaultTerminalType|1
[PcdsDynamicDefault.common.DEFAULT]
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|0
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 623b07f9c6a2..76c92d725380 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1333,6 +1333,16 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision|0x01000013|UINT32|0x30001038
+ ## Indicates the usable type of terminal. 
+ # 0 - PCANSI 
+ # 1 - VT100 
+ # 2 - VT100+ 
+ # 3 - UTF8 
+ # 4 - TTYTERM 
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDefaultTerminalType|0|UINT8|0x0000006f
+
[PcdsPatchableInModule]
## Specify memory size with page number for PEI code when
# Loading Module at Fixed Address feature is enabled.
diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
index 078029679823..c1ad5779ac29 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
@@ -86,7 +86,7 @@ [Protocols]
gEfiSimpleTextOutProtocolGuid ## BY_START
[Pcd]
- gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType ## SOMETIMES_CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDefaultTerminalType ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable ## CONSUMES
# [Event]

I'm a bit unsure about the 0x0000006f token value for the new PCD --
MdeModulePkg.dec seems to use some funky numbering scheme, and I'm
missing the idea behind it. In any case, 0x0000006e is used in the same
section just a little bit higher. So this looks good to me.

Reviewed-by: Laszlo Ersek <***@redhat.com>

Roy Franz

2015-07-14 15:42:48 UTC

Permalink

On Tue, Jul 14, 2015 at 3:36 AM, Ard Biesheuvel

Reviewed-by: Roy Franz <***@linaro.org>

Ard Biesheuvel

2015-07-14 10:36:20 UTC

Permalink

The PCD gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType is not used
anywhere in MdePkg itself, and with the introduction of TTYTERM, it
may now assume values that are not defined in MdePkg either.

So now that we moved it to MdeModulePkg, remove this one that is
now unused.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
MdePkg/MdePkg.dec | 9 ---------
1 file changed, 9 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index bda6550ed6c6..026a94987bef 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2007,15 +2007,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
# @ValidRange 0x80000001 | 0 - 3
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits|1|UINT8|0x00000023

- ## Indicates the usable type of terminal. 
- # 0 - PCANSI 
- # 1 - VT100 
- # 2 - VT100+ 
- # 3 - UTF8 
- # @Prompt Default Terminal Type.
- # @ValidRange 0x80000001 | 0 - 3
- gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|0|UINT8|0x00000024
-
## Error level for hardware recorder.
# If value 0, platform does not support feature of hardware error record.
# @Prompt Error Level For Hardware Recorder

--
1.9.1

Laszlo Ersek

2015-07-14 10:59:28 UTC

Permalink

Post by Ard Biesheuvel
The PCD gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType is not used
anywhere in MdePkg itself, and with the introduction of TTYTERM, it
may now assume values that are not defined in MdePkg either.
So now that we moved it to MdeModulePkg, remove this one that is
now unused.
Contributed-under: TianoCore Contribution Agreement 1.0
---
MdePkg/MdePkg.dec | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index bda6550ed6c6..026a94987bef 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2007,15 +2007,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits|1|UINT8|0x00000023
- ## Indicates the usable type of terminal. 
- # 0 - PCANSI 
- # 1 - VT100 
- # 2 - VT100+ 
- # 3 - UTF8 
- gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|0|UINT8|0x00000024
-
## Error level for hardware recorder.
# If value 0, platform does not support feature of hardware error record.

Reviewed-by: Laszlo Ersek <***@redhat.com>

Roy Franz

2015-07-14 15:43:10 UTC

Permalink

On Tue, Jul 14, 2015 at 3:36 AM, Ard Biesheuvel

Reviewed-by: Roy Franz <***@linaro.org>

Gao, Liming

2015-07-15 06:26:29 UTC

Permalink

Ard:
From compatible view, I don't prefer to make this change. The driver in other EDKII package may use this PCD. After this change, they will build break.

I suggest to keep this PCD, and update its comment to include new type. Its comment can highlight this type is not defined in UEFI specification.

Thanks
Liming
-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Tuesday, July 14, 2015 6:36 PM
To: edk2-***@lists.sourceforge.net; ***@linaro.org; ***@redhat.com; Tian, Feng; Gao, Liming
Cc: ***@linaro.org; Ard Biesheuvel
Subject: [PATCH 2/2] MdePkg: remove PcdDefaultTerminalType

The PCD gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType is not used anywhere in MdePkg itself, and with the introduction of TTYTERM, it may now assume values that are not defined in MdePkg either.

So now that we moved it to MdeModulePkg, remove this one that is now unused.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
MdePkg/MdePkg.dec | 9 ---------
1 file changed, 9 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index bda6550ed6c6..026a94987bef 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2007,15 +2007,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
# @ValidRange 0x80000001 | 0 - 3
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits|1|UINT8|0x00000023

- ## Indicates the usable type of terminal. 
- # 0 - PCANSI 
- # 1 - VT100 
- # 2 - VT100+ 
- # 3 - UTF8 
- # @Prompt Default Terminal Type.
- # @ValidRange 0x80000001 | 0 - 3
- gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|0|UINT8|0x00000024
-
## Error level for hardware recorder.
# If value 0, platform does not support feature of hardware error record.
# @Prompt Error Level For Hardware Recorder
--
1.9.1

Ard Biesheuvel

2015-07-15 08:59:13 UTC

Permalink

Post by Gao, Liming
From compatible view, I don't prefer to make this change. The driver in other EDKII package may use this PCD. After this change, they will build break.
I suggest to keep this PCD, and update its comment to include new type. Its comment can highlight this type is not defined in UEFI specification.

OK, that is also fine,

Thanks,
Ard.

Post by Gao, Liming
-----Original Message-----
Sent: Tuesday, July 14, 2015 6:36 PM
Subject: [PATCH 2/2] MdePkg: remove PcdDefaultTerminalType
The PCD gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType is not used anywhere in MdePkg itself, and with the introduction of TTYTERM, it may now assume values that are not defined in MdePkg either.
So now that we moved it to MdeModulePkg, remove this one that is now unused.
Contributed-under: TianoCore Contribution Agreement 1.0
---
MdePkg/MdePkg.dec | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index bda6550ed6c6..026a94987bef 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2007,15 +2007,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits|1|UINT8|0x00000023
- ## Indicates the usable type of terminal. 
- # 0 - PCANSI 
- # 1 - VT100 
- # 2 - VT100+ 
- # 3 - UTF8 
- gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|0|UINT8|0x00000024
-
## Error level for hardware recorder.
# If value 0, platform does not support feature of hardware error record.
--
1.9.1

Laszlo Ersek

2015-07-15 10:14:36 UTC

Permalink

Post by Ard Biesheuvel

Post by Gao, Liming
From compatible view, I don't prefer to make this change. The
driver in other EDKII package may use this PCD. After this change,
they will build break.
I suggest to keep this PCD, and update its comment to include new
type. Its comment can highlight this type is not defined in UEFI
specification.

OK, that is also fine,

I obviously accept the above decision, but it should be noted that this
makes MdePkg practically unmodifiable. Normally a contributor takes care
to modify not just a declaration of "something", but to bring in sync
*all* uses of the declaration too.

However, that's only possible for such uses that the contributor can
*see*. If everything in MdePkg must be preserved as-is, forever, for the
sake of forks (proprietary or otherwise -- simply stuff that depends on
the declarations but exists out of tree), then that quite guarantees
that MdePkg will grow layers and layers of cruft over time.

The Linux kernel doesn't guarantee any API stability for out-of-tree
code. But, all in-tree code is carefully adapted when APIs change.

On the other hand, for example, Red Hat guarantees some level of kernel
ABI stability. (Google "Red Hat KABI whitelist".) Occasionally it does
come at a hefty price with regard to code clarity in the Red Hat kernels
-- sometimes keeping ABI stability requires elaborate, crufty detours in
code.

While that makes sense for a commercial downstream, in *upstream* edk2 I
don't think it's a very friendly (or clear-cut!) rule / limitation for
contributors. At least Red Hat *clearly* defines "separation points" in
the release cycle across which the kernel ABI stability is *not*
guaranteed -- and then we can shed the cruft. Clear rules for MdePkg,
MdeModulePkg, etc (as to when compat stuff can be phased out) would be
helpful.

Anyway, I do accept the decision; I just had to say my piece.

Thanks
Laszlo

Post by Ard Biesheuvel
Thanks,
Ard.