Re: [edk2] [PATCH 0/9] first round of proposed fixups for "add support for AArch64 QEMU/KVM v6"

Subject: Re: [edk2] [PATCH 0/9] first round of proposed fixups for "add support for AArch64 QEMU/KVM v6"

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

To: Laszlo Ersek <lersek@redhat.com>

Date: 2014-09-05 19:36:48

On 4 September 2014 10:39, Laszlo Ersek  wrote:
> On 09/04/14 08:32, Ard Biesheuvel wrote:
>> On 4 September 2014 04:09, Laszlo Ersek  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.

Cheers,
Ard.


> This is a pity because this approach otherwise allowed me to completely
> remove both PcdPL011BaseAddress and the forked UefiBootServicesTableLib.
> To my dismay however, replacing our dependency on dynamic PCDs in
> SerialPortLib with a dependency on HOBs reintroduces the exact same kind
> of circular mess. This clearly shows that SerialPortLib had always been
> designed to be 100% self-contained, and we're violating that by trying
> to use dynamic PCDs *or* HOBs in it, generally. We only get lucky in
> DXE_CORE with HOBs (because there a HOB dependency happens to work), and
> in the other (later) DXE module types with dynamic PCDs (because you
> hacked around the construction order with your forked
> UefiBootServicesTableLib).
>
> My proposal is to go with this mixed approach (and pray that the above
> luck not run out), or to own up to the design constraints of
> SerialPortLib, and stick with a FixedAtBuild UART base address.
> (Honestly, I don't perceive that to be a grave limitation.)
>
>> Getting the
>> base address from a HOB and caching it doesn't seem more heavy weight
>> than getting it from a Dynamic PCD, and it would allow us to drop some
>> of the nasty stuff.
>
> Yes, it does, but it also reintroduces the same kind of circular
> dependencies, just with different names. I guess it could be worked
> around by cloning DxeHobLib, and removing its DebugLib dependency, but
> then again you did the exact same thing with UefiBootServicesTableLib,
> and that one is actually a much simpler library.
>
> I can send you this version of the "fixup patchset" if you'd like to
> experiment with it. As far as I see, *everything* in edk2 seems to
> depend on DebugLib, whose serial port instance depends on SerialPortLib,
> which is expected to be fully standalone. It is not just an
> implementation shortcoming, it's a theoretical problem -- if you need
> some HOB to be able to log a message, then you can't log messages in the
> HOB library itself, especially not in its CONSTRUCTOR.
>
> Note that such problems don't occur in OVMF due to a very simple reason:
> we have our DebugLib instance that plainly hardwires the qemu debug
> ioport (it's not even a serial port). Which would correspond, on ARM, to
> using a fixed UART base address.
>
> In summary, there are three options:
> - revert the dynamic UART base address feature, stick with fixed
>
> - use your approach (UefiBootServicesTableLib dependency hack), just
>   fix it up with this followup series for the DXE_CORE
>
> - opt for a clean HOB-based approach, and sever the circular
>   dependencies by cloning every library instance that inconveniently
>   depends on DebugLib (like DxeHobLib), and kill those DebugLib deps.
>   At least determining the set of these libraries is fairly simple,
>   because SerialPortLib's CONSTRUCTOR (if it has one) triggers the
>   cycle check.
>
> Basically, by enabling SerialPortLib to depend on other facilities, we
> must shift the "100% self-reliance" property to some of those other
> facilities.
>
> You know what, I'm attaching the HOB-based approach; perhaps you'd like
> to experiment with option #3. The first four patches are shared between
> the two versions (up to and including "ArmVirtualizationPlatformLib:
> lock down client module types"), they diverge at patch #5
> ("ArmVirtualizationPlatformLib: stash dynamic UART base in a HOB in the
> PEI code").
>
> Thanks,
> Laszlo

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel