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

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

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/

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

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