On 08/27/14 13:12, Ard Biesheuvel wrote:
> On 27 August 2014 12:36, Laszlo Ersek <[email protected]> wrote:
>> On 08/27/14 11:35, Ard Biesheuvel wrote:
>>> On 27 August 2014 11:07, Laszlo Ersek <[email protected]> wrote:
>>>> On 08/27/14 09:10, Ard Biesheuvel wrote:
>>>>> On 27 August 2014 00:24, Laszlo Ersek <[email protected]> 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/
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
