Re: [PATCH v2 3/5] ArmVirtPkg: enable ACPI for cloud hypervisor


Jianyong Wu
 

Hi Laszlo,

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, May 19, 2021 2:26 PM
To: devel@edk2.groups.io; Jianyong Wu <Jianyong.Wu@arm.com>;
ardb+tianocore@kernel.org; Sami Mujawar <Sami.Mujawar@arm.com>
Cc: hao.a.wu@intel.com; Justin He <Justin.He@arm.com>
Subject: Re: [edk2-devel] [PATCH v2 3/5] ArmVirtPkg: enable ACPI for cloud
hypervisor

On 05/17/21 08:50, Jianyong Wu wrote:
The current implementation of PlatformHasAcpiDt is not a common
library and is on behalf of qemu. So give a specific version for Cloud
Hypervisor here.

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:
firstly, aquire the Rsdp address from the PCD varaible in the top
".dsc"; secondly, get the Xsdp address from Rsdp structure; thirdly,
get the Acpi tables following the Xsdp structrue and install it
(1) Please consider running a spell checker on the commit message ("aquire"
should be "acquire", "varaible" should be "variable", "structrue" should be
"structure"). Having this many typos in a short commit message gives the
patch a rushed vibe.

one by one.
Thanks for reminder, I will do the spell check before sending out next time.


Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
.../CloudHvAcpiPlatformDxe.inf | 51 +++++++++++++
.../CloudHvHasAcpiDtDxe.inf | 43 +++++++++++
.../CloudHvAcpiPlatformDxe/CloudHvAcpi.c | 73
+++++++++++++++++++
.../CloudHvHasAcpiDtDxe.c | 69 ++++++++++++++++++
4 files changed, 236 insertions(+)
create mode 100644
ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
create mode 100644
ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
create mode 100644 ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
create mode 100644
ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c
(2) Unless there is a specific reason for adding both drivers in the same patch,
please split them to separate patches.
Ok


diff --git
a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
new file mode 100644
index 000000000000..63c74e84eb27
--- /dev/null
+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
@@ -0,0 +1,51 @@
+## @file
+# OVMF ACPI Platform Driver for Cloud Hypervisor # # Copyright (c)
+2008 - 2014, Intel Corporation. All rights reserved.<BR>
(3) Missing ARM (C).
Yeah, I will add it.


+# SPDX-License-Identifier: BSD-2-Clause-Patent # ##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = ClhFwCfgAcpiPlatform
(4) This should be "CloudHvAcpiPlatformDxe", matching the basename of the
INF file.
Yeah,


+ 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 = IA32 X64 ARM AARCH64
(5) Do you really want this driver to be used on, say, IA32?
No, I will only keep AArch64 here as I'm sure arm32 can use it.

+#
+
+[Sources]
+ CloudHvAcpi.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ BaseLib
+ DebugLib
+ MemoryAllocationLib
+ OrderedCollectionLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+
+[Protocols]
+ gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
+ gEfiPciIoProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
+
+[Guids]
+ gRootBridgesConnectedEventGroupGuid
+
+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
+ gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiRsdpBaseAddress
+
+[Depex]
+ gEfiAcpiTableProtocolGuid
diff --git
a/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
new file mode 100644
index 000000000000..f511a4f5064c
--- /dev/null
+++
b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
@@ -0,0 +1,43 @@
+## @file
+# Decide whether the firmware should expose an ACPI- and/or a Device
+Tree-based # hardware description to the operating system.
+#
+# Copyright (c) 2017, Red Hat, Inc.
(6) ARM (C) missing.
Sure,


+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent ##
+
+[Defines]
+ INF_VERSION = 1.25
+ BASE_NAME = ClhPlatformHasAcpiDtDxe
(7) Should be "CloudHvHasAcpiDtDxe".
Ok

+ FILE_GUID = 71fe72f9-6dc1-199d-5054-13b4200ee88d
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = PlatformHasAcpiDt
+
+[Sources]
+ CloudHvHasAcpiDtDxe.c
+
+[Packages]
+ ArmVirtPkg/ArmVirtPkg.dec
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ BaseLib
+ DebugLib
+ PcdLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+
+[Guids]
+ gEdkiiPlatformHasAcpiGuid ## SOMETIMES_PRODUCES ## PROTOCOL
+ gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ##
PROTOCOL
+
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdForceNoAcpi
+
+[Depex]
+ gEfiVariableArchProtocolGuid
diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
new file mode 100644
index 000000000000..c2344e7b29fa
--- /dev/null
+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
@@ -0,0 +1,73 @@
+#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>
(8) File-top comment block missing altogether, including the @file Doxygen
directive plus short explanation, ARM (C) notice, "SPDX-License-Identifier".
Yeah, will add it.

+
+#define ACPI_ENTRY_SIZE 8
+#define XSDT_LEN 36
+
+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;
+}
+
+EFI_STATUS
+EFIAPI
+InstallCloudHvAcpiTables (
+ IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
+ )
+{
+ UINTN InstalledKey, TableSize;
+ UINT64 Rsdp, Xsdt, table_offset, PointerValue;
+ EFI_STATUS Status = 0;
+ int size;
+
+ Rsdp = PcdGet64 (PcdAcpiRsdpBaseAddress); Xsdt =
+ ((EFI_ACPI_6_3_ROOT_SYSTEM_DESCRIPTION_POINTER *)Rsdp)-
XsdtAddress;
+ PointerValue = Xsdt; table_offset = Xsdt; size =
+ ((EFI_ACPI_COMMON_HEADER *)PointerValue)->Length - XSDT_LEN;
+ table_offset += XSDT_LEN;
+
+ while(!Status && size > 0) {
+ PointerValue = *(UINT64 *)table_offset;
+ TableSize = ((EFI_ACPI_COMMON_HEADER *)PointerValue)->Length;
+ Status = AcpiProtocol->InstallAcpiTable (AcpiProtocol,
+ (VOID *)(UINT64)PointerValue, TableSize,
+ &InstalledKey);
+ table_offset += ACPI_ENTRY_SIZE;
+ size -= ACPI_ENTRY_SIZE;
+ }
+
+ return Status;
+}
+
+EFI_STATUS
+EFIAPI
+CloudHvAcpiPlatformEntryPoint (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+
+ Status = InstallCloudHvAcpiTables (FindAcpiTableProtocol ());
+ return Status;
+}
+
diff --git
a/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c
b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c
new file mode 100644
index 000000000000..ae07c91f5705
--- /dev/null
+++
b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c
@@ -0,0 +1,69 @@
+/** @file
+ Decide whether the firmware should expose an ACPI- and/or a Device
+Tree-based
+ hardware description to the operating system.
+
+ Copyright (c) 2017, Red Hat, Inc.
(9) ARM (C) missing.
Sure

+
+ SPDX-License-Identifier: BSD-2-Clause-Patent **/
+
+#include <Guid/PlatformHasAcpi.h>
+#include <Guid/PlatformHasDeviceTree.h> #include <Library/BaseLib.h>
+#include <Library/DebugLib.h> #include <Library/PcdLib.h> #include
+<Library/UefiBootServicesTableLib.h>
+
+EFI_STATUS
+EFIAPI
+PlatformHasAcpiDt (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+
+ //
+ // If we fail to install any of the necessary protocols below, the
+ OS will be // unbootable anyway (due to lacking hardware
+ description), so tolerate no // errors here.
+ //
+ if (MAX_UINTN == MAX_UINT64 &&
+ !PcdGetBool (PcdForceNoAcpi))
+ {
+ Status = gBS->InstallProtocolInterface (
+ &ImageHandle,
+ &gEdkiiPlatformHasAcpiGuid,
+ EFI_NATIVE_INTERFACE,
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ goto Failed;
+ }
+
+ return Status;
+ }
+
+ //
+ // Expose the Device Tree otherwise.
+ //
+ Status = gBS->InstallProtocolInterface (
+ &ImageHandle,
+ &gEdkiiPlatformHasDeviceTreeGuid,
+ EFI_NATIVE_INTERFACE,
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ goto Failed;
+ }
+
+ return Status;
+
+Failed:
+ ASSERT_EFI_ERROR (Status);
+ CpuDeadLoop ();
+ //
+ // Keep compilers happy.
+ //
+ return Status;
+}
I've only pointed out what I consider the bare minimum for my ACK; the
actual logic in the patch will still need an R-b from Ard and/or Leif and/or Sami.
Thanks
Jianyong Wu


Thanks
Laszlo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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