[edk2-devel] [PATCH v5] MinPlatformPkg: Update HWSignature filed in FACS
Ni, Ray
ray.ni at intel.com
Mon May 8 03:23:07 UTC 2023
Vincent,
I don't quite understand your code logic.
For example:
1. Why you use XSDT only? What if there is only RSDT?
2. Why there is a do-while in AcpiTableCrcCalculator()?
3. Why (VOID *) is converted to (UINTN) then to (EFI_ACPI_DESCRIPTION_HEADER*)?
4. Why you "measure" the XSDT and RSDT table? The two tables only contain pointers.
5. Can you please review " MdePkg/Library/UefiLib/Acpi.c" which contains logic that iterates the acpi tables?
Thanks,
Ray
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> VincentX Ke
> Sent: Wednesday, May 3, 2023 10:40 AM
> To: devel at edk2.groups.io
> Cc: Ke, VincentX <vincentx.ke at intel.com>; Chiu, Chasel
> <chasel.chiu at intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone at intel.com>; Oram, Isaac W
> <isaac.w.oram at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>; Dong,
> Eric <eric.dong at intel.com>
> Subject: [edk2-devel] [PATCH v5] MinPlatformPkg: Update HWSignature
> filed in FACS
>
> From: VincentX Ke <vincentx.ke at intel.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4428
>
> Calculating CRC based on each ACPI table.
> Update HWSignature filed in FACS based on CRC while ACPI table changed.
>
> Change-Id: Ic0ca66ff10cda0fbcd0683020fab1bc9aea9b78c
> Signed-off-by: VincentX Ke <vincentx.ke at intel.com>
> Cc: Chasel Chiu <chasel.chiu at intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone at intel.com>
> Cc: Isaac Oram <isaac.w.oram at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Eric Dong <eric.dong at intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke at intel.com>
> ---
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 182
> +++++++++++++-------
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
> 2 files changed, 118 insertions(+), 65 deletions(-)
>
> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index e967031a3b..a940424ced 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1191,98 +1191,150 @@ PlatformUpdateTables (
> }
>
>
>
> /**
>
> - This function calculates RCR based on PCI Device ID and Vendor ID from the
> devices
>
> - available on the platform.
>
> - It also includes other instances of BIOS change to calculate CRC and
> provides as
>
> - HWSignature filed in FADT table.
>
> + This function calculates CRC based on each offset in the ACPI table.
>
> +
>
> + @param[in] Table The pointer to ACPI table that
>
> + required to calculate CRC.
>
> +
>
> + @retval CRC A pointer to allocate UINT32 that
>
> + contains the CRC32 data.
>
> +**/
>
> +UINT32
>
> +AcpiTableCrcCalculator (
>
> + IN VOID *Table
>
> + )
>
> +{
>
> + UINT32 CRC;
>
> +
>
> + CRC = 0;
>
> +
>
> + //
>
> + // Calculate CRC value.
>
> + //
>
> + if (((EFI_ACPI_6_5_COMMON_HEADER *)(UINTN)Table)->Signature ==
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
>
> + //
>
> + // Zero HardwareSignature field before Calculating FACS CRC
>
> + //
>
> + do {
>
> + ((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)Table)->HardwareSignature = 0;
>
> + } while (((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)Table)->HardwareSignature);
>
> +
>
> + gBS->CalculateCrc32 ((UINT8 *)Table,
> (UINTN)((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)Table)->Length, &CRC);
>
> + } else {
>
> + gBS->CalculateCrc32 ((UINT8 *)Table,
> (UINTN)((EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Table)->Length, &CRC);
>
> + }
>
> +
>
> + return CRC;
>
> +}
>
> +
>
> +/**
>
> + This function calculates CRC based on each ACPI table.
>
> + It also calculates CRC and provides as HWSignature filed in FACS.
>
> **/
>
> VOID
>
> -IsHardwareChange (
>
> +IsAcpiTableChange (
>
> VOID
>
> )
>
> {
>
> - EFI_STATUS Status;
>
> - UINTN Index;
>
> - UINTN HandleCount;
>
> - EFI_HANDLE *HandleBuffer;
>
> - EFI_PCI_IO_PROTOCOL *PciIo;
>
> - UINT32 CRC;
>
> - UINT32 *HWChange;
>
> - UINTN HWChangeSize;
>
> - UINT32 PciId;
>
> - UINTN Handle;
>
> - EFI_ACPI_6_3_FIRMWARE_ACPI_CONTROL_STRUCTURE *FacsPtr;
>
> - EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE *pFADT;
>
> -
>
> - HandleCount = 0;
>
> - HandleBuffer = NULL;
>
> -
>
> - Status = gBS->LocateHandleBuffer (
>
> - ByProtocol,
>
> - &gEfiPciIoProtocolGuid,
>
> - NULL,
>
> - &HandleCount,
>
> - &HandleBuffer
>
> - );
>
> - if (EFI_ERROR (Status)) {
>
> - return; // PciIO protocol not installed yet!
>
> + EFI_STATUS Status;
>
> + UINTN Index;
>
> + UINTN AcpiTableCount;
>
> + UINT32 Table;
>
> + UINT32 HWSignature;
>
> + UINT32 *AcpiTableCrc;
>
> + EFI_ACPI_6_5_ROOT_SYSTEM_DESCRIPTION_POINTER *Rsdp;
>
> + EFI_ACPI_DESCRIPTION_HEADER *Rsdt;
>
> + EFI_ACPI_DESCRIPTION_HEADER *Xsdt;
>
> + EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *FacsPtr;
>
> + EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *pFADT;
>
> +
>
> + AcpiTableCount = 0;
>
> + AcpiTableCrc = NULL;
>
> + Rsdp = NULL;
>
> + Rsdt = NULL;
>
> + Xsdt = NULL;
>
> + FacsPtr = NULL;
>
> + pFADT = NULL;
>
> +
>
> + DEBUG ((DEBUG_INFO, "%a() - Start\n", __FUNCTION__));
>
> +
>
> + Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, (VOID
> **)&Rsdp);
>
> + if (EFI_ERROR (Status) || (Rsdp == NULL)) {
>
> + return;
>
> }
>
>
>
> //
>
> - // Allocate memory for HWChange and add additional entrie for
>
> - // pFADT->XDsdt
>
> + // ACPI table count starts with 2 as RSDT and XSDT are already located.
>
> + // Then add ACPI tables found by XSDT and FADT.
>
> //
>
> - HWChangeSize = HandleCount + 1;
>
> - HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
>
> - ASSERT(HWChange != NULL);
>
> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> >RsdtAddress;
>
> + Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> >XsdtAddress;
>
> + AcpiTableCount += 2;
>
> + AcpiTableCount = AcpiTableCount + (Xsdt->Length - sizeof
> (EFI_ACPI_DESCRIPTION_HEADER))/sizeof (UINT64);
>
>
>
> - if (HWChange == NULL) return;
>
> + for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < (Xsdt-
> >Length); Index = Index + sizeof (UINT64)) {
>
> + Table = *((UINT32 *)((UINT8 *)Xsdt + Index));
>
> + if (((EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Table)->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
>
> + pFADT = (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE
> *)(UINTN)Table;
>
> + if (pFADT->FirmwareCtrl) {
>
> + AcpiTableCount++;
>
> + }
>
>
>
> - //
>
> - // add HWChange inputs: PCI devices
>
> - //
>
> - for (Index = 0; HandleCount > 0; HandleCount--) {
>
> - PciId = 0;
>
> - Status = gBS->HandleProtocol (HandleBuffer[Index],
> &gEfiPciIoProtocolGuid, (VOID **) &PciIo);
>
> - if (!EFI_ERROR (Status)) {
>
> - Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint32, 0, 1, &PciId);
>
> - if (EFI_ERROR (Status)) {
>
> - continue;
>
> + if ((pFADT->XDsdt) || (pFADT->Dsdt)) {
>
> + AcpiTableCount++;
>
> }
>
> - HWChange[Index++] = PciId;
>
> }
>
> }
>
>
>
> //
>
> - // Locate FACP Table
>
> + // Allocate memory for founded ACPI tables.
>
> //
>
> - Handle = 0;
>
> - Status = LocateAcpiTableBySignature (
>
> - EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
>
> - (EFI_ACPI_DESCRIPTION_HEADER **) &pFADT,
>
> - &Handle
>
> - );
>
> - if (EFI_ERROR (Status) || (pFADT == NULL)) {
>
> - return; //Table not found or out of memory resource for pFADT table
>
> + AcpiTableCrc = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
>
> + if (AcpiTableCrc == NULL) {
>
> + return;
>
> + }
>
> +
>
> + AcpiTableCount = 0;
>
> + AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator ((VOID
> *)(UINTN)Rsdt);
>
> + AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator ((VOID
> *)(UINTN)Xsdt);
>
> +
>
> + for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < (Xsdt-
> >Length); Index = Index + sizeof (UINT64)) {
>
> + Table = *((UINT32 *)((UINT8 *)Xsdt + Index));
>
> + AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator ((VOID
> *)(UINTN)Table);
>
> + if (((EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Table)->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
>
> + pFADT = (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE
> *)(UINTN)Table;
>
> + if (pFADT->FirmwareCtrl) {
>
> + AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator ((VOID
> *)(UINTN)pFADT->FirmwareCtrl);
>
> + }
>
> +
>
> + if (pFADT->XDsdt) {
>
> + AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator ((VOID
> *)(UINTN)pFADT->XDsdt);
>
> + } else {
>
> + AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator ((VOID
> *)(UINTN)pFADT->Dsdt);
>
> + }
>
> + }
>
> }
>
>
>
> //
>
> - // add HWChange inputs: others
>
> + // pFADT table not found.
>
> //
>
> - HWChange[Index++] = (UINT32)pFADT->XDsdt;
>
> + if (pFADT == NULL) {
>
> + return;
>
> + }
>
>
>
> //
>
> - // Calculate CRC value with HWChange data.
>
> + // Calculate HWSignature data.
>
> //
>
> - Status = gBS->CalculateCrc32(HWChange, HWChangeSize, &CRC);
>
> - DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
>
> + Status = gBS->CalculateCrc32 (AcpiTableCrc, AcpiTableCount,
> &HWSignature);
>
> + DEBUG ((DEBUG_INFO, "HardwareSignature = %x and Status = %r\n",
> HWSignature, Status));
>
>
>
> //
>
> // Set HardwareSignature value based on CRC value.
>
> //
>
> - FacsPtr = (EFI_ACPI_6_3_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)pFADT->FirmwareCtrl;
>
> - FacsPtr->HardwareSignature = CRC;
>
> - FreePool (HWChange);
>
> + FacsPtr =
> (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)pFADT-
> >FirmwareCtrl;
>
> + FacsPtr->HardwareSignature = HWSignature;
>
> + FreePool (AcpiTableCrc);
>
> + DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
>
> }
>
>
>
> VOID
>
> @@ -1329,7 +1381,7 @@ AcpiEndOfDxeEvent (
> //
>
> // Calculate Hardware Signature value based on current platform
> configurations
>
> //
>
> - IsHardwareChange ();
>
> + IsAcpiTableChange ();
>
> }
>
>
>
> /**
>
> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> index 694492112b..f47cc3908d 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> @@ -128,6 +128,7 @@
> gEfiGlobalVariableGuid ## CONSUMES
>
> gEfiHobListGuid ## CONSUMES
>
> gEfiEndOfDxeEventGroupGuid ## CONSUMES
>
> + gEfiAcpiTableGuid ## CONSUMES
>
>
>
> [Depex]
>
> gEfiAcpiTableProtocolGuid AND
>
> --
> 2.39.2.windows.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#103877):
> https://edk2.groups.io/g/devel/message/103877
> Mute This Topic: https://groups.io/mt/98654282/1712937
> Group Owner: devel+owner at edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni at intel.com]
> -=-=-=-=-=-=
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104241): https://edk2.groups.io/g/devel/message/104241
Mute This Topic: https://groups.io/mt/98654282/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list