<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:DengXian;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
@font-face
{font-family:"\@DengXian";
panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body bgcolor="white" lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal"><span style="color:windowtext">Hi Sami,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext">Thanks for lots of great comments here, I will fix it one by one.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext">Thanks<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext">Jianyong<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext"><o:p> </o:p></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="color:windowtext">From:</span></b><span style="color:windowtext"> Sami Mujawar <Sami.Mujawar@arm.com>
<br>
<b>Sent:</b> Wednesday, May 19, 2021 4:26 AM<br>
<b>To:</b> Jianyong Wu <Jianyong.Wu@arm.com>; devel@edk2.groups.io; lersek@redhat.com; ardb+tianocore@kernel.org<br>
<b>Cc:</b> hao.a.wu@intel.com; Justin He <Justin.He@arm.com>; nd <nd@arm.com><br>
<b>Subject:</b> Re: [PATCH v2 3/5] ArmVirtPkg: enable ACPI for cloud hypervisor<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p>Hi Jianyon,<o:p></o:p></p>
<p>Thank you for this patch.<o:p></o:p></p>
<p>Please find my response inline marked [SAMI].<o:p></o:p></p>
<p>Regards,<o:p></o:p></p>
<p>Sami Mujawar<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">On 17/05/2021 07:50 AM, Jianyong Wu wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>The current implementation of PlatformHasAcpiDt is not a common<o:p></o:p></pre>
<pre>library and is on behalf of qemu. So give a specific version for<o:p></o:p></pre>
<pre>Cloud Hypervisor here.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>There is no device like Fw-cfg in qemu in Cloud Hypervisor, so a specific<o:p></o:p></pre>
<pre>Acpi handler is introduced here.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>The handler implemented here is in a very simple way:<o:p></o:p></pre>
<pre>firstly, aquire the Rsdp address from the PCD varaible in the top<o:p></o:p></pre>
<pre>".dsc";<o:p></o:p></pre>
<pre>secondly, get the Xsdp address from Rsdp structure;<o:p></o:p></pre>
<pre>thirdly, get the Acpi tables following the Xsdp structrue and install it<o:p></o:p></pre>
<pre>one by one.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Signed-off-by: Jianyong Wu <a href="mailto:jianyong.wu@arm.com"><jianyong.wu@arm.com></a><o:p></o:p></pre>
<pre>---<o:p></o:p></pre>
<pre> .../CloudHvAcpiPlatformDxe.inf | 51 +++++++++++++<o:p></o:p></pre>
<pre> .../CloudHvHasAcpiDtDxe.inf | 43 +++++++++++<o:p></o:p></pre>
<pre> .../CloudHvAcpiPlatformDxe/CloudHvAcpi.c | 73 +++++++++++++++++++<o:p></o:p></pre>
<pre> .../CloudHvHasAcpiDtDxe.c | 69 ++++++++++++++++++<o:p></o:p></pre>
<pre> 4 files changed, 236 insertions(+)<o:p></o:p></pre>
<pre> create mode 100644 ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf<o:p></o:p></pre>
<pre> create mode 100644 ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf<o:p></o:p></pre>
<pre> create mode 100644 ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c<o:p></o:p></pre>
<pre> create mode 100644 ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] I think these should be split as 2 patches covering CloudHvAcpiPlatformDx/* and CloudHvPlatformHasAcpiDtDx/*
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf<o:p></o:p></pre>
<pre>new file mode 100644<o:p></o:p></pre>
<pre>index 000000000000..63c74e84eb27<o:p></o:p></pre>
<pre>--- /dev/null<o:p></o:p></pre>
<pre>+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf<o:p></o:p></pre>
<pre>@@ -0,0 +1,51 @@<o:p></o:p></pre>
<pre>+## @file<o:p></o:p></pre>
<pre>+# OVMF ACPI Platform Driver for Cloud Hypervisor<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] I think the reference to OVMF can be removed, right?<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+#<o:p></o:p></pre>
<pre>+# Copyright (c) 2008 - 2014, Intel Corporation. All rights reserved.<BR><o:p></o:p></pre>
<pre>+# SPDX-License-Identifier: BSD-2-Clause-Patent<o:p></o:p></pre>
<pre>+#<o:p></o:p></pre>
<pre>+##<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Defines]<o:p></o:p></pre>
<pre>+ INF_VERSION = 0x00010005<o:p></o:p></pre>
<pre>+ BASE_NAME = ClhFwCfgAcpiPlatform<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] Can ClhFwCfgAcpiPlatform be changed to CloudHvAcpiPlatform?
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+ FILE_GUID = 6c76e407-73f2-dc1c-938f-5d6c4691ea93<o:p></o:p></pre>
<pre>+ MODULE_TYPE = DXE_DRIVER<o:p></o:p></pre>
<pre>+ VERSION_STRING = 1.0<o:p></o:p></pre>
<pre>+ ENTRY_POINT = CloudHvAcpiPlatformEntryPoint<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+#<o:p></o:p></pre>
<pre>+# The following information is for reference only and not required by the build tools.<o:p></o:p></pre>
<pre>+#<o:p></o:p></pre>
<pre>+# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] Minor. The above line is just a comment, but can you revisit this, please?<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+#<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Sources]<o:p></o:p></pre>
<pre>+ CloudHvAcpi.c<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Packages]<o:p></o:p></pre>
<pre>+ MdePkg/MdePkg.dec<o:p></o:p></pre>
<pre>+ MdeModulePkg/MdeModulePkg.dec<o:p></o:p></pre>
<pre>+ OvmfPkg/OvmfPkg.dec<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[LibraryClasses]<o:p></o:p></pre>
<pre>+ BaseLib<o:p></o:p></pre>
<pre>+ DebugLib<o:p></o:p></pre>
<pre>+ MemoryAllocationLib<o:p></o:p></pre>
<pre>+ OrderedCollectionLib<o:p></o:p></pre>
<pre>+ UefiBootServicesTableLib<o:p></o:p></pre>
<pre>+ UefiDriverEntryPoint<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Protocols]<o:p></o:p></pre>
<pre>+ gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED<o:p></o:p></pre>
<pre>+ gEfiPciIoProtocolGuid # PROTOCOL SOMETIMES_CONSUMED<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] gEfiPciIoProtocolGuidis not used in this module.<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Guids]<o:p></o:p></pre>
<pre>+ gRootBridgesConnectedEventGroupGuid<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] gRootBridgesConnectedEventGroupGuid not used in this module.
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Pcd]<o:p></o:p></pre>
<pre>+ gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] PcdPciDisableBusEnumerationis not used in this module.<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+ gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiRsdpBaseAddress<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Depex]<o:p></o:p></pre>
<pre>+ gEfiAcpiTableProtocolGuid<o:p></o:p></pre>
<pre>diff --git a/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf<o:p></o:p></pre>
<pre>new file mode 100644<o:p></o:p></pre>
<pre>index 000000000000..f511a4f5064c<o:p></o:p></pre>
<pre>--- /dev/null<o:p></o:p></pre>
<pre>+++ b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf<o:p></o:p></pre>
<pre>@@ -0,0 +1,43 @@<o:p></o:p></pre>
<pre>+## @file<o:p></o:p></pre>
<pre>+# Decide whether the firmware should expose an ACPI- and/or a Device Tree-based<o:p></o:p></pre>
<pre>+# hardware description to the operating system.<o:p></o:p></pre>
<pre>+#<o:p></o:p></pre>
<pre>+# Copyright (c) 2017, Red Hat, Inc.<o:p></o:p></pre>
<pre>+#<o:p></o:p></pre>
<pre>+# SPDX-License-Identifier: BSD-2-Clause-Patent<o:p></o:p></pre>
<pre>+##<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Defines]<o:p></o:p></pre>
<pre>+ INF_VERSION = 1.25<o:p></o:p></pre>
<pre>+ BASE_NAME = ClhPlatformHasAcpiDtDxe<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] Is it possible to change the Clh prefix to CloudHv. Same comment for other places in this patch series.
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+ FILE_GUID = 71fe72f9-6dc1-199d-5054-13b4200ee88d<o:p></o:p></pre>
<pre>+ MODULE_TYPE = DXE_DRIVER<o:p></o:p></pre>
<pre>+ VERSION_STRING = 1.0<o:p></o:p></pre>
<pre>+ ENTRY_POINT = PlatformHasAcpiDt<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Sources]<o:p></o:p></pre>
<pre>+ CloudHvHasAcpiDtDxe.c<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Packages]<o:p></o:p></pre>
<pre>+ ArmVirtPkg/ArmVirtPkg.dec<o:p></o:p></pre>
<pre>+ EmbeddedPkg/EmbeddedPkg.dec<o:p></o:p></pre>
<pre>+ MdeModulePkg/MdeModulePkg.dec<o:p></o:p></pre>
<pre>+ MdePkg/MdePkg.dec<o:p></o:p></pre>
<pre>+ OvmfPkg/OvmfPkg.dec<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[LibraryClasses]<o:p></o:p></pre>
<pre>+ BaseLib<o:p></o:p></pre>
<pre>+ DebugLib<o:p></o:p></pre>
<pre>+ PcdLib<o:p></o:p></pre>
<pre>+ UefiBootServicesTableLib<o:p></o:p></pre>
<pre>+ UefiDriverEntryPoint<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Guids]<o:p></o:p></pre>
<pre>+ gEdkiiPlatformHasAcpiGuid ## SOMETIMES_PRODUCES ## PROTOCOL<o:p></o:p></pre>
<pre>+ gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Pcd]<o:p></o:p></pre>
<pre>+ gArmVirtTokenSpaceGuid.PcdForceNoAcpi<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+[Depex]<o:p></o:p></pre>
<pre>+ gEfiVariableArchProtocolGuid<o:p></o:p></pre>
<pre>diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c<o:p></o:p></pre>
<pre>new file mode 100644<o:p></o:p></pre>
<pre>index 000000000000..c2344e7b29fa<o:p></o:p></pre>
<pre>--- /dev/null<o:p></o:p></pre>
<pre>+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c<o:p></o:p></pre>
<pre>@@ -0,0 +1,73 @@<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] File license header is missing.<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+#include <Library/BaseLib.h><o:p></o:p></pre>
<pre>+#include <Library/MemoryAllocationLib.h><o:p></o:p></pre>
<pre>+#include <IndustryStandard/Acpi63.h><o:p></o:p></pre>
<pre>+#include <Protocol/AcpiTable.h><o:p></o:p></pre>
<pre>+#include <Library/UefiBootServicesTableLib.h><o:p></o:p></pre>
<pre>+#include <Library/UefiDriverEntryPoint.h><o:p></o:p></pre>
<pre>+#include <Library/DebugLib.h><o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+#define ACPI_ENTRY_SIZE 8<o:p></o:p></pre>
<pre>+#define XSDT_LEN 36<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+STATIC<o:p></o:p></pre>
<pre>+EFI_ACPI_TABLE_PROTOCOL *<o:p></o:p></pre>
<pre>+FindAcpiTableProtocol (<o:p></o:p></pre>
<pre>+ VOID<o:p></o:p></pre>
<pre>+ )<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] Please add doxygen header for function. Same comment for other functions introduced in this series.<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+{<o:p></o:p></pre>
<pre>+ EFI_STATUS Status;<o:p></o:p></pre>
<pre>+ EFI_ACPI_TABLE_PROTOCOL *AcpiTable;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ Status = gBS->LocateProtocol (<o:p></o:p></pre>
<pre>+ &gEfiAcpiTableProtocolGuid,<o:p></o:p></pre>
<pre>+ NULL,<o:p></o:p></pre>
<pre>+ (VOID**)&AcpiTable<o:p></o:p></pre>
<pre>+ );<o:p></o:p></pre>
<pre>+ ASSERT_EFI_ERROR (Status);<o:p></o:p></pre>
<pre>+ return AcpiTable;<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+EFI_STATUS<o:p></o:p></pre>
<pre>+EFIAPI<o:p></o:p></pre>
<pre>+InstallCloudHvAcpiTables (<o:p></o:p></pre>
<pre>+ IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol<o:p></o:p></pre>
<pre>+ )<o:p></o:p></pre>
<pre>+{<o:p></o:p></pre>
<pre>+ UINTN InstalledKey, TableSize;<o:p></o:p></pre>
<pre>+ UINT64 Rsdp, Xsdt, table_offset, PointerValue;<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] Please have a look at variable naming conventions in EDKII coding standard at
<a href="https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions">
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions</a>
<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+ EFI_STATUS Status = 0;<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] Status = EFI_SUCCESS.<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+ int size;<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] UINTN should be used instead of 'int'. Also variable name does not confirm to coding standard.<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ Rsdp = PcdGet64 (PcdAcpiRsdpBaseAddress);<o:p></o:p></pre>
<pre>+ Xsdt = ((EFI_ACPI_6_3_ROOT_SYSTEM_DESCRIPTION_POINTER *)Rsdp)->XsdtAddress;<o:p></o:p></pre>
<pre>+ PointerValue = Xsdt;<o:p></o:p></pre>
<pre>+ table_offset = Xsdt;<o:p></o:p></pre>
<pre>+ size = ((EFI_ACPI_COMMON_HEADER *)PointerValue)->Length - XSDT_LEN;<o:p></o:p></pre>
<pre>+ table_offset += XSDT_LEN;<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] I think the macro name XSDT_LEN is a bit misleading. 'sizeof (EFI_ACPI_DESCRIPTION_HEADER)' should be used instead of XSDT_LEN.
<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ while(!Status && size > 0) {<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] Status is not boolean, use 'EFI_ERROR (Status)' instead. Also use paranthesis to add clarity.<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+ PointerValue = *(UINT64 *)table_offset;<o:p></o:p></pre>
<pre>+ TableSize = ((EFI_ACPI_COMMON_HEADER *)PointerValue)->Length;<o:p></o:p></pre>
<pre>+ Status = AcpiProtocol->InstallAcpiTable (AcpiProtocol,<o:p></o:p></pre>
<pre>+ (VOID *)(UINT64)PointerValue, TableSize,<o:p></o:p></pre>
<pre>+ &InstalledKey);<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] See coding convention for spacing rules at <a href="https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing">
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing</a><br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+ table_offset += ACPI_ENTRY_SIZE;<o:p></o:p></pre>
<pre>+ size -= ACPI_ENTRY_SIZE;<o:p></o:p></pre>
<pre>+ }<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">[SAMI] Can you look at simplifying the code in this function, please? You could refer to
<a href="https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c#L131..L139">
https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c#L131..L139</a><br>
<br>
<br>
<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ return Status;<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+EFI_STATUS<o:p></o:p></pre>
<pre>+EFIAPI<o:p></o:p></pre>
<pre>+CloudHvAcpiPlatformEntryPoint (<o:p></o:p></pre>
<pre>+ IN EFI_HANDLE ImageHandle,<o:p></o:p></pre>
<pre>+ IN EFI_SYSTEM_TABLE *SystemTable<o:p></o:p></pre>
<pre>+ )<o:p></o:p></pre>
<pre>+{<o:p></o:p></pre>
<pre>+ EFI_STATUS Status;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ Status = InstallCloudHvAcpiTables (FindAcpiTableProtocol ());<o:p></o:p></pre>
<pre>+ return Status;<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>diff --git a/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c<o:p></o:p></pre>
<pre>new file mode 100644<o:p></o:p></pre>
<pre>index 000000000000..ae07c91f5705<o:p></o:p></pre>
<pre>--- /dev/null<o:p></o:p></pre>
<pre>+++ b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c<o:p></o:p></pre>
<pre>@@ -0,0 +1,69 @@<o:p></o:p></pre>
<pre>+/** @file<o:p></o:p></pre>
<pre>+ Decide whether the firmware should expose an ACPI- and/or a Device Tree-based<o:p></o:p></pre>
<pre>+ hardware description to the operating system.<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ Copyright (c) 2017, Red Hat, Inc.<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ SPDX-License-Identifier: BSD-2-Clause-Patent<o:p></o:p></pre>
<pre>+**/<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+#include <Guid/PlatformHasAcpi.h><o:p></o:p></pre>
<pre>+#include <Guid/PlatformHasDeviceTree.h><o:p></o:p></pre>
<pre>+#include <Library/BaseLib.h><o:p></o:p></pre>
<pre>+#include <Library/DebugLib.h><o:p></o:p></pre>
<pre>+#include <Library/PcdLib.h><o:p></o:p></pre>
<pre>+#include <Library/UefiBootServicesTableLib.h><o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+EFI_STATUS<o:p></o:p></pre>
<pre>+EFIAPI<o:p></o:p></pre>
<pre>+PlatformHasAcpiDt (<o:p></o:p></pre>
<pre>+ IN EFI_HANDLE ImageHandle,<o:p></o:p></pre>
<pre>+ IN EFI_SYSTEM_TABLE *SystemTable<o:p></o:p></pre>
<pre>+ )<o:p></o:p></pre>
<pre>+{<o:p></o:p></pre>
<pre>+ EFI_STATUS Status;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ //<o:p></o:p></pre>
<pre>+ // If we fail to install any of the necessary protocols below, the OS will be<o:p></o:p></pre>
<pre>+ // unbootable anyway (due to lacking hardware description), so tolerate no<o:p></o:p></pre>
<pre>+ // errors here.<o:p></o:p></pre>
<pre>+ //<o:p></o:p></pre>
<pre>+ if (MAX_UINTN == MAX_UINT64 &&<o:p></o:p></pre>
<pre>+ !PcdGetBool (PcdForceNoAcpi))<o:p></o:p></pre>
<pre>+ {<o:p></o:p></pre>
<pre>+ Status = gBS->InstallProtocolInterface (<o:p></o:p></pre>
<pre>+ &ImageHandle,<o:p></o:p></pre>
<pre>+ &gEdkiiPlatformHasAcpiGuid,<o:p></o:p></pre>
<pre>+ EFI_NATIVE_INTERFACE,<o:p></o:p></pre>
<pre>+ NULL<o:p></o:p></pre>
<pre>+ );<o:p></o:p></pre>
<pre>+ if (EFI_ERROR (Status)) {<o:p></o:p></pre>
<pre>+ goto Failed;<o:p></o:p></pre>
<pre>+ }<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ return Status;<o:p></o:p></pre>
<pre>+ }<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ //<o:p></o:p></pre>
<pre>+ // Expose the Device Tree otherwise.<o:p></o:p></pre>
<pre>+ //<o:p></o:p></pre>
<pre>+ Status = gBS->InstallProtocolInterface (<o:p></o:p></pre>
<pre>+ &ImageHandle,<o:p></o:p></pre>
<pre>+ &gEdkiiPlatformHasDeviceTreeGuid,<o:p></o:p></pre>
<pre>+ EFI_NATIVE_INTERFACE,<o:p></o:p></pre>
<pre>+ NULL<o:p></o:p></pre>
<pre>+ );<o:p></o:p></pre>
<pre>+ if (EFI_ERROR (Status)) {<o:p></o:p></pre>
<pre>+ goto Failed;<o:p></o:p></pre>
<pre>+ }<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ return Status;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+Failed:<o:p></o:p></pre>
<pre>+ ASSERT_EFI_ERROR (Status);<o:p></o:p></pre>
<pre>+ CpuDeadLoop ();<o:p></o:p></pre>
<pre>+ //<o:p></o:p></pre>
<pre>+ // Keep compilers happy.<o:p></o:p></pre>
<pre>+ //<o:p></o:p></pre>
<pre>+ return Status;<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</body>
</html>
<div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links:<p> You receive all messages sent to this group. <p> <a target="_blank" href="https://edk2.groups.io/g/devel/message/75746">View/Reply Online (#75746)</a> | | <a target="_blank" href="https://groups.io/mt/82880901/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a><br> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com]<br> <div width="1" style="color:white;clear:both">_._,_._,_</div>