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:
>>> 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.
>
> 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.
--
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
