From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Liu, Zhiguang" <zhiguang.liu@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Wang, Jian J" <jian.j.wang@intel.com>,
"Bi, Dandan" <dandan.bi@intel.com>,
Liming Gao <gaoliming@byosoft.com.cn>,
"Ni, Ray" <ray.ni@intel.com>
Subject: Re: [PATCH 8/9] MdeModulePkg/ACPI: Install ACPI table from HOB.
Date: Thu, 27 May 2021 00:42:55 +0000 [thread overview]
Message-ID: <BN8PR11MB36669916EA402B8DE93222F1CA239@BN8PR11MB3666.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20210524071234.1056-9-zhiguang.liu@intel.com>
Some inline comments below:
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Monday, May 24, 2021 3:13 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH 8/9] MdeModulePkg/ACPI: Install ACPI table from HOB.
>
> If HOB contains APCI table information, entry point of AcpiTableDxe.inf
> should parse the APCI table from HOB, and install these tables.
> We assume the whole ACPI table (starting with
> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
> is contained by a single gEfiAcpiTableGuid HOB.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h | 4 +++-
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 4 +++-
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 144
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++--------
> 3 files changed, 142 insertions(+), 10 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
> index 9d7cf7ccfc..7fd393aab3 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
> @@ -1,7 +1,7 @@
> /** @file
>
> ACPI Table Protocol Driver
>
>
>
> - Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>
> + Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> **/
>
> @@ -24,6 +24,8 @@
> #include <Library/MemoryAllocationLib.h>
>
> #include <Library/UefiBootServicesTableLib.h>
>
> #include <Library/PcdLib.h>
>
> +#include <Library/HobLib.h>
>
> +#include <UniversalPayload/AcpiTable.h>
>
>
>
> //
>
> // Statements that include other files
>
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> index d341df439e..df80c4db35 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> @@ -4,7 +4,7 @@
> # This driver initializes ACPI tables (Rsdp, Rsdt and Xsdt) and produces
> UEFI/PI
>
> # services to install/uninstall/manage ACPI tables.
>
> #
>
> -# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>
> +# Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
>
> # Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #
>
> @@ -51,10 +51,12 @@
> DebugLib
>
> BaseLib
>
> PcdLib
>
> + HobLib
>
>
>
> [Guids]
>
> gEfiAcpi10TableGuid ## PRODUCES ## SystemTable
>
> gEfiAcpiTableGuid ## PRODUCES ## SystemTable
>
> + gPldAcpiTableGuid
Please help to add "## SOMETIMES_CONSUMES ## HOB" as simple description of the GUID usage.
>
>
>
> [FeaturePcd]
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol ##
> CONSUMES
>
> diff --git
> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index 5a2afdff27..24962843a1 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -1,7 +1,7 @@
> /** @file
>
> ACPI Table Protocol Implementation
>
>
>
> - Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>
> + Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
>
> Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> @@ -30,6 +30,7 @@ STATIC EFI_ALLOCATE_TYPE mAcpiTableAllocType;
> @param Table Table to add.
>
> @param Checksum Does the table require checksumming.
>
> @param Version The version of the list to add the table to.
>
> + @param IsFromHob True, if add Apci Table from Hob List.
>
> @param Handle Pointer for returning the handle.
>
>
>
> @return EFI_SUCCESS The function completed successfully.
>
> @@ -44,6 +45,7 @@ AddTableToList (
> IN VOID *Table,
>
> IN BOOLEAN Checksum,
>
> IN EFI_ACPI_TABLE_VERSION Version,
>
> + IN BOOLEAN IsFromHob,
>
> OUT UINTN *Handle
>
> );
>
>
>
> @@ -238,6 +240,7 @@ InstallAcpiTable (
> AcpiTableBufferConst,
>
> TRUE,
>
> Version,
>
> + FALSE,
>
> TableKey
>
> );
>
> if (!EFI_ERROR (Status)) {
>
> @@ -472,6 +475,7 @@ FreeTableMemory (
> @param Table Table to add.
>
> @param Checksum Does the table require checksumming.
>
> @param Version The version of the list to add the table to.
>
> + @param IsFromHob True, if add Apci Table from Hob List.
>
> @param Handle Pointer for returning the handle.
>
>
>
> @return EFI_SUCCESS The function completed successfully.
>
> @@ -487,6 +491,7 @@ AddTableToList (
> IN VOID *Table,
>
> IN BOOLEAN Checksum,
>
> IN EFI_ACPI_TABLE_VERSION Version,
>
> + IN BOOLEAN IsFromHob,
>
> OUT UINTN *Handle
>
> )
>
> {
>
> @@ -552,13 +557,16 @@ AddTableToList (
> // could be updated by OS present agent. For example, BufferPtrAddress
> in
>
> // SMM communication ACPI table.
>
> //
>
> - ASSERT ((EFI_PAGE_SIZE % 64) == 0);
Is there any special consideration for removing the above check when the table is not from HOB?
Also, will the tables from HOB violate the rule mentioned in the below comment?
// Allocate memory for the FACS. This structure must be aligned
// on a 64 byte boundary and must be ACPI NVS memory.
>
> - Status = gBS->AllocatePages (
>
> - AllocateMaxAddress,
>
> - EfiACPIMemoryNVS,
>
> - EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
>
> - &AllocPhysAddress
>
> - );
>
> + if (IsFromHob){
>
> + AllocPhysAddress = (UINTN)Table;
>
> + } else {
>
> + Status = gBS->AllocatePages (
>
> + AllocateMaxAddress,
>
> + EfiACPIMemoryNVS,
>
> + EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
>
> + &AllocPhysAddress
>
> + );
>
> + }
>
> } else if (mAcpiTableAllocType == AllocateAnyPages) {
>
> //
>
> // If there is no allocation limit, there is also no need to use page
>
> @@ -1689,6 +1697,124 @@ ChecksumCommonTables (
> return EFI_SUCCESS;
>
> }
>
>
>
> +/**
>
> + This function will find gPldAcpiTableGuid Guid Hob, and install Acpi table
> from it.
>
> +
>
> + @param AcpiTableInstance Protocol instance private data.
>
> +
>
> + @return EFI_SUCCESS The function completed successfully.
>
> + @return EFI_NOT_FOUND The function doesn't find the
> gEfiAcpiTableGuid Guid Hob.
>
> + @return EFI_ABORTED The function could not complete successfully.
>
> +
>
> +**/
>
> +EFI_STATUS
>
> +InstallAcpiTableFromHob (
>
> + EFI_ACPI_TABLE_INSTANCE *AcpiTableInstance
>
> + )
>
> +{
>
> + EFI_HOB_GUID_TYPE *GuidHob;
>
> + EFI_ACPI_TABLE_VERSION Version;
>
> + EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER *Rsdp;
>
> + EFI_ACPI_DESCRIPTION_HEADER *Rsdt;
>
> + EFI_ACPI_DESCRIPTION_HEADER *ChildTable;
>
> + UINT64 ChildTableAddress;
>
> + UINTN Count;
>
> + UINTN Index;
>
> + UINTN TableKey;
>
> + EFI_STATUS Status;
>
> + UINTN EntrySize;
>
> + PLD_ACPI_TABLE *AcpiTableAdress;
>
> + VOID *TableToInstall;
>
> + PLD_GENERIC_HEADER *GenericHeader;
>
> +
>
> + TableKey = 0;
>
> + Version = PcdGet32 (PcdAcpiExposedTableVersions);
>
> +
>
> + //
>
> + // HOB only contains the ACPI table in 2.0+ format.
>
> + //
>
> + GuidHob = GetFirstGuidHob (&gPldAcpiTableGuid);
>
> + if (GuidHob == NULL) {
>
> + return EFI_NOT_FOUND;
>
> + }
>
> +
>
> + GenericHeader = (PLD_GENERIC_HEADER *) GET_GUID_HOB_DATA
> (GuidHob);
>
> + if ((sizeof (PLD_GENERIC_HEADER) > GET_GUID_HOB_DATA_SIZE
> (GuidHob)) || (GenericHeader->Length > GET_GUID_HOB_DATA_SIZE
> (GuidHob))) {
>
> + return EFI_NOT_FOUND;
>
> + }
>
> + if (GenericHeader->Revision == PLD_ACPI_TABLE_REVISION) {
>
> + //
>
> + // PLD_ACPI_TABLE structure is used when Revision equals to
> PLD_ACPI_TABLE_REVISION
>
> + //
>
> + AcpiTableAdress = (PLD_ACPI_TABLE *) GET_GUID_HOB_DATA
> (GuidHob);
>
> + if (GenericHeader->Length < PLD_SIZEOF_THROUGH_FIELD
> (PLD_ACPI_TABLE, Rsdp)) {
>
> + //
>
> + // Retrun if can't find the ACPI Info Hob with enough length
>
> + //
>
> + return EFI_NOT_FOUND;
>
> + }
>
> + Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER *) (UINTN)
> (AcpiTableAdress->Rsdp);
>
> +
>
> + //
>
> + // An ACPI-compatible OS must use the XSDT if present.
>
> + // It shouldn't happen that XsdtAddress points beyond 4G range in 32-bit
> environment.
>
> + //
>
> + ASSERT ((UINTN) Rsdp->XsdtAddress == Rsdp->XsdtAddress);
>
> +
>
> + EntrySize = sizeof (UINT64);
>
> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->XsdtAddress;
>
> + if (Rsdt == NULL) {
>
> + //
>
> + // XsdtAddress is zero, then we use Rsdt which has 32 bit entry
>
> + //
>
> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->RsdtAddress;
>
> + EntrySize = sizeof (UINT32);
>
> + }
>
> + Count = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> EntrySize;
Do we need a check on the validity of 'Rsdt->Length'?
If 'Dsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)' underflows,
I think it is possible to consume invalid content.
>
> +
>
> + for (Index = 0; Index < Count; Index++){
>
> + ChildTableAddress = 0;
>
> + CopyMem (&ChildTableAddress, (UINT8 *) (Rsdt + 1) + EntrySize * Index,
> EntrySize);
>
> + //
>
> + // If the address is of UINT64 while this module runs at 32 bits,
>
> + // make sure the upper bits are all-zeros.
>
> + //
>
> + ASSERT (ChildTableAddress == (UINTN) ChildTableAddress);
Sorry for a question, is the condition in the above ASSERT a case that will never happen or
the above ASSERT is used for table content check?
If it is a check, please help to add error handling logic.
>
> +
>
> + ChildTable = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN)
> ChildTableAddress;
>
> + Status = AddTableToList (AcpiTableInstance, ChildTable, TRUE, Version,
> TRUE, &TableKey);
>
> + if (EFI_ERROR (Status)) {
>
> + DEBUG ((DEBUG_ERROR, "InstallAcpiTableFromHob: Fail to add ACPI
> table at 0x%p\n", ChildTable));
>
> + ASSERT_EFI_ERROR (Status);
For a single ChildTable, if the installation fails here, is it still needed to install the FACS and DSDT within the ChildTable?
>
> + }
>
> + if (ChildTable->Signature ==
> EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE){
>
> + //
>
> + // Add the FACS and DSDT tables if it is not NULL.
>
> + //
>
> + if (((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)-
> >FirmwareCtrl != 0) {
>
> + TableToInstall = (VOID *) (UINTN)
> ((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)-
> >FirmwareCtrl;
>
> + Status = AddTableToList (AcpiTableInstance, TableToInstall, TRUE,
> Version, TRUE, &TableKey);
>
> + if (EFI_ERROR (Status)) {
>
> + DEBUG ((DEBUG_ERROR, "InstallAcpiTableFromHob: Fail to add ACPI
> table FACS\n"));
>
> + ASSERT_EFI_ERROR (Status);
If the installation of FACS fails, is it still needed to install the DSDT below?
>
> + }
>
> + }
>
> +
>
> + if (((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)-
> >Dsdt != 0) {
>
> + TableToInstall = (VOID *) (UINTN)
> ((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)->Dsdt;
>
> + Status = AddTableToList (AcpiTableInstance, TableToInstall, TRUE,
> Version, TRUE, &TableKey);
>
> + if (EFI_ERROR (Status)) {
>
> + DEBUG ((DEBUG_ERROR, "InstallAcpiTableFromHob: Fail to add ACPI
> table DSDT\n"));
>
> + ASSERT_EFI_ERROR (Status);
>
> + }
>
> + }
>
> + }
>
> + }
>
> + }
>
> + Status = PublishTables (AcpiTableInstance, Version);
If there are errors occurred during the parse of the tables, do we still need to publish them anyway?
Best Regards,
Hao Wu
>
> + ASSERT_EFI_ERROR (Status);
>
> + return Status;
>
> +}
>
>
>
> /**
>
> Constructor for the ACPI table protocol. Initializes instance
>
> @@ -1918,6 +2044,8 @@ AcpiTableAcpiTableConstructor (
>
>
> ChecksumCommonTables (AcpiTableInstance);
>
>
>
> + InstallAcpiTableFromHob (AcpiTableInstance);
>
> +
>
> //
>
> // Completed successfully
>
> //
>
> --
> 2.30.0.windows.2
next prev parent reply other threads:[~2021-05-27 0:43 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-24 7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
2021-05-24 7:12 ` [PATCH 1/9] MdePkg: Add Universal Payload general defination header file Zhiguang Liu
2021-05-24 7:12 ` [PATCH 2/9] MdePkg: Add new structure for the PCI Root Bridge Info Hob Zhiguang Liu
2021-05-24 7:12 ` [PATCH 3/9] UefiPayloadPkg: UefiPayload retrieve PCI root bridge from Guid Hob Zhiguang Liu
2021-05-24 7:12 ` [PATCH 4/9] MdePkg: Add new structure for the Universal Payload SMBios Table Info Hob Zhiguang Liu
2021-05-24 7:12 ` [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables Zhiguang Liu
2021-05-26 6:15 ` Wu, Hao A
2021-05-26 6:32 ` [edk2-devel] " Wu, Hao A
2021-05-26 8:56 ` Zhiguang Liu
2021-05-26 13:04 ` Patrick Rudolph
2021-05-24 7:12 ` [PATCH 6/9] UefiPayloadPkg: Creat gPldSmbiosTableGuid Hob Zhiguang Liu
2021-06-02 3:39 ` Guo Dong
2021-05-24 7:12 ` [PATCH 7/9] MdePkg: Add new structure for the Universal Payload ACPI Table Info Hob Zhiguang Liu
2021-05-24 7:12 ` [PATCH 8/9] MdeModulePkg/ACPI: Install ACPI table from HOB Zhiguang Liu
2021-05-27 0:42 ` Wu, Hao A [this message]
2021-05-24 7:12 ` [PATCH 9/9] UefiPayloadPkg: Creat gPldAcpiTableGuid Hob Zhiguang Liu
2021-05-26 13:50 ` [edk2-devel] " Patrick Rudolph
2021-06-02 3:47 ` Guo Dong
2021-05-28 3:00 ` 回复: [edk2-devel] [PATCH 0/9] Create multiple Hobs for Universal Payload gaoliming
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN8PR11MB36669916EA402B8DE93222F1CA239@BN8PR11MB3666.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox