[edk2] Console for Qemu

Post by Roy Franz
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?

https://svn.code.sf.net/p/edk2/code/trunk/edk2/Nt32Pkg/Library/Nt32BdsLib/PlatformData.c <https://svn.code.sf.net/p/edk2/code/trunk/edk2/Nt32Pkg/Library/Nt32BdsLib/PlatformData.c>
If I remember correctly gPlatformConsole[] is would need a device path that would include the terminal type.

Thanks,

Andrew Fish

Post by Roy Franz
Thanks,
Roy
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Laszlo Ersek

2015-06-19 13:04:06 UTC

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.

(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.

For more background, please refer to:

commit 60dc18a17c51697be6a06e2ec1944a0d8b06d501
Author: Laszlo Ersek <***@redhat.com>
Date: Wed Feb 25 17:54:05 2015 +0000

ArmVirtualizationPkg: PlatformIntelBdsLib: detect consoles dynamically

Thanks
Laszlo

------------------------------------------------------------------------------

Roy Franz

2015-06-19 21:11:00 UTC

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.

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?

Post by Laszlo Ersek
(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 :)

Thanks for all your help,
Roy

Post by Laszlo Ersek
commit 60dc18a17c51697be6a06e2ec1944a0d8b06d501
Date: Wed Feb 25 17:54:05 2015 +0000
ArmVirtualizationPkg: PlatformIntelBdsLib: detect consoles dynamically
Thanks
Laszlo

------------------------------------------------------------------------------

Laszlo Ersek

2015-06-19 22:44:09 UTC

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.

Post by Roy Franz
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).

Post by Laszlo Ersek
(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.

Yes, they will have to.

Thanks,
Laszlo

Post by Roy Franz
Thanks for all your help,
Roy

Post by Laszlo Ersek
commit 60dc18a17c51697be6a06e2ec1944a0d8b06d501
Date: Wed Feb 25 17:54:05 2015 +0000
ArmVirtualizationPkg: PlatformIntelBdsLib: detect consoles dynamically
Thanks
Laszlo

Roy Franz

2015-06-22 22:47:37 UTC

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.

I always disliked
gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths
gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths
They are too long, and the generality they offer only gets in the way of
easy configuration.
- 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).

Thanks Laszlo - this works quite nicely. I added the TtyTerminal GUID,
and changed the "LINUX_TERMINAL" define name to "TTY_TERMINAL" to match
the naming of the terminal type.

I'm not sure of how to include this patch in my submission - I have
you as the author of the
patch, and I've added my SOB and the Contributed-under.
This needs your SOB line as well, right? Since I made changes beyond
just the GUID
I don't want to add that for you :)

Thanks,
Roy