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

["Jianyong Wu" <jianyong.wu@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 11551 bytes --]

Hi Sami,

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

Thanks
Jianyong

From: Sami Mujawar <Sami.Mujawar@arm.com>
Sent: Wednesday, May 19, 2021 4:26 AM
To: Jianyong Wu <Jianyong.Wu@arm.com>; devel@edk2.groups.io; lersek@redhat.com; ardb+tianocore@kernel.org
Cc: hao.a.wu@intel.com; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
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@arm.com><mailto: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
[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;

+}


[-- Attachment #2: Type: text/html, Size: 27787 bytes --]