On 27 August 2014 00:24, Laszlo Ersek <[email protected]> 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 <[email protected]> >> --- [...] > > 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/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
