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

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

Hi Sami,

All comments are accepted. Thanks for your elaborate review work!

BR
Jianyong

> -----Original Message-----
> From: Sami Mujawar <Sami.Mujawar@arm.com>
> Sent: Monday, June 28, 2021 8:22 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 v3 2/3] Acpi: Install Acpi tables for Cloud hypervisor
> 
> 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|0
> x00
> > + 000005
> > +
> >   [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;
> > +}