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