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>
---
.../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
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>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = ClhFwCfgAcpiPlatform
+ 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
+#
+
+[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.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+ INF_VERSION = 1.25
+ BASE_NAME = ClhPlatformHasAcpiDtDxe
+ 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>
+
+#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.
+
+ 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;
+}
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75186): https://edk2.groups.io/g/devel/message/75186
Mute This Topic: https://groups.io/mt/82880901/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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>
> ---
> .../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;
> +}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75287): https://edk2.groups.io/g/devel/message/75287
Mute This Topic: https://groups.io/mt/82880901/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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;
+}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75746): https://edk2.groups.io/g/devel/message/75746
Mute This Topic: https://groups.io/mt/82880901/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
>
> 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.
>
> 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).
> +# 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.
> + 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?
> +#
> +
> +[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.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +[Defines]
> + INF_VERSION = 1.25
> + BASE_NAME = ClhPlatformHasAcpiDtDxe
(7) Should be "CloudHvHasAcpiDtDxe".
> + 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".
> +
> +#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.
> +
> + 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
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75348): https://edk2.groups.io/g/devel/message/75348
Mute This Topic: https://groups.io/mt/82880901/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75747): https://edk2.groups.io/g/devel/message/75747
Mute This Topic: https://groups.io/mt/82880901/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.