dynamic DRAM base for ArmVirtQemu

dynamic DRAM base for ArmVirtQemu

[Laszlo Ersek]

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.


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

Re: dynamic DRAM base for ArmVirtQemu

[Ard Biesheuvel]

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

Re: dynamic DRAM base for ArmVirtQemu

[Laszlo Ersek]

On 10/13/17 15:21, Ard Biesheuvel wrote:
> 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?

No, I don't think it's inherently necessary to put ECAM under 4GB.

It's just that I approached the problem such that the firmware would be
burdened with the lion's share of the complexity, in order to keep
things "compatible" for the rest of the guest world.

(Speaking personally, I'd absolutely prefer to keep the DRAM base
untouched, at 1GB, but I was invited to take a good hard look at what
moving the DRAM base would take for the firmware, and I made an honest
effort to determine the impact. It doesn't imply that I like the result
in the least, or that I personally subscribe to the idea.)

Thanks!
Laszlo

Re: dynamic DRAM base for ArmVirtQemu

[Laszlo Ersek]

On 10/13/17 18:18, Peter Maydell wrote:
> On 13 October 2017 at 13:51, Laszlo Ersek <lersek@redhat.com> wrote:
>> 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.
> 
> I strongly don't want to move the DRAM base in the "virt" board.

You really cannot *not* want it more strongly than I :)

(See my answer to Ard for why I went to such lengths nonetheless in
mapping out the consequences for the firmware -- I knew and feared I'd
find monsters there, but when I'm invited to look, it's only honest to
look.)

> This is one of the few fixed things we've said that guest code
> can rely on without having to fish the information out of the
> device tree.

And, as one co-maintainer of one guest firmware, I'm immensely relieved
to learn about, and benefit from, this guarantee.

Thanks,
Laszlo