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 <[email protected]> > --- > 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/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
