On 27 August 2014 13:53, Laszlo Ersek <ler...@redhat.com> wrote: > On 08/27/14 13:12, Ard Biesheuvel wrote: >> On 27 August 2014 12:36, Laszlo Ersek <ler...@redhat.com> wrote: >>> On 08/27/14 11:35, Ard Biesheuvel wrote: >>>> On 27 August 2014 11:07, Laszlo Ersek <ler...@redhat.com> wrote: >>>>> On 08/27/14 09:10, Ard Biesheuvel wrote: >>>>>> On 27 August 2014 00:24, Laszlo Ersek <ler...@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. -- Ard. ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
