On 10/12/18 16:40, Sami Mujawar wrote:
> The KvmtoolPlatformDxe performs the platform specific
> initialization like:
> - It parses the kvmtool DT for Non-Volatile memory
> range to use for runtime variable storage and
> initialises the PcdEmuVariableNvStoreReserved.
> - It decides if the firmware should expose ACPI or
> Device Tree-based hardware description to the
> operating system.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sami Mujawar <[email protected]>
> ---
> The changes can be seen at
> https://github.com/samimujawar/edk2/commit/57ffa0da043fd73907b24a6833d2797ea3dae564
>
> Notes:
> v1:
> - Add kvmtool platform driver to support loading platform [SAMI]
> specific information.
>
> ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c | 211
> ++++++++++++++++++++
> ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf | 60 ++++++
> 2 files changed, 271 insertions(+)
>
> diff --git a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c
> b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..c8c1d50882bb9205194214217d17ed5f3644cc98
> --- /dev/null
> +++ b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c
> @@ -0,0 +1,211 @@
> +/** @file
> +
> + The KvmtoolPlatformDxe performs the platform specific initialization like:
> + - It parses the kvmtool DT for Non-Volatile memory range to use for runtime
> + variable storage and initialises the PcdEmuVariableNvStoreReserved.
> + - It decides if the firmware should expose ACPI or Device Tree-based
> + hardware description to the operating system.
> +
> + Copyright (c) 2018, 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/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiDriverEntryPoint.h>
> +#include <Protocol/FdtClient.h>
> +
> +/** Parse the kvmtool DT for Non-Volatile Memory range and initialize
> + PcdEmuVariableNvStoreReserved.
> +
> + @retval EFI_SUCCESS Success.
> + @retval EFI_ACCESS_DENIED Failed to update Pcd.
> + @retval EFI_BUFFER_TOO_SMALL Non-Volatile memory region less than storage
> + required for runtime variable storage.
> +**/
> +STATIC
> +EFI_STATUS
> +InitializeNvStorageBase (
> + VOID
> +)
> +{
> + EFI_STATUS Status;
> + FDT_CLIENT_PROTOCOL * FdtClient;
> + INT32 Node;
> + CONST UINT64 * Reg;
> + UINT32 Len;
> + UINT64 RegSize;
> + UINT64 RegBase;
> + RETURN_STATUS PcdStatus;
> +
> + Status = gBS->LocateProtocol (
> + &gFdtClientProtocolGuid,
> + NULL,
> + (VOID **)&FdtClient
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "Failed to locate Fdt Client Protocol. Status = %r\n",
> + Status
> + ));
> + ASSERT_EFI_ERROR (Status);
> + return Status;
> + }
> +
> + Status = FdtClient->FindNextCompatibleNode (
> + FdtClient,
> + "kvmtool,NVMem",
> + Node,
> + &Node
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "%a: Cannot find NV memory DT node to use for Runtime variable
> storage."
> + " Expected node in DT is \'compatible = \"kvmtool,NVMem\"\'."
> + " Status = %r\n",
> + __FUNCTION__,
> + Status
> + ));
> + ASSERT_EFI_ERROR (Status);
> + return Status;
> + }
> +
> + Status = FdtClient->GetNodeProperty (
> + FdtClient,
> + Node,
> + "reg",
> + (CONST VOID **)&Reg,
> + &Len
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "%a: GetNodeProperty () failed. Status = %r\n",
> + __FUNCTION__,
> + Status
> + ));
> + return Status;
> + }
> +
> + if (Len != (2 * sizeof (UINT64))) {
> + Status = EFI_INVALID_PARAMETER;
> + DEBUG ((
> + DEBUG_ERROR,
> + "%a: Invalid DT Node data. Status = %r\n",
> + __FUNCTION__,
> + Status
> + ));
> + return Status;
> + }
> +
> + RegBase = SwapBytes64 (((CONST UINT64 *)Reg)[0]);
> + RegSize = SwapBytes64 (((CONST UINT64 *)Reg)[1]);
> + DEBUG ((DEBUG_INFO, "RegBase = 0x%lx, RegSize = 0x%lx\n", RegBase,
> RegSize));
> +
> + if (RegSize < PcdGet32 (PcdVariableStoreSize)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "Not enough NV memory available for Runtime variable storage\n"
> + ));
> + return EFI_BUFFER_TOO_SMALL;
> + }
> +
> + PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, RegBase);
> + if (RETURN_ERROR (PcdStatus)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "Failed to update PcdEmuVariableNvStoreReserved. Status = %r\n",
> + PcdStatus
> + ));
> + ASSERT_RETURN_ERROR (PcdStatus);
> + return EFI_ACCESS_DENIED;
> + }
> + return EFI_SUCCESS;
> +}
It's likely best to keep this code in the platform-specific FVB driver
itself (see my comments on v1 3/6).
With the main point remaining, of course, that almost-non-volatile
variables are a very bad idea.
> +/** Decide if the firmware should expose ACPI tables or Device Tree and
> + install the appropriate protocol interface.
> +
> + @param [in] ImageHandle Handle for this image.
> +
> + @retval EFI_SUCCESS Success.
> + @retval EFI_OUT_OF_RESOURCES There was not enough memory to install the
> + protocols.
> + @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +PlatformHasAcpiDt (
> + IN EFI_HANDLE ImageHandle
> + )
> +{
> + if (!PcdGetBool (PcdForceNoAcpi)) {
> + // Expose ACPI tables
> + return gBS->InstallProtocolInterface (
> + &ImageHandle,
> + &gEdkiiPlatformHasAcpiGuid,
> + EFI_NATIVE_INTERFACE,
> + NULL
> + );
> + }
> +
> + // Expose the Device Tree.
> + return gBS->InstallProtocolInterface (
> + &ImageHandle,
> + &gEdkiiPlatformHasDeviceTreeGuid,
> + EFI_NATIVE_INTERFACE,
> + NULL
> + );
> +}
This function seems to be derived from
"ArmVirtPkg/PlatformHasAcpiDtDxe", by dropping the word size check, and
the fw_cfg check. Is that correct? I think it's worth documenting.
(Especially the fact that, on this platform, PcdForceNoAcpi will be a
plain dynamic PCD, not an HII one; which is why the INF file need not
depend on the variable architectural protocol.)
Thanks
Laszlo
> +
> +/** Entry point for Kvmtool Platform Dxe
> +
> + @param [in] ImageHandle Handle for this image.
> + @param [in] SystemTable Pointer to the EFI system table.
> +
> + @retval EFI_SUCCESS Success.
> + @retval EFI_OUT_OF_RESOURCES There was not enough memory to install the
> + protocols.
> + @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +KvmtoolPlatformDxeEntryPoint (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + EFI_STATUS Status;
> +
> + Status = InitializeNvStorageBase ();
> + if (EFI_ERROR (Status)) {
> + goto Failed;
> + }
> +
> + Status = PlatformHasAcpiDt (ImageHandle);
> + if (EFI_ERROR (Status)) {
> + goto Failed;
> + }
> +
> + return Status;
> +
> +Failed:
> + ASSERT_EFI_ERROR (Status);
> + CpuDeadLoop ();
> +
> + return Status;
> +}
> diff --git a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
> b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..dddacacdc8e6182e0d063d32e3ae7ff8432c4b20
> --- /dev/null
> +++ b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf
> @@ -0,0 +1,60 @@
> +#/** @file
> +#
> +# The KvmtoolPlatformDxe performs the platform specific initialization like:
> +# - It parses the kvmtool DT for Non-Volatile memory range to use for
> runtime
> +# variable storage and initialises the PcdEmuVariableNvStoreReserved.
> +# - It decides if the firmware should expose ACPI or Device Tree-based
> +# hardware description to the operating system.
> +#
> +# Copyright (c) 2018, 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 = 0x0001001B
> + BASE_NAME = KvmtoolPlatformDxe
> + FILE_GUID = 7479CCCD-D721-442A-8C73-A72DBB886669
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING = 1.0
> + ENTRY_POINT = KvmtoolPlatformDxeEntryPoint
> +
> +[Sources]
> + KvmtoolPlatformDxe.c
> +
> +[Packages]
> + ArmVirtPkg/ArmVirtPkg.dec
> + EmbeddedPkg/EmbeddedPkg.dec
> + MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> +
> +[LibraryClasses]
> + BaseLib
> + BaseMemoryLib
> + DebugLib
> + MemoryAllocationLib
> + UefiBootServicesTableLib
> + UefiDriverEntryPoint
> +
> +[Guids]
> + gEdkiiPlatformHasAcpiGuid ## SOMETIMES_PRODUCES ## PROTOCOL
> + gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL
> +
> +[Pcd]
> + gArmVirtTokenSpaceGuid.PcdForceNoAcpi
> + gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
> + gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize
> +
> +[Protocols]
> + gFdtClientProtocolGuid ## CONSUMES
> +
> +[Depex]
> + gFdtClientProtocolGuid
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
