On 4 September 2014 04:09, Laszlo Ersek <ler...@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? 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.
> 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.
>
I will try to send out v7 by noon tomorrow, thanks
Cheers,
Ard.
> 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
