Re: [PATCH v3 2/3] Acpi: Install Acpi tables for Cloud hypervisor


Jianyong Wu
 

Hi Sami,

All comments are accepted. Thanks for your elaborate review work!

BR
Jianyong

-----Original Message-----
From: Sami Mujawar <Sami.Mujawar@arm.com>
Sent: Monday, June 28, 2021 8:22 PM
To: Jianyong Wu <Jianyong.Wu@arm.com>; devel@edk2.groups.io
Cc: lersek@redhat.com; ardb+tianocore@kernel.org; Justin He
<Justin.He@arm.com>; nd <nd@arm.com>
Subject: Re: [PATCH v3 2/3] Acpi: Install Acpi tables for Cloud hypervisor

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.wu@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|0
x00
+ 000005
+
[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;
+}

Join devel@edk2.groups.io to automatically receive all group messages.