[edk2,v2,6/8] ArmPkg, ArmPlatformPkg: allow dynamic PCDs for memory base and size

This changes the definition and a bunch of references to
gArmTokenSpaceGuid.PcdSystemMemoryBase and
gArmTokenSpaceGuid.PcdSystemMemorySize so they can be declared as dynamic PCDs
by the platform. Also, move the non-SEC call to
ArmPlatformInitializeSystemMemory() earlier, so a platform has a chance to set
these PCDs before they are first referenced.

The purpose is allowing dynamically instantiated virtual machines to declare
the system memory by passing a device tree.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/ArmPkg.dec                                        | 12 ++++++------
 ArmPkg/Library/BdsLib/BdsLib.inf                         |  3 ++-
 .../PrePi/PrePiArmPlatformGlobalVariableLib.inf          |  7 ++++---
 ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf        |  6 ++++--
 ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c            | 16 ++++++++--------
 ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf          |  6 ++++--
 ArmPlatformPkg/PrePi/PeiUniCore.inf                      |  6 ++++--
 7 files changed, 32 insertions(+), 24 deletions(-)

On 08/26/14 15:03, Ard Biesheuvel wrote:
> This changes the definition and a bunch of references to
> gArmTokenSpaceGuid.PcdSystemMemoryBase and
> gArmTokenSpaceGuid.PcdSystemMemorySize so they can be declared as dynamic PCDs
> by the platform. Also, move the non-SEC call to
> ArmPlatformInitializeSystemMemory() earlier, so a platform has a chance to set
> these PCDs before they are first referenced.
>
> The purpose is allowing dynamically instantiated virtual machines to declare
> the system memory by passing a device tree.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/ArmPkg.dec                                        | 12 ++++++------
>  ArmPkg/Library/BdsLib/BdsLib.inf                         |  3 ++-
>  .../PrePi/PrePiArmPlatformGlobalVariableLib.inf          |  7 ++++---
>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf        |  6 ++++--
>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c            | 16 ++++++++--------
>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf          |  6 ++++--
>  ArmPlatformPkg/PrePi/PeiUniCore.inf                      |  6 ++++--
>  7 files changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index e5ed445ca367..43eff6641294 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -116,12 +116,6 @@
>    gArmTokenSpaceGuid.PcdHypFvBaseAddress|0|UINT32|0x0000003C
>    gArmTokenSpaceGuid.PcdHypFvSize|0|UINT32|0x0000003D
>
> -  # System Memory (DRAM): These PCDs define the region of in-built system memory
> -  # Some platforms can get DRAM extensions, these additional regions will be declared
> -  # to UEFI by ArmPlatformLib
> -  gArmTokenSpaceGuid.PcdSystemMemoryBase|0|UINT64|0x00000029
> -  gArmTokenSpaceGuid.PcdSystemMemorySize|0|UINT64|0x0000002A
> -
>    # Use ClusterId + CoreId to identify the PrimaryCore
>    gArmTokenSpaceGuid.PcdArmPrimaryCoreMask|0xF03|UINT32|0x00000031
>    # The Primary Core is ClusterId[0] & CoreId[0]
> @@ -143,6 +137,12 @@
>
>
>  [PcdsFixedAtBuild.common,PcdsDynamic.common]
> +  # System Memory (DRAM): These PCDs define the region of in-built system memory
> +  # Some platforms can get DRAM extensions, these additional regions will be declared
> +  # to UEFI by ArmPlatformLib
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase|0|UINT64|0x00000029
> +  gArmTokenSpaceGuid.PcdSystemMemorySize|0|UINT64|0x0000002A
> +
>    #
>    # ARM Architectural Timer
>    #
> diff --git a/ArmPkg/Library/BdsLib/BdsLib.inf b/ArmPkg/Library/BdsLib/BdsLib.inf
> index 3e1ae8914abf..7302d6ab1b1c 100644
> --- a/ArmPkg/Library/BdsLib/BdsLib.inf
> +++ b/ArmPkg/Library/BdsLib/BdsLib.inf
> @@ -77,10 +77,11 @@
>  [FeaturePcd]
>    gArmTokenSpaceGuid.PcdArmLinuxSpinTable
>
> -[FixedPcd]
> +[Pcd]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase
>    gArmTokenSpaceGuid.PcdSystemMemorySize
>
> +[FixedPcd]
>    gArmTokenSpaceGuid.PcdArmMachineType
>    gArmTokenSpaceGuid.PcdArmLinuxFdtMaxOffset
>    gArmTokenSpaceGuid.PcdArmLinuxFdtAlignment
> diff --git a/ArmPlatformPkg/Library/ArmPlatformGlobalVariableLib/PrePi/PrePiArmPlatformGlobalVariableLib.inf b/ArmPlatformPkg/Library/ArmPlatformGlobalVariableLib/PrePi/PrePiArmPlatformGlobalVariableLib.inf
> index 5d3a93fb7207..596f5595412e 100644
> --- a/ArmPlatformPkg/Library/ArmPlatformGlobalVariableLib/PrePi/PrePiArmPlatformGlobalVariableLib.inf
> +++ b/ArmPlatformPkg/Library/ArmPlatformGlobalVariableLib/PrePi/PrePiArmPlatformGlobalVariableLib.inf
> @@ -37,9 +37,10 @@
>    gArmTokenSpaceGuid.PcdFdBaseAddress
>    gArmTokenSpaceGuid.PcdFdSize
>
> -  gArmTokenSpaceGuid.PcdSystemMemoryBase
> -  gArmTokenSpaceGuid.PcdSystemMemorySize
> -
>    gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize
>    gArmPlatformTokenSpaceGuid.PcdPeiGlobalVariableSize
>
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
> index 4b21caa0279e..441f8848e7d9 100755
> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
> @@ -48,8 +48,6 @@
>    gArmTokenSpaceGuid.PcdFdBaseAddress
>    gArmTokenSpaceGuid.PcdFdSize
>
> -  gArmTokenSpaceGuid.PcdSystemMemoryBase
> -  gArmTokenSpaceGuid.PcdSystemMemorySize
>    gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize
>
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
> @@ -62,5 +60,9 @@
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData
>
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
>  [depex]
>    TRUE
> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> index 4821fdc2fa89..587c4b5ce3a3 100755
> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> @@ -103,14 +103,6 @@ InitializeMemory (
>
>    DEBUG ((EFI_D_ERROR, "Memory Init PEIM Loaded\n"));
>
> -  // Ensure PcdSystemMemorySize has been set
> -  ASSERT (FixedPcdGet64 (PcdSystemMemorySize) != 0);
> -
> -  SystemMemoryBase = (UINTN)FixedPcdGet64 (PcdSystemMemoryBase);
> -  SystemMemoryTop = SystemMemoryBase + (UINTN)FixedPcdGet64 (PcdSystemMemorySize);
> -  FdBase = (UINTN)PcdGet32 (PcdFdBaseAddress);
> -  FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);
> -
>    //
>    // Initialize the System Memory (DRAM)
>    //
> @@ -119,6 +111,14 @@ InitializeMemory (
>      ArmPlatformInitializeSystemMemory ();
>    }
>
> +  // Ensure PcdSystemMemorySize has been set
> +  ASSERT (PcdGet64 (PcdSystemMemorySize) != 0);
> +
> +  SystemMemoryBase = (UINTN)PcdGet64 (PcdSystemMemoryBase);
> +  SystemMemoryTop = SystemMemoryBase + (UINTN)PcdGet64 (PcdSystemMemorySize);
> +  FdBase = (UINTN)PcdGet32 (PcdFdBaseAddress);
> +  FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);
> +
>    //
>    // Declare the UEFI memory to PEI
>    //
> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> index 504026e90f6c..9643392cada2 100755
> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> @@ -55,8 +55,6 @@
>    gArmTokenSpaceGuid.PcdFdBaseAddress
>    gArmTokenSpaceGuid.PcdFdSize
>
> -  gArmTokenSpaceGuid.PcdSystemMemoryBase
> -  gArmTokenSpaceGuid.PcdSystemMemorySize
>    gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize
>
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
> @@ -69,5 +67,9 @@
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData
>
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
>  [Depex]
>    TRUE
> diff --git a/ArmPlatformPkg/PrePi/PeiUniCore.inf b/ArmPlatformPkg/PrePi/PeiUniCore.inf
> index 1c0737bdedcb..f8925737ff62 100755
> --- a/ArmPlatformPkg/PrePi/PeiUniCore.inf
> +++ b/ArmPlatformPkg/PrePi/PeiUniCore.inf
> @@ -88,8 +88,6 @@
>
>    gArmPlatformTokenSpaceGuid.PcdPeiGlobalVariableSize
>
> -  gArmTokenSpaceGuid.PcdSystemMemoryBase
> -  gArmTokenSpaceGuid.PcdSystemMemorySize
>    gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize
>
>    gArmPlatformTokenSpaceGuid.PcdCoreCount
> @@ -106,3 +104,7 @@
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
>

I spent several hours pondering this patch (and the next one).

The most important question (for me) concerns the lifecycle of the DTB,
which has been placed by QEMU at 0x4000_0000. In the following I'll
first try to gather a few impressions, then I'll ask my questions.

Impression 1: I peeked forward to v2 7/8 and the virt-specific
ArmPlatformInitializeSystemMemory() seems to set the PCDs, based on the
DTB. Okay.

Impression 2: This series follows "porting case 1" from here:

  http://tianocore.sourceforge.net/wiki/ArmPlatformPkg

and I agree that that choice is correct. We'll use PrePeiCore, not
PrePi.

Impression 3: Also, PcdSystemMemoryInitializeInSec will be FALSE in our
case.

Impression 4: on our platform, the
"ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c" source file will only
be built into the PEIM called "MemoryInit".

Namely, there are two source files in the directory, and two INF files:

- MemoryInitPeiLib.c
- MemoryInitPeiLib.inf
- MemoryInitPeim.c
- MemoryInitPeim.inf

"MemoryInitPeiLib.inf" only uses "MemoryInitPeiLib.c", and builds a
library.

Only the following modules seem to depend on MemoryInitPeiLib:

- ArmPlatformPkg/PrePi/PeiMPCore.inf
- ArmPlatformPkg/PrePi/PeiUniCore.inf

We won't use PrePi. The librarized build of this source directory is
irrelevant for us, which is good.

The PEIM build of this source directory, MemoryInitPeim.inf, depends on
both C files, and we'll include this PEIM, along with PrePeiCore, in our
platform DSC and FDF files in the next patch. Good.

These impressions now allow me to formulate my question about the
lifecycle of the DTB.

There are three "worlds" that we need to investigate wrt. the protection
of the DTB at address 0x4000_0000. I'm using the word "world" and not
"phase", because these world boundaries do not coincide with the UEFI
phase boundaries.

- The first world covers startup (in SEC) and the first half of PEI,
  until permanent PEI memory is installed.
- The second world covers PEI *after* permanent RAM has been installed.
- The third world is DXE and later.

Let's where these worlds border on each other.

At some point during the PEI phase, the PEIM that is the subject of this
patch will be loaded & executed, and then the following happens:

InitializeMemory()                    [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
  ArmPlatformInitializeSystemMemory() [ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVM.c]
    # set PCDs based on DTB
  PeiServicesInstallPeiMemory()       [MdePkg/Library/PeiServicesLib/PeiServicesLib.c]
    PeiInstallPeiMemory()             [MdeModulePkg/Core/Pei/Memory/MemoryServices.c]
      # SwitchStackSignal = TRUE
  MemoryPeim()                        [ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c]
    # create memory resource
    #   descriptor HOBs

PeiInstallPeiMemory() is the boundary between worlds (1) and (2). (Once
PeiInstallPeiMemory() is called, registering the PEI permanent RAM, the
PEI dispatcher will migrate the temporary heap and stack contents to
permanent RAM. (See the leading comment on PeiInstallPeiMemory(), and
grep the tree for SwitchStackSignal.) ArmPlatformPkg's PrePeiCore does
support temporary RAM migration (see gEfiTemporaryRamSupportPpiGuid in
ArmPlatformPkg/PrePeiCore/PrePeiCore.c). But I digress.)

The boundary between worlds (2) and (3) is not visible here, but the way
DXE (and later) phases will work, wrt. the UEFI memory *space* map, is
controlled by the memory resource descriptor HOBs that MemoryPeim()
installs.

(a) My first question is what protects the DTB *before*
PeiServicesInstallPeiMemory() is called; that is, in the "first world".

Before PeiServicesInstallPeiMemory() is called,
- the code runs directly from flash,
- the stack and the heap that the firmware uses are "temporary" (and
  reside likely at some fixed RAM range).

Where is the temporary RAM range located? It must be distinct from the
range the DTB occupies, starting at 0x4000_0000.

(b) Predictably, my second question is about the second world. The
permanent PEI memory must be distinct from the DTB.

InitializeMemory() calculates where to place the permanent PEI memory.
The permanent PEI memory does not cover the entire system RAM. It's
(calculated) base address is UefiMemoryBase, and it's size comes from
PcdSystemMemoryUefiRegionSize (64MB, see the next patch).

InitializeMemory() takes care to place the permanent PEI memory at one
of three spots, in the complete system memory:
- At the top, if the firmware volume has not been shadowed /
  decompressed into main system memory.
- Otherwise, at the very top again, if there's enough room above the
  shadowed firmware volume.
- Otherwise, just below the shadowed firmware volume.

Assumig that the DTB's range, starting at 0x4000_0000, will be itself
covered by the full system RAM range, I can't see any *guarantee* (just
a high probability), that the above permanent PEI RAM placement logic
will not intersect with the DTB.

(c) Third, before we enter DXE (and after we installed permanent PEI
memory), we must ensure that DXE won't trample on the DTB.

This is possible in two ways. First, don't install a memory resource
descriptor HOB in MemoryPeim() that would cover the DTB. (This is very
unlikely to work if the DTB's range is otherwise covered by the full
system memory, as reported in the DTB's own self.)

Second (the canonical way), create a memory allocation HOB (with the
BuildMemoryAllocationHob() function of HobLib) that covers the DTB. This
way, DXE will create a memory *space* map (== hw properties map), based
on MemoryPeim()'s memory resource descriptor HOBs, that covers the DTB;
but seeing the memory allocation HOB, DXE will also start with a UEFI
memory map (== sw properties map) that already lists the DTB as covered.
This way DXE will steer clear of that memory range.

None of these two seems to happen anywhere. I believe that your testing
got lucky, because all generic memory allocations in edk2 prefer high
addresses, so the DTB, at the bottom of the physical memory, survived by
sheer luck.

Only if all three "worlds" guarantee the protection of the DTB, can you
rely on the DTB's integrity in some DXE_DRIVER module. (But, if that
protection is there, you don't need to copy the DTB at all, you can
directly link it into the configuration table.)

Summary:
(a) please prove, with specific addresses, that the temporary RAM range,
    used in SEC and the first half of PEI, is distinct from the
    pre-placed DTB.
(b) please prove, with specific range sizes, and by walking me through
    the exact branches in InitializeMemory(), that the permanent PEI RAM
    will not overlap the pre-placed DTB.
(c) please find a good spot in some PEIM -- that runs on permanent PEI
    RAM -- for a BuildMemoryAllocationHob() call.

(I'll note that this is the reason why downloading the DTB would be
*much* easier from an fw_cfg-like ioport or MMIO register. You simply
wouldn't advertise that MMIO register in QEMU's DTB as part of the
system memory. Consequently, none of the above worlds would have a
chance to trample over it, auto-protecting the register. Finally, the
MMIO register would always be available to fetch and re-fetch the DTB
from.)

Thanks
Laszlo

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/

On 27 August 2014 00:24, Laszlo Ersek <lersek@redhat.com> wrote:
> On 08/26/14 15:03, Ard Biesheuvel wrote:
>> This changes the definition and a bunch of references to
>> gArmTokenSpaceGuid.PcdSystemMemoryBase and
>> gArmTokenSpaceGuid.PcdSystemMemorySize so they can be declared as dynamic PCDs
>> by the platform. Also, move the non-SEC call to
>> ArmPlatformInitializeSystemMemory() earlier, so a platform has a chance to set
>> these PCDs before they are first referenced.
>>
>> The purpose is allowing dynamically instantiated virtual machines to declare
>> the system memory by passing a device tree.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
[...]
>
> I spent several hours pondering this patch (and the next one).
>
> The most important question (for me) concerns the lifecycle of the DTB,
> which has been placed by QEMU at 0x4000_0000. In the following I'll
> first try to gather a few impressions, then I'll ask my questions.
>

First of all, let me clarify that I was aware that this range needed
some protection in some way, and I was currently getting away with the
DTB surviving by luck.
Ideally, the install configuration table should be performed as early
as possible, and the subsequent references of the device tree should
find it using the config table GUID.
But before that, we need something else, indeed.

So you mention that passing it in DRAM is perhaps not the best way to
go about this. Could you elaborate a bit
> Impression 1: I peeked forward to v2 7/8 and the virt-specific
> ArmPlatformInitializeSystemMemory() seems to set the PCDs, based on the
> DTB. Okay.
>
> Impression 2: This series follows "porting case 1" from here:
>
>   http://tianocore.sourceforge.net/wiki/ArmPlatformPkg
>
> and I agree that that choice is correct. We'll use PrePeiCore, not
> PrePi.
>
> Impression 3: Also, PcdSystemMemoryInitializeInSec will be FALSE in our
> case.
>
> Impression 4: on our platform, the
> "ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c" source file will only
> be built into the PEIM called "MemoryInit".
>
> Namely, there are two source files in the directory, and two INF files:
>
> - MemoryInitPeiLib.c
> - MemoryInitPeiLib.inf
> - MemoryInitPeim.c
> - MemoryInitPeim.inf
>
> "MemoryInitPeiLib.inf" only uses "MemoryInitPeiLib.c", and builds a
> library.
>
> Only the following modules seem to depend on MemoryInitPeiLib:
>
> - ArmPlatformPkg/PrePi/PeiMPCore.inf
> - ArmPlatformPkg/PrePi/PeiUniCore.inf
>
> We won't use PrePi. The librarized build of this source directory is
> irrelevant for us, which is good.
>
> The PEIM build of this source directory, MemoryInitPeim.inf, depends on
> both C files, and we'll include this PEIM, along with PrePeiCore, in our
> platform DSC and FDF files in the next patch. Good.
>

Check.

> These impressions now allow me to formulate my question about the
> lifecycle of the DTB.
>
> There are three "worlds" that we need to investigate wrt. the protection
> of the DTB at address 0x4000_0000. I'm using the word "world" and not
> "phase", because these world boundaries do not coincide with the UEFI
> phase boundaries.
>
> - The first world covers startup (in SEC) and the first half of PEI,
>   until permanent PEI memory is installed.
> - The second world covers PEI *after* permanent RAM has been installed.
> - The third world is DXE and later.
>
> Let's where these worlds border on each other.
>
> At some point during the PEI phase, the PEIM that is the subject of this
> patch will be loaded & executed, and then the following happens:
>
> InitializeMemory()                    [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>   ArmPlatformInitializeSystemMemory() [ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVM.c]
>     # set PCDs based on DTB
>   PeiServicesInstallPeiMemory()       [MdePkg/Library/PeiServicesLib/PeiServicesLib.c]
>     PeiInstallPeiMemory()             [MdeModulePkg/Core/Pei/Memory/MemoryServices.c]
>       # SwitchStackSignal = TRUE
>   MemoryPeim()                        [ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c]
>     # create memory resource
>     #   descriptor HOBs
>
> PeiInstallPeiMemory() is the boundary between worlds (1) and (2). (Once
> PeiInstallPeiMemory() is called, registering the PEI permanent RAM, the
> PEI dispatcher will migrate the temporary heap and stack contents to
> permanent RAM. (See the leading comment on PeiInstallPeiMemory(), and
> grep the tree for SwitchStackSignal.) ArmPlatformPkg's PrePeiCore does
> support temporary RAM migration (see gEfiTemporaryRamSupportPpiGuid in
> ArmPlatformPkg/PrePeiCore/PrePeiCore.c). But I digress.)
>
> The boundary between worlds (2) and (3) is not visible here, but the way
> DXE (and later) phases will work, wrt. the UEFI memory *space* map, is
> controlled by the memory resource descriptor HOBs that MemoryPeim()
> installs.
>
> (a) My first question is what protects the DTB *before*
> PeiServicesInstallPeiMemory() is called; that is, in the "first world".
>
> Before PeiServicesInstallPeiMemory() is called,
> - the code runs directly from flash,
> - the stack and the heap that the firmware uses are "temporary" (and
>   reside likely at some fixed RAM range).
>
> Where is the temporary RAM range located? It must be distinct from the
> range the DTB occupies, starting at 0x4000_0000.
>

The presence of at least 1 MB of DRAM at 0x4000_0000 is assumed, and
it is divided into a lower range for the DTB, and an upper range for
the temporary stack and heap.

I.e., ArmPlatformPkg/PrePeiCore/MainUniCore.c references the following PCDs:

  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000
  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000

which it uses for a temp stack at the top and a temp heap at the
bottom. This leaves plenty of room for the DTB.

> (b) Predictably, my second question is about the second world. The
> permanent PEI memory must be distinct from the DTB.
>
> InitializeMemory() calculates where to place the permanent PEI memory.
> The permanent PEI memory does not cover the entire system RAM. It's
> (calculated) base address is UefiMemoryBase, and it's size comes from
> PcdSystemMemoryUefiRegionSize (64MB, see the next patch).
>

Yes, so running with 64 MB will result in this region colliding with the DTB.

> InitializeMemory() takes care to place the permanent PEI memory at one
> of three spots, in the complete system memory:
> - At the top, if the firmware volume has not been shadowed /
>   decompressed into main system memory.
> - Otherwise, at the very top again, if there's enough room above the
>   shadowed firmware volume.
> - Otherwise, just below the shadowed firmware volume.
>
> Assumig that the DTB's range, starting at 0x4000_0000, will be itself
> covered by the full system RAM range, I can't see any *guarantee* (just
> a high probability), that the above permanent PEI RAM placement logic
> will not intersect with the DTB.
>

True.

> (c) Third, before we enter DXE (and after we installed permanent PEI
> memory), we must ensure that DXE won't trample on the DTB.
>
> This is possible in two ways. First, don't install a memory resource
> descriptor HOB in MemoryPeim() that would cover the DTB. (This is very
> unlikely to work if the DTB's range is otherwise covered by the full
> system memory, as reported in the DTB's own self.)
>
> Second (the canonical way), create a memory allocation HOB (with the
> BuildMemoryAllocationHob() function of HobLib) that covers the DTB. This
> way, DXE will create a memory *space* map (== hw properties map), based
> on MemoryPeim()'s memory resource descriptor HOBs, that covers the DTB;
> but seeing the memory allocation HOB, DXE will also start with a UEFI
> memory map (== sw properties map) that already lists the DTB as covered.
> This way DXE will steer clear of that memory range.
>

But this means the range will turn up as reserved once we boot the
kernel, and this is undesirable as well. So I would really prefer to
reallocate to the top of DRAM.

> None of these two seems to happen anywhere. I believe that your testing
> got lucky, because all generic memory allocations in edk2 prefer high
> addresses, so the DTB, at the bottom of the physical memory, survived by
> sheer luck.
>

True

> Only if all three "worlds" guarantee the protection of the DTB, can you
> rely on the DTB's integrity in some DXE_DRIVER module. (But, if that
> protection is there, you don't need to copy the DTB at all, you can
> directly link it into the configuration table.)
>


> Summary:
> (a) please prove, with specific addresses, that the temporary RAM range,
>     used in SEC and the first half of PEI, is distinct from the
>     pre-placed DTB.
> (b) please prove, with specific range sizes, and by walking me through
>     the exact branches in InitializeMemory(), that the permanent PEI RAM
>     will not overlap the pre-placed DTB.
> (c) please find a good spot in some PEIM -- that runs on permanent PEI
>     RAM -- for a BuildMemoryAllocationHob() call.
>

I will try to follow up with a v3 that relocates the DTB as early as
possible. What would be the best way to communicate the new offset?
Another dynamic PCD?

> (I'll note that this is the reason why downloading the DTB would be
> *much* easier from an fw_cfg-like ioport or MMIO register. You simply
> wouldn't advertise that MMIO register in QEMU's DTB as part of the
> system memory. Consequently, none of the above worlds would have a
> chance to trample over it, auto-protecting the register. Finally, the
> MMIO register would always be available to fetch and re-fetch the DTB
> from.)
>

I will take this up internally.

Many thanks for the thorough review, very much appreciated!

Cheers,
Ard.

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/

On 08/27/14 09:10, Ard Biesheuvel wrote:
> On 27 August 2014 00:24, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 08/26/14 15:03, Ard Biesheuvel wrote:
>>> This changes the definition and a bunch of references to
>>> gArmTokenSpaceGuid.PcdSystemMemoryBase and
>>> gArmTokenSpaceGuid.PcdSystemMemorySize so they can be declared as dynamic PCDs
>>> by the platform. Also, move the non-SEC call to
>>> ArmPlatformInitializeSystemMemory() earlier, so a platform has a chance to set
>>> these PCDs before they are first referenced.
>>>
>>> The purpose is allowing dynamically instantiated virtual machines to declare
>>> the system memory by passing a device tree.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
> [...]
>>
>> I spent several hours pondering this patch (and the next one).
>>
>> The most important question (for me) concerns the lifecycle of the DTB,
>> which has been placed by QEMU at 0x4000_0000. In the following I'll
>> first try to gather a few impressions, then I'll ask my questions.
>>
> 
> First of all, let me clarify that I was aware that this range needed
> some protection in some way, and I was currently getting away with the
> DTB surviving by luck.

Alright, good to know that you knew, thanks!

> Ideally, the install configuration table should be performed as early
> as possible, and the subsequent references of the device tree should
> find it using the config table GUID.
> But before that, we need something else, indeed.

Right. InstallConfigurationTable() is a UEFI boot service, you can't use
it before DXE. In addition, that service really only adds a (GUID,
pointer) pair to the list of config tables, so the pointed-to area must
be preserved / secured even in DXE by separate means.

> So you mention that passing it in DRAM is perhaps not the best way to
> go about this. Could you elaborate a bit

Sure. Peter didn't like this (for ARM / mach virt), but the fw_cfg
mechanism seen on the x86_64 target would be one alternative. Basically
you have two ioports, one for writing selector values into, and another
to read data from. ARM doesn't have ioports, but this could be done with
MIIO registers as well. (The guest code would do volatile reads and writes.)

Since this would "host" the DTB in something different than emulated
system RAM, the DTB could not, by nature, overlap with any part of the
system RAM.

The current approach is workable as well (ie. DTB copied by QEMU into
guest RAM upfront), it just needs more protection in the guest fw code.

[snip]

>> (a) My first question is what protects the DTB *before*
>> PeiServicesInstallPeiMemory() is called; that is, in the "first world".
>>
>> Before PeiServicesInstallPeiMemory() is called,
>> - the code runs directly from flash,
>> - the stack and the heap that the firmware uses are "temporary" (and
>>   reside likely at some fixed RAM range).
>>
>> Where is the temporary RAM range located? It must be distinct from the
>> range the DTB occupies, starting at 0x4000_0000.
>>
> 
> The presence of at least 1 MB of DRAM at 0x4000_0000 is assumed,

Okay.

> and
> it is divided into a lower range for the DTB, and an upper range for
> the temporary stack and heap.
> 
> I.e., ArmPlatformPkg/PrePeiCore/MainUniCore.c references the following PCDs:
> 
>   gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000
>   gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000
> 
> which it uses for a temp stack at the top and a temp heap at the
> bottom. This leaves plenty of room for the DTB.

Good answer, this is what I was looking for, thanks. In
ArmPlatformPkg/PrePeiCore/, there are two INF files,
PrePeiCoreMPCore.inf and PrePeiCoreUniCore.inf, they are both of module
type SEC, and indeed the above PCDs are used for determining the
temporary RAM range (in PrimaryMain(), SecCoreData.XXXX). OK.

> 
>> (b) Predictably, my second question is about the second world. The
>> permanent PEI memory must be distinct from the DTB.
>>
>> InitializeMemory() calculates where to place the permanent PEI memory.
>> The permanent PEI memory does not cover the entire system RAM. It's
>> (calculated) base address is UefiMemoryBase, and it's size comes from
>> PcdSystemMemoryUefiRegionSize (64MB, see the next patch).
>>
> 
> Yes, so running with 64 MB will result in this region colliding with the DTB.

Right.

Note that this question can be short-circuited by a list of guarantees,
for example:
- the firmware volume will always (or never) be shadowed from flash to
  system RAM (--> because this helps us see which outer branch in
  InitializeMemory() will be taken),
- and we'll never boot a guest with less than 128 MB RAM (--> this
  determines the inner branch, and given the size of the firmware
  volume, allows deducing the exact location of the permanent PEI RAM)

If we can guarantee such things, I'm completely fine with that as an answer.

>> InitializeMemory() takes care to place the permanent PEI memory at one
>> of three spots, in the complete system memory:
>> - At the top, if the firmware volume has not been shadowed /
>>   decompressed into main system memory.
>> - Otherwise, at the very top again, if there's enough room above the
>>   shadowed firmware volume.
>> - Otherwise, just below the shadowed firmware volume.
>>
>> Assumig that the DTB's range, starting at 0x4000_0000, will be itself
>> covered by the full system RAM range, I can't see any *guarantee* (just
>> a high probability), that the above permanent PEI RAM placement logic
>> will not intersect with the DTB.
>>
> 
> True.
> 
>> (c) Third, before we enter DXE (and after we installed permanent PEI
>> memory), we must ensure that DXE won't trample on the DTB.
>>
>> This is possible in two ways. First, don't install a memory resource
>> descriptor HOB in MemoryPeim() that would cover the DTB. (This is very
>> unlikely to work if the DTB's range is otherwise covered by the full
>> system memory, as reported in the DTB's own self.)
>>
>> Second (the canonical way), create a memory allocation HOB (with the
>> BuildMemoryAllocationHob() function of HobLib) that covers the DTB. This
>> way, DXE will create a memory *space* map (== hw properties map), based
>> on MemoryPeim()'s memory resource descriptor HOBs, that covers the DTB;
>> but seeing the memory allocation HOB, DXE will also start with a UEFI
>> memory map (== sw properties map) that already lists the DTB as covered.
>> This way DXE will steer clear of that memory range.
>>
> 
> But this means the range will turn up as reserved once we boot the
> kernel, and this is undesirable as well. So I would really prefer to
> reallocate to the top of DRAM.

No, not as reserved.

BuildMemoryAllocationHob() takes a base address, a size, and an EFI
memory type. From "MdePkg/Include/Library/HobLib.h":

--------------
/**
  Builds a HOB for the memory allocation.

  This function builds a HOB for the memory allocation.
  It can only be invoked during PEI phase;
  for DXE phase, it will ASSERT() since PEI HOB is read-only for DXE
  phase.

  If there is no additional space for HOB creation, then ASSERT().

  @param  BaseAddress   The 64 bit physical address of the memory.
  @param  Length        The length of the memory allocation in bytes.
  @param  MemoryType    Type of memory allocated by this HOB.

**/
VOID
EFIAPI
BuildMemoryAllocationHob (
  IN EFI_PHYSICAL_ADDRESS        BaseAddress,
  IN UINT64                      Length,
  IN EFI_MEMORY_TYPE             MemoryType
  );
--------------

We can, and should, pass EfiBootServicesData as MemoryType. Then DXE
would leave this range alone, as said before, and the range would show
up as boot services code in the UEFI memmap that the kernel's EFI stub
(or grub, etc) fetches. The range would be released and reused during OS
runtime.

(Of course that release / reuse in the EFI stub has to be coordinated
with accessing the DTB through the EFI config table, but this
coordination must be present *already* in the EFI stub. Because, right
now in patch v2 7/8, you allocate FdtImageData as EfiBootServicesData
anyway, with AllocatePages(). For the EFI stub of the kernel, there's no
difference really, the difference is how DXE sees things.)

In short,

  EFI_PHYSICAL_ADDRESS Base;
  UINTN                FdtSize;

  Base = PcdGet64 (PcdSystemMemoryBase);
  FdtSize = fdt_totalsize ((VOID *)(UINTN)Base);

  BuildMemoryAllocationHob (
    Base,
    EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (FdtSize)),
    EfiBootServicesData
    );

would be correct. No need for relocation or for a later AllocatePages()
call. This would answer the third question, and you could link the DTB
at 0x4000_0000 into the config table directly.

We just need to find a good spot for this code -- it must run in a PEIM,
after permanent PEI RAM has been installed.

[snip]

>> Summary:
>> (a) please prove, with specific addresses, that the temporary RAM range,
>>     used in SEC and the first half of PEI, is distinct from the
>>     pre-placed DTB.
>> (b) please prove, with specific range sizes, and by walking me through
>>     the exact branches in InitializeMemory(), that the permanent PEI RAM
>>     will not overlap the pre-placed DTB.
>> (c) please find a good spot in some PEIM -- that runs on permanent PEI
>>     RAM -- for a BuildMemoryAllocationHob() call.
>>
> 
> I will try to follow up with a v3 that relocates the DTB as early as
> possible. What would be the best way to communicate the new offset?
> Another dynamic PCD?

Let's not relocate it. :) If I understand correctly, your main concern
with BuildMemoryAllocationHob() was that it would reserve the memory
area even during OS runtime. This is not the case, see above.

>> (I'll note that this is the reason why downloading the DTB would be
>> *much* easier from an fw_cfg-like ioport or MMIO register. You simply
>> wouldn't advertise that MMIO register in QEMU's DTB as part of the
>> system memory. Consequently, none of the above worlds would have a
>> chance to trample over it, auto-protecting the register. Finally, the
>> MMIO register would always be available to fetch and re-fetch the DTB
>> from.)
>>
> 
> I will take this up internally.

Yeah, this is still an option, if you can convince Peter to add some
fw_cfg-like MMIO registers to mach virt in QEMU :)

But, again, Q1 has been answered, Q2 can be answered simply by
guaranteeing a few constants, and Q3 can be covered by a small snippet
of code, which just has to be inserted in a good spot.

Not sure if in ARM world there are "platform PEIMs". In OVMF we have
such BuildMemoryAllocationHob() calls in OvmfPkg/PlatformPei/ (which
take place after permanent PEI RAM gets installed; same as discussed above).

According to patch v2 7/8, this is our list of PEIMs:

+  APRIORI PEI {
+    INF  MdeModulePkg/Universal/PCD/Pei/Pcd.inf
+  }
+  INF ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
+  INF MdeModulePkg/Core/Pei/PeiMain.inf
+  INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
+  INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
+  INF ArmPkg/Drivers/CpuPei/CpuPei.inf
+  INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
+  INF IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf
+  INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf

So, yes, there is a platform PEIM:

  ArmPlatformPkg/PlatformPei/PlatformPeim.inf

The by-the-book way would be to
- create a *deep copy* of this module, under
ArmPlatformPkg/AArch64VirtualizationPkg/,
- reference that copy in the new DSC and FDF file(s),
- and add the above code snippet to the PlatformPeim() function.

(This could be made more lightweight by exposing PlatformPeim() as a
standalone library class in ARM, and then you wouldn't need to copy the
entire ArmPlatformPkg/PlatformPei directory to
AArch64VirtualizationPkg/, just provide a new library instance.)

Another, independent possibility would be to introduce a FeaturePcd
(called PcdReserveFdtRange or some such), and dependent on that, run the
code snippet in "ArmPlatformPkg/PlatformPei/PlatformPeiLib.c" directly.
This is probably the lightest solution, but it would take the most
convincing with Olivier :)

*** In addition, I *think* that the platform PEIM's [Depex] section
should spell out a [Depex] on gEfiPeiMemoryDiscoveredPpiGuid, if the
platform PEIM calls BuildMemoryAllocationHob().

"ArmPkg/Drivers/CpuPei/CpuPei.inf" (listed above) does so already, for
example. (Likely for other reasons; I'm just saying there's already a
Depex example one could copy.)

The presence of this PPI guarantees that the PEIM runs after permanent
PEI RAM has been installed. Refer to:
- "MdePkg/Include/Ppi/MemoryDiscovered.h",
- Chapter 11, PEI Physical Memory Usage, in Vol 1 of the PI spec.
  (EFI_PEI_PERMANENT_MEMORY_INSTALLED_PPI)

In OVMF's PlatformPei, we don't depend on
gEfiPeiMemoryDiscoveredPpiGuid, but for a good reason: we install the
permanent PEI RAM and build the memalloc HOBs in that *same* PEIM, hence
we can ensure their correct ordering easily.

I'll ask for confirmation on the list separately.

> Many thanks for the thorough review, very much appreciated!

Many thanks for writing this code :)

Cheers,
Laszlo

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/

On 27 August 2014 11:07, Laszlo Ersek <lersek@redhat.com> wrote:
> On 08/27/14 09:10, Ard Biesheuvel wrote:
>> On 27 August 2014 00:24, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 08/26/14 15:03, Ard Biesheuvel wrote:
>>>> This changes the definition and a bunch of references to
>>>> gArmTokenSpaceGuid.PcdSystemMemoryBase and
>>>> gArmTokenSpaceGuid.PcdSystemMemorySize so they can be declared as dynamic PCDs
>>>> by the platform. Also, move the non-SEC call to
>>>> ArmPlatformInitializeSystemMemory() earlier, so a platform has a chance to set
>>>> these PCDs before they are first referenced.
>>>>
>>>> The purpose is allowing dynamically instantiated virtual machines to declare
>>>> the system memory by passing a device tree.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>> [...]
>>>
>>> I spent several hours pondering this patch (and the next one).
>>>
>>> The most important question (for me) concerns the lifecycle of the DTB,
>>> which has been placed by QEMU at 0x4000_0000. In the following I'll
>>> first try to gather a few impressions, then I'll ask my questions.
>>>
>>
>> First of all, let me clarify that I was aware that this range needed
>> some protection in some way, and I was currently getting away with the
>> DTB surviving by luck.
>
> Alright, good to know that you knew, thanks!
>
>> Ideally, the install configuration table should be performed as early
>> as possible, and the subsequent references of the device tree should
>> find it using the config table GUID.
>> But before that, we need something else, indeed.
>
> Right. InstallConfigurationTable() is a UEFI boot service, you can't use
> it before DXE. In addition, that service really only adds a (GUID,
> pointer) pair to the list of config tables, so the pointed-to area must
> be preserved / secured even in DXE by separate means.
>
>> So you mention that passing it in DRAM is perhaps not the best way to
>> go about this. Could you elaborate a bit
>
> Sure. Peter didn't like this (for ARM / mach virt), but the fw_cfg
> mechanism seen on the x86_64 target would be one alternative. Basically
> you have two ioports, one for writing selector values into, and another
> to read data from. ARM doesn't have ioports, but this could be done with
> MIIO registers as well. (The guest code would do volatile reads and writes.)
>
> Since this would "host" the DTB in something different than emulated
> system RAM, the DTB could not, by nature, overlap with any part of the
> system RAM.
>
> The current approach is workable as well (ie. DTB copied by QEMU into
> guest RAM upfront), it just needs more protection in the guest fw code.
>

OK, let's keep that as a working assumption then, at least for now ...

> [snip]
>
>>> (a) My first question is what protects the DTB *before*
>>> PeiServicesInstallPeiMemory() is called; that is, in the "first world".
>>>
>>> Before PeiServicesInstallPeiMemory() is called,
>>> - the code runs directly from flash,
>>> - the stack and the heap that the firmware uses are "temporary" (and
>>>   reside likely at some fixed RAM range).
>>>
>>> Where is the temporary RAM range located? It must be distinct from the
>>> range the DTB occupies, starting at 0x4000_0000.
>>>
>>
>> The presence of at least 1 MB of DRAM at 0x4000_0000 is assumed,
>
> Okay.
>
>> and
>> it is divided into a lower range for the DTB, and an upper range for
>> the temporary stack and heap.
>>
>> I.e., ArmPlatformPkg/PrePeiCore/MainUniCore.c references the following PCDs:
>>
>>   gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000
>>   gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000
>>
>> which it uses for a temp stack at the top and a temp heap at the
>> bottom. This leaves plenty of room for the DTB.
>
> Good answer, this is what I was looking for, thanks. In
> ArmPlatformPkg/PrePeiCore/, there are two INF files,
> PrePeiCoreMPCore.inf and PrePeiCoreUniCore.inf, they are both of module
> type SEC, and indeed the above PCDs are used for determining the
> temporary RAM range (in PrimaryMain(), SecCoreData.XXXX). OK.
>
>>
>>> (b) Predictably, my second question is about the second world. The
>>> permanent PEI memory must be distinct from the DTB.
>>>
>>> InitializeMemory() calculates where to place the permanent PEI memory.
>>> The permanent PEI memory does not cover the entire system RAM. It's
>>> (calculated) base address is UefiMemoryBase, and it's size comes from
>>> PcdSystemMemoryUefiRegionSize (64MB, see the next patch).
>>>
>>
>> Yes, so running with 64 MB will result in this region colliding with the DTB.
>
> Right.
>
> Note that this question can be short-circuited by a list of guarantees,
> for example:
> - the firmware volume will always (or never) be shadowed from flash to
>   system RAM (--> because this helps us see which outer branch in
>   InitializeMemory() will be taken),
> - and we'll never boot a guest with less than 128 MB RAM (--> this
>   determines the inner branch, and given the size of the firmware
>   volume, allows deducing the exact location of the permanent PEI RAM)
>
> If we can guarantee such things, I'm completely fine with that as an answer.
>

OK, so by shadowing in this context, you mean invoking QEMU in such a
way that the UEFI image is preloaded into DRAM? I don't think there is
any point at all in doing that, so I don't see why we should support
it. And having at least 128 MB of DRAM is reasonable as well.

So I could perhaps enforce these rules in the same place the system
DRAM is discovered from the DTB?

>>> InitializeMemory() takes care to place the permanent PEI memory at one
>>> of three spots, in the complete system memory:
>>> - At the top, if the firmware volume has not been shadowed /
>>>   decompressed into main system memory.
>>> - Otherwise, at the very top again, if there's enough room above the
>>>   shadowed firmware volume.
>>> - Otherwise, just below the shadowed firmware volume.
>>>
>>> Assumig that the DTB's range, starting at 0x4000_0000, will be itself
>>> covered by the full system RAM range, I can't see any *guarantee* (just
>>> a high probability), that the above permanent PEI RAM placement logic
>>> will not intersect with the DTB.
>>>
>>
>> True.
>>
>>> (c) Third, before we enter DXE (and after we installed permanent PEI
>>> memory), we must ensure that DXE won't trample on the DTB.
>>>
>>> This is possible in two ways. First, don't install a memory resource
>>> descriptor HOB in MemoryPeim() that would cover the DTB. (This is very
>>> unlikely to work if the DTB's range is otherwise covered by the full
>>> system memory, as reported in the DTB's own self.)
>>>
>>> Second (the canonical way), create a memory allocation HOB (with the
>>> BuildMemoryAllocationHob() function of HobLib) that covers the DTB. This
>>> way, DXE will create a memory *space* map (== hw properties map), based
>>> on MemoryPeim()'s memory resource descriptor HOBs, that covers the DTB;
>>> but seeing the memory allocation HOB, DXE will also start with a UEFI
>>> memory map (== sw properties map) that already lists the DTB as covered.
>>> This way DXE will steer clear of that memory range.
>>>
>>
>> But this means the range will turn up as reserved once we boot the
>> kernel, and this is undesirable as well. So I would really prefer to
>> reallocate to the top of DRAM.
>
> No, not as reserved.
>
> BuildMemoryAllocationHob() takes a base address, a size, and an EFI
> memory type. From "MdePkg/Include/Library/HobLib.h":
>
> --------------
> /**
>   Builds a HOB for the memory allocation.
>
>   This function builds a HOB for the memory allocation.
>   It can only be invoked during PEI phase;
>   for DXE phase, it will ASSERT() since PEI HOB is read-only for DXE
>   phase.
>
>   If there is no additional space for HOB creation, then ASSERT().
>
>   @param  BaseAddress   The 64 bit physical address of the memory.
>   @param  Length        The length of the memory allocation in bytes.
>   @param  MemoryType    Type of memory allocated by this HOB.
>
> **/
> VOID
> EFIAPI
> BuildMemoryAllocationHob (
>   IN EFI_PHYSICAL_ADDRESS        BaseAddress,
>   IN UINT64                      Length,
>   IN EFI_MEMORY_TYPE             MemoryType
>   );
> --------------
>
> We can, and should, pass EfiBootServicesData as MemoryType. Then DXE
> would leave this range alone, as said before, and the range would show
> up as boot services code in the UEFI memmap that the kernel's EFI stub
> (or grub, etc) fetches. The range would be released and reused during OS
> runtime.
>
> (Of course that release / reuse in the EFI stub has to be coordinated
> with accessing the DTB through the EFI config table, but this
> coordination must be present *already* in the EFI stub. Because, right
> now in patch v2 7/8, you allocate FdtImageData as EfiBootServicesData
> anyway, with AllocatePages(). For the EFI stub of the kernel, there's no
> difference really, the difference is how DXE sees things.)
>
> In short,
>
>   EFI_PHYSICAL_ADDRESS Base;
>   UINTN                FdtSize;
>
>   Base = PcdGet64 (PcdSystemMemoryBase);
>   FdtSize = fdt_totalsize ((VOID *)(UINTN)Base);
>
>   BuildMemoryAllocationHob (
>     Base,
>     EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (FdtSize)),
>     EfiBootServicesData
>     );
>
> would be correct. No need for relocation or for a later AllocatePages()
> call. This would answer the third question, and you could link the DTB
> at 0x4000_0000 into the config table directly.
>

Indeed.

The only problem is the following: the EFI stub relocates the DTB to
somewhere higher up in memory (it needs to add and modify entries, and
doing so in place is not feasible)
Then, it uses AllocatePages() to find the lowest 2 MB aligned region
of memory to relocate the kernel Image itself, which should ideally be
the base of DRAM. However, with the DTB located there, and protected
by the HOB, it will disregard the base of DRAM and move the kernel
Image 2 MB up instead.

> We just need to find a good spot for this code -- it must run in a PEIM,
> after permanent PEI RAM has been installed.
>
> [snip]
>
>>> Summary:
>>> (a) please prove, with specific addresses, that the temporary RAM range,
>>>     used in SEC and the first half of PEI, is distinct from the
>>>     pre-placed DTB.
>>> (b) please prove, with specific range sizes, and by walking me through
>>>     the exact branches in InitializeMemory(), that the permanent PEI RAM
>>>     will not overlap the pre-placed DTB.
>>> (c) please find a good spot in some PEIM -- that runs on permanent PEI
>>>     RAM -- for a BuildMemoryAllocationHob() call.
>>>
>>
>> I will try to follow up with a v3 that relocates the DTB as early as
>> possible. What would be the best way to communicate the new offset?
>> Another dynamic PCD?
>
> Let's not relocate it. :) If I understand correctly, your main concern
> with BuildMemoryAllocationHob() was that it would reserve the memory
> area even during OS runtime. This is not the case, see above.
>

Not entirely, see above

>>> (I'll note that this is the reason why downloading the DTB would be
>>> *much* easier from an fw_cfg-like ioport or MMIO register. You simply
>>> wouldn't advertise that MMIO register in QEMU's DTB as part of the
>>> system memory. Consequently, none of the above worlds would have a
>>> chance to trample over it, auto-protecting the register. Finally, the
>>> MMIO register would always be available to fetch and re-fetch the DTB
>>> from.)
>>>
>>
>> I will take this up internally.
>
> Yeah, this is still an option, if you can convince Peter to add some
> fw_cfg-like MMIO registers to mach virt in QEMU :)
>
> But, again, Q1 has been answered, Q2 can be answered simply by
> guaranteeing a few constants, and Q3 can be covered by a small snippet
> of code, which just has to be inserted in a good spot.
>
> Not sure if in ARM world there are "platform PEIMs". In OVMF we have
> such BuildMemoryAllocationHob() calls in OvmfPkg/PlatformPei/ (which
> take place after permanent PEI RAM gets installed; same as discussed above).
>
> According to patch v2 7/8, this is our list of PEIMs:
>
> +  APRIORI PEI {
> +    INF  MdeModulePkg/Universal/PCD/Pei/Pcd.inf
> +  }
> +  INF ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> +  INF MdeModulePkg/Core/Pei/PeiMain.inf
> +  INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
> +  INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> +  INF ArmPkg/Drivers/CpuPei/CpuPei.inf
> +  INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
> +  INF IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf
> +  INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>
> So, yes, there is a platform PEIM:
>
>   ArmPlatformPkg/PlatformPei/PlatformPeim.inf
>
> The by-the-book way would be to
> - create a *deep copy* of this module, under
> ArmPlatformPkg/AArch64VirtualizationPkg/,
> - reference that copy in the new DSC and FDF file(s),
> - and add the above code snippet to the PlatformPeim() function.
>
> (This could be made more lightweight by exposing PlatformPeim() as a
> standalone library class in ARM, and then you wouldn't need to copy the
> entire ArmPlatformPkg/PlatformPei directory to
> AArch64VirtualizationPkg/, just provide a new library instance.)
>
> Another, independent possibility would be to introduce a FeaturePcd
> (called PcdReserveFdtRange or some such), and dependent on that, run the
> code snippet in "ArmPlatformPkg/PlatformPei/PlatformPeiLib.c" directly.
> This is probably the lightest solution, but it would take the most
> convincing with Olivier :)
>
> *** In addition, I *think* that the platform PEIM's [Depex] section
> should spell out a [Depex] on gEfiPeiMemoryDiscoveredPpiGuid, if the
> platform PEIM calls BuildMemoryAllocationHob().
>
> "ArmPkg/Drivers/CpuPei/CpuPei.inf" (listed above) does so already, for
> example. (Likely for other reasons; I'm just saying there's already a
> Depex example one could copy.)
>
> The presence of this PPI guarantees that the PEIM runs after permanent
> PEI RAM has been installed. Refer to:
> - "MdePkg/Include/Ppi/MemoryDiscovered.h",
> - Chapter 11, PEI Physical Memory Usage, in Vol 1 of the PI spec.
>   (EFI_PEI_PERMANENT_MEMORY_INSTALLED_PPI)
>
> In OVMF's PlatformPei, we don't depend on
> gEfiPeiMemoryDiscoveredPpiGuid, but for a good reason: we install the
> permanent PEI RAM and build the memalloc HOBs in that *same* PEIM, hence
> we can ensure their correct ordering easily.
>
> I'll ask for confirmation on the list separately.
>

So even if we do end up relocating the DTB (which I expect), I assume
this would still be the place to implement that?
If so, I will go and implement the PlatformPeim() while we continue
the discussion regarding the relocation.

On 08/27/14 11:35, Ard Biesheuvel wrote:
> On 27 August 2014 11:07, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 08/27/14 09:10, Ard Biesheuvel wrote:
>>> On 27 August 2014 00:24, Laszlo Ersek <lersek@redhat.com> wrote:

>>>> (b) Predictably, my second question is about the second world. The
>>>> permanent PEI memory must be distinct from the DTB.
>>>>
>>>> InitializeMemory() calculates where to place the permanent PEI memory.
>>>> The permanent PEI memory does not cover the entire system RAM. It's
>>>> (calculated) base address is UefiMemoryBase, and it's size comes from
>>>> PcdSystemMemoryUefiRegionSize (64MB, see the next patch).
>>>>
>>>
>>> Yes, so running with 64 MB will result in this region colliding with the DTB.
>>
>> Right.
>>
>> Note that this question can be short-circuited by a list of guarantees,
>> for example:
>> - the firmware volume will always (or never) be shadowed from flash to
>>   system RAM (--> because this helps us see which outer branch in
>>   InitializeMemory() will be taken),
>> - and we'll never boot a guest with less than 128 MB RAM (--> this
>>   determines the inner branch, and given the size of the firmware
>>   volume, allows deducing the exact location of the permanent PEI RAM)
>>
>> If we can guarantee such things, I'm completely fine with that as an answer.
>>
> 
> OK, so by shadowing in this context, you mean invoking QEMU in such a
> way that the UEFI image is preloaded into DRAM?

No. What I mean is the following. In edk2 you can build flash images (FD
files) that contain various firmware volumes. A firmware volume's FFS
(firmware volume file system) may contain an LZMA compressed file that
is itself a firmware volume, after decompression.

When your platform starts up, the reset vector and SEC run from flash.
On some platforms, SEC locates these compressed FFS files in the initial
boot firmware volume (mapped into flash), and decompresses them to
system RAM. Thereby creating "shadowed" firmware volumes.

I have no clue how this looks on the ARM platforms, but it's not
something that changes from run to run. It's a design choice that is
reflected in the FDF file, and the SEC code. If you just instrument
these branches in question with a few DEBUG() calls, you'll see what the
case is for this new platform.

We "agreed" that PcdSystemMemoryInitializeInSec would be FALSE in our
case. Given that SEC doesn't initialize the system memory, it's hard to
imagine it would decompress firmware volumes to it. Hence I expect your
debug messages should prove that there's no shadowing here. (Actually
you might be able to deduce that statically, by tracking down
PcdFdBaseAddress.)

> I don't think there is
> any point at all in doing that, so I don't see why we should support
> it. And having at least 128 MB of DRAM is reasonable as well.

I agree that requiring at least 128 MB of DRAM is reasonable.

> So I could perhaps enforce these rules in the same place the system
> DRAM is discovered from the DTB?

You mean your implementation of ArmPlatformInitializeSystemMemory().

Yes, that would be reasonable. You can assert there that the size of the
system DRAM is at least 128 MB.

The idea is that by controlling all of the circumstances (system DRAM
size, PcdFdBaseAddress, PcdFdSize, PcdSystemMemoryInitializeInSec), you
would *force* InitializeMemory() to a pre-determined path.

This would then protect the DTB.

>>>> (c) Third, before we enter DXE (and after we installed permanent PEI
>>>> memory), we must ensure that DXE won't trample on the DTB.
>>>>
>>>> This is possible in two ways. First, don't install a memory resource
>>>> descriptor HOB in MemoryPeim() that would cover the DTB. (This is very
>>>> unlikely to work if the DTB's range is otherwise covered by the full
>>>> system memory, as reported in the DTB's own self.)
>>>>
>>>> Second (the canonical way), create a memory allocation HOB (with the
>>>> BuildMemoryAllocationHob() function of HobLib) that covers the DTB. This
>>>> way, DXE will create a memory *space* map (== hw properties map), based
>>>> on MemoryPeim()'s memory resource descriptor HOBs, that covers the DTB;
>>>> but seeing the memory allocation HOB, DXE will also start with a UEFI
>>>> memory map (== sw properties map) that already lists the DTB as covered.
>>>> This way DXE will steer clear of that memory range.

>> In short,
>>
>>   EFI_PHYSICAL_ADDRESS Base;
>>   UINTN                FdtSize;
>>
>>   Base = PcdGet64 (PcdSystemMemoryBase);
>>   FdtSize = fdt_totalsize ((VOID *)(UINTN)Base);
>>
>>   BuildMemoryAllocationHob (
>>     Base,
>>     EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (FdtSize)),
>>     EfiBootServicesData
>>     );
>>
>> would be correct. No need for relocation or for a later AllocatePages()
>> call. This would answer the third question, and you could link the DTB
>> at 0x4000_0000 into the config table directly.
>>
> 
> Indeed.
> 
> The only problem is the following: the EFI stub relocates the DTB to
> somewhere higher up in memory (it needs to add and modify entries, and
> doing so in place is not feasible)
> Then, it uses AllocatePages() to find the lowest 2 MB aligned region
> of memory to relocate the kernel Image itself, which should ideally be
> the base of DRAM. However, with the DTB located there, and protected
> by the HOB, it will disregard the base of DRAM and move the kernel
> Image 2 MB up instead.

This analysis seems to be correct. However, I think this should be no
problem.

For example, in OVMF guests, the kernel is also located "higher than
usual". This usually works transparently, and the kernel itself can
(should) later reuse the then-free low area for buffers and some such.

Some tools might need adjustment. For example Dave Anderson is adapting
the "crash" tool so that the user is not required to pass

  --machdep phys_base=16m

manually to "crash", when the vmcore to be analyzed originates from an
OVMF guest.

Alternatively, QEMU and AArch64VirtualizationPkg can agree on a
different location of the DTB as well. For example, QEMU could place the
DTB at 16MB, not 0MB (relative to 0x4000_0000, of course!)

- This would require your patchset to state "as a minimum, 17 MB DRAM is
  assumed", rather than just 1 MB; in the blurb and the commit messages.

- The temporary SEC/PEI RAM would remain distinct just the same (still
  below the 1 MB).

- Assuming 128 MB of DRAM to be guaranteed, the above reasoning about
  the permanent PEI RAM would continue to hold. It would still not
  overlap with the DTB at 16 MB (relative to 0x4000_0000, again).

- PlatformPei could create the EfiBootServicesData memalloc HOB at 16 MB
  (relative to 0x4000_0000). Then DXE would stay away just the same,
  but the EFI stub would find address 0 (relative to 0x4000_0000)
  unoccupied.

> 
>> We just need to find a good spot for this code -- it must run in a PEIM,
>> after permanent PEI RAM has been installed.
>>
>> [snip]
>>
>>>> Summary:
>>>> (a) please prove, with specific addresses, that the temporary RAM range,
>>>>     used in SEC and the first half of PEI, is distinct from the
>>>>     pre-placed DTB.
>>>> (b) please prove, with specific range sizes, and by walking me through
>>>>     the exact branches in InitializeMemory(), that the permanent PEI RAM
>>>>     will not overlap the pre-placed DTB.
>>>> (c) please find a good spot in some PEIM -- that runs on permanent PEI
>>>>     RAM -- for a BuildMemoryAllocationHob() call.
>>>>
>>>
>>> I will try to follow up with a v3 that relocates the DTB as early as
>>> possible. What would be the best way to communicate the new offset?
>>> Another dynamic PCD?
>>
>> Let's not relocate it. :) If I understand correctly, your main concern
>> with BuildMemoryAllocationHob() was that it would reserve the memory
>> area even during OS runtime. This is not the case, see above.
>>
> 
> Not entirely, see above

Well, relocation *is* possible, if you want to do it.

Namely, rather than calling BuildMemoryAllocationHob(), in order to
cover the DTB *in place*, you can simply call AllocatePages() or
AllocatePool() and copy the DTB there, from its original place. Importantly,

- this allocation and copying needs to be done in the exact same place
  as you'd call BuildMemoryAllocationHob() -- namely, still in PEI, and
  after the permanent PEI RAM has been installed;

- the AllocatePages() / AllocatePool() call itself would be internally
  served *from* the permanent PEI RAM,

- and, internally again, a very similar memory allocation HOB would be
  created.

Again: you can implement the protection from DXE with a relocation that
is done in PEI (after permanent PEI RAM has been installed), instead of
with an in-place BuildMemoryAllocationHob(). The only difference would
be that ultimately the DTB would go on to live in the permanent PEI RAM
range, not at it's original address, where QEMU has placed it.

The problem in this case, however, is that in DXE you'd have a hard time
finding the relocated DTB (for parsing it). As you said, you'd need yet
another dynamic PCD, to store the address.

>> But, again, Q1 has been answered, Q2 can be answered simply by
>> guaranteeing a few constants, and Q3 can be covered by a small snippet
>> of code, which just has to be inserted in a good spot.
>>
>> Not sure if in ARM world there are "platform PEIMs". In OVMF we have
>> such BuildMemoryAllocationHob() calls in OvmfPkg/PlatformPei/ (which
>> take place after permanent PEI RAM gets installed; same as discussed above).
>>
>> According to patch v2 7/8, this is our list of PEIMs:
>>
>> +  APRIORI PEI {
>> +    INF  MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>> +  }
>> +  INF ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
>> +  INF MdeModulePkg/Core/Pei/PeiMain.inf
>> +  INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
>> +  INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
>> +  INF ArmPkg/Drivers/CpuPei/CpuPei.inf
>> +  INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>> +  INF IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf
>> +  INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>
>> So, yes, there is a platform PEIM:
>>
>>   ArmPlatformPkg/PlatformPei/PlatformPeim.inf
>>
>> The by-the-book way would be to
>> - create a *deep copy* of this module, under
>> ArmPlatformPkg/AArch64VirtualizationPkg/,
>> - reference that copy in the new DSC and FDF file(s),
>> - and add the above code snippet to the PlatformPeim() function.
>>
>> (This could be made more lightweight by exposing PlatformPeim() as a
>> standalone library class in ARM, and then you wouldn't need to copy the
>> entire ArmPlatformPkg/PlatformPei directory to
>> AArch64VirtualizationPkg/, just provide a new library instance.)
>>
>> Another, independent possibility would be to introduce a FeaturePcd
>> (called PcdReserveFdtRange or some such), and dependent on that, run the
>> code snippet in "ArmPlatformPkg/PlatformPei/PlatformPeiLib.c" directly.
>> This is probably the lightest solution, but it would take the most
>> convincing with Olivier :)
>>
>> *** In addition, I *think* that the platform PEIM's [Depex] section
>> should spell out a [Depex] on gEfiPeiMemoryDiscoveredPpiGuid, if the
>> platform PEIM calls BuildMemoryAllocationHob().
>>
>> "ArmPkg/Drivers/CpuPei/CpuPei.inf" (listed above) does so already, for
>> example. (Likely for other reasons; I'm just saying there's already a
>> Depex example one could copy.)
>>
>> The presence of this PPI guarantees that the PEIM runs after permanent
>> PEI RAM has been installed. Refer to:
>> - "MdePkg/Include/Ppi/MemoryDiscovered.h",
>> - Chapter 11, PEI Physical Memory Usage, in Vol 1 of the PI spec.
>>   (EFI_PEI_PERMANENT_MEMORY_INSTALLED_PPI)
>>
>> In OVMF's PlatformPei, we don't depend on
>> gEfiPeiMemoryDiscoveredPpiGuid, but for a good reason: we install the
>> permanent PEI RAM and build the memalloc HOBs in that *same* PEIM, hence
>> we can ensure their correct ordering easily.
>>
>> I'll ask for confirmation on the list separately.
>>
> 
> So even if we do end up relocating the DTB (which I expect), I assume
> this would still be the place to implement that?
> If so, I will go and implement the PlatformPeim() while we continue
> the discussion regarding the relocation.

Yes. You can decide between
- in-place coverage with BuildMemoryAllocationHob()
- and relocation with AllocateCopyPool() + PcdSet64(PcdRelocatedFdtBase)

but whichever you pick, it must occur in the same place: in PEI, after
permanent PEI RAM installation, and preferably in the platform PEI
(which should spell out the depex).

... Okay, summary time.
- Temporary SEC/PEI RAM seems to be safely distinct.
- permanent PEI RAM can be made distinct, by controlling a handful of
  PCDs, and by asserting 128 MB minimum required memory size in
  ArmPlatformInitializeSystemMemory(), when you parse the DTB.
- We discussed four options for DXE protection:
  - in-place DTB coverage at 0x4000_0000, with a memalloc HOB. Pro:
    simple to do; con: might surprise the kernel and some tools.
  - in-place DTB coverage at 0x4100_0000, with a memalloc HOB. Pro:
    simple to do and likely no surprises for the kernel; con: QEMU
    needs to be adapted.
  - relocation from 0x4000_0000 to a dynamically allocated chunk of the
    permanent PEI RAM. Pro: no surprises for the kernel; con: needs at
    least one further dynamic PCD.
  - (more than just DXE protection, actually): present the DTB over an
    MMIO "channel". Whoever cares can fetch it into a temporary array,
    parse it, then throw it away. Pro: DTB is unable to overlap with
    system DRAM, and this discussion goes away. Con: needs new hardware
    in machtype virt.

Thanks
Laszlo

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/

On 27 August 2014 12:36, Laszlo Ersek <lersek@redhat.com> wrote:
> On 08/27/14 11:35, Ard Biesheuvel wrote:
>> On 27 August 2014 11:07, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 08/27/14 09:10, Ard Biesheuvel wrote:
>>>> On 27 August 2014 00:24, Laszlo Ersek <lersek@redhat.com> wrote:
>
>>>>> (b) Predictably, my second question is about the second world. The
>>>>> permanent PEI memory must be distinct from the DTB.
>>>>>
>>>>> InitializeMemory() calculates where to place the permanent PEI memory.
>>>>> The permanent PEI memory does not cover the entire system RAM. It's
>>>>> (calculated) base address is UefiMemoryBase, and it's size comes from
>>>>> PcdSystemMemoryUefiRegionSize (64MB, see the next patch).
>>>>>
>>>>
>>>> Yes, so running with 64 MB will result in this region colliding with the DTB.
>>>
>>> Right.
>>>
>>> Note that this question can be short-circuited by a list of guarantees,
>>> for example:
>>> - the firmware volume will always (or never) be shadowed from flash to
>>>   system RAM (--> because this helps us see which outer branch in
>>>   InitializeMemory() will be taken),
>>> - and we'll never boot a guest with less than 128 MB RAM (--> this
>>>   determines the inner branch, and given the size of the firmware
>>>   volume, allows deducing the exact location of the permanent PEI RAM)
>>>
>>> If we can guarantee such things, I'm completely fine with that as an answer.
>>>
>>
>> OK, so by shadowing in this context, you mean invoking QEMU in such a
>> way that the UEFI image is preloaded into DRAM?
>
> No. What I mean is the following. In edk2 you can build flash images (FD
> files) that contain various firmware volumes. A firmware volume's FFS
> (firmware volume file system) may contain an LZMA compressed file that
> is itself a firmware volume, after decompression.
>
> When your platform starts up, the reset vector and SEC run from flash.
> On some platforms, SEC locates these compressed FFS files in the initial
> boot firmware volume (mapped into flash), and decompresses them to
> system RAM. Thereby creating "shadowed" firmware volumes.
>
> I have no clue how this looks on the ARM platforms, but it's not
> something that changes from run to run. It's a design choice that is
> reflected in the FDF file, and the SEC code. If you just instrument
> these branches in question with a few DEBUG() calls, you'll see what the
> case is for this new platform.
>
> We "agreed" that PcdSystemMemoryInitializeInSec would be FALSE in our
> case. Given that SEC doesn't initialize the system memory, it's hard to
> imagine it would decompress firmware volumes to it. Hence I expect your
> debug messages should prove that there's no shadowing here. (Actually
> you might be able to deduce that statically, by tracking down
> PcdFdBaseAddress.)
>

Looking the the logs, the first module that executed from DRAM is
Build/AArch64Virtualization-KVM/DEBUG_ARMLINUXGCC/AARCH64/MdeModulePkg/Core/Pei/PeiMain/DEBUG/PeiCore.dll,
everything before that is executed in place from NOR.

So something like this should do the trick then?

in ArmPlatformInitialize():
  ASSERT (!FeaturePcdGet (PcdSystemMemoryInitializeInSec));

and in ArmPlatformInitializeSystemMemory():
  ASSERT (NewSize >= SIZE_128MB);
  ASSERT (PcdGet32 (PcdFdBaseAddress) + PcdGet32 (PcdFdSize) < NewBase ||
          PcdGet32 (PcdFdBaseAddress) > NewBase + NewSize);

(with comments describing the rationale, of course)

The only thing that still needs some consideration is whether to allow
NewBase to be different from 0x4000_0000. It kind of complicates the
logic, and for no obvious gain, so I should probably add an assert for
that as well.

>> I don't think there is
>> any point at all in doing that, so I don't see why we should support
>> it. And having at least 128 MB of DRAM is reasonable as well.
s
> I agree that requiring at least 128 MB of DRAM is reasonable.
>
>> So I could perhaps enforce these rules in the same place the system
>> DRAM is discovered from the DTB?
>
> You mean your implementation of ArmPlatformInitializeSystemMemory().
>
> Yes, that would be reasonable. You can assert there that the size of the
> system DRAM is at least 128 MB.
>
> The idea is that by controlling all of the circumstances (system DRAM
> size, PcdFdBaseAddress, PcdFdSize, PcdSystemMemoryInitializeInSec), you
> would *force* InitializeMemory() to a pre-determined path.
>
> This would then protect the DTB.
>

As above

>>>>> (c) Third, before we enter DXE (and after we installed permanent PEI
>>>>> memory), we must ensure that DXE won't trample on the DTB.
>>>>>
>>>>> This is possible in two ways. First, don't install a memory resource
>>>>> descriptor HOB in MemoryPeim() that would cover the DTB. (This is very
>>>>> unlikely to work if the DTB's range is otherwise covered by the full
>>>>> system memory, as reported in the DTB's own self.)
>>>>>
>>>>> Second (the canonical way), create a memory allocation HOB (with the
>>>>> BuildMemoryAllocationHob() function of HobLib) that covers the DTB. This
>>>>> way, DXE will create a memory *space* map (== hw properties map), based
>>>>> on MemoryPeim()'s memory resource descriptor HOBs, that covers the DTB;
>>>>> but seeing the memory allocation HOB, DXE will also start with a UEFI
>>>>> memory map (== sw properties map) that already lists the DTB as covered.
>>>>> This way DXE will steer clear of that memory range.
>
>>> In short,
>>>
>>>   EFI_PHYSICAL_ADDRESS Base;
>>>   UINTN                FdtSize;
>>>
>>>   Base = PcdGet64 (PcdSystemMemoryBase);
>>>   FdtSize = fdt_totalsize ((VOID *)(UINTN)Base);
>>>
>>>   BuildMemoryAllocationHob (
>>>     Base,
>>>     EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (FdtSize)),
>>>     EfiBootServicesData
>>>     );
>>>
>>> would be correct. No need for relocation or for a later AllocatePages()
>>> call. This would answer the third question, and you could link the DTB
>>> at 0x4000_0000 into the config table directly.
>>>
>>
>> Indeed.
>>
>> The only problem is the following: the EFI stub relocates the DTB to
>> somewhere higher up in memory (it needs to add and modify entries, and
>> doing so in place is not feasible)
>> Then, it uses AllocatePages() to find the lowest 2 MB aligned region
>> of memory to relocate the kernel Image itself, which should ideally be
>> the base of DRAM. However, with the DTB located there, and protected
>> by the HOB, it will disregard the base of DRAM and move the kernel
>> Image 2 MB up instead.
>
> This analysis seems to be correct. However, I think this should be no
> problem.
>
> For example, in OVMF guests, the kernel is also located "higher than
> usual". This usually works transparently, and the kernel itself can
> (should) later reuse the then-free low area for buffers and some such.
>

Well, the arm64 kernel does not deal very well with DRAM below the
kernel Image. It would be much preferable to put the Image at base of
DRAM if the only thing occupying that area is the initial DTB (as the
EFI stub will relocate it anyway)

> Some tools might need adjustment. For example Dave Anderson is adapting
> the "crash" tool so that the user is not required to pass
>
>   --machdep phys_base=16m
>
> manually to "crash", when the vmcore to be analyzed originates from an
> OVMF guest.
>
> Alternatively, QEMU and AArch64VirtualizationPkg can agree on a
> different location of the DTB as well. For example, QEMU could place the
> DTB at 16MB, not 0MB (relative to 0x4000_0000, of course!)
>
> - This would require your patchset to state "as a minimum, 17 MB DRAM is
>   assumed", rather than just 1 MB; in the blurb and the commit messages.
>
> - The temporary SEC/PEI RAM would remain distinct just the same (still
>   below the 1 MB).
>
> - Assuming 128 MB of DRAM to be guaranteed, the above reasoning about
>   the permanent PEI RAM would continue to hold. It would still not
>   overlap with the DTB at 16 MB (relative to 0x4000_0000, again).
>
> - PlatformPei could create the EfiBootServicesData memalloc HOB at 16 MB
>   (relative to 0x4000_0000). Then DXE would stay away just the same,
>   but the EFI stub would find address 0 (relative to 0x4000_0000)
>   unoccupied.
>

I don't know. Loading the DTB at the base of DRAM and only requiring 1
MB could be a meaningful ABI for other projects to adopt, that don't
necessarily have those additional requirements like UEFI does. So I
would strongly prefer a solution that addresses this on the Tianocore
end.

>>
>>> We just need to find a good spot for this code -- it must run in a PEIM,
>>> after permanent PEI RAM has been installed.
>>>
>>> [snip]
>>>
>>>>> Summary:
>>>>> (a) please prove, with specific addresses, that the temporary RAM range,
>>>>>     used in SEC and the first half of PEI, is distinct from the
>>>>>     pre-placed DTB.
>>>>> (b) please prove, with specific range sizes, and by walking me through
>>>>>     the exact branches in InitializeMemory(), that the permanent PEI RAM
>>>>>     will not overlap the pre-placed DTB.
>>>>> (c) please find a good spot in some PEIM -- that runs on permanent PEI
>>>>>     RAM -- for a BuildMemoryAllocationHob() call.
>>>>>
>>>>
>>>> I will try to follow up with a v3 that relocates the DTB as early as
>>>> possible. What would be the best way to communicate the new offset?
>>>> Another dynamic PCD?
>>>
>>> Let's not relocate it. :) If I understand correctly, your main concern
>>> with BuildMemoryAllocationHob() was that it would reserve the memory
>>> area even during OS runtime. This is not the case, see above.
>>>
>>
>> Not entirely, see above
>
> Well, relocation *is* possible, if you want to do it.
>
> Namely, rather than calling BuildMemoryAllocationHob(), in order to
> cover the DTB *in place*, you can simply call AllocatePages() or
> AllocatePool() and copy the DTB there, from its original place. Importantly,
>
> - this allocation and copying needs to be done in the exact same place
>   as you'd call BuildMemoryAllocationHob() -- namely, still in PEI, and
>   after the permanent PEI RAM has been installed;
>
> - the AllocatePages() / AllocatePool() call itself would be internally
>   served *from* the permanent PEI RAM,
>
> - and, internally again, a very similar memory allocation HOB would be
>   created.
>
> Again: you can implement the protection from DXE with a relocation that
> is done in PEI (after permanent PEI RAM has been installed), instead of
> with an in-place BuildMemoryAllocationHob(). The only difference would
> be that ultimately the DTB would go on to live in the permanent PEI RAM
> range, not at it's original address, where QEMU has placed it.
>
> The problem in this case, however, is that in DXE you'd have a hard time
> finding the relocated DTB (for parsing it). As you said, you'd need yet
> another dynamic PCD, to store the address.
>

So would a dynamic PCD be the only feasible way to do this?

>>> But, again, Q1 has been answered, Q2 can be answered simply by
>>> guaranteeing a few constants, and Q3 can be covered by a small snippet
>>> of code, which just has to be inserted in a good spot.
>>>
>>> Not sure if in ARM world there are "platform PEIMs". In OVMF we have
>>> such BuildMemoryAllocationHob() calls in OvmfPkg/PlatformPei/ (which
>>> take place after permanent PEI RAM gets installed; same as discussed above).
>>>
>>> According to patch v2 7/8, this is our list of PEIMs:
>>>
>>> +  APRIORI PEI {
>>> +    INF  MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>>> +  }
>>> +  INF ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
>>> +  INF MdeModulePkg/Core/Pei/PeiMain.inf
>>> +  INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
>>> +  INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
>>> +  INF ArmPkg/Drivers/CpuPei/CpuPei.inf
>>> +  INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>>> +  INF IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf
>>> +  INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>>
>>> So, yes, there is a platform PEIM:
>>>
>>>   ArmPlatformPkg/PlatformPei/PlatformPeim.inf
>>>
>>> The by-the-book way would be to
>>> - create a *deep copy* of this module, under
>>> ArmPlatformPkg/AArch64VirtualizationPkg/,
>>> - reference that copy in the new DSC and FDF file(s),
>>> - and add the above code snippet to the PlatformPeim() function.
>>>
>>> (This could be made more lightweight by exposing PlatformPeim() as a
>>> standalone library class in ARM, and then you wouldn't need to copy the
>>> entire ArmPlatformPkg/PlatformPei directory to
>>> AArch64VirtualizationPkg/, just provide a new library instance.)
>>>
>>> Another, independent possibility would be to introduce a FeaturePcd
>>> (called PcdReserveFdtRange or some such), and dependent on that, run the
>>> code snippet in "ArmPlatformPkg/PlatformPei/PlatformPeiLib.c" directly.
>>> This is probably the lightest solution, but it would take the most
>>> convincing with Olivier :)
>>>
>>> *** In addition, I *think* that the platform PEIM's [Depex] section
>>> should spell out a [Depex] on gEfiPeiMemoryDiscoveredPpiGuid, if the
>>> platform PEIM calls BuildMemoryAllocationHob().
>>>
>>> "ArmPkg/Drivers/CpuPei/CpuPei.inf" (listed above) does so already, for
>>> example. (Likely for other reasons; I'm just saying there's already a
>>> Depex example one could copy.)
>>>
>>> The presence of this PPI guarantees that the PEIM runs after permanent
>>> PEI RAM has been installed. Refer to:
>>> - "MdePkg/Include/Ppi/MemoryDiscovered.h",
>>> - Chapter 11, PEI Physical Memory Usage, in Vol 1 of the PI spec.
>>>   (EFI_PEI_PERMANENT_MEMORY_INSTALLED_PPI)
>>>
>>> In OVMF's PlatformPei, we don't depend on
>>> gEfiPeiMemoryDiscoveredPpiGuid, but for a good reason: we install the
>>> permanent PEI RAM and build the memalloc HOBs in that *same* PEIM, hence
>>> we can ensure their correct ordering easily.
>>>
>>> I'll ask for confirmation on the list separately.
>>>
>>
>> So even if we do end up relocating the DTB (which I expect), I assume
>> this would still be the place to implement that?
>> If so, I will go and implement the PlatformPeim() while we continue
>> the discussion regarding the relocation.
>
> Yes. You can decide between
> - in-place coverage with BuildMemoryAllocationHob()
> - and relocation with AllocateCopyPool() + PcdSet64(PcdRelocatedFdtBase)
>
> but whichever you pick, it must occur in the same place: in PEI, after
> permanent PEI RAM installation, and preferably in the platform PEI
> (which should spell out the depex).
>
> ... Okay, summary time.
> - Temporary SEC/PEI RAM seems to be safely distinct.
> - permanent PEI RAM can be made distinct, by controlling a handful of
>   PCDs, and by asserting 128 MB minimum required memory size in
>   ArmPlatformInitializeSystemMemory(), when you parse the DTB.
> - We discussed four options for DXE protection:
>   - in-place DTB coverage at 0x4000_0000, with a memalloc HOB. Pro:
>     simple to do; con: might surprise the kernel and some tools.
>   - in-place DTB coverage at 0x4100_0000, with a memalloc HOB. Pro:
>     simple to do and likely no surprises for the kernel; con: QEMU
>     needs to be adapted.
>   - relocation from 0x4000_0000 to a dynamically allocated chunk of the
>     permanent PEI RAM. Pro: no surprises for the kernel; con: needs at
>     least one further dynamic PCD.
>   - (more than just DXE protection, actually): present the DTB over an
>     MMIO "channel". Whoever cares can fetch it into a temporary array,
>     parse it, then throw it away. Pro: DTB is unable to overlap with
>     system DRAM, and this discussion goes away. Con: needs new hardware
>     in machtype virt.
>

OK, got my work cut out for me then
Thanks.

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/

On 08/27/14 13:12, Ard Biesheuvel wrote:
> On 27 August 2014 12:36, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 08/27/14 11:35, Ard Biesheuvel wrote:
>>> On 27 August 2014 11:07, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> On 08/27/14 09:10, Ard Biesheuvel wrote:
>>>>> On 27 August 2014 00:24, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>>>>> (b) Predictably, my second question is about the second world. The
>>>>>> permanent PEI memory must be distinct from the DTB.
>>>>>>
>>>>>> InitializeMemory() calculates where to place the permanent PEI memory.
>>>>>> The permanent PEI memory does not cover the entire system RAM. It's
>>>>>> (calculated) base address is UefiMemoryBase, and it's size comes from
>>>>>> PcdSystemMemoryUefiRegionSize (64MB, see the next patch).
>>>>>>
>>>>>
>>>>> Yes, so running with 64 MB will result in this region colliding with the DTB.
>>>>
>>>> Right.
>>>>
>>>> Note that this question can be short-circuited by a list of guarantees,
>>>> for example:
>>>> - the firmware volume will always (or never) be shadowed from flash to
>>>>   system RAM (--> because this helps us see which outer branch in
>>>>   InitializeMemory() will be taken),
>>>> - and we'll never boot a guest with less than 128 MB RAM (--> this
>>>>   determines the inner branch, and given the size of the firmware
>>>>   volume, allows deducing the exact location of the permanent PEI RAM)
>>>>
>>>> If we can guarantee such things, I'm completely fine with that as an answer.
>>>>
>>>
>>> OK, so by shadowing in this context, you mean invoking QEMU in such a
>>> way that the UEFI image is preloaded into DRAM?
>>
>> No. What I mean is the following. In edk2 you can build flash images (FD
>> files) that contain various firmware volumes. A firmware volume's FFS
>> (firmware volume file system) may contain an LZMA compressed file that
>> is itself a firmware volume, after decompression.
>>
>> When your platform starts up, the reset vector and SEC run from flash.
>> On some platforms, SEC locates these compressed FFS files in the initial
>> boot firmware volume (mapped into flash), and decompresses them to
>> system RAM. Thereby creating "shadowed" firmware volumes.
>>
>> I have no clue how this looks on the ARM platforms, but it's not
>> something that changes from run to run. It's a design choice that is
>> reflected in the FDF file, and the SEC code. If you just instrument
>> these branches in question with a few DEBUG() calls, you'll see what the
>> case is for this new platform.
>>
>> We "agreed" that PcdSystemMemoryInitializeInSec would be FALSE in our
>> case. Given that SEC doesn't initialize the system memory, it's hard to
>> imagine it would decompress firmware volumes to it. Hence I expect your
>> debug messages should prove that there's no shadowing here. (Actually
>> you might be able to deduce that statically, by tracking down
>> PcdFdBaseAddress.)
>>
> 
> Looking the the logs, the first module that executed from DRAM is
> Build/AArch64Virtualization-KVM/DEBUG_ARMLINUXGCC/AARCH64/MdeModulePkg/Core/Pei/PeiMain/DEBUG/PeiCore.dll,
> everything before that is executed in place from NOR.
> 
> So something like this should do the trick then?
> 
> in ArmPlatformInitialize():
>   ASSERT (!FeaturePcdGet (PcdSystemMemoryInitializeInSec));

I can't immediately see where ArmPlatformInitialize() is called. If this
ASSERT() runs before we enter InitializeMemory(), then it's fine.

> 
> and in ArmPlatformInitializeSystemMemory():
>   ASSERT (NewSize >= SIZE_128MB);

Seems OK.

>   ASSERT (PcdGet32 (PcdFdBaseAddress) + PcdGet32 (PcdFdSize) < NewBase ||
>           PcdGet32 (PcdFdBaseAddress) > NewBase + NewSize);
> 
> (with comments describing the rationale, of course)

Ah okay, these would assert that the FV and the system DRAM do not overlap.

Seems good, but equality should be allowed in both expressions. You
compare exclusive high limits against inclusive low limits, *plus*
PcdFdSize is nonzero.

If the first expression holds (with the proposed <=):

  PcdGet32 (PcdFdBaseAddress) + PcdGet32 (PcdFdSize) <= NewBase

Then that implies

  PcdGet32 (PcdFdBaseAddress)                        <  NewBase

which is exactly the negative of InitializeMemory()'s

  FdBase                                             >= SystemMemoryBase


Similarly, if the second one holds (with the proposed >=):

  PcdGet32 (PcdFdBaseAddress) >= NewBase + NewSize

then that implies

     PcdGet32 (PcdFdBaseAddress) + PcdGet32 (PcdFdSize)
  >  PcdGet32 (PcdFdBaseAddress)
  >= NewBase + NewSize

ie.

  PcdGet32 (PcdFdBaseAddress) + PcdGet32 (PcdFdSize) > NewBase + NewSize

which is exactly the negative of InitializeMemory()'s

  FdTop                                              <= SystemMemoryTop


So, yes these are good, but you should also allow equality.

The comments would be appreciated.

> The only thing that still needs some consideration is whether to allow
> NewBase to be different from 0x4000_0000. It kind of complicates the
> logic, and for no obvious gain, so I should probably add an assert for
> that as well.

Probably a good idea, yes.

> Well, the arm64 kernel does not deal very well with DRAM below the
> kernel Image. It would be much preferable to put the Image at base of
> DRAM if the only thing occupying that area is the initial DTB (as the
> EFI stub will relocate it anyway)

Okay. In this case:

>> Well, relocation *is* possible, if you want to do it.
>>
>> Namely, rather than calling BuildMemoryAllocationHob(), in order to
>> cover the DTB *in place*, you can simply call AllocatePages() or
>> AllocatePool() and copy the DTB there, from its original place. Importantly,
>>
>> - this allocation and copying needs to be done in the exact same place
>>   as you'd call BuildMemoryAllocationHob() -- namely, still in PEI, and
>>   after the permanent PEI RAM has been installed;
>>
>> - the AllocatePages() / AllocatePool() call itself would be internally
>>   served *from* the permanent PEI RAM,
>>
>> - and, internally again, a very similar memory allocation HOB would be
>>   created.
>>
>> Again: you can implement the protection from DXE with a relocation that
>> is done in PEI (after permanent PEI RAM has been installed), instead of
>> with an in-place BuildMemoryAllocationHob(). The only difference would
>> be that ultimately the DTB would go on to live in the permanent PEI RAM
>> range, not at it's original address, where QEMU has placed it.
>>
>> The problem in this case, however, is that in DXE you'd have a hard time
>> finding the relocated DTB (for parsing it). As you said, you'd need yet
>> another dynamic PCD, to store the address.
>>
> 
> So would a dynamic PCD be the only feasible way to do this?

Yes, that's required. We use this pattern in OVMF as well (allocate some
RAM in PEI, in the permanent PEI RAM, then point a dynamic PCD at it,
then access the area in DXE).

This is most certainly doable. I frowned at the introduction of another
dynamic PCD only for a moment, and only because of extra "mental load".
It's a clean technical approach though.

>> ... Okay, summary time.
>> - Temporary SEC/PEI RAM seems to be safely distinct.
>> - permanent PEI RAM can be made distinct, by controlling a handful of
>>   PCDs, and by asserting 128 MB minimum required memory size in
>>   ArmPlatformInitializeSystemMemory(), when you parse the DTB.
>> - We discussed four options for DXE protection:
>>   - in-place DTB coverage at 0x4000_0000, with a memalloc HOB. Pro:
>>     simple to do; con: might surprise the kernel and some tools.
>>   - in-place DTB coverage at 0x4100_0000, with a memalloc HOB. Pro:
>>     simple to do and likely no surprises for the kernel; con: QEMU
>>     needs to be adapted.
>>   - relocation from 0x4000_0000 to a dynamically allocated chunk of the
>>     permanent PEI RAM. Pro: no surprises for the kernel; con: needs at
>>     least one further dynamic PCD.
>>   - (more than just DXE protection, actually): present the DTB over an
>>     MMIO "channel". Whoever cares can fetch it into a temporary array,
>>     parse it, then throw it away. Pro: DTB is unable to overlap with
>>     system DRAM, and this discussion goes away. Con: needs new hardware
>>     in machtype virt.
>>
> 
> OK, got my work cut out for me then

Looks like we agreed on the asserts for Q2 then.

For Q3, apparently you'll go with the relocation (option 3), because we
don't really mind the new dynamic PCD, and other than that, option 3 is
pure win.

Sounds great to me! :)

Thanks,
Laszlo


------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/

On 27 August 2014 13:53, Laszlo Ersek <lersek@redhat.com> wrote:
> On 08/27/14 13:12, Ard Biesheuvel wrote:
>> On 27 August 2014 12:36, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 08/27/14 11:35, Ard Biesheuvel wrote:
>>>> On 27 August 2014 11:07, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>> On 08/27/14 09:10, Ard Biesheuvel wrote:
>>>>>> On 27 August 2014 00:24, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>>>>>> (b) Predictably, my second question is about the second world. The
>>>>>>> permanent PEI memory must be distinct from the DTB.
>>>>>>>
>>>>>>> InitializeMemory() calculates where to place the permanent PEI memory.
>>>>>>> The permanent PEI memory does not cover the entire system RAM. It's
>>>>>>> (calculated) base address is UefiMemoryBase, and it's size comes from
>>>>>>> PcdSystemMemoryUefiRegionSize (64MB, see the next patch).
>>>>>>>
>>>>>>
>>>>>> Yes, so running with 64 MB will result in this region colliding with the DTB.
>>>>>
>>>>> Right.
>>>>>
>>>>> Note that this question can be short-circuited by a list of guarantees,
>>>>> for example:
>>>>> - the firmware volume will always (or never) be shadowed from flash to
>>>>>   system RAM (--> because this helps us see which outer branch in
>>>>>   InitializeMemory() will be taken),
>>>>> - and we'll never boot a guest with less than 128 MB RAM (--> this
>>>>>   determines the inner branch, and given the size of the firmware
>>>>>   volume, allows deducing the exact location of the permanent PEI RAM)
>>>>>
>>>>> If we can guarantee such things, I'm completely fine with that as an answer.
>>>>>
>>>>
>>>> OK, so by shadowing in this context, you mean invoking QEMU in such a
>>>> way that the UEFI image is preloaded into DRAM?
>>>
>>> No. What I mean is the following. In edk2 you can build flash images (FD
>>> files) that contain various firmware volumes. A firmware volume's FFS
>>> (firmware volume file system) may contain an LZMA compressed file that
>>> is itself a firmware volume, after decompression.
>>>
>>> When your platform starts up, the reset vector and SEC run from flash.
>>> On some platforms, SEC locates these compressed FFS files in the initial
>>> boot firmware volume (mapped into flash), and decompresses them to
>>> system RAM. Thereby creating "shadowed" firmware volumes.
>>>
>>> I have no clue how this looks on the ARM platforms, but it's not
>>> something that changes from run to run. It's a design choice that is
>>> reflected in the FDF file, and the SEC code. If you just instrument
>>> these branches in question with a few DEBUG() calls, you'll see what the
>>> case is for this new platform.
>>>
>>> We "agreed" that PcdSystemMemoryInitializeInSec would be FALSE in our
>>> case. Given that SEC doesn't initialize the system memory, it's hard to
>>> imagine it would decompress firmware volumes to it. Hence I expect your
>>> debug messages should prove that there's no shadowing here. (Actually
>>> you might be able to deduce that statically, by tracking down
>>> PcdFdBaseAddress.)
>>>
>>
>> Looking the the logs, the first module that executed from DRAM is
>> Build/AArch64Virtualization-KVM/DEBUG_ARMLINUXGCC/AARCH64/MdeModulePkg/Core/Pei/PeiMain/DEBUG/PeiCore.dll,
>> everything before that is executed in place from NOR.
>>
>> So something like this should do the trick then?
>>
>> in ArmPlatformInitialize():
>>   ASSERT (!FeaturePcdGet (PcdSystemMemoryInitializeInSec));
>
> I can't immediately see where ArmPlatformInitialize() is called. If this
> ASSERT() runs before we enter InitializeMemory(), then it's fine.
>

It is called from CEntryPoint () in
ArmPlatformPkg/PrePeiCore/PrePeiCore.c, so that should be ok.

>>
>> and in ArmPlatformInitializeSystemMemory():
>>   ASSERT (NewSize >= SIZE_128MB);
>
> Seems OK.
>
>>   ASSERT (PcdGet32 (PcdFdBaseAddress) + PcdGet32 (PcdFdSize) < NewBase ||
>>           PcdGet32 (PcdFdBaseAddress) > NewBase + NewSize);
>>
>> (with comments describing the rationale, of course)
>
> Ah okay, these would assert that the FV and the system DRAM do not overlap.
>
> Seems good, but equality should be allowed in both expressions. You
> compare exclusive high limits against inclusive low limits, *plus*
> PcdFdSize is nonzero.
>
> If the first expression holds (with the proposed <=):
>
>   PcdGet32 (PcdFdBaseAddress) + PcdGet32 (PcdFdSize) <= NewBase
>
> Then that implies
>
>   PcdGet32 (PcdFdBaseAddress)                        <  NewBase
>
> which is exactly the negative of InitializeMemory()'s
>
>   FdBase                                             >= SystemMemoryBase
>
>
> Similarly, if the second one holds (with the proposed >=):
>
>   PcdGet32 (PcdFdBaseAddress) >= NewBase + NewSize
>
> then that implies
>
>      PcdGet32 (PcdFdBaseAddress) + PcdGet32 (PcdFdSize)
>   >  PcdGet32 (PcdFdBaseAddress)
>   >= NewBase + NewSize
>
> ie.
>
>   PcdGet32 (PcdFdBaseAddress) + PcdGet32 (PcdFdSize) > NewBase + NewSize
>
> which is exactly the negative of InitializeMemory()'s
>
>   FdTop                                              <= SystemMemoryTop
>
>
> So, yes these are good, but you should also allow equality.
>

ok

> The comments would be appreciated.
>

ok

>> The only thing that still needs some consideration is whether to allow
>> NewBase to be different from 0x4000_0000. It kind of complicates the
>> logic, and for no obvious gain, so I should probably add an assert for
>> that as well.
>
> Probably a good idea, yes.
>
>> Well, the arm64 kernel does not deal very well with DRAM below the
>> kernel Image. It would be much preferable to put the Image at base of
>> DRAM if the only thing occupying that area is the initial DTB (as the
>> EFI stub will relocate it anyway)
>
> Okay. In this case:
>
>>> Well, relocation *is* possible, if you want to do it.
>>>
>>> Namely, rather than calling BuildMemoryAllocationHob(), in order to
>>> cover the DTB *in place*, you can simply call AllocatePages() or
>>> AllocatePool() and copy the DTB there, from its original place. Importantly,
>>>
>>> - this allocation and copying needs to be done in the exact same place
>>>   as you'd call BuildMemoryAllocationHob() -- namely, still in PEI, and
>>>   after the permanent PEI RAM has been installed;
>>>
>>> - the AllocatePages() / AllocatePool() call itself would be internally
>>>   served *from* the permanent PEI RAM,
>>>
>>> - and, internally again, a very similar memory allocation HOB would be
>>>   created.
>>>
>>> Again: you can implement the protection from DXE with a relocation that
>>> is done in PEI (after permanent PEI RAM has been installed), instead of
>>> with an in-place BuildMemoryAllocationHob(). The only difference would
>>> be that ultimately the DTB would go on to live in the permanent PEI RAM
>>> range, not at it's original address, where QEMU has placed it.
>>>
>>> The problem in this case, however, is that in DXE you'd have a hard time
>>> finding the relocated DTB (for parsing it). As you said, you'd need yet
>>> another dynamic PCD, to store the address.
>>>
>>
>> So would a dynamic PCD be the only feasible way to do this?
>
> Yes, that's required. We use this pattern in OVMF as well (allocate some
> RAM in PEI, in the permanent PEI RAM, then point a dynamic PCD at it,
> then access the area in DXE).
>
> This is most certainly doable. I frowned at the introduction of another
> dynamic PCD only for a moment, and only because of extra "mental load".
> It's a clean technical approach though.
>

OK, good to know.

>>> ... Okay, summary time.
>>> - Temporary SEC/PEI RAM seems to be safely distinct.
>>> - permanent PEI RAM can be made distinct, by controlling a handful of
>>>   PCDs, and by asserting 128 MB minimum required memory size in
>>>   ArmPlatformInitializeSystemMemory(), when you parse the DTB.
>>> - We discussed four options for DXE protection:
>>>   - in-place DTB coverage at 0x4000_0000, with a memalloc HOB. Pro:
>>>     simple to do; con: might surprise the kernel and some tools.
>>>   - in-place DTB coverage at 0x4100_0000, with a memalloc HOB. Pro:
>>>     simple to do and likely no surprises for the kernel; con: QEMU
>>>     needs to be adapted.
>>>   - relocation from 0x4000_0000 to a dynamically allocated chunk of the
>>>     permanent PEI RAM. Pro: no surprises for the kernel; con: needs at
>>>     least one further dynamic PCD.
>>>   - (more than just DXE protection, actually): present the DTB over an
>>>     MMIO "channel". Whoever cares can fetch it into a temporary array,
>>>     parse it, then throw it away. Pro: DTB is unable to overlap with
>>>     system DRAM, and this discussion goes away. Con: needs new hardware
>>>     in machtype virt.
>>>
>>
>> OK, got my work cut out for me then
>
> Looks like we agreed on the asserts for Q2 then.
>
> For Q3, apparently you'll go with the relocation (option 3), because we
> don't really mind the new dynamic PCD, and other than that, option 3 is
> pure win.
>

OK, glad we cleared that up.