* [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
@ 2023-05-11 9:59 VincentX Ke
2023-05-12 9:32 ` [edk2-devel] " Ni, Ray
2023-05-15 21:16 ` Ankit Sinha
0 siblings, 2 replies; 4+ messages in thread
From: VincentX Ke @ 2023-05-11 9:59 UTC (permalink / raw)
To: devel
Cc: VincentX Ke, Chasel Chiu, Nate DeSimone, Isaac Oram, Liming Gao,
Eric Dong, Ankit Sinha
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>
Cc: Ankit Sinha<ankit.sinha@intel.com>
Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 287 +++++++++++++++-----
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
2 files changed, 223 insertions(+), 65 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index e967031a3b..3dca6f99f7 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -1191,98 +1191,255 @@ 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 ACPI table required to calculate CRC.
+
+ @retval CRC A pointer to allocate UINT32 that
+ contains the CRC32 data.
+**/
+UINT32
+AcpiTableCrcCalculator (
+ IN EFI_ACPI_COMMON_HEADER *Table
+ )
+{
+ EFI_STATUS Status;
+ UINT32 CRC;
+
+ Status = EFI_SUCCESS;
+ CRC = 0;
+
+ //
+ // Calculate CRC value.
+ //
+ if (Table->Signature == EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
+ //
+ // Zero HardwareSignature field before Calculating FACS CRC
+ //
+ ((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)Table)->HardwareSignature = 0;
+ }
+
+ Status = gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length, &CRC);
+ return CRC;
+}
+
+/**
+ This function count ACPI tables in RSDT/XSDT and return the result.
+
+ @param[in] Sdt ACPI XSDT/RSDT.
+ @param[in] TablePointerSize Size of table pointer:
+ 4(RSDT) or 8(XSDT).
+
+ @retval TableCount The total number of ACPI tables in
+ RSDT or XSDT.
+**/
+UINTN
+CountTableInSDT (
+ IN EFI_ACPI_DESCRIPTION_HEADER *Sdt,
+ IN UINTN TablePointerSize
+ )
+{
+ UINTN Index;
+ UINTN TableCount;
+ UINTN EntryCount;
+ UINT64 EntryPtr;
+ UINTN BasePtr;
+ EFI_ACPI_COMMON_HEADER *Table;
+
+ EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / TablePointerSize;
+ BasePtr = (UINTN)(Sdt + 1);
+
+ for (Index = 0, TableCount = 0; Index < EntryCount; Index++) {
+ EntryPtr = 0;
+ Table = NULL;
+ CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize), TablePointerSize);
+ Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
+ if (Table) {
+ TableCount++;
+ }
+
+ if (Table->Signature == EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
+ CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
+ if (Fadt.FirmwareCtrl || Fadt.XFirmwareCtrl) {
+ TableCount++;
+ }
+
+ if (Fadt.Dsdt || Fadt.XDsdt) {
+ TableCount++;
+ }
+ }
+ }
+
+ return TableCount;
+}
+
+/**
+ 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;
+ UINTN EntryCount;
+ UINTN BasePtr;
+ UINT64 EntryPtr;
+ UINT32 *TableCrcRecord;
+ UINT32 HWSignature;
+ EFI_ACPI_COMMON_HEADER *Table;
+ 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;
+
+ Index = 0;
+ AcpiTableCount = 0;
+ EntryCount = 0;
+ BasePtr = 0;
+ EntryPtr = 0;
+ HWSignature = 0;
+ TableCrcRecord = NULL;
+ Rsdp = NULL;
+ Rsdt = NULL;
+ Xsdt = NULL;
+ FacsPtr = NULL;
+
+ DEBUG ((DEBUG_INFO, "%a() - Start\n", __FUNCTION__));
+
+ Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, (VOID **)&Rsdp);
+ if (EFI_ERROR (Status) || (Rsdp == NULL)) {
+ return;
+ }
+
+ Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->RsdtAddress;
+ Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->XsdtAddress;
+
+ if (Xsdt->Signature != EFI_ACPI_6_5_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
+ if (Rsdt->Signature != EFI_ACPI_6_5_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
+ return;
+ }
}
//
- // Allocate memory for HWChange and add additional entrie for
- // pFADT->XDsdt
+ // ACPI table count starts with 0.
+ // Then add ACPI tables found by RSDT/XSDT and FADT.
//
- HWChangeSize = HandleCount + 1;
- HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
- ASSERT(HWChange != NULL);
+ AcpiTableCount = CountTableInSDT (Xsdt, sizeof (UINT64));
+ if (AcpiTableCount == 0) {
+ AcpiTableCount = CountTableInSDT (Rsdt, sizeof (UINT32));
+ }
- if (HWChange == NULL) return;
+ //
+ // Allocate memory for founded ACPI tables.
+ //
+ TableCrcRecord = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
+ if (TableCrcRecord == NULL) {
+ return;
+ }
//
- // add HWChange inputs: PCI devices
+ // Search XSDT
//
- 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;
+ AcpiTableCount = 0;
+ EntryCount = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / sizeof (UINT64);
+ BasePtr = (UINTN)(Xsdt + 1);
+ for (Index = 0; Index < EntryCount; Index++) {
+ EntryPtr = 0;
+ Table = NULL;
+ CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof (UINT64)), sizeof (UINT64));
+ Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
+ if (Table) {
+ TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
+ }
+
+ if (Table->Signature == EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
+ CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
+ //
+ // Locate FACS in FADT
+ //
+ CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XFirmwareCtrl, sizeof (UINT64));
+ if (EntryPtr != 0) {
+ FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)->XFirmwareCtrl;
+
+ TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
+ } else {
+ FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)->FirmwareCtrl;
+
+ TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
+ }
+
+ //
+ // Locate DSDT in FADT
+ //
+ CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XDsdt, sizeof (UINT64));
+ if (EntryPtr != 0) {
+ TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
+ } else {
+ TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
}
- HWChange[Index++] = PciId;
}
}
- //
- // Locate FACP Table
- //
- 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
+ if (FacsPtr->Signature != EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
+ //
+ // Search RSDT
+ //
+ AcpiTableCount = 0;
+ EntryCount = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / sizeof (UINT32);
+ BasePtr = (UINTN)(Rsdt + 1);
+ for (Index = 0; Index < EntryCount; Index++) {
+ EntryPtr = 0;
+ Table = NULL;
+ CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof (UINT32)), sizeof (UINT32));
+ Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
+ if (Table) {
+ TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
+ }
+
+ if (Table->Signature == EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
+ CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
+ //
+ // Locate FACS in FADT
+ //
+ if (Fadt.FirmwareCtrl) {
+ FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)->FirmwareCtrl;
+
+ TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
+ }
+
+ //
+ // Locate DSDT in FADT
+ //
+ if (Fadt.Dsdt) {
+ TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
+ }
+ }
+ }
}
//
- // add HWChange inputs: others
+ // FACS not found
//
- HWChange[Index++] = (UINT32)pFADT->XDsdt;
+ if (FacsPtr->Signature != EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
+ 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 (TableCrcRecord, 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->HardwareSignature = HWSignature;
+ FreePool (TableCrcRecord);
+ DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
}
VOID
@@ -1329,7 +1486,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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
2023-05-11 9:59 [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS VincentX Ke
@ 2023-05-12 9:32 ` Ni, Ray
2023-05-16 10:06 ` VincentX Ke
2023-05-15 21:16 ` Ankit Sinha
1 sibling, 1 reply; 4+ messages in thread
From: Ni, Ray @ 2023-05-12 9:32 UTC (permalink / raw)
To: devel@edk2.groups.io, Ke, VincentX
Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
Dong, Eric, Sinha, Ankit
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of VincentX Ke
> Sent: Thursday, May 11, 2023 6:00 PM
> 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>;
> Sinha, Ankit <ankit.sinha@intel.com>
> Subject: [edk2-devel] [PATCH v7] 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>
> Cc: Ankit Sinha<ankit.sinha@intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 287
> +++++++++++++++-----
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
> 2 files changed, 223 insertions(+), 65 deletions(-)
>
> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index e967031a3b..3dca6f99f7 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1191,98 +1191,255 @@ 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 ACPI table required to calculate CRC.
>
> +
>
> + @retval CRC A pointer to allocate UINT32 that
>
> + contains the CRC32 data.
>
> +**/
>
> +UINT32
>
> +AcpiTableCrcCalculator (
>
> + IN EFI_ACPI_COMMON_HEADER *Table
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> + UINT32 CRC;
>
> +
>
> + Status = EFI_SUCCESS;
>
> + CRC = 0;
>
> +
>
> + //
>
> + // Calculate CRC value.
>
> + //
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
>
> + //
>
> + // Zero HardwareSignature field before Calculating FACS CRC
>
> + //
>
> + ((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)Table)-
> >HardwareSignature = 0;
>
> + }
>
> +
>
> + Status = gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length, &CRC);
>
> + return CRC;
>
> +}
>
> +
>
> +/**
>
> + This function count ACPI tables in RSDT/XSDT and return the result.
>
> +
>
> + @param[in] Sdt ACPI XSDT/RSDT.
>
> + @param[in] TablePointerSize Size of table pointer:
>
> + 4(RSDT) or 8(XSDT).
>
> +
>
> + @retval TableCount The total number of ACPI tables in
>
> + RSDT or XSDT.
>
> +**/
>
> +UINTN
>
> +CountTableInSDT (
>
> + IN EFI_ACPI_DESCRIPTION_HEADER *Sdt,
>
> + IN UINTN TablePointerSize
>
> + )
>
> +{
>
> + UINTN Index;
>
> + UINTN TableCount;
>
> + UINTN EntryCount;
>
> + UINT64 EntryPtr;
>
> + UINTN BasePtr;
>
> + EFI_ACPI_COMMON_HEADER *Table;
>
> +
>
> + EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> TablePointerSize;
>
> + BasePtr = (UINTN)(Sdt + 1);
>
> +
>
> + for (Index = 0, TableCount = 0; Index < EntryCount; Index++) {
>
> + EntryPtr = 0;
>
> + Table = NULL;
>
> + CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize),
> TablePointerSize);
>
> + Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
>
> + if (Table) {
1. if (Table != NULL)
>
> + TableCount++;
>
> + }
>
> +
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
>
> + CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
2. Different versions of FADT have different sizes. This CopyMem is dangerous.
And why do you CopyMem()?
>
> + if (Fadt.FirmwareCtrl || Fadt.XFirmwareCtrl) {
3. if (Fadt.FirmwareCtrl != 0 || ...)
>
> + TableCount++;
>
> + }
>
> +
>
> + if (Fadt.Dsdt || Fadt.XDsdt) {
4. if (Fadt. Dsdt != 0 || ...)
>
> + TableCount++;
>
> + }
>
> + }
>
> + }
>
> +
>
> + return TableCount;
>
> +}
>
> +
>
> +/**
>
> + 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;
>
> + UINTN EntryCount;
>
> + UINTN BasePtr;
>
> + UINT64 EntryPtr;
>
> + UINT32 *TableCrcRecord;
>
> + UINT32 HWSignature;
>
> + EFI_ACPI_COMMON_HEADER *Table;
>
> + 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;
>
> +
>
> + Index = 0;
>
> + AcpiTableCount = 0;
>
> + EntryCount = 0;
>
> + BasePtr = 0;
>
> + EntryPtr = 0;
>
> + HWSignature = 0;
>
> + TableCrcRecord = NULL;
>
> + Rsdp = NULL;
>
> + Rsdt = NULL;
>
> + Xsdt = NULL;
>
> + FacsPtr = NULL;
>
> +
>
> + DEBUG ((DEBUG_INFO, "%a() - Start\n", __FUNCTION__));
>
> +
>
> + Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, (VOID
> **)&Rsdp);
>
> + if (EFI_ERROR (Status) || (Rsdp == NULL)) {
>
> + return;
>
> + }
>
> +
>
> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->RsdtAddress;
>
> + Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->XsdtAddress;
>
> +
>
> + if (Xsdt->Signature !=
> EFI_ACPI_6_5_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
>
> + if (Rsdt->Signature !=
> EFI_ACPI_6_5_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
>
> + return;
>
> + }
>
> }
5. You doesn't need to check the Signature. You can check if Xsdt is not 0, use XSDT, otherwise use RSDT.
>
>
>
> //
>
> - // Allocate memory for HWChange and add additional entrie for
>
> - // pFADT->XDsdt
>
> + // ACPI table count starts with 0.
>
> + // Then add ACPI tables found by RSDT/XSDT and FADT.
>
> //
>
> - HWChangeSize = HandleCount + 1;
>
> - HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
>
> - ASSERT(HWChange != NULL);
>
> + AcpiTableCount = CountTableInSDT (Xsdt, sizeof (UINT64));
>
> + if (AcpiTableCount == 0) {
>
> + AcpiTableCount = CountTableInSDT (Rsdt, sizeof (UINT32));
>
> + }
6. No need to choose between XSDT and RSDT again. Use #5 comment to choose between the two.
>
>
>
> - if (HWChange == NULL) return;
7. Why not return earlier but here?
8. I will stop reviewing. Can you please review within your team first then send out?
>
> + //
>
> + // Allocate memory for founded ACPI tables.
>
> + //
>
> + TableCrcRecord = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
>
> + if (TableCrcRecord == NULL) {
>
> + return;
>
> + }
>
>
>
> //
>
> - // add HWChange inputs: PCI devices
>
> + // Search XSDT
>
> //
>
> - 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;
>
> + AcpiTableCount = 0;
>
> + EntryCount = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof (UINT64);
>
> + BasePtr = (UINTN)(Xsdt + 1);
>
> + for (Index = 0; Index < EntryCount; Index++) {
>
> + EntryPtr = 0;
>
> + Table = NULL;
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof (UINT64)),
> sizeof (UINT64));
>
> + Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
>
> + if (Table) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
>
> + }
>
> +
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
>
> + CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
>
> + //
>
> + // Locate FACS in FADT
>
> + //
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XFirmwareCtrl, sizeof
> (UINT64));
>
> + if (EntryPtr != 0) {
>
> + FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >XFirmwareCtrl;
>
> +
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
>
> + } else {
>
> + FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
>
> +
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
>
> + }
>
> +
>
> + //
>
> + // Locate DSDT in FADT
>
> + //
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XDsdt, sizeof (UINT64));
>
> + if (EntryPtr != 0) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
>
> + } else {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
>
> }
>
> - HWChange[Index++] = PciId;
>
> }
>
> }
>
>
>
> - //
>
> - // Locate FACP Table
>
> - //
>
> - 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
>
> + if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
>
> + //
>
> + // Search RSDT
>
> + //
>
> + AcpiTableCount = 0;
>
> + EntryCount = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof (UINT32);
>
> + BasePtr = (UINTN)(Rsdt + 1);
>
> + for (Index = 0; Index < EntryCount; Index++) {
>
> + EntryPtr = 0;
>
> + Table = NULL;
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof (UINT32)),
> sizeof (UINT32));
>
> + Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
>
> + if (Table) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
>
> + }
>
> +
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
>
> + CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
>
> + //
>
> + // Locate FACS in FADT
>
> + //
>
> + if (Fadt.FirmwareCtrl) {
>
> + FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
>
> +
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
>
> + }
>
> +
>
> + //
>
> + // Locate DSDT in FADT
>
> + //
>
> + if (Fadt.Dsdt) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
>
> + }
>
> + }
>
> + }
>
> }
>
>
>
> //
>
> - // add HWChange inputs: others
>
> + // FACS not found
>
> //
>
> - HWChange[Index++] = (UINT32)pFADT->XDsdt;
>
> + if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
>
> + 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 (TableCrcRecord, 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->HardwareSignature = HWSignature;
>
> + FreePool (TableCrcRecord);
>
> + DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
>
> }
>
>
>
> VOID
>
> @@ -1329,7 +1486,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 (#104685): https://edk2.groups.io/g/devel/message/104685
> Mute This Topic: https://groups.io/mt/98824282/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
2023-05-11 9:59 [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS VincentX Ke
2023-05-12 9:32 ` [edk2-devel] " Ni, Ray
@ 2023-05-15 21:16 ` Ankit Sinha
1 sibling, 0 replies; 4+ messages in thread
From: Ankit Sinha @ 2023-05-15 21:16 UTC (permalink / raw)
To: Ke, VincentX, devel@edk2.groups.io
Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
Dong, Eric
Reviewed-by: Ankit Sinha<ankit.sinha@intel.com>
> -----Original Message-----
> From: Ke, VincentX <vincentx.ke@intel.com>
> Sent: Thursday, May 11, 2023 3:00 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>; Sinha, Ankit <ankit.sinha@intel.com>
> Subject: [PATCH v7] 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>
> Cc: Ankit Sinha<ankit.sinha@intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 287
> +++++++++++++++-----
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
> 2 files changed, 223 insertions(+), 65 deletions(-)
>
> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index e967031a3b..3dca6f99f7 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1191,98 +1191,255 @@ 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 ACPI table required to calculate CRC.
>
> +
>
> + @retval CRC A pointer to allocate UINT32 that
>
> + contains the CRC32 data.
>
> +**/
>
> +UINT32
>
> +AcpiTableCrcCalculator (
>
> + IN EFI_ACPI_COMMON_HEADER *Table
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> + UINT32 CRC;
>
> +
>
> + Status = EFI_SUCCESS;
>
> + CRC = 0;
>
> +
>
> + //
>
> + // Calculate CRC value.
>
> + //
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
>
> + //
>
> + // Zero HardwareSignature field before Calculating FACS CRC
>
> + //
>
> + ((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)Table)-
> >HardwareSignature = 0;
>
> + }
>
> +
>
> + Status = gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length,
> &CRC);
>
> + return CRC;
>
> +}
>
> +
>
> +/**
>
> + This function count ACPI tables in RSDT/XSDT and return the result.
>
> +
>
> + @param[in] Sdt ACPI XSDT/RSDT.
>
> + @param[in] TablePointerSize Size of table pointer:
>
> + 4(RSDT) or 8(XSDT).
>
> +
>
> + @retval TableCount The total number of ACPI tables in
>
> + RSDT or XSDT.
>
> +**/
>
> +UINTN
>
> +CountTableInSDT (
>
> + IN EFI_ACPI_DESCRIPTION_HEADER *Sdt,
>
> + IN UINTN TablePointerSize
>
> + )
>
> +{
>
> + UINTN Index;
>
> + UINTN TableCount;
>
> + UINTN EntryCount;
>
> + UINT64 EntryPtr;
>
> + UINTN BasePtr;
>
> + EFI_ACPI_COMMON_HEADER *Table;
>
> +
>
> + EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> TablePointerSize;
>
> + BasePtr = (UINTN)(Sdt + 1);
>
> +
>
> + for (Index = 0, TableCount = 0; Index < EntryCount; Index++) {
>
> + EntryPtr = 0;
>
> + Table = NULL;
>
> + CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize),
> TablePointerSize);
>
> + Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
>
> + if (Table) {
>
> + TableCount++;
>
> + }
>
> +
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
>
> + CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
>
> + if (Fadt.FirmwareCtrl || Fadt.XFirmwareCtrl) {
>
> + TableCount++;
>
> + }
>
> +
>
> + if (Fadt.Dsdt || Fadt.XDsdt) {
>
> + TableCount++;
>
> + }
>
> + }
>
> + }
>
> +
>
> + return TableCount;
>
> +}
>
> +
>
> +/**
>
> + 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;
>
> + UINTN EntryCount;
>
> + UINTN BasePtr;
>
> + UINT64 EntryPtr;
>
> + UINT32 *TableCrcRecord;
>
> + UINT32 HWSignature;
>
> + EFI_ACPI_COMMON_HEADER *Table;
>
> + 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;
>
> +
>
> + Index = 0;
>
> + AcpiTableCount = 0;
>
> + EntryCount = 0;
>
> + BasePtr = 0;
>
> + EntryPtr = 0;
>
> + HWSignature = 0;
>
> + TableCrcRecord = NULL;
>
> + Rsdp = NULL;
>
> + Rsdt = NULL;
>
> + Xsdt = NULL;
>
> + FacsPtr = NULL;
>
> +
>
> + DEBUG ((DEBUG_INFO, "%a() - Start\n", __FUNCTION__));
>
> +
>
> + Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, (VOID
> **)&Rsdp);
>
> + if (EFI_ERROR (Status) || (Rsdp == NULL)) {
>
> + return;
>
> + }
>
> +
>
> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->RsdtAddress;
>
> + Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->XsdtAddress;
>
> +
>
> + if (Xsdt->Signature !=
> EFI_ACPI_6_5_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
>
> + if (Rsdt->Signature !=
> EFI_ACPI_6_5_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
>
> + return;
>
> + }
>
> }
>
>
>
> //
>
> - // Allocate memory for HWChange and add additional entrie for
>
> - // pFADT->XDsdt
>
> + // ACPI table count starts with 0.
>
> + // Then add ACPI tables found by RSDT/XSDT and FADT.
>
> //
>
> - HWChangeSize = HandleCount + 1;
>
> - HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
>
> - ASSERT(HWChange != NULL);
>
> + AcpiTableCount = CountTableInSDT (Xsdt, sizeof (UINT64));
>
> + if (AcpiTableCount == 0) {
>
> + AcpiTableCount = CountTableInSDT (Rsdt, sizeof (UINT32));
>
> + }
>
>
>
> - if (HWChange == NULL) return;
>
> + //
>
> + // Allocate memory for founded ACPI tables.
>
> + //
>
> + TableCrcRecord = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
>
> + if (TableCrcRecord == NULL) {
>
> + return;
>
> + }
>
>
>
> //
>
> - // add HWChange inputs: PCI devices
>
> + // Search XSDT
>
> //
>
> - 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;
>
> + AcpiTableCount = 0;
>
> + EntryCount = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER))
> / sizeof (UINT64);
>
> + BasePtr = (UINTN)(Xsdt + 1);
>
> + for (Index = 0; Index < EntryCount; Index++) {
>
> + EntryPtr = 0;
>
> + Table = NULL;
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof
> (UINT64)), sizeof (UINT64));
>
> + Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
>
> + if (Table) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
>
> + }
>
> +
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
>
> + CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
>
> + //
>
> + // Locate FACS in FADT
>
> + //
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XFirmwareCtrl, sizeof
> (UINT64));
>
> + if (EntryPtr != 0) {
>
> + FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >XFirmwareCtrl;
>
> +
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
>
> + } else {
>
> + FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
>
> +
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
>
> + }
>
> +
>
> + //
>
> + // Locate DSDT in FADT
>
> + //
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XDsdt, sizeof (UINT64));
>
> + if (EntryPtr != 0) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
>
> + } else {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
>
> }
>
> - HWChange[Index++] = PciId;
>
> }
>
> }
>
>
>
> - //
>
> - // Locate FACP Table
>
> - //
>
> - 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
>
> + if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
>
> + //
>
> + // Search RSDT
>
> + //
>
> + AcpiTableCount = 0;
>
> + EntryCount = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER))
> / sizeof (UINT32);
>
> + BasePtr = (UINTN)(Rsdt + 1);
>
> + for (Index = 0; Index < EntryCount; Index++) {
>
> + EntryPtr = 0;
>
> + Table = NULL;
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof
> (UINT32)), sizeof (UINT32));
>
> + Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
>
> + if (Table) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
>
> + }
>
> +
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
>
> + CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
>
> + //
>
> + // Locate FACS in FADT
>
> + //
>
> + if (Fadt.FirmwareCtrl) {
>
> + FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
>
> +
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
>
> + }
>
> +
>
> + //
>
> + // Locate DSDT in FADT
>
> + //
>
> + if (Fadt.Dsdt) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
>
> + }
>
> + }
>
> + }
>
> }
>
>
>
> //
>
> - // add HWChange inputs: others
>
> + // FACS not found
>
> //
>
> - HWChange[Index++] = (UINT32)pFADT->XDsdt;
>
> + if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
>
> + 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 (TableCrcRecord, 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->HardwareSignature = HWSignature;
>
> + FreePool (TableCrcRecord);
>
> + DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
>
> }
>
>
>
> VOID
>
> @@ -1329,7 +1486,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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
2023-05-12 9:32 ` [edk2-devel] " Ni, Ray
@ 2023-05-16 10:06 ` VincentX Ke
0 siblings, 0 replies; 4+ messages in thread
From: VincentX Ke @ 2023-05-16 10:06 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
Dong, Eric, Sinha, Ankit
Hi, Ray
Please see below replies. And thanks for the code review.
#2. Different versions of FADT have different sizes. This CopyMem is dangerous. And why do you CopyMem()?
Reply: Based on https://edk2.groups.io/g/devel/topic/98821097#104899, The extern variable "Facs" will only keep the unique one in AcpiPlatform.c, "extern EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE Facs" should be follow up if version changed. And since the purpose is calculating CRC, we only need data of FACS rather than physical pointer to FACS.
#5. You don't need to check the Signature. You can check if Xsdt is not 0, use XSDT, otherwise use RSDT.
Reply: To avoid the case that XSDT is garbage in RSDT only platform. Only checking with "Xsdt is not 0" cannot meet the requirement.
#7. Why not return earlier but here?
Reply: The reason to return here is AllocateZeroPool() failed or XSDT/RSDT doesn't find out any pointer entry. This return should be kept.
Sincerely,
Vincent
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Friday, May 12, 2023 5:33 PM
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 v7] MinPlatformPkg: Update HWSignature filed in FACS
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> VincentX Ke
> Sent: Thursday, May 11, 2023 6:00 PM
> 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>; Sinha, Ankit <ankit.sinha@intel.com>
> Subject: [edk2-devel] [PATCH v7] 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>
> Cc: Ankit Sinha<ankit.sinha@intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 287
> +++++++++++++++-----
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
> 2 files changed, 223 insertions(+), 65 deletions(-)
>
> diff --git
> a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index e967031a3b..3dca6f99f7 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1191,98 +1191,255 @@ 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 ACPI table required to calculate CRC.
>
> +
>
> + @retval CRC A pointer to allocate UINT32 that
>
> + contains the CRC32 data.
>
> +**/
>
> +UINT32
>
> +AcpiTableCrcCalculator (
>
> + IN EFI_ACPI_COMMON_HEADER *Table
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> + UINT32 CRC;
>
> +
>
> + Status = EFI_SUCCESS;
>
> + CRC = 0;
>
> +
>
> + //
>
> + // Calculate CRC value.
>
> + //
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
>
> + //
>
> + // Zero HardwareSignature field before Calculating FACS CRC
>
> + //
>
> + ((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)Table)-
> >HardwareSignature = 0;
>
> + }
>
> +
>
> + Status = gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length,
> + &CRC);
>
> + return CRC;
>
> +}
>
> +
>
> +/**
>
> + This function count ACPI tables in RSDT/XSDT and return the result.
>
> +
>
> + @param[in] Sdt ACPI XSDT/RSDT.
>
> + @param[in] TablePointerSize Size of table pointer:
>
> + 4(RSDT) or 8(XSDT).
>
> +
>
> + @retval TableCount The total number of ACPI tables in
>
> + RSDT or XSDT.
>
> +**/
>
> +UINTN
>
> +CountTableInSDT (
>
> + IN EFI_ACPI_DESCRIPTION_HEADER *Sdt,
>
> + IN UINTN TablePointerSize
>
> + )
>
> +{
>
> + UINTN Index;
>
> + UINTN TableCount;
>
> + UINTN EntryCount;
>
> + UINT64 EntryPtr;
>
> + UINTN BasePtr;
>
> + EFI_ACPI_COMMON_HEADER *Table;
>
> +
>
> + EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> TablePointerSize;
>
> + BasePtr = (UINTN)(Sdt + 1);
>
> +
>
> + for (Index = 0, TableCount = 0; Index < EntryCount; Index++) {
>
> + EntryPtr = 0;
>
> + Table = NULL;
>
> + CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize),
> TablePointerSize);
>
> + Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
>
> + if (Table) {
1. if (Table != NULL)
>
> + TableCount++;
>
> + }
>
> +
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
>
> + CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
2. Different versions of FADT have different sizes. This CopyMem is dangerous.
And why do you CopyMem()?
>
> + if (Fadt.FirmwareCtrl || Fadt.XFirmwareCtrl) {
3. if (Fadt.FirmwareCtrl != 0 || ...)
>
> + TableCount++;
>
> + }
>
> +
>
> + if (Fadt.Dsdt || Fadt.XDsdt) {
4. if (Fadt. Dsdt != 0 || ...)
>
> + TableCount++;
>
> + }
>
> + }
>
> + }
>
> +
>
> + return TableCount;
>
> +}
>
> +
>
> +/**
>
> + 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;
>
> + UINTN EntryCount;
>
> + UINTN BasePtr;
>
> + UINT64 EntryPtr;
>
> + UINT32 *TableCrcRecord;
>
> + UINT32 HWSignature;
>
> + EFI_ACPI_COMMON_HEADER *Table;
>
> + 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;
>
> +
>
> + Index = 0;
>
> + AcpiTableCount = 0;
>
> + EntryCount = 0;
>
> + BasePtr = 0;
>
> + EntryPtr = 0;
>
> + HWSignature = 0;
>
> + TableCrcRecord = NULL;
>
> + Rsdp = NULL;
>
> + Rsdt = NULL;
>
> + Xsdt = NULL;
>
> + FacsPtr = NULL;
>
> +
>
> + DEBUG ((DEBUG_INFO, "%a() - Start\n", __FUNCTION__));
>
> +
>
> + Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, (VOID
> **)&Rsdp);
>
> + if (EFI_ERROR (Status) || (Rsdp == NULL)) {
>
> + return;
>
> + }
>
> +
>
> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->RsdtAddress;
>
> + Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->XsdtAddress;
>
> +
>
> + if (Xsdt->Signature !=
> EFI_ACPI_6_5_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
>
> + if (Rsdt->Signature !=
> EFI_ACPI_6_5_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
>
> + return;
>
> + }
>
> }
5. You doesn't need to check the Signature. You can check if Xsdt is not 0, use XSDT, otherwise use RSDT.
>
>
>
> //
>
> - // Allocate memory for HWChange and add additional entrie for
>
> - // pFADT->XDsdt
>
> + // ACPI table count starts with 0.
>
> + // Then add ACPI tables found by RSDT/XSDT and FADT.
>
> //
>
> - HWChangeSize = HandleCount + 1;
>
> - HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
>
> - ASSERT(HWChange != NULL);
>
> + AcpiTableCount = CountTableInSDT (Xsdt, sizeof (UINT64));
>
> + if (AcpiTableCount == 0) {
>
> + AcpiTableCount = CountTableInSDT (Rsdt, sizeof (UINT32));
>
> + }
6. No need to choose between XSDT and RSDT again. Use #5 comment to choose between the two.
>
>
>
> - if (HWChange == NULL) return;
7. Why not return earlier but here?
8. I will stop reviewing. Can you please review within your team first then send out?
>
> + //
>
> + // Allocate memory for founded ACPI tables.
>
> + //
>
> + TableCrcRecord = AllocateZeroPool (sizeof (UINT32) *
> + AcpiTableCount);
>
> + if (TableCrcRecord == NULL) {
>
> + return;
>
> + }
>
>
>
> //
>
> - // add HWChange inputs: PCI devices
>
> + // Search XSDT
>
> //
>
> - 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;
>
> + AcpiTableCount = 0;
>
> + EntryCount = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof (UINT64);
>
> + BasePtr = (UINTN)(Xsdt + 1);
>
> + for (Index = 0; Index < EntryCount; Index++) {
>
> + EntryPtr = 0;
>
> + Table = NULL;
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof
> + (UINT64)),
> sizeof (UINT64));
>
> + Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
>
> + if (Table) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> + (Table);
>
> + }
>
> +
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
>
> + CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
>
> + //
>
> + // Locate FACS in FADT
>
> + //
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XFirmwareCtrl, sizeof
> (UINT64));
>
> + if (EntryPtr != 0) {
>
> + FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >XFirmwareCtrl;
>
> +
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
>
> + } else {
>
> + FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
>
> +
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
>
> + }
>
> +
>
> + //
>
> + // Locate DSDT in FADT
>
> + //
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XDsdt, sizeof
> + (UINT64));
>
> + if (EntryPtr != 0) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
>
> + } else {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
>
> }
>
> - HWChange[Index++] = PciId;
>
> }
>
> }
>
>
>
> - //
>
> - // Locate FACP Table
>
> - //
>
> - 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
>
> + if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
>
> + //
>
> + // Search RSDT
>
> + //
>
> + AcpiTableCount = 0;
>
> + EntryCount = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof (UINT32);
>
> + BasePtr = (UINTN)(Rsdt + 1);
>
> + for (Index = 0; Index < EntryCount; Index++) {
>
> + EntryPtr = 0;
>
> + Table = NULL;
>
> + CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof
> + (UINT32)),
> sizeof (UINT32));
>
> + Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
>
> + if (Table) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> + (Table);
>
> + }
>
> +
>
> + if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
>
> + CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
>
> + //
>
> + // Locate FACS in FADT
>
> + //
>
> + if (Fadt.FirmwareCtrl) {
>
> + FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
>
> +
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
>
> + }
>
> +
>
> + //
>
> + // Locate DSDT in FADT
>
> + //
>
> + if (Fadt.Dsdt) {
>
> + TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
>
> + }
>
> + }
>
> + }
>
> }
>
>
>
> //
>
> - // add HWChange inputs: others
>
> + // FACS not found
>
> //
>
> - HWChange[Index++] = (UINT32)pFADT->XDsdt;
>
> + if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
>
> + 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 (TableCrcRecord, 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->HardwareSignature = HWSignature;
>
> + FreePool (TableCrcRecord);
>
> + DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
>
> }
>
>
>
> VOID
>
> @@ -1329,7 +1486,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 (#104685):
> https://edk2.groups.io/g/devel/message/104685
> Mute This Topic: https://groups.io/mt/98824282/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-05-16 10:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-11 9:59 [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS VincentX Ke
2023-05-12 9:32 ` [edk2-devel] " Ni, Ray
2023-05-16 10:06 ` VincentX Ke
2023-05-15 21:16 ` Ankit Sinha
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox