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


Jianyong Wu
 

Hi Sami,

 

Thanks for lots of great comments here, I will fix it one by one.

 

Thanks

Jianyong

 

From: Sami Mujawar <Sami.Mujawar@...>
Sent: Wednesday, May 19, 2021 4:26 AM
To: Jianyong Wu <Jianyong.Wu@...>; devel@edk2.groups.io; lersek@...; ardb+tianocore@...
Cc: hao.a.wu@...; Justin He <Justin.He@...>; nd <nd@...>
Subject: Re: [PATCH v2 3/5] ArmVirtPkg: enable ACPI for cloud hypervisor

 

Hi Jianyon,

Thank you for this patch.

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

 

On 17/05/2021 07:50 AM, 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
one by one.
 
Signed-off-by: Jianyong Wu <jianyong.wu@...>
---
 .../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

[SAMI] I think these should be split as 2 patches covering CloudHvAcpiPlatformDx/* and CloudHvPlatformHasAcpiDtDx/*

 
 
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

[SAMI] I think the reference to OVMF can be removed, right?

 
+#
+#  Copyright (c) 2008 - 2014, Intel Corporation. All rights reserved.<BR>
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = ClhFwCfgAcpiPlatform

[SAMI] Can ClhFwCfgAcpiPlatform  be changed to CloudHvAcpiPlatform?

 
+  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

[SAMI] Minor. The above line is just a comment, but can you revisit this, please?

 
+#
+
+[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

[SAMI] gEfiPciIoProtocolGuidis not used in this module.

 
+
+[Guids]
+  gRootBridgesConnectedEventGroupGuid

[SAMI] gRootBridgesConnectedEventGroupGuid not used in this module.

 
+
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration

[SAMI] PcdPciDisableBusEnumerationis not used in this module.

 
+  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.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+  INF_VERSION                    = 1.25
+  BASE_NAME                      = ClhPlatformHasAcpiDtDxe

[SAMI] Is it possible to change the Clh prefix to CloudHv. Same comment for other places in this patch series.

 
+  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 @@

[SAMI] File license header is missing.

 
+#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>
+
+#define ACPI_ENTRY_SIZE 8
+#define XSDT_LEN 36
+
+STATIC
+EFI_ACPI_TABLE_PROTOCOL *
+FindAcpiTableProtocol (
+  VOID
+  )

[SAMI] Please add doxygen header for function. Same comment for other functions introduced in this series.

 
+{
+  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;

[SAMI] Please have a look at variable naming conventions in EDKII coding standard at https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions

 
+  EFI_STATUS Status = 0;

[SAMI] Status = EFI_SUCCESS.

 
+  int size;

[SAMI] UINTN should be used instead of 'int'. Also variable name does not confirm to coding standard.

 
+
+  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;

[SAMI] I think the macro name XSDT_LEN is a bit misleading. 'sizeof (EFI_ACPI_DESCRIPTION_HEADER)' should be used instead of XSDT_LEN.

 
+
+  while(!Status && size > 0) {

[SAMI] Status is not boolean, use 'EFI_ERROR (Status)' instead. Also use paranthesis to add clarity.

 
+    PointerValue = *(UINT64 *)table_offset;
+    TableSize = ((EFI_ACPI_COMMON_HEADER *)PointerValue)->Length;
+    Status = AcpiProtocol->InstallAcpiTable (AcpiProtocol,
+             (VOID *)(UINT64)PointerValue, TableSize,
+             &InstalledKey);

[SAMI] See coding convention for spacing rules at https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing

 
+    table_offset += ACPI_ENTRY_SIZE;
+    size -= ACPI_ENTRY_SIZE;
+  }

[SAMI] Can you look at simplifying the code in this function, please? You could refer to https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c#L131..L139


 
+
+  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.
+
+  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;
+}

 

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