On 30 November 2015 at 10:22, Shannon Zhao <zhaoshengl...@huawei.com> wrote:
>
>
> On 2015/11/30 16:53, Laszlo Ersek wrote:
>> On 11/29/15 07:31, Shannon Zhao wrote:
>>> From: Shannon Zhao <shannon.z...@linaro.org>
>>>
>>> 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 <shannon.z...@linaro.org>
>>> ---
>>> 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.
>>> + 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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
