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


Jianyong Wu
 

Hi Sami,

Thanks for your rework on my patch. I tried the change and it works well. You can do what you like on the patch set.

Thanks
Jianyong

-----Original Message-----
From: Sami Mujawar <Sami.Mujawar@arm.com>
Sent: Tuesday, July 6, 2021 4:52 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 v4 2/3] Acpi: Install Acpi tables for Cloud hypervisor

Hi Jianyong,

I should have caught this earlier in my review. However, if you agree, I will
do the following changes before pushing the patch.

1. The subject line of the commit message does not confirm to the edk2
coding standard. It should have ‘ArmVirtPkg: <subject line for the patch>’
2. The ACPI table signature can be simplified further. Can you try the
following and let me know if it works, please?

diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
index f5a47aa7f3cd..51b012676e7d 100644
--- a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
@@ -52,8 +52,8 @@ FindAcpiTableProtocol ( EFI_STATUS EFIAPI
InstallCloudHvAcpiTables (
- IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
- )
+ IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
+ )
{
UINTN InstalledKey;
UINTN TableSize;
@@ -97,11 +97,12 @@ InstallCloudHvAcpiTables (
//
// Get DSDT from FADT
//
- if (DsdtPtr == NULL
- && !AsciiStrnCmp ((CHAR8 *)&((EFI_ACPI_COMMON_HEADER
*)AcpiTablePtr)->Signature, "FACP", 4)) {
+ if ((DsdtPtr == NULL)
+ && (EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE ==
+ ((EFI_ACPI_COMMON_HEADER *)AcpiTablePtr)->Signature)) {
DsdtPtr = (UINT64 *)(((EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE
*)AcpiTablePtr)->XDsdt);
}
- }
+ } // while

if (DsdtPtr == NULL) {
DEBUG ((DEBUG_ERROR, "%a: no DSDT found\n", __FUNCTION__));

Regards,

Sami Mujawar

On 05/07/2021, 11:07, "Jianyong Wu" <jianyong.wu@arm.com> 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.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
ArmVirtPkg/ArmVirtPkg.dec | 6 +
.../CloudHvAcpiPlatformDxe.inf | 47 ++++++
.../CloudHvAcpiPlatformDxe/CloudHvAcpi.c | 155
++++++++++++++++++
3 files changed, 208 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
x00000005
+
[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..f5a47aa7f3cd
--- /dev/null
+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
@@ -0,0 +1,155 @@
+/** @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
+
+ @return AcpiTable Protocol, which is used to handle Acpi Table, on
SUCCESS or NULL on FAILURE.
+
+**/
+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 AcpiProtocol, AcpiTablePtr or
DsdtPtr is NULL
+ and the size field embedded in the ACPI table pointed
+ by AcpiTablePtr or DsdtPtr are not in sync.
+ @return EFI_OUT_OF_RESOURCES Insufficient resources exist to
complete the request.
+ @return EFI_NOT_FOUND DSDT table not found.
+**/
+EFI_STATUS
+EFIAPI
+InstallCloudHvAcpiTables (
+ IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
+ )
+{
+ UINTN InstalledKey;
+ UINTN TableSize;
+ UINTN AcpiTableLength;
+ UINT64 RsdpPtr;
+ UINT64 XsdtPtr;
+ UINT64 TableOffset;
+ UINT64 AcpiTablePtr;
+ UINT64 *DsdtPtr = NULL;
+ EFI_STATUS Status;
+
+ if (AcpiProtocol == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ RsdpPtr = PcdGet64 (PcdCloudHvAcpiRsdpBaseAddress);
+ XsdtPtr = ((EFI_ACPI_6_3_ROOT_SYSTEM_DESCRIPTION_POINTER
*)RsdpPtr)->XsdtAddress;
+ AcpiTableLength = ((EFI_ACPI_COMMON_HEADER *)XsdtPtr)->Length;
+ TableOffset = sizeof (EFI_ACPI_DESCRIPTION_HEADER);
+
+ while (TableOffset < AcpiTableLength) {
+ AcpiTablePtr = *(UINT64 *)(XsdtPtr + TableOffset);
+ TableSize = ((EFI_ACPI_COMMON_HEADER *)AcpiTablePtr)->Length;
+
+ //
+ // Install ACPI tables from XSDT
+ //
+ Status = AcpiProtocol->InstallAcpiTable (
+ AcpiProtocol,
+ (VOID *)AcpiTablePtr,
+ TableSize,
+ &InstalledKey
+ );
+
+ if (EFI_ERROR(Status)) {
+ return Status;
+ }
+
+ TableOffset += sizeof (UINT64);
+
+ //
+ // Get DSDT from FADT
+ //
+ if (DsdtPtr == NULL
+ && !AsciiStrnCmp ((CHAR8 *)&((EFI_ACPI_COMMON_HEADER
*)AcpiTablePtr)->Signature, "FACP", 4)) {
+ DsdtPtr = (UINT64
*)(((EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE *)AcpiTablePtr)-
XDsdt);
+ }
+ }
+
+ if (DsdtPtr == NULL) {
+ DEBUG ((DEBUG_ERROR, "%a: no DSDT found\n", __FUNCTION__));
+ return EFI_NOT_FOUND;
+ }
+
+ //
+ // Install DSDT table
+ //
+ TableSize = ((EFI_ACPI_COMMON_HEADER *)DsdtPtr)->Length;
+ Status = AcpiProtocol->InstallAcpiTable (
+ AcpiProtocol,
+ 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 AcpiProtocol, AcpiTablePtr or
DsdtPtr is NULL
+ and the size field embedded in the ACPI table pointed to
+ by AcpiTablePtr or DsdtPtr are not in sync.
+ @return EFI_OUT_OF_RESOURCES Insufficient resources exist to
complete the request.
+ @return EFI_NOT_FOUND DSDT table not found
+**/
+EFI_STATUS
+EFIAPI
+CloudHvAcpiPlatformEntryPoint (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+
+ Status = InstallCloudHvAcpiTables (FindAcpiTableProtocol ());
+
+ if (EFI_ERROR(Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: Fail to install Acpi table: %r\n",
__FUNCTION__,
+ Status));
+ CpuDeadLoop ();
+ }
+
+ return EFI_SUCCESS;
+}
--
2.17.1

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