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] -=-=-=-=-=-=-=-=-=-=-=-