Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 182 +++++++++++++------- Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 + 2 files changed, 118 insertions(+), 65 deletions(-)
From: VincentX Ke <vincentx.ke@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@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: VincentX Ke <vincentx.ke@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > VincentX Ke > Sent: Wednesday, May 3, 2023 10:40 AM > To: devel@edk2.groups.io > Cc: Ke, VincentX <vincentx.ke@intel.com>; Chiu, Chasel > <chasel.chiu@intel.com>; Desimone, Nathaniel L > <nathaniel.l.desimone@intel.com>; Oram, Isaac W > <isaac.w.oram@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, > Eric <eric.dong@intel.com> > Subject: [edk2-devel] [PATCH v5] MinPlatformPkg: Update HWSignature > filed in FACS > > From: VincentX Ke <vincentx.ke@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@intel.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> > Cc: Isaac Oram <isaac.w.oram@intel.com> > Cc: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Eric Dong <eric.dong@intel.com> > Signed-off-by: VincentX Ke <vincentx.ke@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@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi, Ray To fix the concern you mentioned. [Patch V6] updated with https://edk2.groups.io/g/devel/message/104537 Please kindly code review and let me know your suggestion. Thanks a lot. Vincent -----Original Message----- From: Ni, Ray <ray.ni@intel.com> Sent: Monday, May 8, 2023 11:23 AM To: devel@edk2.groups.io; Ke, VincentX <vincentx.ke@intel.com> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>; Sinha, Ankit <ankit.sinha@intel.com> Subject: RE: [edk2-devel] [PATCH v5] MinPlatformPkg: Update HWSignature filed in FACS 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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > VincentX Ke > Sent: Wednesday, May 3, 2023 10:40 AM > To: devel@edk2.groups.io > Cc: Ke, VincentX <vincentx.ke@intel.com>; Chiu, Chasel > <chasel.chiu@intel.com>; Desimone, Nathaniel L > <nathaniel.l.desimone@intel.com>; Oram, Isaac W > <isaac.w.oram@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; > Dong, Eric <eric.dong@intel.com> > Subject: [edk2-devel] [PATCH v5] MinPlatformPkg: Update HWSignature > filed in FACS > > From: VincentX Ke <vincentx.ke@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@intel.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> > Cc: Isaac Oram <isaac.w.oram@intel.com> > Cc: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Eric Dong <eric.dong@intel.com> > Signed-off-by: VincentX Ke <vincentx.ke@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@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com] > -=-=-=-=-=-= > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#104538): https://edk2.groups.io/g/devel/message/104538 Mute This Topic: https://groups.io/mt/98654282/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.