On 06/19/15 23:11, Roy Franz wrote:
> On Fri, Jun 19, 2015 at 6:04 AM, Laszlo Ersek <ler...@redhat.com> wrote:
>> On 06/19/15 07:16, Roy Franz wrote:
>>> When I rebased my TtyTerm changes, I found that I had been working on
>>> a rather out of date Linaro branch without realizing it. After
>>> rebasing my patchset, the
>>> gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths PCD only controls the
>>> console for the ARM BDS, and doesn't affect the Intel BDS. (Changes
>>> seem to be in commit ba67a145410)
>>> I poked around a bit and and didn't see where the terminal type was
>>> configured for the Intel BDS.
>>> Where should I look for this?
>>
>> Please see the "mSerialConsole" object in
>>
>> ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>>
>> You might want to overide the
>>
>> mSerialConsole.Vt100.Guid
>>
>> field conditionally; its value is now EFI_VT_100_GUID statically.
>
> What is the preferred method for doing this? I don't see a way to directly
> put a GUID in a PCD so that I could use that to initialize the static
> structure.
I don't know about such either. You could use a buffer / pointer PCD,
but that's sorta awful for a GUID.
> Since as I understand it mSerialConsole is a statically constructed device
> path,
> would it be reasonable to use a PCD for the entire path (as in the ARM
> BDS case),
> and simply call BdsLibUpdateConsoleVariable() with the PCD, if present, rather
> than mSerialConsole?
I always disliked
gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths
gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths
They are too long, and the generality they offer only gets in the way of
easy configuration.
Here's what I would propose (but might not, actually):
- introduce a new string PCD for ArmVirtPkg's Intel BDS lib.
- the PCD should carry a single devpath node, like L"VenVt100()"
- In the PlatformBdsPolicyBehavior() function, in "IntelBdsPlatform.c",
please parse that string with ConvertTextToDeviceNode() library
function (from DevicePathLib).
- Once you have the node (in binary), please validate its type
(MESSAGING_DEVICE_PATH), subtype (MSG_VENDOR_DP), and size (full size
should be that of VENDOR_DEVICE_PATH). If everything's fine, please
copy the GUID into "mSerialConsole", right before using
mSerialConsole. Then release the parsed (dynamically allocated) node.
However... as far as I remember, ConvertTextToDeviceNode() will not
work, because the GUID that you use for the new terminal type is not
standard. This nails the coffin not only for my proposal, but also your
suggestion -- "simply call BdsLibUpdateConsoleVariable()" presupposes a
text to binary conversion first.
So, after all, I will propose to introduce a pointer (VOID*) PCD. The
GUID data should be hard-coded in this Fixed PCD for *both*
EFI_VT_100_GUID and your new GUID, and a build time flag (-D) should
select between them.
Then, in the code, the initializer of mSerialConsole should leave out
that field (it will be set to all zeroes). Right before the first use, a
CopyMem() or CopyGuid() call should be issued, that copies the data from
the Ptr PCD into the GUID.
This way the user won't have to fiddle with any device paths, he/she
will not mess up the size of the GUIDs' binary dumps in the PCD
alternatives; only a -D flag needs to be passed (optionally). I think
this setting is important enough to warrant a new -D flag, and I
certainly think that providing all the "flexibility" of the
gArmPlatformTokenSpaceGuid PCDs I listed above is actually detrimental.
Two possibilities should be plenty.
Please find the patch attached (tested for the Vt100 case).
>
>
>>
>> (And then renaming the "Vt100" field itself to something more generic
>> might make sense too.)
>>
>> ... As I said, please don't change the default behavior.
>
> I know you don't like this, but I think there is a reasonable argument
> to be made
> that most people would benefit from this change. I expect the new setting to
> be
> the default in the Linaro builds. I'm not going push hard for a
> default change - others
> who have broken backspace will have to chime in :)
Yes, they will have to.
Thanks,
Laszlo
>
> Thanks for all your help,
> Roy
>
>>
>> For more background, please refer to:
>>
>> commit 60dc18a17c51697be6a06e2ec1944a0d8b06d501
>> Author: Laszlo Ersek <ler...@redhat.com>
>> Date: Wed Feb 25 17:54:05 2015 +0000
>>
>> ArmVirtualizationPkg: PlatformIntelBdsLib: detect consoles dynamically
>>
>> Thanks
>> Laszlo
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index c6e684f..885c1db 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -14,6 +14,7 @@
[Defines]
DEFINE DEBUG_PRINT_ERROR_LEVEL = 0x8000004F
+ DEFINE LINUX_TERMINAL = FALSE
[LibraryClasses.common]
!if $(TARGET) == RELEASE
@@ -354,6 +355,13 @@ [PcdsFixedAtBuild.common]
gEfiSecurityPkgTokenSpaceGuid.PcdRemovableMediaImageVerificationPolicy|0x04
!endif
+!if $(LINUX_TERMINAL) == TRUE
+ #
+ # fix this up
+ #
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
+!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 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
#
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 499cce5..a04d603 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
@@ -34,7 +34,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 ()
@@ -66,14 +66,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
+ //
},
//
@@ -385,6 +387,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 d8f8926..b9fb536 100644
--- a/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
+++ b/ArmVirtPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
@@ -39,6 +39,7 @@ [Packages]
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
[LibraryClasses]
BaseLib
@@ -61,6 +62,9 @@ [FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer
+
[Guids]
gEfiFileInfoGuid
gEfiFileSystemInfoGuid
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
