On 09/05/14 11:36, Ard Biesheuvel wrote: > On 4 September 2014 10:39, Laszlo Ersek <[email protected]> wrote: >> On 09/04/14 08:32, Ard Biesheuvel wrote: >>> On 4 September 2014 04:09, Laszlo Ersek <[email protected]> wrote: >>>> Hi Ard, >>>> >>>> I started to review your v6 patchset in reverse order -- I first created >>>> a map between your v5 and v6 patches (as much as it was possible), then >>>> started to look at the DSC file(s) first. The requirement to dynamically >>>> determine the UART base address from the DTB is a very intrusive one, >>>> unfortunately. So, the first thing that caught my eye was the various >>>> new instances of the SerialPortLib class. >>>> >>>> As we discussed on #linaro-enterprise, "EarlyFdtPL011SerialPortLib" is >>>> not appropriate for DXE_CORE, because it accesses the initial copy of >>>> the DTB (at the bottom of the system DRAM), and at that point, when the >>>> DXE_CORE is already running, that memory area is no longer protected >>>> (because we decided to relocate it instead of protecting it in-place). >>>> >>>> So, I didn't get very far in the v6 review, but I do think I can propose >>>> an improvement for the DXE_CORE's serial port library. Please see the >>>> following patches. >>>> >>> >>> Looks great, thanks! I will look in more detail later today, but one >>> thing comes to mind already: >>> since the PcdGet() call in the constructor of the ordinary FdtPL011 >>> library is causing dependency hell and the need for a cloned >>> UefiBootServicesTableLib, is there any reason we can't use your >>> DXE_CORE version for *all* stages after DXE_CORE as well? >> >> I'm very pleased that you too realized this possibility. >> >> Unfortunately, it doesn't work. I tested it, and only upon seeing that >> it didn't work did I elect to post this version of the fixup series. >> (This is the reason I posted the series after 4AM, and not at 01:30AM.) >> >> Namely, the HobLib class has three instances: >> >> HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf >> HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf >> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf >> >> The PEI flavor allows linking against PEIM PEI_CORE SEC, but it doesn't >> work in SEC (at least with the other lib resolutions that are used in >> ArmPlatformPkg -- FWIW I didn't test it out elsewhere). It is not >> relevant right now, I'm just mentioning it because originally I even >> tried to set the HOB in SEC, so that *all* SerialPortLib instances would >> the HOB based approach, but it didn't fly. PeiHobLib doesn't work in >> SEC, because needed PEI services are not available. And, in PEI itself, >> it would be a mess to order some PEIMs (providing those services) >> against other PEIMs. Hence I preserved your EarlyFdt implementation for >> PEIM PEI_CORE SEC; it works fine. >> >> The DxeCoreHobLib instance is only usable with DXE_CORE, and it has no >> CONSTRUCTOR. (You do see where this is going!) This allows the >> DxeCoreSerialPortLib instance I posted to work "automagically", even >> though it depends on HobLib. (Because, DxeCoreHobLib depends on DebugLib >> depends on DxeCoreSerialPortLib...) It all works because DxeCoreHobLib >> uses "gHobList", without a constructor, which just comes from >> "MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.c". IOW we do >> exploit that we're in the DXE_CORE -- the DXE_CORE has a special HobLib >> instance. >> >> No such luck with DxeHobLib. That one has a CONSTRUCTOR, and BuildTools >> immediately detect a circular dependency involving DxeHobLib (depending >> on DebugLib depending on this new SerialPortLib depending on HobLib). If >> I update the new, HOB-dependent SerialPortLib instance, removing its >> constructor, and initializing it in its SerialPortInitialize() function >> instead, then BuildTools don't detect a cycle -- and upon realizing >> this, I *mistakenly* posted "I've seen the light" in the other thread. >> Except it doesn't work, because the *exact same* constructor call order >> problem hits. Namely, in the VirtFdtDxe module, DebugLib's constructor >> calls SerialPortInitialize() manually, which calls GetFirstGuidHob(), >> and that blows up because DxeHobLib's CONSTRUCTOR has not yet run. >> > > OK, I think I have nailed it: > I took your patches, and introduced a private DxeHobLib in the > ArmVirtualizationPkg whose constructor looks like this: > > EFI_STATUS > EFIAPI > HobLibConstructor ( > IN EFI_HANDLE ImageHandle, > IN EFI_SYSTEM_TABLE *SystemTable > ) > { > UINTN Index; > > for (Index = 0; Index < SystemTable->NumberOfTableEntries; Index++) { > if (CompareGuid (&gEfiHobListGuid, > &(SystemTable->ConfigurationTable[Index].VendorGuid))) { > mHobList = SystemTable->ConfigurationTable[Index].VendorTable; > return EFI_SUCCESS; > } > } > > return EFI_NOT_FOUND; > } > > This allows me to drop the dependencies on both DebugLib (after > defusing the ASSERTs) and UefiLib, and after folding your DXE PL011 > into my PL011 driver, everything seems to work happily. > As I don't need PcdLib anymore, I could also drop the > UefiBootServicesTableLib private instance, and the bogus fake > dependency I needed to get the constructor ordering right. > > So we end up with 2 SerialPortLib instances: > - EarlyFdtPL011||SEC PEI_CORE PEIM > - FdtPL011|DXE_CORE DXE_DRIVER UEFI_DRIVER DXE_RUNTIME_DRIVER > > where DXE_CORE uses its own HobLib instance > > If this looks reasonable to you, I will go ahead and post my v7.
Very nice. Please go ahead. Open-coding EfiGetSystemConfigurationTable() (ie. manually scanning the config table) is valid; the UEFI spec documents the configuration table itself, so that's a public interface. (Not that we'd care too much if we accessed an internal-only interface :) ; I'm just saying that this is "elegant", in addition to "working".) Cheers Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
