comments below On 08/27/14 17:12, Ard Biesheuvel wrote:
> diff --git > a/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/AArch64KVMLib.inf > > b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/AArch64KVMLib.inf > new file mode 100644 > index 000000000000..b30d70f3a1ed > --- /dev/null > +++ > b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/AArch64KVMLib.inf > @@ -0,0 +1,57 @@ > +#/* @file > +# Copyright (c) 2011-2013, ARM Limited. All rights reserved. > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD > License > +# which accompanies this distribution. The full text of the license may be > found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > +# > +#*/ > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = AArch64KVMLib BASE_NAME and containing directory name should be cleaned up, plus perhaps Aarch64 reference should be generalized > + FILE_GUID = 00214cc1-06d1-45fe-9700-dca5726ad7bf > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = ArmPlatformLib > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + ArmPkg/ArmPkg.dec > + ArmPlatformPkg/ArmPlatformPkg.dec > + > +[LibraryClasses] > + IoLib > + ArmLib > + PrintLib > + FdtLib > + > +[Sources.common] > + KVM.c > + KVMMem.c > + > +[Sources.AARCH64] > + KVMHelper.S | GCC > + > +[FeaturePcd] > + gEmbeddedTokenSpaceGuid.PcdCacheEnable > + gArmPlatformTokenSpaceGuid.PcdSystemMemoryInitializeInSec > + > +[Pcd] > + gArmTokenSpaceGuid.PcdSystemMemoryBase > + gArmTokenSpaceGuid.PcdSystemMemorySize > + gArmTokenSpaceGuid.PcdDeviceTreeBaseAddress > + > +[FixedPcd] > + gArmTokenSpaceGuid.PcdArmPrimaryCoreMask > + gArmTokenSpaceGuid.PcdArmPrimaryCore > + > + gArmPlatformTokenSpaceGuid.PcdCoreCount > + gArmTokenSpaceGuid.PcdFdBaseAddress > + gArmTokenSpaceGuid.PcdFdSize > diff --git > a/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVM.c > > b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVM.c > new file mode 100644 > index 000000000000..cd9edfe5138b > --- /dev/null > +++ > b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVM.c > @@ -0,0 +1,143 @@ > +/** @file > +* > +* Copyright (c) 2011-2013, ARM Limited. All rights reserved. > +* Copyright (c) 2014, Linaro Limited. All rights reserved. > +* > +* > +* This program and the accompanying materials > +* are licensed and made available under the terms and conditions of the BSD > License > +* which accompanies this distribution. The full text of the license may be > found at > +* http://opensource.org/licenses/bsd-license.php > +* > +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > +* > +**/ > + > +#include <Library/IoLib.h> > +#include <Library/ArmPlatformLib.h> > +#include <Library/DebugLib.h> > +#include <Library/PcdLib.h> > +#include <ArmPlatform.h> > +#include <libfdt.h> > + > +/** > + Return the current Boot Mode > + > + This function returns the boot reason on the platform > + > + @return Return the current Boot Mode of the platform > + > +**/ > +EFI_BOOT_MODE > +ArmPlatformGetBootMode ( > + VOID > + ) > +{ > + return BOOT_WITH_FULL_CONFIGURATION; > +} Yes, this seems sane. > + > +/** > + Initialize controllers that must setup in the normal world > + > + This function is called by the ArmPlatformPkg/Pei or > ArmPlatformPkg/Pei/PlatformPeim > + in the PEI phase. > + > +**/ > +RETURN_STATUS > +ArmPlatformInitialize ( > + IN UINTN MpId > + ) > +{ > + // > + // We are relying on ArmPlatformInitializeSystemMemory () being called from > + // InitializeMemory (), which only occurs if the following feature is > disabled > + // > + ASSERT (!FeaturePcdGet (PcdSystemMemoryInitializeInSec)); > + return RETURN_SUCCESS; > +} Great; we agreed upon this in <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/8969/focus=9040>. Thanks. > + > +/** > + Initialize the system (or sometimes called permanent) memory > + > + This memory is generally represented by the DRAM. > + > +**/ > +VOID > +ArmPlatformInitializeSystemMemory ( > + VOID > + ) > +{ > + VOID *DeviceTreeBase; > + INT32 Node, Prev; > + UINT64 NewBase = 0; > + UINT64 NewSize = 0; Initialization of auto variables is frowned upon in edk2; please use separate assignments. > + > + DeviceTreeBase = (VOID *)PcdGet64 (PcdDeviceTreeBaseAddress); I suggest an intervening (UINTN) cast here, otherwise you might get warnings in a 32-bit build. > + ASSERT (DeviceTreeBase != NULL); In the hope that I'm not suggesting something insane, you might also want to assert at this point that PcdDeviceTreeBaseAddress equals PcdSystemMemoryBase. > + > + // > + // Make sure we have a valid device tree blob at the base of DRAM > + // > + if (fdt_check_header (DeviceTreeBase) != 0) > + return; I believe this should be asserted instead. If this fails, we're doomed. > + > + // > + // Look for a memory node > + // TODO handle disjoint memory > + // Please drop the TODO; I don't think we want to handle disjoint memory very soon, and even then it would take more intrusive changes than what is marked here, locally, with this comment. > + for (Prev = 0;; Prev = Node) { > + CONST CHAR8 *Type; > + INT32 Len; > + > + Node = fdt_next_node (DeviceTreeBase, Prev, NULL); > + if (Node < 0) > + return; Braces. Also, I think we should rather break here, not return; the ASSERT()s after the loop should run in either case. If we don't find any good node, then the ASSERTs() should trigger. > + > + Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len); > + if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) { > + CONST UINT64 *RegProp; > + > + // > + // Get the 'reg' property of this node. For now, we will assume > + // two 8 byte quantities for base and size, respectively. > + // TODO use #cells root properties instead not sure if this TODO is up-to-date > + // > + RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len); > + if (RegProp != 0 && Len == (2 * sizeof (UINT64))) { > + > + NewBase = fdt64_to_cpu (RegProp[0]); > + NewSize = fdt64_to_cpu (RegProp[1]); > + > + PcdSet64 (PcdSystemMemoryBase, NewBase); > + PcdSet64 (PcdSystemMemorySize, NewSize); If I recall correctly, you had the idea that PcdSystemMemoryBase should not change (ie. we should assert that), and I think I agreed... Hm... Yes, it's in here: <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/8969/focus=9039>. Search for "NewBase to be different from 0x4000_0000". So please add that assert as well. (And, just to be fully safe, you might consider turning PcdSystemMemoryBase into fixed-at-build again!) > + > + DEBUG ((EFI_D_INFO, "KVM: System RAM @ 0x%lx - 0x%lx\n", > + NewBase, NewBase + NewSize - 1)); > + } else { > + DEBUG ((EFI_D_ERROR, "KVM: Failed to parse FDT memory node\n")); > + } > + break; > + } > + } > + // > + // We need to make sure that the machine we are running on has at least > + // 128 MB of memory configured, and does not run from shadowed NOR. This suggesting wording cleanup: "and is currently executing this binary from NOR flash". > + // prevents the device tree at base of DRAM from getting clobbered before > we "... from getting clobbered [when our caller installs permanent PEI RAM,] before ..." > + // have a chance of marking its location as reserved or moving it out of > the > + // way. ... copy it to a freshly allocated block in the permanent PEI RAM in the platform PEIM... I think we can be specific. > + // > + ASSERT (NewSize >= SIZE_128MB); > + ASSERT ((UINT64)PcdGet32 (PcdFdBaseAddress) + (UINT64)PcdGet32 (PcdFdSize) > <= NewBase > + || (UINT64)PcdGet32 (PcdFdBaseAddress) >= NewBase + NewSize); > +} Operator || should go to the end of the first line. Also, edk2's line wrapping style prescribes one or two spaces indentation, relative to the function's or macro's *name*, whose argument list you are wrapping. Like this: ASSERT ( ((UINT64)PcdGet32 (PcdFdBaseAddress) + (UINT64)PcdGet32 (PcdFdSize)) <= NewBase || (UINT64)PcdGet32 (PcdFdBaseAddress) >= NewBase + NewSize); Otherwise, this matches our discussion in the same message (linked above), thanks! > + > +VOID > +ArmPlatformGetPlatformPpiList ( > + OUT UINTN *PpiListSize, > + OUT EFI_PEI_PPI_DESCRIPTOR **PpiList > + ) > +{ > + *PpiListSize = 0; > + *PpiList = NULL; > +} I guess this is fine. > diff --git > a/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVMHelper.S > > b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVMHelper.S > new file mode 100644 > index 000000000000..af028038b4a6 > --- /dev/null > +++ > b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVMHelper.S > @@ -0,0 +1,86 @@ > +# > +# Copyright (c) 2011-2013, ARM Limited. All rights reserved. > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD > License > +# which accompanies this distribution. The full text of the license may be > found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > +# > +# > + > +#include <AsmMacroIoLibV8.h> > +#include <Base.h> > +#include <Library/ArmLib.h> > +#include <Library/PcdLib.h> > +#include <AutoGen.h> > + > +.text > +.align 2 > + > +GCC_ASM_EXPORT(ArmPlatformPeiBootAction) > +GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore) > +GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId) > +GCC_ASM_EXPORT(ArmPlatformGetCorePosition) > +GCC_ASM_EXPORT(ArmGetCpuCountPerCluster) > +GCC_ASM_EXPORT(ArmGetPhysAddrTop) > + > +GCC_ASM_IMPORT(_gPcd_FixedAtBuild_PcdArmPrimaryCore) > +GCC_ASM_IMPORT(_gPcd_FixedAtBuild_PcdArmPrimaryCoreMask) > +GCC_ASM_IMPORT(_gPcd_FixedAtBuild_PcdCoreCount) > + > +ASM_PFX(ArmPlatformPeiBootAction): > + ret > + > +//UINTN > +//ArmPlatformGetPrimaryCoreMpId ( > +// VOID > +// ); > +ASM_PFX(ArmPlatformGetPrimaryCoreMpId): > + LoadConstantToReg (_gPcd_FixedAtBuild_PcdArmPrimaryCore, x0) > + ldrh w0, [x0] > + ret > + > +# IN None > +# OUT x0 = number of cores present in the system > +ASM_PFX(ArmGetCpuCountPerCluster): > + mov x0, #1 > + ret > + > +//UINTN > +//ArmPlatformIsPrimaryCore ( > +// IN UINTN MpId > +// ); > +ASM_PFX(ArmPlatformIsPrimaryCore): > + mov x0, #1 > + ret > + > +//UINTN > +//ArmPlatformGetCorePosition ( > +// IN UINTN MpId > +// ); > +// With this function: CorePos = (ClusterId * 4) + CoreId > +ASM_PFX(ArmPlatformGetCorePosition): > + and x1, x0, #ARM_CORE_MASK > + and x0, x0, #ARM_CLUSTER_MASK > + add x0, x1, x0, LSR #6 > + ret > + > +//EFI_PHYSICAL_ADDRESS > +//GetPhysAddrTop ( > +// VOID > +// ); > +ASM_PFX(ArmGetPhysAddrTop): > + mrs x0, id_aa64mmfr0_el1 > + adr x1, .LPARanges > + and x0, x0, #7 > + ldrb w1, [x1, x0] > + mov x0, #1 > + lsl x0, x0, x1 > + ret > +.LPARanges: > + .byte 32, 36, 40, 42, 44, 48, -1, -1 > + > +ASM_FUNCTION_REMOVE_IF_UNREFERENCED This I can't validate... > diff --git > a/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVMMem.c > > b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVMMem.c > new file mode 100644 > index 000000000000..f1ea66468027 > --- /dev/null > +++ > b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVMMem.c > @@ -0,0 +1,102 @@ > +/** @file > +* > +* Copyright (c) 2011-2013, ARM Limited. All rights reserved. > +* > +* This program and the accompanying materials > +* are licensed and made available under the terms and conditions of the BSD > License > +* which accompanies this distribution. The full text of the license may be > found at > +* http://opensource.org/licenses/bsd-license.php > +* > +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > +* > +**/ > + > +#include <Library/ArmPlatformLib.h> > +#include <Library/DebugLib.h> > +#include <Library/PcdLib.h> > +#include <Library/IoLib.h> > +#include <Library/MemoryAllocationLib.h> > +#include <Library/ArmPlatformGlobalVariableLib.h> > +#include <ArmPlatform.h> > + > +// Number of Virtual Memory Map Descriptors without a Logic Tile > +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 4 > + > +// DDR attributes > +#define DDR_ATTRIBUTES_CACHED ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK > +#define DDR_ATTRIBUTES_UNCACHED > ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED > + > +EFI_PHYSICAL_ADDRESS > +ArmGetPhysAddrTop ( > + VOID > + ); > + > +/** > + Return the Virtual Memory Map of your platform > + > + This Virtual Memory Map is used by MemoryInitPei Module to initialize the > MMU > + on your platform. > + > + @param[out] VirtualMemoryMap Array of ARM_MEMORY_REGION_DESCRIPTOR > + describing a Physical-to-Virtual Memory > + mapping. This array must be ended by a > + zero-filled entry > + > +**/ > +VOID > +ArmPlatformGetVirtualMemoryMap ( > + IN ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap > + ) > +{ > + ARM_MEMORY_REGION_ATTRIBUTES CacheAttributes; > + ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable; > + > + ASSERT (VirtualMemoryMap != NULL); > + > + VirtualMemoryTable = AllocatePages (EFI_SIZE_TO_PAGES ( > + sizeof (ARM_MEMORY_REGION_DESCRIPTOR) > + * MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS)); The utterly pedantic formatting would be VirtualMemoryTable = AllocatePages ( EFI_SIZE_TO_PAGES ( sizeof (ARM_MEMORY_REGION_DESCRIPTOR) * MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS) ) ); I leave it to you... > + if (VirtualMemoryTable == NULL) { > + DEBUG ((EFI_D_ERROR, "%a: Error: Failed AllocatePages()\n", > __FUNCTION__)); > + return; > + } > + > + if (FeaturePcdGet (PcdCacheEnable) == TRUE) { > + CacheAttributes = DDR_ATTRIBUTES_CACHED; > + } else { > + CacheAttributes = DDR_ATTRIBUTES_UNCACHED; > + } Okay, we set this PCD to TRUE in the DSC. > + > + // System DRAM > + VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase); > + VirtualMemoryTable[0].VirtualBase = VirtualMemoryTable[0].PhysicalBase; > + VirtualMemoryTable[0].Length = PcdGet64 (PcdSystemMemorySize); > + VirtualMemoryTable[0].Attributes = CacheAttributes; > + > + DEBUG ((EFI_D_INFO, "%a: Dumping System DRAM Memory Map:\n" > + "\tPhysicalBase: 0x%lX\n" > + "\tVirtualBase: 0x%lX\n" > + "\tLength: 0x%lX\n", > + __FUNCTION__, > + VirtualMemoryTable[0].PhysicalBase, > + VirtualMemoryTable[0].VirtualBase, > + VirtualMemoryTable[0].Length)); > + > + // Peripheral space before DRAM > + VirtualMemoryTable[1].PhysicalBase = 0x0; > + VirtualMemoryTable[1].VirtualBase = 0x0; > + VirtualMemoryTable[1].Length = VirtualMemoryTable[0].PhysicalBase; > + VirtualMemoryTable[1].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > + > + // Peripheral space after DRAM > + VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + > VirtualMemoryTable[1].Length; > + VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase; > + VirtualMemoryTable[2].Length = ArmGetPhysAddrTop () - > VirtualMemoryTable[2].PhysicalBase; > + VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > + Seems reasonable. > + // End of Table > + VirtualMemoryTable[3] = (ARM_MEMORY_REGION_DESCRIPTOR){}; Yikes! Edk2 coding style violation galore! :) - Structure assignment! - Compound literal!! - GNUism in initializer list: {} -- meaning in fact { 0 }!!! Please replace this with a simple ZeroMem() call. > + > + *VirtualMemoryMap = VirtualMemoryTable; > +} Thanks! Laszlo ------------------------------------------------------------------------------ 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
