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)
> + 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".
(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"?
> + 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.
Thanks
Laszlo
> + }
> +
> Type = fdt_getprop (DeviceTreeBase, Node, "compatible", &Len);
> if (Type == NULL) {
> continue;
> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> index ee2503a..0e6927b 100644
> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> @@ -43,12 +43,16 @@
> VirtioMmioDeviceLib
> HobLib
> XenIoMmioLib
> + DxeServicesTableLib
>
> [Guids]
> gFdtTableGuid
> gVirtioMmioTransportGuid
> gFdtHobGuid
>
> +[FixedPcd]
> + gArmTokenSpaceGuid.PcdSystemMemoryBase
> +
> [Pcd]
> gArmVirtTokenSpaceGuid.PcdArmPsciMethod
> gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
