Re: [PATCH v3 2/3] Acpi: Install Acpi tables for Cloud hypervisor

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Jianyong,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 28/06/2021 10:55 AM, Jianyong Wu wrote:
> 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:
> 1. acquire the RSDP from the PCD variable in the top ".dsc";
> 2. get the XSDT address from RSDP structure;
> 3. get the ACPI tables following the XSDT structure and install them
> one by one;
> 4. get DSDT address from FADT and install DSDT table.
>
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>   ArmVirtPkg/ArmVirtPkg.dec                     |   6 +
>   .../CloudHvAcpiPlatformDxe.inf                |  47 ++++++
>   .../CloudHvAcpiPlatformDxe/CloudHvAcpi.c      | 141 ++++++++++++++++++
>   3 files changed, 194 insertions(+)
>   create mode 100644 ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
>   create mode 100644 ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
>
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index bf82f7f1f3f2..4e4d758015bc 100644
> --- a/ArmVirtPkg/ArmVirtPkg.dec
> +++ b/ArmVirtPkg/ArmVirtPkg.dec
> @@ -66,6 +66,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>     #
>     gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
>   
> +  ##
> +  # This is the physical address of Rsdp which is the core struct of Acpi.
> +  # Cloud Hypervisor has no other way to pass Rsdp address to the guest except use a PCD.
> +  #
> +  gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x0|UINT64|0x00000005
> +
>   [PcdsDynamic]
>     #
>     # Whether to force disable ACPI, regardless of the fw_cfg settings
> diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
> new file mode 100644
> index 000000000000..01de76486686
> --- /dev/null
> +++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
> @@ -0,0 +1,47 @@
> +## @file
> +#  ACPI Platform Driver for Cloud Hypervisor
> +#
> +#  Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = CloudHvgAcpiPlatform
> +  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           = AARCH64
> +#
> +
> +[Sources]
> +  CloudHvAcpi.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  MemoryAllocationLib
> +  OrderedCollectionLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +
> +[Protocols]
> +  gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED
> +
> +[Pcd]
> +  gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress
> +
> +[Depex]
> +  gEfiAcpiTableProtocolGuid
> diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
> new file mode 100644
> index 000000000000..0f1a50d63cd6
> --- /dev/null
> +++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
> @@ -0,0 +1,141 @@
> +/** @file
> +  Install Acpi tables for Cloud Hypervisor
> +
> +  Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#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>
> +
> +/**
> +   Find Acpi table Protocol and return it
[SAMI]Please add description of value returned by this function.
[/SAMI]
> +**/
> +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;
> +}
> +
> +/** Install Acpi tables for Cloud Hypervisor
> +
> +  @param [in]  AcpiProtocol  Acpi Protocol which is used to install Acpi talbles
> +
> +  @return EFI_SUCCESS            The table was successfully inserted.
> +  @return EFI_INVALID_PARAMETER  Either AcpiTableBuffer is NULL, TableKey is NULL, or AcpiTableBufferSize
> +                                 and the size field embedded in the ACPI table pointed to by AcpiTableBuffer
> +                                 are not in sync.
> +  @return EFI_OUT_OF_RESOURCES   Insufficient resources exist to complete the request.
> +  @retval EFI_ACCESS_DENIED      The table signature matches a table already
> +                                 present in the system and platform policy
> +                                 does not allow duplicate tables of this type.
> +**/
> +EFI_STATUS
> +EFIAPI
> +InstallCloudHvAcpiTables (
> + IN     EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
> + )
> +{
> +  UINTN InstalledKey, TableSize, AcpiTableLength;
[SAMI] Define each local variable separately on a new line.
> +  UINT64 RsdpPtr, XsdtPtr, TableOffset, AcpiTablePtr, DsdtPtr = ~0;
[SAMI] I think DsdtPtr can be UINT64 pointer and initialised to NULL 
before first use.
> +  EFI_STATUS Status = EFI_SUCCESS;
> +  BOOLEAN GotFacp = FALSE;
[SAMI] I think GotFacp could probably be avoided, see comments below.
> +
> +  RsdpPtr = PcdGet64 (PcdCloudHvAcpiRsdpBaseAddress);
> +  XsdtPtr = ((EFI_ACPI_6_3_ROOT_SYSTEM_DESCRIPTION_POINTER *) RsdpPtr)->XsdtAddress;
[SAMI] No space between typecast and their object. Same comment for 
similar instances in this patch series.
> +  AcpiTableLength = ((EFI_ACPI_COMMON_HEADER *) XsdtPtr)->Length;
> +  TableOffset = sizeof (EFI_ACPI_DESCRIPTION_HEADER);
> +
[SAMI] Set DsdtPtr = NULL, here.
> +  while (!EFI_ERROR(Status)
> +    && (TableOffset < AcpiTableLength))
> +  {
[SAMI] Starting curly brace at the end of previous line, please.
> +    AcpiTablePtr = *(UINT64 *) (XsdtPtr + TableOffset);
> +    TableSize = ((EFI_ACPI_COMMON_HEADER *) AcpiTablePtr)->Length;
> +
> +    //
> +    // Install ACPI tables from XSDT
> +    //
> +    Status = AcpiProtocol->InstallAcpiTable (
[SAMI] AcpiProtocol pointer not checked before use. In release builds 
the ASSERTs in FindAcpiTableProtocol() would vanish and a failure to get 
the protocol would result in a crash when dereferencing the pointer here.
> +                             AcpiProtocol,
> +                             (VOID *)(UINT64)AcpiTablePtr,
[SAMI] Can you check if typecast to UINT64 is needed here, please? 
Similarly, also check at other places.
> +                             TableSize,
> +                             &InstalledKey
> +                             );
[SAMI] Please reconsider error handling in this function. Probably best 
check and return failure from here. This would mean the status would not 
need to be checked in the while () statement and correspondingly there 
is no need to initialise Status to EFI_SUCCESS at the begining of this 
function.
> +
> +    TableOffset += sizeof (UINT64);
> +
> +    //
> +    // Get DSDT from FADT
> +    //
> +    if (!GotFacp
> +      && !AsciiStrnCmp ((CHAR8 *) &((EFI_ACPI_COMMON_HEADER *) AcpiTablePtr)->Signature, "FACP", 4))
> +    {
[SAMI] Curly brace on previous line, please. '!GotFacp' could be 
replaced with (DsdtPtr != NULL).
> +      DsdtPtr = ((EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE *) AcpiTablePtr)->XDsdt;
> +      GotFacp = TRUE;
> +    }
> +  }
> +
> +  if (DsdtPtr == ~0) {
[SAMI] Please change to' If (DsdtPtr == NULL)'.
> +    DEBUG ((DEBUG_ERROR, "%a: no DSDT found\n", __FUNCTION__));
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
[SAMI] I think EFI_NOT_FOUND could be returned here, and the CpuDeadLoop 
() could be moved to CloudHvAcpiPlatformEntryPoint().
> +  }
> +
> +  //
> +  // Install DSDT table
> +  //
> +  TableSize = ((EFI_ACPI_COMMON_HEADER *) DsdtPtr)->Length;
> +  Status = AcpiProtocol->InstallAcpiTable (
> +             AcpiProtocol,
> +             (VOID *)(UINT64) DsdtPtr,
> +             TableSize,
> +             &InstalledKey
> +             );
> +
> +  return Status;
> +}
> +
> +/** Entry point for Cloud Hypervisor Platform Dxe
> +
> +  @param [in]  ImageHandle  Handle for this image.
> +  @param [in]  SystemTable  Pointer to the EFI system table.
> +
> +  @return EFI_SUCCESS            The table was successfully inserted.
> +  @return EFI_INVALID_PARAMETER  Either AcpiTableBuffer is NULL, TableKey is NULL, or AcpiTableBufferSize
> +                                 and the size field embedded in the ACPI table pointed to by AcpiTableBuffer
> +                                 are not in sync.
> +  @return EFI_OUT_OF_RESOURCES   Insufficient resources exist to complete the request.
> +  @retval EFI_ACCESS_DENIED      The table signature matches a table already
> +                                 present in the system and platform policy
> +                                 does not allow duplicate tables of this type.
> +**/
> +EFI_STATUS
> +EFIAPI
> +CloudHvAcpiPlatformEntryPoint (
> +  IN EFI_HANDLE         ImageHandle,
> +  IN EFI_SYSTEM_TABLE   *SystemTable
> +  )
> +{
> +  EFI_STATUS                         Status;
> +
> +  Status = InstallCloudHvAcpiTables (FindAcpiTableProtocol ());
[SAMI] Check status code here and on failure execute CpuDeadLoop ().
> +  return Status;
> +}