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?
>> + 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
