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.
I sought to separate these patches out in such a way that you can review
them easily, and more importantly, apply them on top of your v6 tree,
then rebase the entire thing, and *squash* my patches into your patches.
It should be obvious for each one of mine which one of yours it should
be squashed into, purely from the paths in each patch.
As a summary, with these "fixups" applied, we have the following
situation (marking where the "fixups" make a difference relative to your
v6):
(1) In SEC:
- code runs from flash
- code uses temporary RAM only
- qemu's DTB is protected (distinct from temp RAM)
- SerialPortLib ("EarlyFdt" variant) parses QEMU's original DTB for
each write call (because it cannot store the UART base anywhere,
as state)
- PCDs are unavailable
- HOBs are unavailable (HobLib definitely)
- the SEC half or ArmVirtualizationPlatformLib, ie. function
ArmPlatformInitialize(), only asserts
!PcdSystemMemoryInitializeInSec
(2) In PEI, while on temporary RAM (covering PEI_CORE and PEIMs):
- code runs from flash
- code uses temporary RAM only
- qemu's DTB is protected (distinct from temp RAM)
- SerialPortLib ("EarlyFdt" variant) parses QEMU's original DTB for
each write call (because it cannot store the UART base anywhere,
as state)
- the PCD database is available, in a HOB, located in temp RAM,
- HOBs are available, in temp RAM,
- the PEI half of ArmVirtualizationPlatformLib, ie. function
ArmPlatformInitializeSystemMemory(), running as part of
MemoryInitPeim, does the following:
- parses QEMU's DTB,
- saves the DRAM size in the dynamic PcdSystemMemorySize,
- saves the UART base in the dynamic PcdPL011BaseAddress,
- saves the UART base address in a new, special HOB [CHANGE].
(Note that in general we can't assume that this HOB would be
available to any other PEIMs, especially not the PCD PEIM.)
- once ArmPlatformInitializeSystemMemory() returns to
InitializeMemory(), in ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c:
- permanent PEI RAM is installed, based on PcdSystemMemorySize
- the PEI_CORE relocates itself to said perm RAM
- the HOB list and the PCD database are migrated to perm RAM
(3) In PEI, while on permanent RAM (covering PEI_CORE and PEIMs):
- code runs from flash
- code uses permanent RAM
- qemu's DTB is protected (distinct from perm RAM)
- SerialPortLib ("EarlyFdt" variant) still parses QEMU's original
DTB for each write call. Maybe this could be optimized, but it is
good enough for the rest of PEI, so DO NOT TOUCH IT. :)
- the PCD database is available in perm RAM
- HOBs are available in perm RAM,
- ArmPlatformPkg/PlatformPei/PlatformPeim can now run (see the Depex
it inherits from our PlatformPeiLib instance), and in
PlatformPeim():
- it copies QEMU's DTB to a freshly allocated block in perm PEI
RAM, setting the dynamic PcdDeviceTreeBaseAddress
- an FV HOB is built for the DXE IPL PEIM
- the original DTB is left in place for the rest of PEI
(4) DXE_CORE
- code has been decompressed and runs from RAM
- code uses the entirety of the system DRAM
- qemu's DTB is *not* protected (must be assumed lost)
- the "EarlyFdt" variant of SerialPortLib must not attempt to parse
the original DTB as a consequence -- this is the problem in v6
- The relocated DTB is protected
- the PCD database is protected, but it is inaccessible, because the
PPI (PEI driver) is not available any longer, and the protocol
(DXE driver) is not available yet
- HOBs are protected and accessible (read-only) with HobLib
- therefore a new instance of SerialPortLib,
DxeCorePL011SerialPortLibInitialize, is added that doesn't parse
the old DTB (because that must be considered destroyed), doesn't
use any dynamic PCDs (because those are unavailable, see above),
instead it relies on the special HOB from step (2) to retrieve the
UART base address. [CHANGE]
(5) DXE
- code has been decompressed and runs from RAM
- code uses the entirety of the system DRAM
- qemu's DTB is *not* protected (must be assumed lost)
- The relocated DTB is protected
- PCD database is available
- HOBs are available (r/o)
- the "Fdt" instance of SerialPortLib uses the dynamic
PcdPL011BaseAddress
- VirtFdtDxe runs early (APRIORI DXE) and sets a number of dynamic
PCDs from the relocated DTB for other drivers, and links the
relocated DTB into the system table for the kernel as well
If you agree, please post v7 with these squashed (and potentially cleaned
up further, as you see fit).
Thanks
Laszlo
Laszlo Ersek (9):
ArmVirtualizationPkg: DSC: fix line endings
ArmVirtualizationPkg: introduce gEarlyPL011BaseAddressGuid
ArmVirtualizationPkg: DSC: resolve HobLib dependencies for SEC
ArmVirtualizationPlatformLib: lock down client module types
ArmVirtualizationPlatformLib: stash dynamic UART base in a HOB in the
PEI code
ArmVirtualizationPkg: FdtPL011SerialPortLib: lock down allowed clients
FdtPL011SerialPortLib.inf: drop bogus PcdSystemMemorySize reference
FdtPL011SerialPortLib: add DXE_CORE implementation
ArmVirtualizationPkg: DSC: resolve SerialPortLib for DXE_CORE
.../ArmVirtualizationPlatformLib.inf | 6 +-
.../DxeCorePL011SerialPortLib.inf | 46 +++++++
.../EarlyFdtPL011SerialPortLib.inf | 2 +-
.../FdtPL011SerialPortLib.inf | 3 +-
.../Include/Guid/EarlyPL011BaseAddress.h | 27 ++++
.../Library/ArmVirtualizationPlatformLib/Virt.c | 22 +++-
.../DxeCorePL011SerialPortLib.c | 142 +++++++++++++++++++++
.../ArmVirtualizationPkg/ArmVirtualization.dsc.inc | 50 ++++----
.../ArmVirtualizationPkg/ArmVirtualizationPkg.dec | 1 +
9 files changed, 267 insertions(+), 32 deletions(-)
create mode 100644
ArmPlatformPkg/ArmVirtualizationPkg/Library/FdtPL011SerialPortLib/DxeCorePL011SerialPortLib.inf
create mode 100644
ArmPlatformPkg/ArmVirtualizationPkg/Include/Guid/EarlyPL011BaseAddress.h
create mode 100644
ArmPlatformPkg/ArmVirtualizationPkg/Library/FdtPL011SerialPortLib/DxeCorePL011SerialPortLib.c
--
1.8.3.1
------------------------------------------------------------------------------
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
