On 27 August 2014 01:36, Laszlo Ersek <[email protected]> wrote:
> I'll skip the DSC, FDF and INF files; I assume those are customized
> copies from existing platform files. I'll also skip the INF files for now.
>
> On 08/26/14 15:03, Ard Biesheuvel wrote:
>> This adds support for executing UEFI in a QEMU/mach-virt emulated
>> environment.
>> The following assumptions are made about the target:
>> - ARM architected timer
>> - PL011 UART at 0x9000000
>> - at least 1 MB of DRAM at 0x40000000, containing the device tree blob
>>
>> The following information is retrieved from the device tree:
>> - GIC base addresses
>> - virtual timer interrupt
>> - RTC base address
>> - system memory base and size
>> - virtio MMIO transports
>>
>> The DTB image is relocated and installed as a configuration table so an
>> EFI stub enabled kernel can be booted directly without the need for a
>> bootloader.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Michael Casadevall <[email protected]>
>> Signed-off-by: Ard Biesheuvel <[email protected]>
>> ---
>> .../AArch64Virtualization-KVM.dsc | 223 +++++++++++++
>> .../AArch64Virtualization-KVM.fdf | 307 ++++++++++++++++++
>> .../AArch64Virtualization.dsc.inc | 354
>> +++++++++++++++++++++
>> .../AArch64VirtualizationPkg/Driver/VirtFdt.inf | 60 ++++
>> .../AArch64VirtualizationPkg/Driver/VirtFdtDxe.c | 229 +++++++++++++
>> .../Include/Platform/KVM/ArmPlatform.h | 32 ++
>> .../AArch64VirtualizationLibKVM/AArch64KVMLib.inf | 53 +++
>> .../Library/AArch64VirtualizationLibKVM/KVM.c | 127 ++++++++
>> .../AArch64VirtualizationLibKVM/KVMHelper.S | 70 ++++
>> .../Library/AArch64VirtualizationLibKVM/KVMMem.c | 106 ++++++
>> .../AArch64VirtualizationSysConfigLibKVM.c | 95 ++++++
>> .../AArch64VirtualizationSysConfigLibKVM.inf | 35 ++
>> .../Library/ResetSystemLib/ResetSystemLib.c | 96 ++++++
>> .../Library/ResetSystemLib/ResetSystemLib.inf | 33 ++
>> 14 files changed, 1820 insertions(+)
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/AArch64Virtualization-KVM.dsc
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/AArch64Virtualization-KVM.fdf
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/AArch64Virtualization.dsc.inc
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/Driver/VirtFdt.inf
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/Driver/VirtFdtDxe.c
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/Include/Platform/KVM/ArmPlatform.h
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/AArch64KVMLib.inf
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVM.c
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVMHelper.S
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVMMem.c
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationSysConfigLibKVM/AArch64VirtualizationSysConfigLibKVM.c
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationSysConfigLibKVM/AArch64VirtualizationSysConfigLibKVM.inf
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/Library/ResetSystemLib/ResetSystemLib.c
>> create mode 100644
>> ArmPlatformPkg/AArch64VirtualizationPkg/Library/ResetSystemLib/ResetSystemLib.inf
>
> [snip]
>
>> diff --git a/ArmPlatformPkg/AArch64VirtualizationPkg/Driver/VirtFdtDxe.c
>> b/ArmPlatformPkg/AArch64VirtualizationPkg/Driver/VirtFdtDxe.c
>> new file mode 100644
>> index 000000000000..04840e5d1a57
>> --- /dev/null
>> +++ b/ArmPlatformPkg/AArch64VirtualizationPkg/Driver/VirtFdtDxe.c
>> @@ -0,0 +1,229 @@
>> +/** @file
>> +* Device tree enumeration DXE driver for AArch64 VMs
>> +*
>> +* Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
>> +* 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/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/UefiLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/UefiDriverEntryPoint.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/VirtioMmioDeviceLib.h>
>> +#include <Library/DevicePathLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/DxeServicesLib.h>
>> +#include <libfdt.h>
>> +
>> +#include <Guid/Fdt.h>
>> +
>> +#pragma pack (1)
>> +typedef struct {
>> + VENDOR_DEVICE_PATH Vendor;
>> + UINT32 Instance;
>> + EFI_DEVICE_PATH_PROTOCOL End;
>> +} VIRTIO_BLK_DEVICE_PATH;
>> +#pragma pack ()
>
> Ugh. :)
>
> Have you seen my earlier patches where the vendor device path node
> corresponding to a virtio-mmio transport actually included the base
> address of the MMIO register block? I think such a hwvendor devpath node
> is much more useful; it can actually help you debug.
>
I will just stick a EFI_PHYSICAL_ADDRESS in there instead, or would
you prefer UINT64?
> In any case, if you decide to stick with this hwvendor devpath node,
> plase don't call it VIRTIO_BLK. Call it VIRTIO_MMIO, or
> VIRTIO_TRANSPORT, or something else virtio-device-type-agnostic.
>
>> +
>> +static struct _Ptypes {
>> + enum _Ptype {
>> + UNKNOWN, GIC, RTC, VIRTIO, UART, TIMER
>> + } Ptype;
>> + CHAR8 Comp[20];
>> +} const Ptypes[] = {
>> + { GIC, "arm,cortex-a15-gic" },
>> + { RTC, "arm,pl031" },
>> + { VIRTIO, "virtio,mmio" },
>> + { UART, "arm,pl011" },
>> + { TIMER, "arm,armv7-timer" },
>> + { TIMER, "arm,armv8-timer" },
>> + { UNKNOWN, "" }
>> +};
>
> This is a smorgasbord of edk2 coding style violations. Rather than
> explaining each, let me just rewrite the definition. I'll assume that
> "P" stands for "Property".
>
> typedef enum {
> PropertyTypeUnknown,
> PropertyTypeGic,
> PropertyTypeRtc,
> PropertyTypeVirtio,
> ...
> } PROPERTY_TYPE;
>
> typedef struct {
> PROPERTY_TYPE Type;
> CHAR8 Compatible[20];
> } PROPERTY;
>
> STATIC CONST PROPERTY[] = {
> ...
> };
>
>> +
>> +enum _Ptype GetTypeFromNode (CHAR8 CONST *NodeType, INT32 Size)
>
> STATIC
> PROPERTY_TYPE
> GetTypeFromNode (
> IN CONST CHAR8 *NodeType,
> IN UINTN Size
> )
> {
> ...
> }
>
>> +{
>> + CHAR8 CONST *s;
>
> CONST goes first. Please rename "s" to something sensible, and make it
> CamelCase.
>
> (... Yes, I know.)
>
OK, thanks, will fix it up
>> +
>> + /*
>> + * A 'compatible' node may contain a sequence of NULL terminated
>> + * compatible strings so check each one
>> + */
>
> //
> // A 'compatible' node ...
> // ...
> //
>
ok
>> + for (s = NodeType; s < NodeType + Size && *s; s += 1 + AsciiStrLen (s)) {
>> + struct _Ptypes const *Type;
>> +
>> + for (Type = Ptypes; Type->Comp[0]; Type++)
>
> Braces are mandatory.
>
ok
>> + if (AsciiStrCmp (Type->Comp, s) == 0)
>> + return Type->Ptype;
>> + }
>> + return UNKNOWN;
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +InitializeVirtFdtDxe (
>> + IN EFI_HANDLE ImageHandle,
>> + IN EFI_SYSTEM_TABLE *SystemTable
>> + )
>> +{
>> + VOID *FdtImageData;
>> + VOID *MemoryBase;
>> + INT32 Node, Prev, VirtIoInstance;
>> + EFI_STATUS Status;
>> +
>> + MemoryBase = (VOID *)PcdGet64 (PcdSystemMemoryBase);
>
> Given that this code is exclusively 64-bit, this should be fine.
>
>> +
>> + if (fdt_check_header (MemoryBase) != 0) {
>> + DEBUG ((EFI_D_ERROR, "InitializeVirtFdtDxe: No DTB found @ 0x%08x\n",
>> + MemoryBase));
>
> I suggest "%a" instead of "InitializeVirtFdtDxe" in the format string,
> and __FUNCTION__ in the variable argument list, accordingly.
>
> And, referencing my v2 6/8 review, the DTB may have been corrupted by
> the time we get here.
>
>> + return EFI_NOT_FOUND;
>> + }
>> +
>> + FdtImageData = AllocatePages (EFI_SIZE_TO_PAGES (fdt_totalsize
>> (MemoryBase)));
>
> What's wrong with AllocatePool()? Does the EFI stub require full pages,
> and page alignment?
>
No, AllocatePool() should be fine. This was already in the code I
inherited from Michael Casadevall, so I didn't think about it twice.
>> + if (FdtImageData == NULL) {
>> + DEBUG ((EFI_D_ERROR, "InitializeVirtFdtDxe: AllocatePages() failed\n"));
>
> "%a", __FUNCTION
>
>> + return EFI_OUT_OF_RESOURCES;
>> + }
>> +
>> + CopyMem (FdtImageData, MemoryBase, fdt_totalsize (MemoryBase));
>> + Status = gBS->InstallConfigurationTable (&gFdtTableGuid, FdtImageData);
>> + ASSERT_EFI_ERROR (Status);
>> +
>> + DEBUG ((EFI_D_ERROR, "InitializeVirtFdtDxe: DTB @ 0x%08x\n",
>> FdtImageData));
>
> Yow. The frequent problem of ARM builds, namely that they suppress all
> debug messages under EFI_D_ERROR, should not be remedied by printing
> everything as EFI_D_ERROR. EFI_D_ERROR should be reserved for, well,
> errors. This is an EFI_D_INFO or EFI_D_VERBOSE message.
>
> The problem of over-suppressing is remedied by enabling EFI_D_INFO and
> maybe even EFI_D_VERBOSE in the DSC file, PcdDebugPrintErrorLevel.
>
ok
> In addition, the %x format specifier is inappropriate for a pointer. You
> should use %p. If you'd like zero padding (and let's then also consider
> this is 64-bit code only), then:
>
> DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%016Lx\n",
> __FUNCTION__, (UINT64)(UINTN)FdtImageData));
>
ok
>> +
>> + /*
>> + * Now enumerate the nodes and install peripherals that we are interested
>> in,
>> + * i.e., GIC, RTC and virtio MMIO nodes
>> + */
>
> //
> // comment
> //
>
>> + Status = EFI_SUCCESS;
>> +
>> + for (Prev = VirtIoInstance = 0; Status == EFI_SUCCESS; Prev = Node) {
>
> Please say
>
> Prev = 0;
> VirtIoInstance = 0;
>
>
VirtIoInstance will be removed anyway
>> + CONST CHAR8 *Type;
>> + INT32 Len;
>
> (Usually UINTN is preferred for sizes and lengths.)
>
>> + enum _Ptype Ptype;
>> + VOID CONST *RegProp;
>> +
>> + Node = fdt_next_node (MemoryBase, Prev, NULL);
>> + if (Node < 0)
>> + break;
>> +
>> + Type = fdt_getprop (MemoryBase, Node, "compatible", &Len);
>> + if (Type == NULL)
>> + continue;
>> +
>> + Ptype = GetTypeFromNode (Type, Len);
>> + if (Ptype == UNKNOWN)
>> + continue;
>> +
>> + /*
>> + * Get the 'reg' property of this node. For now, we will assume
>> + * 8 byte quantities for base and size, respectively.
>> + * TODO use #cells root properties instead
>> + */
>
> Prior comments apply (braces, CONST, comment style, etc).
>
>> + RegProp = fdt_getprop (MemoryBase, Node, "reg", &Len);
>> + ASSERT (RegProp != NULL || Ptype == TIMER);
>> +
>> + switch (Ptype) {
>> + VIRTIO_BLK_DEVICE_PATH *DevicePath;
>> + EFI_HANDLE Handle;
>> + UINT64 RegBase;
>> + UINT32 DistBase, CpuBase;
>
> Yikes! Definition of automatic storage duration objects in a switch
> block. Very valid, yet very unacceptable for edk2. :) Please move them out.
>
>> +
>> + case VIRTIO:
>> + /* Create a unique device path for this transport on the fly */
>
> //
> // comment
> //
>
>> + DevicePath = (VIRTIO_BLK_DEVICE_PATH *)CreateDeviceNode (
>> + HARDWARE_DEVICE_PATH,
>> + HW_VENDOR_DP,
>> + sizeof (VIRTIO_BLK_DEVICE_PATH));
>> +
>> + CopyMem (&DevicePath->Vendor.Guid, &gEfiCallerIdGuid, sizeof
>> (EFI_GUID));
>> + DevicePath->Instance = VirtIoInstance++;
>> + SetDevicePathNodeLength (&DevicePath->Vendor,
>> + sizeof (*DevicePath) - sizeof
>> (DevicePath->End));
>> + SetDevicePathEndNode (&DevicePath->End);
>
> Seems sane.
>
>> +
>> + Handle = NULL;
>> + Status = gBS->InstallProtocolInterface (&Handle,
>> + &gEfiDevicePathProtocolGuid, EFI_NATIVE_INTERFACE,
>> + DevicePath);
>> + if (EFI_ERROR (Status)) {
>> + DEBUG ((EFI_D_ERROR, "Failed to install VirtIO protocol
>> interface\n"));
>> + break;
>> + }
>
> Incorrect error message; what failed was the handle creation (requested
> by installing a devpath on an initially NULL handle).
>
> Also, if this fails, we'll correctly terminate the loop. However we will
> also simply leave the function without undoing / rolling back the virtio
> installations that did succeed. This may or may not be intended.
>
I will check the logic carefully. What should happen is that the
traversal of the device tree should proceed, so other peripherals are
detected and installed even if the virtio transports are not.
>> +
>> + RegBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>> + Status = VirtioMmioInstallDevice (RegBase, Handle);
>> + if (EFI_ERROR (Status))
>> + DEBUG ((EFI_D_ERROR, "Failed to install VirtIO transport @ 0x%x\n",
>> + RegBase));
>
> 0x%Lx (or lx, as you prefer)
>
>> + break;
>> +
>> + case GIC:
>> + ASSERT (Len == 32);
>> +
>> + DistBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>> + CpuBase = fdt64_to_cpu (((UINT64 *)RegProp)[2]);
>> + PcdSet32 (PcdGicDistributorBase, DistBase);
>> + PcdSet32 (PcdGicInterruptInterfaceBase, CpuBase);
>
> DistBase and CpuBase are UINT32s, matching the PCDs, but they don't
> match the FDT (UINT64). Is that okay?
>
Well, it is highly unlikely that peripherals like GIC would be moved
beyond 4 GB in the physical address space, and the device tree just
uses 64-bit addresses in this instantiation, so for now, I think this
is fine. Otherwise, we should update the GIC driver to use UINT64
instead.
>> +
>> + DEBUG ((EFI_D_ERROR, "Found GIC @ 0x%x/0x%x\n", DistBase, CpuBase));
>
> EFI_D_INFO
>
>> + break;
>> +
>> + case RTC:
>> + ASSERT (Len == 16);
>> +
>> + RegBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>> + PcdSet32 (PcdPL031RtcBase, (UINT32)RegBase);
>
> Since we're asserting the length, we could also assert that conversion
> to UINT32 doesn't cause truncation.
>
Same applies as above, I will try to clean this up in a more consistent manner.
>> +
>> + DEBUG ((EFI_D_ERROR, "Found PL031 RTC @ 0x%x\n", RegBase));
>
> EFI_D_INFO, 0x%Lx
>
>> + break;
>> +
>> + case TIMER: {
>> + struct {
>> + UINT32 Type;
>> + UINT32 Number;
>> + UINT32 Flags;
>> + } const *Interrupts;
>
> please move the struct def out, and give it a typedef, as seen above
>
ok
>> + INT32 SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum;
>> +
>> + /*
>> + * - interrupts : Interrupt list for secure, non-secure, virtual and
>> + * hypervisor timers, in that order.
>> + */
>
> //
> // comment
> //
>
>> + Interrupts = fdt_getprop (MemoryBase, Node, "interrupts", &Len);
>> + ASSERT (Len == 48);
>> +
>> + SecIntrNum = fdt32_to_cpu (Interrupts[0].Number)
>> + + (Interrupts[0].Type ? 16 : 0);
>> + IntrNum = fdt32_to_cpu (Interrupts[1].Number)
>> + + (Interrupts[1].Type ? 16 : 0);
>> + VirtIntrNum = fdt32_to_cpu (Interrupts[2].Number)
>> + + (Interrupts[2].Type ? 16 : 0);
>> + HypIntrNum = fdt32_to_cpu (Interrupts[3].Number)
>> + + (Interrupts[3].Type ? 16 : 0);
>> +
>> + DEBUG ((EFI_D_ERROR, "Found Timer interrupts %d, %d, %d, %d\n",
>> + SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum));
>> +
>> + PcdSet32 (PcdArmArchTimerSecIntrNum, SecIntrNum);
>> + PcdSet32 (PcdArmArchTimerIntrNum, IntrNum);
>> + PcdSet32 (PcdArmArchTimerVirtIntrNum, VirtIntrNum);
>> + PcdSet32 (PcdArmArchTimerHypIntrNum, HypIntrNum);
>> + break;
>> + }
>> +
>> + default:
>> + break;
>> + }
>> + }
>> + return Status;
>> +}
>
>
> [snip]
>
>
>> diff --git
>> a/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVM.c
>>
>> b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVM.c
>> new file mode 100644
>> index 000000000000..dcbe076f5d58
>> --- /dev/null
>> +++
>> b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVM.c
>> @@ -0,0 +1,127 @@
>> +/** @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;
>> +}
>> +
>> +/**
>> + 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
>> + )
>> +{
>> + return RETURN_SUCCESS;
>> +}
>> +
>> +/**
>> + Initialize the system (or sometimes called permanent) memory
>> +
>> + This memory is generally represented by the DRAM.
>> +
>> +**/
>> +VOID
>> +ArmPlatformInitializeSystemMemory (
>> + VOID
>> + )
>> +{
>> + VOID *SystemMemoryBase;
>> + INT32 Node, Prev;
>> +
>> + SystemMemoryBase = (VOID *)PcdGet64 (PcdSystemMemoryBase);
>> +
>> + /*
>> + * Make sure we have a valid device tree blob at the base of DRAM
>> + */
>> + if (fdt_check_header (SystemMemoryBase) != 0)
>> + return;
>> +
>> + /*
>> + * Look for a memory node
>> + * TODO handle disjoint memory
>> + */
>> + for (Prev = 0;; Prev = Node) {
>> + CONST CHAR8 *Type;
>> + INT32 Len;
>> +
>> + Node = fdt_next_node (SystemMemoryBase, Prev, NULL);
>> + if (Node < 0)
>> + return;
>> +
>> + Type = fdt_getprop (SystemMemoryBase, Node, "device_type", &Len);
>> + if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
>> + UINT64 CONST *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
>> + */
>> + RegProp = fdt_getprop (SystemMemoryBase, Node, "reg", &Len);
>> + if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
>> + UINT64 NewBase, NewSize;
>> +
>> + NewBase = fdt64_to_cpu (RegProp[0]);
>> + NewSize = fdt64_to_cpu (RegProp[1]);
>> +
>> + PcdSet64 (PcdSystemMemoryBase, NewBase);
>> + PcdSet64 (PcdSystemMemorySize, NewSize);
>> +
>> + DEBUG ((EFI_D_ERROR, "KVM: System RAM @ 0x%lx - 0x%lx\n",
>> + NewBase, NewBase + NewSize - 1));
>> + }
>> + continue;
>> + }
>> + }
>> +}
>
> Too tired to check semantics overly deeply. Prior style remarks apply.
>
> In addition, I think the "continue" is superfluous.
>
>
>> +
>> +EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
>> +};
>
> GNUism, please avoid. Please spell out
>
> ... gPlatformPpiTable[] = { { 0 } };
>
ok
>> +
>> +VOID
>> +ArmPlatformGetPlatformPpiList (
>> + OUT UINTN *PpiListSize,
>> + OUT EFI_PEI_PPI_DESCRIPTOR **PpiList
>> + )
>> +{
>> + *PpiListSize = sizeof (gPlatformPpiTable);
>> + *PpiList = gPlatformPpiTable;
>> +}
>
> [snip KVMHelper.S]
>
>> diff --git
>> a/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVMMem.c
>>
>> b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVMMem.c
>> new file mode 100644
>> index 000000000000..55dcf31a5666
>> --- /dev/null
>> +++
>> b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/AArch64VirtualizationLibKVM/KVMMem.c
>> @@ -0,0 +1,106 @@
>> +/** @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
>> +
>> +/**
>> + 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;
>> + EFI_PHYSICAL_ADDRESS PhysAddrTop;
>> +
>> + static INT16 const PARange[] = { 32, 36, 40, 42, 44, 48, -1, -1 };
>> +
>> + ASSERT (VirtualMemoryMap != NULL);
>> +
>> + /*
>> + * Get the max phys address from the CPU config registers
>> + */
>> + asm ("mrs %0, id_aa64mmfr0_el1" : "=r" (PhysAddrTop));
>
> This should be moved into a dedicated helper function / assembly
> language file.
>
ok
>> + PhysAddrTop = 1ULL << PARange[PhysAddrTop & 7];
>> + ASSERT (PhysAddrTop != 0);
>> +
>> + VirtualMemoryTable = AllocatePages (EFI_SIZE_TO_PAGES (
>> + sizeof (ARM_MEMORY_REGION_DESCRIPTOR)
>> + * MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS));
>> + if (VirtualMemoryTable == NULL) {
>> + DEBUG ((EFI_D_ERROR, "ArmPlatformGetVirtualMemoryMap: Error: Failed
>> AllocatePages()\n"));
>> + return;
>> + }
>> +
>> + if (FeaturePcdGet (PcdCacheEnable) == TRUE) {
>> + CacheAttributes = DDR_ATTRIBUTES_CACHED;
>> + } else {
>> + CacheAttributes = DDR_ATTRIBUTES_UNCACHED;
>> + }
>> +
>> + // 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_ERROR, "ArmPlatformGetVirtualMemoryMap: Dumping DDR Memory
>> Map:\n \
>> + DDR PhysicalBase: 0x%lX\n \
>> + VirtualBase: 0x%lX\n \
>> + Length: 0x%lX\n",
>> + 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 = PhysAddrTop -
>> VirtualMemoryTable[2].PhysicalBase;
>> + VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>> +
>> + // End of Table
>> + VirtualMemoryTable[3] = (ARM_MEMORY_REGION_DESCRIPTOR){};
>> +
>> + *VirtualMemoryMap = VirtualMemoryTable;
>> +}
>
> No comments as to validity, but style observations apply. In particular,
> kill those line-trailing backslashes, please; it's "perfectly possible "
> "to " "concatenate string " "literals".
>
> [snip]
>
I know, sorry about that.
>> diff --git
>> a/ArmPlatformPkg/AArch64VirtualizationPkg/Library/ResetSystemLib/ResetSystemLib.c
>>
>> b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/ResetSystemLib/ResetSystemLib.c
>> new file mode 100644
>> index 000000000000..e5edbc407b56
>> --- /dev/null
>> +++
>> b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/ResetSystemLib/ResetSystemLib.c
>> @@ -0,0 +1,96 @@
>> +/** @file
>> + Template library implementation to support ResetSystem Runtime call.
>> +
>> + Fill in the templates with what ever makes you system reset.
>> +
>> + Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
>> + Copyright (c) 2013, ARM Ltd. All rights reserved.<BR>
>> +
>> + 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 <PiDxe.h>
>> +
>> +#include <Library/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/EfiResetSystemLib.h>
>> +
>> +#include <ArmPlatform.h>
>> +
>> +/**
>> + Resets the entire platform.
>> +
>> + @param ResetType The type of reset to perform.
>> + @param ResetStatus The status code for the reset.
>> + @param DataSize The size, in bytes, of WatchdogData.
>> + @param ResetData For a ResetType of EfiResetCold,
>> EfiResetWarm, or
>> + EfiResetShutdown the data buffer starts
>> with a Null-terminated
>> + Unicode string, optionally followed by
>> additional binary data.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +LibResetSystem (
>> + IN EFI_RESET_TYPE ResetType,
>> + IN EFI_STATUS ResetStatus,
>> + IN UINTN DataSize,
>> + IN CHAR16 *ResetData OPTIONAL
>> + )
>> +{
>> + register UINTN Rx asm ("x0");
>> +
>> + switch (ResetType) {
>> +
>> + case EfiResetPlatformSpecific:
>> + // Map the platform specific reset as reboot
>> + case EfiResetWarm:
>> + // Map a warm reset into a cold reset
>> + case EfiResetCold:
>> + // Send a PSCI 0.2 SYSTEM_RESET command
>> + Rx = PSCI_0_2_FN_SYSTEM_RESET;
>> + break;
>> + case EfiResetShutdown:
>> + // Send a PSCI 0.2 SYSTEM_OFF command
>> + Rx = PSCI_0_2_FN_SYSTEM_OFF;
>> + break;
>> +
>> + default:
>> + ASSERT (FALSE);
>> + return EFI_UNSUPPORTED;
>> + }
>> +
>> + /*
>> + * Being a virtualized platform, we are allowed to assume that we
>> + * are running at EL1, so HVC is appropriate for making PSCI calls.
>> + */
>> + asm ("hvc #0" :: "r" (Rx));
>
> Should be a separate... etc etc
>
>> +
>> + // We should never be here
>> + while (1);
>> +}
>
> CpuDeadLoop() is more idiomatic in edk2.
>
>> +
>> +/**
>> + Initialize any infrastructure required for LibResetSystem () to function.
>> +
>> + @param ImageHandle The firmware allocated handle for the EFI image.
>> + @param SystemTable A pointer to the EFI System Table.
>> +
>> + @retval EFI_SUCCESS The constructor always returns EFI_SUCCESS.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +LibInitializeResetSystem (
>> + IN EFI_HANDLE ImageHandle,
>> + IN EFI_SYSTEM_TABLE *SystemTable
>> + )
>> +{
>> + return EFI_SUCCESS;
>> +}
>
Thanks again for taking the time.
--
Ard.
------------------------------------------------------------------------------
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
