Hi Jianyong,
Please find my response inline marked [SAMI].
Regards,
Sami Mujawar
On 28/06/2021 10:55 AM, Jianyong Wu wrote:
There is no device like Fw-cfg in Qemu in Cloud Hypervisor, so a specific
Acpi handler is introduced here.
The handler implemented here is in a very simple way:
1. acquire the RSDP from the PCD variable in the top ".dsc";
2. get the XSDT address from RSDP structure;
3. get the ACPI tables following the XSDT structure and install them
one by one;
4. get DSDT address from FADT and install DSDT table.
Signed-off-by: Jianyong Wu <jianyong...@arm.com>
---
ArmVirtPkg/ArmVirtPkg.dec | 6 +
.../CloudHvAcpiPlatformDxe.inf | 47 ++++++
.../CloudHvAcpiPlatformDxe/CloudHvAcpi.c | 141 ++++++++++++++++++
3 files changed, 194 insertions(+)
create mode 100644
ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
create mode 100644 ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index bf82f7f1f3f2..4e4d758015bc 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -66,6 +66,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
#
gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF,
0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1,
0x4D}|VOID*|0x00000007
+ ##
+ # This is the physical address of Rsdp which is the core struct of Acpi.
+ # Cloud Hypervisor has no other way to pass Rsdp address to the guest except
use a PCD.
+ #
+ gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x0|UINT64|0x00000005
+
[PcdsDynamic]
#
# Whether to force disable ACPI, regardless of the fw_cfg settings
diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
new file mode 100644
index 000000000000..01de76486686
--- /dev/null
+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
@@ -0,0 +1,47 @@
+## @file
+# ACPI Platform Driver for Cloud Hypervisor
+#
+# Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = CloudHvgAcpiPlatform
+ FILE_GUID = 6c76e407-73f2-dc1c-938f-5d6c4691ea93
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = CloudHvAcpiPlatformEntryPoint
+
+#
+# The following information is for reference only and not required by the
build tools.
+#
+ VALID_ARCHITECTURES = AARCH64
+#
+
+[Sources]
+ CloudHvAcpi.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ OvmfPkg/OvmfPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
+
+[LibraryClasses]
+ BaseLib
+ DebugLib
+ MemoryAllocationLib
+ OrderedCollectionLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+
+[Protocols]
+ gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
+
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress
+
+[Depex]
+ gEfiAcpiTableProtocolGuid
diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
new file mode 100644
index 000000000000..0f1a50d63cd6
--- /dev/null
+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
@@ -0,0 +1,141 @@
+/** @file
+ Install Acpi tables for Cloud Hypervisor
+
+ Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/BaseLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <IndustryStandard/Acpi63.h>
+#include <Protocol/AcpiTable.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiDriverEntryPoint.h>
+#include <Library/DebugLib.h>
+
+/**
+ Find Acpi table Protocol and return it
[SAMI]Please add description of value returned by this function.
[/SAMI]
+**/
+STATIC
+EFI_ACPI_TABLE_PROTOCOL *
+FindAcpiTableProtocol (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ EFI_ACPI_TABLE_PROTOCOL *AcpiTable;
+
+ Status = gBS->LocateProtocol (
+ &gEfiAcpiTableProtocolGuid,
+ NULL,
+ (VOID**)&AcpiTable
+ );
+ ASSERT_EFI_ERROR (Status);
+ return AcpiTable;
+}
+
+/** Install Acpi tables for Cloud Hypervisor
+
+ @param [in] AcpiProtocol Acpi Protocol which is used to install Acpi
talbles
+
+ @return EFI_SUCCESS The table was successfully inserted.
+ @return EFI_INVALID_PARAMETER Either AcpiTableBuffer is NULL, TableKey is
NULL, or AcpiTableBufferSize
+ and the size field embedded in the ACPI table
pointed to by AcpiTableBuffer
+ are not in sync.
+ @return EFI_OUT_OF_RESOURCES Insufficient resources exist to complete the
request.
+ @retval EFI_ACCESS_DENIED The table signature matches a table already
+ present in the system and platform policy
+ does not allow duplicate tables of this type.
+**/
+EFI_STATUS
+EFIAPI
+InstallCloudHvAcpiTables (
+ IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
+ )
+{
+ UINTN InstalledKey, TableSize, AcpiTableLength;
[SAMI] Define each local variable separately on a new line.
+ UINT64 RsdpPtr, XsdtPtr, TableOffset, AcpiTablePtr, DsdtPtr = ~0;
[SAMI] I think DsdtPtr can be UINT64 pointer and initialised to NULL
before first use.
+ EFI_STATUS Status = EFI_SUCCESS;
+ BOOLEAN GotFacp = FALSE;
[SAMI] I think GotFacp could probably be avoided, see comments below.
+
+ RsdpPtr = PcdGet64 (PcdCloudHvAcpiRsdpBaseAddress);
+ XsdtPtr = ((EFI_ACPI_6_3_ROOT_SYSTEM_DESCRIPTION_POINTER *)
RsdpPtr)->XsdtAddress;
[SAMI] No space between typecast and their object. Same comment for
similar instances in this patch series.
+ AcpiTableLength = ((EFI_ACPI_COMMON_HEADER *) XsdtPtr)->Length;
+ TableOffset = sizeof (EFI_ACPI_DESCRIPTION_HEADER);
+
[SAMI] Set DsdtPtr = NULL, here.
+ while (!EFI_ERROR(Status)
+ && (TableOffset < AcpiTableLength))
+ {
[SAMI] Starting curly brace at the end of previous line, please.
+ AcpiTablePtr = *(UINT64 *) (XsdtPtr + TableOffset);
+ TableSize = ((EFI_ACPI_COMMON_HEADER *) AcpiTablePtr)->Length;
+
+ //
+ // Install ACPI tables from XSDT
+ //
+ Status = AcpiProtocol->InstallAcpiTable (
[SAMI] AcpiProtocol pointer not checked before use. In release builds
the ASSERTs in FindAcpiTableProtocol() would vanish and a failure to get
the protocol would result in a crash when dereferencing the pointer here.
+ AcpiProtocol,
+ (VOID *)(UINT64)AcpiTablePtr,
[SAMI] Can you check if typecast to UINT64 is needed here, please?
Similarly, also check at other places.
+ TableSize,
+ &InstalledKey
+ );
[SAMI] Please reconsider error handling in this function. Probably best
check and return failure from here. This would mean the status would not
need to be checked in the while () statement and correspondingly there
is no need to initialise Status to EFI_SUCCESS at the begining of this
function.
+
+ TableOffset += sizeof (UINT64);
+
+ //
+ // Get DSDT from FADT
+ //
+ if (!GotFacp
+ && !AsciiStrnCmp ((CHAR8 *) &((EFI_ACPI_COMMON_HEADER *) AcpiTablePtr)->Signature,
"FACP", 4))
+ {
[SAMI] Curly brace on previous line, please. '!GotFacp' could be
replaced with (DsdtPtr != NULL).
+ DsdtPtr = ((EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE *)
AcpiTablePtr)->XDsdt;
+ GotFacp = TRUE;
+ }
+ }
+
+ if (DsdtPtr == ~0) {
[SAMI] Please change to' If (DsdtPtr == NULL)'.
+ DEBUG ((DEBUG_ERROR, "%a: no DSDT found\n", __FUNCTION__));
+ ASSERT (FALSE);
+ CpuDeadLoop ();
[SAMI] I think EFI_NOT_FOUND could be returned here, and the CpuDeadLoop
() could be moved to CloudHvAcpiPlatformEntryPoint().
+ }
+
+ //
+ // Install DSDT table
+ //
+ TableSize = ((EFI_ACPI_COMMON_HEADER *) DsdtPtr)->Length;
+ Status = AcpiProtocol->InstallAcpiTable (
+ AcpiProtocol,
+ (VOID *)(UINT64) DsdtPtr,
+ TableSize,
+ &InstalledKey
+ );
+
+ return Status;
+}
+
+/** Entry point for Cloud Hypervisor Platform Dxe
+
+ @param [in] ImageHandle Handle for this image.
+ @param [in] SystemTable Pointer to the EFI system table.
+
+ @return EFI_SUCCESS The table was successfully inserted.
+ @return EFI_INVALID_PARAMETER Either AcpiTableBuffer is NULL, TableKey is
NULL, or AcpiTableBufferSize
+ and the size field embedded in the ACPI table
pointed to by AcpiTableBuffer
+ are not in sync.
+ @return EFI_OUT_OF_RESOURCES Insufficient resources exist to complete the
request.
+ @retval EFI_ACCESS_DENIED The table signature matches a table already
+ present in the system and platform policy
+ does not allow duplicate tables of this type.
+**/
+EFI_STATUS
+EFIAPI
+CloudHvAcpiPlatformEntryPoint (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+
+ Status = InstallCloudHvAcpiTables (FindAcpiTableProtocol ());
[SAMI] Check status code here and on failure execute CpuDeadLoop ().
+ return Status;
+}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77179): https://edk2.groups.io/g/devel/message/77179
Mute This Topic: https://groups.io/mt/83841323/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
