On 11/30/15 11:03, Ard Biesheuvel wrote:
> On 30 November 2015 at 11:01, Laszlo Ersek <[email protected]> wrote:
>> On 11/30/15 10:50, Laszlo Ersek wrote:
>>> On 11/30/15 10:28, Ard Biesheuvel wrote:
>>>> On 30 November 2015 at 10:22, Shannon Zhao <[email protected]>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 2015/11/30 16:53, Laszlo Ersek wrote:
>>>>>> On 11/29/15 07:31, Shannon Zhao wrote:
>>>>>>> From: Shannon Zhao <[email protected]>
>>>>>>>
>>>>>>> Here we add the memory space for the memory nodes except the lowest one
>>>>>>> in FDT. So these spaces will show up in the UEFI memory map.
>>>>>>>
>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>>>> Signed-off-by: Shannon Zhao <[email protected]>
>>>>>>> ---
>>>>>>> ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c | 34
>>>>>>> ++++++++++++++++++++++++++++++++++
>>>>>>> ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf | 4 ++++
>>>>>>> 2 files changed, 38 insertions(+)
>>>>>>>
>>>>>>> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>>>>>> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>>>>>> index 74f80d1..2c8d785 100644
>>>>>>> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>>>>>> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>>>>>> @@ -27,6 +27,7 @@
>>>>>>> #include <Library/HobLib.h>
>>>>>>> #include <libfdt.h>
>>>>>>> #include <Library/XenIoMmioLib.h>
>>>>>>> +#include <Library/DxeServicesTableLib.h>
>>>>>>>
>>>>>>> #include <Guid/Fdt.h>
>>>>>>> #include <Guid/VirtioMmioTransport.h>
>>>>>>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>>>>>> UINT64 FwCfgDataSize;
>>>>>>> UINT64 FwCfgDmaAddress;
>>>>>>> UINT64 FwCfgDmaSize;
>>>>>>> + UINT64 CurBase;
>>>>>>> + UINT64 CurSize;
>>>>>>>
>>>>>>> Hob = GetFirstGuidHob(&gFdtHobGuid);
>>>>>>> if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
>>>>>>> @@ -332,6 +335,37 @@ InitializeVirtFdtDxe (
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> + //
>>>>>>> + // Check for memory node and add the memory spaces expect the
>>>>>>> lowest one
>>>>>>> + //
>>>>>>
>>>>>> (1) Based on the DTB node, which looks like:
>>>>>>
>>>>>> memory {
>>>>>> reg = <0x0 0x40000000 0x0 0x8000000>;
>>>>>> device_type = "memory";
>>>>>> };
>>>>>>
>>>>>> I agree that checking it here, before we look at "compatible", is a good
>>>>>> thing.
>>>>>>
>>>>>> However, can you please factor this out into a separate function? (Just
>>>>>> within this file, as a STATIC function.)
>>>>>>
>>>>>> I propose that the function return a BOOLEAN:
>>>>>> - TRUE if the node was a match (either successfully or unsuccessfully);
>>>>>> in which case you can "continue;" with the next iteration directly
>>>>>> - FALSE, if there is no match (i.e., the current iteration must
>>>>>> proceed, in order to look for different device types)
>>>>>>
>>>>>
>>>>> Sure.
>>>>>
>>>>>>> + Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
>>>>>>> + if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
>>>>>>> + //
>>>>>>> + // Get the 'reg' property of this node. For now, we will assume
>>>>>>> + // two 8 byte quantities for base and size, respectively.
>>>>>>> + //
>>>>>>> + RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
>>>>>>> + if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
>>>>>>> +
>>>>>>> + CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>>>>>>> + CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
>>>>>>> +
>>>>>>> + if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
>>>>>>> + Status = gDS->AddMemorySpace(EfiGcdMemoryTypeSystemMemory,
>>>>>>> + CurBase, CurSize,
>>>>>>> + EFI_MEMORY_WB |
>>>>>>> EFI_MEMORY_RUNTIME);
>>>>>>
>>>>>> (1) The edk2 coding style requires a space between the function name (or
>>>>>> function pointer) and the opening paren.
>>>>>>
>>>>>> (2) Regarding the indentation of function arguments, they are aligned
>>>>>> with the first or (more usually:) second character of the function or
>>>>>> *member* name. In this case, they should be aligned with the second "d"
>>>>>> in "AddMemorySpace".
>>>>>>
>>>>> Thanks for explain the edk2 coding style. Sorry, I'm not familiar with
>>>>> this.
>>>>>
>>>>>> (3) The last argument is a bitmask of capabilities. As far as I
>>>>>> remember, all system memory regions I've seen dumped by Linux from the
>>>>>> UEFI memmap at startup had all of: UC | WC | WT | WB.
>>>>>>
>>>>>> I agree that RUNTIME and WB should be listed here, but do you have any
>>>>>> particular reason for not allowing UC|WC|WT?
>>>>>>
>>>>>> (4) Related question -- when you boot the guest kernel with this patch
>>>>>> in the firmware, and pass "efi=debug" to Linux, do the memory attributes
>>>>>> of the additional NUMA nodes look identically to the "base" memory? (I'm
>>>>>> asking this to see if we need additional gDS->SetMemorySpaceAttributes()
>>>>>> calls.)
>>>>>>
>>>>>> Can you paste a sample output from "efi=debug"?
>>>>>>
>>>>>
>>>>> Processing EFI memory map:
>>>>> 0x000004000000-0x000007ffffff [Memory Mapped I/O |RUN| |XP| | | |
>>>>> | | | |UC]
>>>>> 0x000009010000-0x000009010fff [Memory Mapped I/O |RUN| |XP| | | |
>>>>> | | | |UC]
>>>>> 0x000040000000-0x00004000ffff [Loader Data | | | | | | |
>>>>> |WB|WT|WC|UC]
>>>>> 0x000040010000-0x00004007ffff [Conventional Memory| | | | | | |
>>>>> |WB|WT|WC|UC]
>>>>>
>>>>> [cut some information here]
>>>>>
>>>>> 0x000060000000-0x0000bfffffff [Conventional Memory|RUN| | | | | |
>>>>> |WB| | | ]
>>>>>
>>>>> Looks like the region is showed with the attributes we set. While
>>>>> comparing with other Conventional Memory, it doesn't have RUNTIME. So
>>>>> drop it?
>>>>>
>>>>
>>>> You should not set the runtime bit, but you should set the UC, WC and
>>>> WT bits, as Laszlo mentioned.
>>>
>>> After reading the specs a bit more, I think the following should be done:
>>>
>>> - Call AddMemorySpace() with Capabilities=WB|WT|WC|UC (and nothing
>>> else) -- as you say. That is, in the existing call, clear RUNTIME, and
>>> add WT|WC|UC.
>>>
>>> - Then call SetMemorySpaceAttributes too, for the same range (each
>>> range), with Attributes=WB only. The default attributes (after the
>>> AddMemorySpace() call) don't seem to be specified by the PI spec, so
>>> we had better specify explicitly what caching we'd like. (The UEFI
>>> memmap dump also shows the capabilities only, not the current
>>> setting.)
>>>
>>> So the point here is to set WB|WT|WC|UC as capabilities, and WB as
>>> actual attribute.
>>
>> Sigh. This could get messy. gDS->SetMemorySpaceAttributes() is
>> ultimately implemented by the CPU arch protocol driver, which in this
>> case is ArmPkg/Drivers/CpuDxe.
>>
>> However, that driver is currently dispatched strictly later than VirtFdtDxe.
>>
>> Should we add ArmPkg/Drivers/CpuDxe to the APRIORI file too, just before
>> VirtFdtDxe?
>>
>> Or else, if we don't call gDS->SetMemorySpaceAttributes(), what do you
>> think will be the default caching attribute for the NUMA ranges?
>>
>> It appears the simplest thing to try is to add CpuDxe to APRIORI DXE.
>>
>
> Couldn't we simply add EFI_RESOURCE_SYSTEM_MEMORY resource descriptor
> HOBs the first time we enumerate the nodes?
I didn't suggest it for two reasons.
(1) I recalled that last time we had had a very long discussion about how the
DXE core selects the initial range for memory allocations (for its own
purposes, primarily). I had trouble remembering all the details now. So there
were three options for me:
- recommend the HOBs, and hope for the best, i.e., hope whatever the DXE core
picks will be good for us
- recommend the HOBs, and actually review what the DXE core does (it was
modified a little bit last time)
- recommend a single HOB (which implies the DXE core gets initialized exactly
the same as before, no thinking or code browsing needed), and delay the
installation of the addtional ranges until later.
I picked the last option for simplicity.
(2) The other reason is that I don't think that the HOB approach would solve
the question of caching attributes. I don't think the DXE core tries to set any
attributes on its own when the GCD is primed from the HOBs.
Remember we have
InitializeMemory()
[ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
ArmPlatformInitializeSystemMemory()
[ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
PcdSet64 (PcdSystemMemorySize, ...)
MemoryPeim()
[ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
PcdGet64 (PcdSystemMemorySize)
BuildResourceDescriptorHob()
InitMmu()
[ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
ArmPlatformGetVirtualMemoryMap()
[ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
PcdGet64 (PcdSystemMemorySize)
ArmConfigureMmu()
[ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c]
In other words, all of the following happens in the memory init PEIM (which
comes from ArmPlatformPkg, but it's plugged chock full of our ArmVirtPkg
library instances):
- the in-DRAM DTB is initially parsed for the memory node
- the size PCD is set
- the base PCD is verified
- a memory descriptor HOB is built, dependent on the PCDs
- a virtual memory map is built, with caching attributes, dependent on the PCDs
- the MMU is configured
Therefore we have a nice fast caching setting for the "base" memory node only
because you cover it explicitly in ArmPlatformGetVirtualMemoryMap() --
dependent on the PCDs --, not because the DXE core covers it when it processes
the HOBs. So if you want to process several memory nodes *for real* in the the
memory init PEIM, then not only do you have to create the HOBs there, but also
extend the virtual memory map for the MMU similarly. And a fixed count of PCDs
won't be enough to carry the information from the DTB to
ArmPlatformGetVirtualMemoryMap() -- some loop would have to exist that connects
the DTB with the virtual memory map.
This is what I meant in my original response by "having more flexibility in
DXE".
Thanks
Laszlo
>
>
>>> - If either of these calls fails, it should be logged on error level
>>> (naming the range and also which call of the above two failed), but
>>> we should continue processing the DTB either way.
>>>
>>> Thanks
>>> Laszlo
>>>
>>>>
>>>>>>> + if (EFI_ERROR(Status)) {
>>>>>>> + ASSERT_EFI_ERROR(Status);
>>>>>>> + }
>>>>>>
>>>>>> (5) "ASSERT_EFI_ERROR (Status)" would suffice.
>>>>>>
>>>>>> However, I think I'd prefer if we just logged an error here (with
>>>>>> EFI_D_ERROR level), and continued (i.e., returned TRUE).
>>>>>>
>>>>>>> + DEBUG ((EFI_D_INFO, "%a: Add System RAM @ 0x%lx - 0x%lx\n",
>>>>>>> + __FUNCTION__, CurBase, CurBase + CurSize - 1));
>>>>>>> + }
>>>>>>
>>>>>> (6) This isn't indented correctly (not just the arguments).
>>>>>>
>>>>>>> + } else {
>>>>>>> + DEBUG ((EFI_D_ERROR, "%a: Failed to parse FDT memory node\n",
>>>>>>> + __FUNCTION__));
>>>>>>> + }
>>>>>>
>>>>>> (7) I think this error log branch is not necessary here. The same will
>>>>>> have been logged in the code that is modified by patch #1.
>>>>>>
>>>>>
>>>>> Will fix these and re-send a new version.
>>>>>
>>>>> Thanks,
>>>>> --
>>>>> Shannon
>>>>>
>>>
>>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
