On 05/14/20 11:29, Ard Biesheuvel wrote: > On 5/14/20 10:40 AM, Sami Mujawar wrote: >> Kvmtool is a virtual machine manager that enables >> hosting KVM guests. It essentially provides an >> emulated platform for guest operating systems. >> >> Kvmtool hands of a device tree containing the >> current hardware configuration to the firmware. >> >> A standards-based operating system would use >> ACPI to consume the platform hardware >> information, while some operating systems may >> prefer to use Device Tree. >> >> The KvmtoolPlatformDxe performs the platform >> actions like determining if the firmware should >> expose ACPI or the Device Tree based hardware >> description to the operating system. >> >> Signed-off-by: Sami Mujawar <sami.muja...@arm.com> >> --- >> >> Notes: >> v2: >> - Updated according to review comments. [Sami] >> v1: >> - Add kvmtool platform driver to support loading platform [Sami] >> specific information. >> - Keep code to initialise the variable storage PCDs in the >> [Laszlo] >> platform-specific FVB driver. >> - Document code derived from >> [Laszlo] >> "ArmVirtPkg/PlatformHasAcpiDtDxe" >> Ref: https://edk2.groups.io/g/devel/topic/30915278#30757 >> >> ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c | 93 >> ++++++++++++++++++++ >> ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf | 47 ++++++++++ >> 2 files changed, 140 insertions(+) >> >> diff --git a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c >> b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c >> new file mode 100644 >> index >> 0000000000000000000000000000000000000000..e7568f66f5ebeb0423fc1c10345cd8dad0800d94 >> >> --- /dev/null >> +++ b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c >> @@ -0,0 +1,93 @@ >> +/** @file >> + >> + The KvmtoolPlatformDxe performs the platform specific >> initialization like: >> + - It decides if the firmware should expose ACPI or Device Tree-based >> + hardware description to the operating system. >> + >> + Copyright (c) 2018 - 2020, ARM Limited. All rights reserved. >> + >> + SPDX-License-Identifier: BSD-2-Clause-Patent >> + >> +**/ >> + >> +#include <Guid/VariableFormat.h> >> +#include <Library/BaseLib.h> >> +#include <Library/BaseMemoryLib.h> >> +#include <Library/DebugLib.h> >> +#include <Library/DxeServicesTableLib.h> >> +#include <Library/UefiBootServicesTableLib.h> >> +#include <Library/UefiDriverEntryPoint.h> >> +#include <Protocol/FdtClient.h> >> + >> +/** Decide if the firmware should expose ACPI tables or Device Tree and >> + install the appropriate protocol interface. >> + >> + Note: This function is derived from "ArmVirtPkg/PlatformHasAcpiDtDxe", >> + by dropping the word size check, and the fw_cfg check. >> + >> + @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 >> + ); >> +} >> + >> +/** 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 = PlatformHasAcpiDt (ImageHandle); >> + if (EFI_ERROR (Status)) { >> + goto Failed; >> + } >> + >> + return Status; >> + >> +Failed: >> + ASSERT_EFI_ERROR (Status); >> + CpuDeadLoop (); >> + >> + return Status; >> +} > > Please don't use CpuDeadLoop()s in your drivers. > > Installing a protocol on an image handle like this should not ever fail, > and if it does, it is unlikely to be an issue in the driver itself. So > just use ASSERT_EFI_ERROR() here, and return EFI_SUCCESS.
I think Sami just followed the original code in "ArmVirtPkg/PlatformHasAcpiDtDxe". I'm fine either way: Acked-by: Laszlo Ersek <ler...@redhat.com> Different question: Should we ask Sami to become a designated reviewer (in Maintainers.txt) for the kvmtool-specific modules under ArmVirtPkg? Personally I'm unlikely to use kvmtool. Thanks Laszlo > > >> diff --git a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf >> b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf >> new file mode 100644 >> index >> 0000000000000000000000000000000000000000..08a0fe5ce14469133479046385bdd48c22698639 >> >> --- /dev/null >> +++ b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf >> @@ -0,0 +1,47 @@ >> +#/** @file >> +# >> +# The KvmtoolPlatformDxe performs the platform specific >> initialization like: >> +# - It decides if the firmware should expose ACPI or Device Tree-based >> +# hardware description to the operating system. >> +# >> +# Copyright (c) 2018 - 2020, ARM Limited. All rights reserved. >> +# >> +# SPDX-License-Identifier: BSD-2-Clause-Patent >> +# >> +#**/ >> + >> +[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 >> + DxeServicesTableLib >> + MemoryAllocationLib >> + UefiBootServicesTableLib >> + UefiDriverEntryPoint >> + >> +[Guids] >> + gEdkiiPlatformHasAcpiGuid ## SOMETIMES_PRODUCES ## PROTOCOL >> + gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL >> + >> +[Pcd] >> + gArmVirtTokenSpaceGuid.PcdForceNoAcpi >> + >> +[Depex] >> + TRUE >> > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#59547): https://edk2.groups.io/g/devel/message/59547 Mute This Topic: https://groups.io/mt/74200911/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
