Hi Sami,
All comments are accepted. Thanks for your elaborate review work!
BR
Jianyong
> -----Original Message-----
> From: Sami Mujawar <sami.muja...@arm.com>
> Sent: Monday, June 28, 2021 8:22 PM
> To: Jianyong Wu <jianyong...@arm.com>; devel@edk2.groups.io
> Cc: ler...@redhat.com; ardb+tianoc...@kernel.org; Justin He
> <justin...@arm.com>; nd <n...@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...@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;
> > +}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77239): https://edk2.groups.io/g/devel/message/77239
Mute This Topic: https://groups.io/mt/83841323/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
