Re: dynamic DRAM base for ArmVirtQemu

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 13 October 2017 at 13:51, Laszlo Ersek <lersek@redhat.com> wrote:
> Hi Ard, Leif,
>
> the current physical memory map of the "virt" machine type doesn't leave
> much room for ECAM / MMCONFIG, which limits the number of PCI Express
> root ports and downstream ports (each port takes a separate bus number,
> and each bus number eats up a chunk of the ECAM area). Also, each port
> can only accommodate a single PCI Express device. In practice this
> limits the number of (hot-pluggable) PCIe devices to approx. 16, which
> is deemed by some "not scaleable enough". (For devices that only need to
> be cold-plugged, they can be placed directly on the root complex, as
> integrated devices, possibly grouping them into multifunction devices
> even; so those don't need bus numbers.)
>
> In order to grow the MMCONFIG area (and for some other reasons
> possibly), the phys memmap of "virt" should be shuffled around a bit.
> This affects the "system" DRAM too.
>

Is it really necessary to put the ECAM area below 4 GB? For ARM, I
understand this would be an issue, but does the spec actually mandate
anything like this?


>
> One idea is to keep the current system DRAM base at 1GB, but limit its
> size to 1GB. And, if there's more DRAM, use another, disjoint address
> range for that, above 4GB. This would be easy to support for ArmVirtQemu
> (basically nothing new would be necessary), as the high area would be
> handled transparently by "ArmVirtPkg/HighMemDxe". However, this appears
> to present complications for QEMU. (I don't exactly know what
> complications -- I would be very happy to hear them, in detail.)
>
>
> Another idea is to move *the* system DRAM base to a different guest-phys
> address. (Likely using a different version of the "virt" machine type,
> or even a different machine type entirely.) This would not be compatible
> with current ArmVirtQemu, which hard-codes the system DRAM base in
> several, quite brittle / sensitive, locations. (More on this later --
> that's going to be the larger part of my email anyway.) In order to
> handle the new base in ArmVirtQemu, two approaches are possible: change
> the hard-coded address(es), or cope with the address dynamically.
>
> Changing the hard-coded addresses is easy for edk2 contributors (just
> add a new build flag like -D DRAM_BASE_AT_XXX_GB, and dependent on it,
> set a number of fixed-at-build PCDs to new values). For RHEL downstream,
> this is not an un-attractive option, as we are free to break
> compatibility at this time. For upstream users and other distros
> however, it likely wouldn't be convenient, because "old" ArmVirtQemu
> firmware wouldn't boot on the "new" machine type, and vice versa.
>
> (If we can agree that the above "boundary" in firmwares and machine
> types is widely tolerable, then we need not discuss the rest of this
> email.)
>
>
> Finally, coping with "any" system DRAM base address in ArmVirtQemu is
> both the most flexible for users, and the most difficult to implement.
> When QEMU launches the guest, the base of the system DRAM (which equals
> the location of the DTB too) is exposed in the x0 register. The
> challenge is to make everything in the earliest phases of ArmVirtQemu to
> adapt to this value dynamically, and to propagate the value to further
> parts of the firmware.
>
> I've been looking into this for a few days now and would like to pick
> your minds on what I've found.
>
> (
>
>   As a side note, I know (superficially) of Ard's ArmVirtXen and
>   ArmVirtQemuKernel work. If I understand correctly, Ard has turned some
>   of the PCDs I'm about to discuss into "patchable" ones, from
>   "fixed-at-build". The difference in storage is that these constants
>   are now placed in the final firmware image such that they are
>   externally patchable, just before "deployment".
>
>   Because the ArmVirtXen and ArmVirtQemuKernel firmware binaries are
>   loaded into DRAM immediately, this self-patching -- based on the
>   initial value of x0 -- is feasible, in the earliest part of the
>   firmware. (I'm not saying "easy" -- to the contrary; but it's
>   feasible.)
>
>   However, ArmVirtQemu is launched from pflash; its SEC and PEI phases
>   execute-in-place from pflash (until MemoryInitPeim installs the
>   permanent PEI RAM, and the PEI_CORE relocates the HOB list, itself,
>   and the PEIMs into DRAM). Therefore the SEC and PEI phases of
>   ArmVirtQemu cannot be patched like this, i.e., through patchable PCDs.
>   (Unless, of course, the patching is implemented in QEMU itself -- but
>   I don't think that's probable).
>
> )
>
> Now, exactly because SEC and PEI execute in place from pflash, their
> execution (= instruction fetches) are not affected by different initial
> x0 values. However, the following are affected:
>
> - the initial stack,
> - the base address of the initial DTB,
> - the location of the permanent PEI RAM.
>
> In ArmVirtQemu, these are represented by the following PCDs (all
> fixed-at-build currently):
>
> - PcdCPUCoresStackBase
> - PcdDeviceTreeInitialBaseAddress
> - PcdSystemMemoryBase
>
> I've attempted to audit each of these PCDs.
>
> (I should note in advance that I focused on their use *only* in the
> ArmVirtQemu firmware platform, and only on AARCH64. I ignored ARM32, and
> ArmVirtXen and ArmVirtQemuKernel. We obviously must not regress those
> other arches / platforms by messing with these PCDs, but this is only a
> "feasibility study" for now.)
>
>
>
> (1) PcdCPUCoresStackBase
>
> The PCD's current value is, from "ArmVirtPkg/ArmVirtQemu.dsc":
>
>> [PcdsFixedAtBuild.common]
>>   gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000
>
> That is, 496KB above the start of the DRAM (which is at 1GB currently).
> This leaves enough room for the initial DTB (placed at the start of
> DRAM).
>
> The PCD is used widely by the SECURITY_CORE (=SEC) module of
> ArmVirtQemu, namely:
>
>   ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
>
> The PCD is used by no other module.
>
>
> * The stack is set up in
>
>   ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
>
> under the label ASM_PFX(MainEntryPoint). This is where the initial value
> of x0 should be considered first, replacing the use of
> "PcdCPUCoresStackBase" with something like
>
>   x0 + 0x7c000
>
> (I don't grok aarch64 assembly, so any help with the implementation
> would be greatly appreciated (if this approach is feasible at all).)
>
>
> * Once we're done with the assembly magic, we enter the CEntryPoint()
> function, and the following tree of calls occurs:
>
>   CEntryPoint()         [ArmPlatformPkg/PrePeiCore/PrePeiCore.c]
>     PrimaryMain()       [ArmPlatformPkg/PrePeiCore/MainUniCore.c]
>       CreatePpiList()   [ArmPlatformPkg/PrePeiCore/PrePeiCore.c]
>     PeiCoreEntryPoint()
>
>
> * The CEntryPoint() function *currently* takes two parameters from the
> assembly entry logic,
>
>> VOID
>> CEntryPoint (
>>   IN  UINTN                     MpId,
>>   IN  EFI_PEI_CORE_ENTRY_POINT  PeiCoreEntryPoint
>>   )
>
> The PeiCoreEntryPoint parameter is the function pointer that is going to
> be invoked at the bottom of the call tree, displayed above. The assembly
> code in "PrePeiCoreEntryPoint.S" fetches the value from the flash image.
> (I'm spelling this out just to clarify the last line in the call tree.)
>
> - The parameter list of the CEntryPoint() function should be extended
>   with
>
>   IN UINT64  MemoryBase,
>   IN UINT64  StackBase
>
> - the assembly code should pass the initial value of x0 in "MemoryBase",
>
> - and the stack base (i.e., (x0 + 0x7c000)) in "StackBase".
>
>
> * Then, the PrimaryMain() function is invoked:
>
>> VOID
>> EFIAPI
>> PrimaryMain (
>>   IN  EFI_PEI_CORE_ENTRY_POINT  PeiCoreEntryPoint
>>   )
>
> This function does the following:
>
> (1a) it creates a PPI list *in DRAM*, at PcdCPUCoresStackBase, by
>      calling CreatePpiList(),
>
> (1b) right above the PPI list, it marks the DRAM area to be used as
>      temporary SEC/PEI stack/heap,
>
> (1c) it enters the PEI_CORE, by calling PeiCoreEntryPoint(), passing it
>      the above-calculated locations, *and* the PPI list also constructed
>      above.
>
> Now, PrimaryMain()'s parameter list should also be extended with
>
>   IN UINT64  MemoryBase,
>   IN UINT64  StackBase
>
> and the "StackBase" parameter should replace "PcdCPUCoresStackBase" in
> the (1a) and (1b) calculations.
>
> In addition, this is where we have the opportunity (and obligation) to
> propagate the "MemoryBase" value to *all* of the PEI phase.
>
> The way SEC propagates *custom* information to PEI is via the PPI list
> parameter of PeiCoreEntryPoint(). Unlike CEntryPoint() and
> PrimaryMain(), PeiCoreEntryPoint() has a standardized function
> prototype:
>
>   MdePkg/Include/Pi/PiPeiCis.h
>
>> typedef
>> VOID
>> (EFIAPI *EFI_PEI_CORE_ENTRY_POINT)(
>>   IN CONST  EFI_SEC_PEI_HAND_OFF    *SecCoreData,
>>   IN CONST  EFI_PEI_PPI_DESCRIPTOR  *PpiList
>> );
>
> (See also the comment block on the typedef! It's super informative.)
>
> The "SecCoreData" pointer passes the information that we calculated in
> (1b). And the "PpiList" pointer passes the PPI list from (1a).
>
>
> * Therefore, if we have to modify the CreatePpiList() function too:
>
>> VOID
>> CreatePpiList (
>>   OUT UINTN                   *PpiListSize,
>>   OUT EFI_PEI_PPI_DESCRIPTOR  **PpiList
>>   )
>
> This function currently concatenates two *constant* (in-flash) PPI
> lists. One comes from the variable
>
>   gCommonPpiTable [ArmPlatformPkg/PrePeiCore/PrePeiCore.c]
>
> and the other is returned by the function
>
>   ArmPlatformGetPlatformPpiList() [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
>
> The concatenated list is placed into DRAM, at "PcdCPUCoresStackBase".
>
> The point here is that both *input* PPI lists are *constant*, they come
> from pflash, and don't have the "MemoryBase" information. Therefore,
>
> - the parameter list of CreatePpiList() should be extended with
>
>   IN UINT64  MemoryBase,
>   IN UINT64  StackBase
>
> - CreatePpiList() should store the concatenated list at "StackBase", not
>   "PcdCPUCoresStackBase",
>
> - and CreatePpiList() should append *two more* PPI descriptors (with
>   separate, custom GUIDs, to be defined by us), where the first's
>   "EFI_PEI_PPI_DESCRIPTOR.Ppi" (of type pointer-to-void) would point
>   *directly* at "MemoryBase", for advertizing the memory base; and the
>   second would point to the same, but advertize the initial DTB base
>   address. (We'd have two distinct PPIs for completeness.)
>
> Then interested PEIMs could locate these PPIs by GUID in the PEI phase,
> and learn about the "MemoryBase" / initial DTB base address values.
>
>
> * Now, clearly, this is quite a few changes to the
>
>   ArmPlatformPkg/PrePeiCore
>
> module, which is likely used as the SEC phase of a bunch of other ARM
> platforms. Should we copy "ArmPlatformPkg/PrePeiCore" under ArmVirtPkg
> for this kind of customization?
>
>
>
> (2) PcdDeviceTreeInitialBaseAddress
>
> (
>
>   Before discussing this PCD, I have to digress a bit. Namely, the
>   ArmPlatformLib class, defined in
>
>     ArmPlatformPkg/Include/Library/ArmPlatformLib.h
>
>   is unfortunately a very confusing lib class. It is a dumping ground
>   for random "platform" functions. The catch is that some of these
>   functions are meant to be invoked from SEC, exclusively, while some
>   other functions are meant to be invoked from PEI, exclusively.
>
>   The end result is that, if a given ArmPlatformLib instance implements
>   a *PEI-only* function with various PCD, PPI, and GUID dependencies,
>   then the exact same, *bogus*, dependencies will show up in the build
>   report file for the SECURITY_CORE module as well. Simply because the
>   SEC module links against the same library instance, for the sake of
>   the SEC-only functions. Never mind that the SEC module never *calls*
>   those PEI-only functions that incur said dependencies.
>
>   This makes dependency analysis, based on the build report file, very
>   cumbersome.
>
>   The ArmPlatformLib class should be split into SEC-only and PEI-only
>   lib classes.
>
> )
>
> "PcdDeviceTreeInitialBaseAddress" is currently set in
> "ArmVirtPkg/ArmVirtQemu.dsc" as follows:
>
>> [PcdsFixedAtBuild.common]
>>   # initial location of the device tree blob passed by QEMU -- base of DRAM
>>   gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress|0x40000000
>
>
> "PcdDeviceTreeInitialBaseAddress" is used in three library instances:
>
> (2a) ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf
>      ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
>
> (2b) ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
>      ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c
>
> (2c) ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
>      ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
>
>
> (2a) "ArmVirtPlatformLib.inf" is built into SEC, discussed above:
>
>   ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
>
> and also into the following two PEIMs:
>
>   ArmPlatformPkg/PlatformPei/PlatformPeim.inf
>   ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
>
> The function in ArmVirtPlatformLib that fetches
> "PcdDeviceTreeInitialBaseAddress", namely
> ArmPlatformInitializeSystemMemory(), is *only* called from the entry
> point function of MemoryInitPeim, InitializeMemory().
>
> (See my gripe above about the ArmPlatformLib class -- SEC's apparent
> dependency on this PCD is bogus!)
>
> I.e., we have the following call tree:
>
>   InitializeMemory()                             [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>     ArmPlatformInitializeSystemMemory()          [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
>       PcdGet64 (PcdDeviceTreeInitialBaseAddress)
>
> Consequently, given that the only reference to
> "PcdDeviceTreeInitialBaseAddress" is made from within PEI, we can
> replace the PcdGet64() call with a PeiServicesLocatePpi() call, and grab
> the initial device tree base address from our custom PPI.
>
>
> (2b) "EarlyFdtPL011SerialPortLib.inf" is a SerialPortLib instance that
> parses the UART base address from the initial DTB on every
> SerialPortWrite() call:
>
>   SerialPortWrite()                              [ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c]
>     SerialPortGetBaseAddress()                   [ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c]
>       PcdGet64 (PcdDeviceTreeInitialBaseAddress)
>     PL011UartWrite()                             [ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c]
>
> The library instance is linked into:
>
> - SEC: ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
>
> - PEI_CORE: MdeModulePkg/Core/Pei/PeiMain.inf
>
> - and all 6 PEIMs included by ArmVirtQemu:
>
>   ArmPkg/Drivers/CpuPei/CpuPei.inf
>   ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
>   ArmPlatformPkg/PlatformPei/PlatformPeim.inf
>   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>   MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>   MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>
> It is not used by other modules.
>
> If we replaced the "PcdDeviceTreeInitialBaseAddress" access in
> SerialPortGetBaseAddress() with a PeiServicesLocatePpi() call, then the
> PEI_CORE and PEIM modules would remain functional. (See again the
> comment block on EFI_PEI_CORE_ENTRY_POINT in
> "MdePkg/Include/Pi/PiPeiCis.h" -- the PEI_CORE itself is allowed to use
> the PPIs originally exported by SEC.)
>
> However, SEC couldn't use a library instance like this -- there's no way
> to search for a PPI in SEC. In other words, the SerialPortLib class is
> unsuitable for such use in SEC. I don't know how to solve this, other
> than by hard-coding the UART base address with a fixed-at-build PCD, in
> a custom SerialPortLib instance. :/
>
>
> (2c) The "PlatformPeiLib.inf" instance used with ArmVirtQemu copies the
> initial DTB into its final (page-allocated) place, in the PlatformPeim()
> function:
>
>   InitializePlatformPeim()                       [ArmPlatformPkg/PlatformPei/PlatformPeim.c]
>     PlatformPeim()                               [ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c]
>       PcdGet64 (PcdDeviceTreeInitialBaseAddress)
>
> We can again replace the PcdGet64() with a PeiServicesLocatePpi() call.
> (No other module uses this PlatformPeiLib instance.)
>
>
>
> (3) PcdSystemMemoryBase
>
> Comes currently from "ArmVirtPkg/ArmVirtQemu.dsc":
>
>> [PcdsFixedAtBuild.common]
>>   # System Memory Base -- fixed at 0x4000_0000
>>   gArmTokenSpaceGuid.PcdSystemMemoryBase|0x40000000
>
>
> Based on the build report file, the following modules depend on this
> PCD, directly, or via library instances:
>
> - SEC:  ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> - PEIM: ArmPlatformPkg/PlatformPei/PlatformPeim.inf
> - PEIM: ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
>
> The last two (i.e., the PEIMs) can use PeiServicesLocatePpi() in place
> of "PcdSystemMemoryBase".
>
>
> The first module (SEC) seems to inherit the dependency on
> "PcdSystemMemoryBase" via ArmVirtPlatformLib. In ArmVirtPlatformLib, we
> consume the PCD in two spots:
>
> (3a) in the ArmPlatformInitializeSystemMemory() function. Referring back
> to my notes in (2a), this function is never called from SEC, so we can
> use PeiServicesLocatePpi(), for grabbing the DRAM base.
>
> (3b) The ArmPlatformGetVirtualMemoryMap() function is the other
> consumer. This function appears to be called on the following path, as
> part of
>
>   ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
>
> *only*:
>
>   InitializeMemory()                     [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>     MemoryPeim()                         [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>       InitMmu()                          [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>         ArmPlatformGetVirtualMemoryMap() [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
>           PcdGet64 (PcdSystemMemoryBase)
>
> Because this PCD consumption site is again never reached in SEC, we can
> replace it with PeiServicesLocatePpi().
>
>
>
> Summary:
>
> - The end goal is to clear out all appearances of PcdCPUCoresStackBase,
>   PcdDeviceTreeInitialBaseAddress, and PcdSystemMemoryBase from the
>   ArmVirtQemu build report file. The first PCD should be replaced by a
>   plain calculation in SEC, from x0, and the other two should be
>   replaced by custom PPIs that SEC produces (passing them to the
>   PEI_CORE).
>
> - I'd need help with the assembly code in SEC
>   ("ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S").
>
> - The changes are intrusive enough -- for ArmPlatformPkg, and for the
>   ArmVirtXen and ArmVirtQemuKernel platforms under ArmVirtPkg -- to
>   justify deep-copying a number of modules, specifically for
>   ArmVirtQemu.
>
> - In ArmVirtQemu, SEC would lose serial output, unless we hard-coded the
>   PL011 UART base address. Neither the DebugLib nor the SerialPortLib
>   APIs can take auxiliary data as function parameters, and in SEC, we
>   have *none* of: writeable global variables, HOBs, *searchable* PPIs,
>   dynamic PCDs. The only way to pass information is via the stack, and
>   the DebugLib and SerialPortLib APIs are unsuited for that.
>
> Thoughts?
>
> Thanks
> Laszlo