On 01/26/16 15:48, Ryan Harkin wrote: > On 26 January 2016 at 14:19, Laszlo Ersek <[email protected]> wrote: >> On 01/26/16 10:40, Leif Lindholm wrote: >>> On Tue, Jan 26, 2016 at 09:12:39AM +0000, Ryan Harkin wrote: >>>>> However there seems to be >>>>> some interference from "ArmPlatformPkg/Library/PlatformIntelBdsLib" that >>>>> is supposed to connect PcdDefaultConInPaths and PcdDefaultConOutPaths. >>>>> In order to experiment with F9, Roy massaged the last node of the >>>>> terminal device path (for controlling the parsing of the escape >>>>> sequences). But, some of those cases proved untestable. >>>>> >>>>> Unfortunately, I can't really help with >>>>> "ArmPlatformPkg/Library/PlatformIntelBdsLib". ArmVirtPkg used that >>>>> library instance as well originally, but I practically rewrote it >>>>> separately for ArmVirtPkg, in SVN r16923 and r16924. Since then, the two >>>>> instances have lived separate lives. >>>> >>>> Is this wise? It looks like BDS history repeating itself :-/ >>> >>> Well, medium-term, I'd (optimally) like to see ArmVirtPkg split and go >>> into OvmfPkg one end and ArmPkg the other. >>> >>> But when Laszlo started out with ArmVirtPkg (before it indeed _was_ >>> ArmVirtPkg), he felt the need to make substantial changes to >>> PlatformIntelBdsLib, and opted to do it separately. >>> We should probably take some time to look into what different needs we >>> may actually have on physical rather than virtual platforms, and how >>> we can go back to sharing the majority of the code. >> >> ** The main goals with the divergence visible in console setup were: >> >> - cut any ties with the ARM BDS >> >> (because, ArmPlatformPkg/Library/PlatformIntelBdsLib, despite being >> an instance of PlatformBdsLib, to be plugged into the Intel BDS >> driver, still uses ARM BDS related PCDs) >> >> - make console detection dynamic >> >> (think USB keyboards, and VGA cards at non-fixed PCI addresses) >> >> IIRC, I had also experienced issues with >> "ArmPlatformPkg/Library/PlatformIntelBdsLib" whenever a virtual machine >> would start up with a brand new (= empty) variable store. For a physical >> host this practically never happens, for a virtual machine, it can >> frequently happen (you lose the varstore file or decide to delete & >> recreate it from scratch). I vaguely recall that in such cases the >> serial console wouldn't work at the first startup. (By now I'm fuzzy on >> this, sorry.) >> >> Another example for the obscure console problems can be seen in SVN >> r16912 (that patch predates the rewrite). >> >> The mailing list thread can be found at >> >> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/12954 >> >> I do believe my "going separate ways" was justified. At that time >> ArmPlatformPkg had no PCI support at all, but ArmVirtualizationPkg did. >> Both the VGA and USB controllers (all types of them), emulated by QEMU, >> were PCI-based. So dynamic console detection made no sense for FVP, >> Foundation, let alone the physical ARM machines, that ArmPlatformPkg >> targeted at the time. >> >> ... The device path of the serial console would not become dynamic; it >> remained build-time static. I modified how it was represented though. >> First, the ARM-related textual PCDs with the device path(s) in them were >> an ARM BDS vestige. (See the first goal above.) Second, they are brittle >> to configure for the end user; do we really want to construct >> VenMsg(guid)-like strings manually? Typos are the norm, not the >> exception. Third, parsing them into binary form in the code is a chore. >> Because, do you do if there is indeed a typo and parsing fails? >> >> I stand by my idea (of patch #3 in the above-linked series) to encode >> the device path of the serial console with a fixed binary structure in >> the code. >> >> Later we would customize this for TTYTERM with SVN r17898. That would >> still not require the user to edit device paths, just to pass a build >> option. >> >> Finally, the rewrite gave the virt PlatformBdsLib a much-needed >> structural cleanup (in my opinion anyway). It is now as minimal as >> possible, and every line in there comes with its own justification. >> Which is not something I thought of the pre-rewrite code. >> >> >> ** Further features that only make sense for virtual machines (not an >> exhaustive list, just what I can think of now immediately): >> >> - direct kernel boot from QEMU's fw_cfg interface >> (see TryRunningQemuKernel() / "QemuKernel.c" / SVN r16578) >> >> - controlling UEFI boot order from the QEMU command line >> (see SVN r16576, and the patches leading up to it) >> >> ... In fact, these last two features were implemented in one series, >> chronologically *first*, and ultimately they are *the* reason for >> splitting away from ArmPlatformPkg: >> >> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/11743/focus=11746 >> >> >> ** Note that Ard and myself have not neglected keeping ArmPlatformPkg's >> and ArmVirtPkg's PlatformBdsLibs in sync, whenever it made sense. (For >> one, the Xen build of ArmVirtPkg still uses ArmPlatformPkg's >> PlatformBdsLib!) For example, compare: >> >> - SVN r17713 >> ArmVirtPkg: signal EndOxDxe event in PlatformBsdInit >> >> - SVN r18394 >> ArmPlatformPkg: signal EndOfDxe event in PlatformBsdInit >> >> see also the discussion: >> http://thread.gmane.org/gmane.comp.bios.edk2.devel/1849/focus=1916 >> >> > > I wasn't criticising the features you've implemented, I hope it didn't > seem that way! > > Mostly, I'm worried that you've made loads of improvements that sounds > like things I would love to have, but don't get because we're not > using common code.
Sorry if I got overly defensive. :) It would be great if ArmPlatformPkg could benefit from at least some of these improvements, but one doesn't really modify platform code (dedicated or shared alike) without *testing* the changes. I could have tested any such changes on the FVP and Foundation models *only*, which are (a) a PITA to use :), (b) not part of my resource budget. > But moving Juno and FVP to Intel BDS leaves me with more questions > than answers. I still haven't worked out how I'm going to boot my > system with this jumble of stuff. So far, getting EDK2 to boot itself > has been a small victory. I guess someone will have to do the same as we did for ArmVirtQemu: bite the bullet and clean up ArmPlatformPkg's PlatformBdsLib from the ground up. Perhaps scavenge as much as possible from other platforms in the process. Maybe in the end we'll be able to factor out and reuse things. BDS has always been a pain point, in OVMF as well. That's where things turn from declarative (you could argue the UEFI driver model is somewhat declarative) to imperative, giving us both the best control, and the worst seduction for ugly hacks. Thanks Laszlo > > >> ** As for splitting up ArmVirtPkg and distributing it between >> ArmPlatformPkg and OvmfPkg -- I think ArmVirtPkg is already as minimal >> as possible. It pulls in a large number of libraries and drivers from >> both ArmPlatformPkg and OvmfPkg. Speaking of the QEMU build, it only has >> three drivers of its own (HighMemDxe, PciHostBridgeDxe, VirtFdtDxe). It >> does customize a larger number of library instances, but library >> instances are meant just for that. >> >> I'm not opposing this on principle, mind you, but I've been pretty >> pleased with the current structure, so I'll have to see concrete ideas >> to set my mind to a different track. :) >> >> Thanks! >> Laszlo >> >>> >>> / >>> Leif >>> >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
