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

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

Hi Sami,

Thanks for your rework on my patch. I tried the change and it works well. You can do what you like on the patch set.

Thanks
Jianyong

> -----Original Message-----
> From: Sami Mujawar <Sami.Mujawar@arm.com>
> Sent: Tuesday, July 6, 2021 4:52 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>; devel@edk2.groups.io
> Cc: lersek@redhat.com; ardb+tianocore@kernel.org; Justin He
> <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v4 2/3] Acpi: Install Acpi tables for Cloud hypervisor
> 
> Hi Jianyong,
> 
> I should have caught this earlier in my review.  However, if you agree, I will
> do the following changes before pushing the patch.
> 
> 1.	The subject line of the commit message does not confirm to the edk2
> coding standard. It should have ‘ArmVirtPkg: <subject line for the patch>’
> 2.	The ACPI table signature can be simplified further. Can you try the
> following and let me know if it works, please?
> 
> diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
> b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
> index f5a47aa7f3cd..51b012676e7d 100644
> --- a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
> +++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
> @@ -52,8 +52,8 @@ FindAcpiTableProtocol (  EFI_STATUS  EFIAPI
> InstallCloudHvAcpiTables (
> - IN     EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
> - )
> +  IN     EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
> +  )
>  {
>    UINTN          InstalledKey;
>    UINTN          TableSize;
> @@ -97,11 +97,12 @@ InstallCloudHvAcpiTables (
>      //
>      // Get DSDT from FADT
>      //
> -    if (DsdtPtr == NULL
> -      && !AsciiStrnCmp ((CHAR8 *)&((EFI_ACPI_COMMON_HEADER
> *)AcpiTablePtr)->Signature, "FACP", 4)) {
> +    if ((DsdtPtr == NULL)
> +      && (EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE ==
> +          ((EFI_ACPI_COMMON_HEADER *)AcpiTablePtr)->Signature)) {
>        DsdtPtr = (UINT64 *)(((EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE
> *)AcpiTablePtr)->XDsdt);
>      }
> -  }
> +  } // while
> 
>    if (DsdtPtr == NULL) {
>      DEBUG ((DEBUG_ERROR, "%a: no DSDT found\n", __FUNCTION__));
> 
> Regards,
> 
> Sami Mujawar
> 
> On 05/07/2021, 11:07, "Jianyong Wu" <jianyong.wu@arm.com> 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.
> 
>     Cc: Laszlo Ersek <lersek@redhat.com>
>     Cc: Sami Mujawar <sami.mujawar@arm.com>
> 
>     Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
>     ---
>      ArmVirtPkg/ArmVirtPkg.dec                     |   6 +
>      .../CloudHvAcpiPlatformDxe.inf                |  47 ++++++
>      .../CloudHvAcpiPlatformDxe/CloudHvAcpi.c      | 155
> ++++++++++++++++++
>      3 files changed, 208 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|0
> x00000005
>     +
>      [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..f5a47aa7f3cd
>     --- /dev/null
>     +++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
>     @@ -0,0 +1,155 @@
>     +/** @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
>     +
>     +   @return AcpiTable  Protocol, which is used to handle Acpi Table, on
> SUCCESS or NULL on FAILURE.
>     +
>     +**/
>     +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 AcpiProtocol, AcpiTablePtr or
> DsdtPtr is NULL
>     +                                 and the size field embedded in the ACPI table pointed
>     +                                 by AcpiTablePtr or DsdtPtr are not in sync.
>     +  @return EFI_OUT_OF_RESOURCES   Insufficient resources exist to
> complete the request.
>     +  @return EFI_NOT_FOUND          DSDT table not found.
>     +**/
>     +EFI_STATUS
>     +EFIAPI
>     +InstallCloudHvAcpiTables (
>     + IN     EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
>     + )
>     +{
>     +  UINTN          InstalledKey;
>     +  UINTN          TableSize;
>     +  UINTN          AcpiTableLength;
>     +  UINT64         RsdpPtr;
>     +  UINT64         XsdtPtr;
>     +  UINT64         TableOffset;
>     +  UINT64         AcpiTablePtr;
>     +  UINT64         *DsdtPtr = NULL;
>     +  EFI_STATUS     Status;
>     +
>     +  if (AcpiProtocol == NULL) {
>     +      return EFI_INVALID_PARAMETER;
>     +  }
>     +
>     +  RsdpPtr = PcdGet64 (PcdCloudHvAcpiRsdpBaseAddress);
>     +  XsdtPtr = ((EFI_ACPI_6_3_ROOT_SYSTEM_DESCRIPTION_POINTER
> *)RsdpPtr)->XsdtAddress;
>     +  AcpiTableLength = ((EFI_ACPI_COMMON_HEADER *)XsdtPtr)->Length;
>     +  TableOffset = sizeof (EFI_ACPI_DESCRIPTION_HEADER);
>     +
>     +  while (TableOffset < AcpiTableLength) {
>     +    AcpiTablePtr = *(UINT64 *)(XsdtPtr + TableOffset);
>     +    TableSize = ((EFI_ACPI_COMMON_HEADER *)AcpiTablePtr)->Length;
>     +
>     +    //
>     +    // Install ACPI tables from XSDT
>     +    //
>     +    Status = AcpiProtocol->InstallAcpiTable (
>     +                             AcpiProtocol,
>     +                             (VOID *)AcpiTablePtr,
>     +                             TableSize,
>     +                             &InstalledKey
>     +                             );
>     +
>     +    if (EFI_ERROR(Status)) {
>     +        return Status;
>     +    }
>     +
>     +    TableOffset += sizeof (UINT64);
>     +
>     +    //
>     +    // Get DSDT from FADT
>     +    //
>     +    if (DsdtPtr == NULL
>     +      && !AsciiStrnCmp ((CHAR8 *)&((EFI_ACPI_COMMON_HEADER
> *)AcpiTablePtr)->Signature, "FACP", 4)) {
>     +      DsdtPtr = (UINT64
> *)(((EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE *)AcpiTablePtr)-
> >XDsdt);
>     +    }
>     +  }
>     +
>     +  if (DsdtPtr == NULL) {
>     +    DEBUG ((DEBUG_ERROR, "%a: no DSDT found\n", __FUNCTION__));
>     +    return EFI_NOT_FOUND;
>     +  }
>     +
>     +  //
>     +  // Install DSDT table
>     +  //
>     +  TableSize = ((EFI_ACPI_COMMON_HEADER *)DsdtPtr)->Length;
>     +  Status = AcpiProtocol->InstallAcpiTable (
>     +             AcpiProtocol,
>     +             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 AcpiProtocol, AcpiTablePtr or
> DsdtPtr is NULL
>     +                                 and the size field embedded in the ACPI table pointed to
>     +                                 by AcpiTablePtr or DsdtPtr are not in sync.
>     +  @return EFI_OUT_OF_RESOURCES   Insufficient resources exist to
> complete the request.
>     +  @return EFI_NOT_FOUND          DSDT table not found
>     +**/
>     +EFI_STATUS
>     +EFIAPI
>     +CloudHvAcpiPlatformEntryPoint (
>     +  IN EFI_HANDLE         ImageHandle,
>     +  IN EFI_SYSTEM_TABLE   *SystemTable
>     +  )
>     +{
>     +  EFI_STATUS                         Status;
>     +
>     +  Status = InstallCloudHvAcpiTables (FindAcpiTableProtocol ());
>     +
>     +  if (EFI_ERROR(Status)) {
>     +     DEBUG ((DEBUG_ERROR, "%a: Fail to install Acpi table: %r\n",
> __FUNCTION__,
>     +       Status));
>     +     CpuDeadLoop ();
>     +  }
>     +
>     +  return EFI_SUCCESS;
>     +}
>     --
>     2.17.1
> 
>