On Fri, Jun 19, 2015 at 3:44 PM, Laszlo Ersek <ler...@redhat.com> wrote: > 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).
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 > >> >> >>> >>> (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 > ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
