* [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
@ 2023-04-28 2:56 VincentX Ke
2023-04-28 3:08 ` [edk2-devel] " Ni, Ray
0 siblings, 1 reply; 7+ messages in thread
From: VincentX Ke @ 2023-04-28 2:56 UTC (permalink / raw)
To: devel
Cc: VincentX Ke, Chasel Chiu, Nate DeSimone, Isaac Oram, Liming Gao,
Eric Dong
From: VincentX Ke <vincentx.ke@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4428
Calculating CRC based on checksum from all ACPI tables.
Update HWSignature filed in FADT based on CRC while ACPI table changed.
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>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 110 +++++++++++++++++++-
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
2 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index e967031a3b..d84c1d4f6d 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -1285,6 +1285,108 @@ IsHardwareChange (
FreePool (HWChange);
}
+/**
+ This function calculates RCR based on Checksum from all ACPI tables.
+ It also calculates CRC and provides as HWSignature filed in FADT table.
+**/
+VOID
+IsAcpiTableChange (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ UINTN Index;
+ UINTN AcpiTableCount;
+ UINT32 Table;
+ UINT32 CRC;
+ UINT32 *AcpiTable;
+ 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;
+ AcpiTable = 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;
+ }
+
+ //
+ // ACPI table count starts with 2 as RSDT and XSDT are already located.
+ // Then add ACPI tables found by XSDT and FADT.
+ //
+ Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->RsdtAddress;
+ Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->XsdtAddress;
+ AcpiTableCount = AcpiTableCount + 2;
+ AcpiTableCount = AcpiTableCount + (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER))/sizeof (UINT64);
+
+ 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->XDsdt != 0) || (pFADT->Dsdt != 0)) {
+ AcpiTableCount = AcpiTableCount + 1;
+ }
+ }
+ }
+
+ //
+ // Allocate memory for founded ACPI tables.
+ //
+ AcpiTable = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
+ if (AcpiTable == NULL) {
+ return;
+ }
+
+ AcpiTableCount = 0;
+ AcpiTable[AcpiTableCount++] = Rsdt->Checksum;
+ AcpiTable[AcpiTableCount++] = Xsdt->Checksum;
+
+ for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < (Xsdt->Length); Index = Index + sizeof (UINT64)) {
+ Table = *((UINT32 *)((UINT8 *)Xsdt + Index));
+ AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Table)->Checksum;
+ 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->XDsdt != 0) {
+ AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)pFADT->XDsdt)->Checksum;
+ } else {
+ AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)pFADT->Dsdt)->Checksum;
+ }
+ }
+ }
+
+ //
+ // pFADT table not found.
+ //
+ if (pFADT == NULL) {
+ return;
+ }
+
+ //
+ // Calculate CRC value.
+ //
+ Status = gBS->CalculateCrc32 (AcpiTable, AcpiTableCount, &CRC);
+ DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
+
+ //
+ // Set HardwareSignature value based on CRC value.
+ //
+ FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)pFADT->FirmwareCtrl;
+ FacsPtr->HardwareSignature = CRC;
+ FreePool (AcpiTable);
+ DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
+}
+
VOID
UpdateLocalTable (
VOID
@@ -1329,7 +1431,13 @@ AcpiEndOfDxeEvent (
//
// Calculate Hardware Signature value based on current platform configurations
//
- IsHardwareChange ();
+ if ((PcdGet8 (PcdFadtMajorVersion) <= EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) &&
+ (PcdGet8 (PcdFadtMinorVersion) <= EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION))
+ {
+ IsHardwareChange ();
+ } else {
+ 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] 7+ messages in thread
* Re: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
2023-04-28 2:56 [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT VincentX Ke
@ 2023-04-28 3:08 ` Ni, Ray
2023-04-28 3:30 ` VincentX Ke
0 siblings, 1 reply; 7+ messages in thread
From: Ni, Ray @ 2023-04-28 3:08 UTC (permalink / raw)
To: devel@edk2.groups.io, Ke, VincentX
Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
Dong, Eric
Vincent,
It's an interesting patch.
The original logic is to use the HardwareSignature field to indicate any changes in adding/removing PCI devices.
Your new logic is to expand this field to indicate any changes in any tables when FADT version is > 6.3.
Why?
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> VincentX Ke
> Sent: Friday, April 28, 2023 10:57 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 v3] MinPlatformPkg: Update HWSignature
> filed in FADT
>
> From: VincentX Ke <vincentx.ke@intel.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4428
>
> Calculating CRC based on checksum from all ACPI tables.
> Update HWSignature filed in FADT based on CRC while ACPI table changed.
>
> 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>
> ---
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 110
> +++++++++++++++++++-
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
> 2 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index e967031a3b..d84c1d4f6d 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1285,6 +1285,108 @@ IsHardwareChange (
> FreePool (HWChange);
>
> }
>
>
>
> +/**
>
> + This function calculates RCR based on Checksum from all ACPI tables.
>
> + It also calculates CRC and provides as HWSignature filed in FADT table.
>
> +**/
>
> +VOID
>
> +IsAcpiTableChange (
>
> + VOID
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> + UINTN Index;
>
> + UINTN AcpiTableCount;
>
> + UINT32 Table;
>
> + UINT32 CRC;
>
> + UINT32 *AcpiTable;
>
> + 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;
>
> + AcpiTable = 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;
>
> + }
>
> +
>
> + //
>
> + // ACPI table count starts with 2 as RSDT and XSDT are already located.
>
> + // Then add ACPI tables found by XSDT and FADT.
>
> + //
>
> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> >RsdtAddress;
>
> + Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> >XsdtAddress;
>
> + AcpiTableCount = AcpiTableCount + 2;
>
> + AcpiTableCount = AcpiTableCount + (Xsdt->Length - sizeof
> (EFI_ACPI_DESCRIPTION_HEADER))/sizeof (UINT64);
>
> +
>
> + 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->XDsdt != 0) || (pFADT->Dsdt != 0)) {
>
> + AcpiTableCount = AcpiTableCount + 1;
>
> + }
>
> + }
>
> + }
>
> +
>
> + //
>
> + // Allocate memory for founded ACPI tables.
>
> + //
>
> + AcpiTable = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
>
> + if (AcpiTable == NULL) {
>
> + return;
>
> + }
>
> +
>
> + AcpiTableCount = 0;
>
> + AcpiTable[AcpiTableCount++] = Rsdt->Checksum;
>
> + AcpiTable[AcpiTableCount++] = Xsdt->Checksum;
>
> +
>
> + for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < (Xsdt-
> >Length); Index = Index + sizeof (UINT64)) {
>
> + Table = *((UINT32 *)((UINT8 *)Xsdt + Index));
>
> + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> *)(UINTN)Table)->Checksum;
>
> + 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->XDsdt != 0) {
>
> + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> *)(UINTN)pFADT->XDsdt)->Checksum;
>
> + } else {
>
> + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> *)(UINTN)pFADT->Dsdt)->Checksum;
>
> + }
>
> + }
>
> + }
>
> +
>
> + //
>
> + // pFADT table not found.
>
> + //
>
> + if (pFADT == NULL) {
>
> + return;
>
> + }
>
> +
>
> + //
>
> + // Calculate CRC value.
>
> + //
>
> + Status = gBS->CalculateCrc32 (AcpiTable, AcpiTableCount, &CRC);
>
> + DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
>
> +
>
> + //
>
> + // Set HardwareSignature value based on CRC value.
>
> + //
>
> + FacsPtr =
> (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)pFADT-
> >FirmwareCtrl;
>
> + FacsPtr->HardwareSignature = CRC;
>
> + FreePool (AcpiTable);
>
> + DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
>
> +}
>
> +
>
> VOID
>
> UpdateLocalTable (
>
> VOID
>
> @@ -1329,7 +1431,13 @@ AcpiEndOfDxeEvent (
> //
>
> // Calculate Hardware Signature value based on current platform
> configurations
>
> //
>
> - IsHardwareChange ();
>
> + if ((PcdGet8 (PcdFadtMajorVersion) <=
> EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) &&
>
> + (PcdGet8 (PcdFadtMinorVersion) <=
> EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION))
>
> + {
>
> + IsHardwareChange ();
>
> + } else {
>
> + 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 (#103734):
> https://edk2.groups.io/g/devel/message/103734
> Mute This Topic: https://groups.io/mt/98551423/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] 7+ messages in thread
* Re: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
2023-04-28 3:08 ` [edk2-devel] " Ni, Ray
@ 2023-04-28 3:30 ` VincentX Ke
2023-04-28 3:46 ` VincentX Ke
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: VincentX Ke @ 2023-04-28 3:30 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
Dong, Eric
Hi, Ray
Based on ACPI Spec 6.5, "Hardware Signature" filed changed the description.
Here is the new description.
"The only thing that determines the hardware signature is the ACPI tables.
If any content or structure of the ACPI tables has changed,
including adding or removing of tables, then the hardware signature must change."
I keep version 6.3 only for Intel code base test. It should be updated to 6.4.
Thanks for the reminder.
BR,
Vincent
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Friday, April 28, 2023 11:08 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>
Subject: RE: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
Vincent,
It's an interesting patch.
The original logic is to use the HardwareSignature field to indicate any changes in adding/removing PCI devices.
Your new logic is to expand this field to indicate any changes in any tables when FADT version is > 6.3.
Why?
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> VincentX Ke
> Sent: Friday, April 28, 2023 10:57 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 v3] MinPlatformPkg: Update HWSignature
> filed in FADT
>
> From: VincentX Ke <vincentx.ke@intel.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4428
>
> Calculating CRC based on checksum from all ACPI tables.
> Update HWSignature filed in FADT based on CRC while ACPI table changed.
>
> 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>
> ---
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 110
> +++++++++++++++++++-
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
> 2 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git
> a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index e967031a3b..d84c1d4f6d 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1285,6 +1285,108 @@ IsHardwareChange (
> FreePool (HWChange);
>
> }
>
>
>
> +/**
>
> + This function calculates RCR based on Checksum from all ACPI tables.
>
> + It also calculates CRC and provides as HWSignature filed in FADT table.
>
> +**/
>
> +VOID
>
> +IsAcpiTableChange (
>
> + VOID
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> + UINTN Index;
>
> + UINTN AcpiTableCount;
>
> + UINT32 Table;
>
> + UINT32 CRC;
>
> + UINT32 *AcpiTable;
>
> + 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;
>
> + AcpiTable = 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;
>
> + }
>
> +
>
> + //
>
> + // ACPI table count starts with 2 as RSDT and XSDT are already located.
>
> + // Then add ACPI tables found by XSDT and FADT.
>
> + //
>
> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> >RsdtAddress;
>
> + Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> >XsdtAddress;
>
> + AcpiTableCount = AcpiTableCount + 2;
>
> + AcpiTableCount = AcpiTableCount + (Xsdt->Length - sizeof
> (EFI_ACPI_DESCRIPTION_HEADER))/sizeof (UINT64);
>
> +
>
> + 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->XDsdt != 0) || (pFADT->Dsdt != 0)) {
>
> + AcpiTableCount = AcpiTableCount + 1;
>
> + }
>
> + }
>
> + }
>
> +
>
> + //
>
> + // Allocate memory for founded ACPI tables.
>
> + //
>
> + AcpiTable = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
>
> + if (AcpiTable == NULL) {
>
> + return;
>
> + }
>
> +
>
> + AcpiTableCount = 0;
>
> + AcpiTable[AcpiTableCount++] = Rsdt->Checksum;
>
> + AcpiTable[AcpiTableCount++] = Xsdt->Checksum;
>
> +
>
> + for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < (Xsdt-
> >Length); Index = Index + sizeof (UINT64)) {
>
> + Table = *((UINT32 *)((UINT8 *)Xsdt + Index));
>
> + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> *)(UINTN)Table)->Checksum;
>
> + 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->XDsdt != 0) {
>
> + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> *)(UINTN)pFADT->XDsdt)->Checksum;
>
> + } else {
>
> + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> *)(UINTN)pFADT->Dsdt)->Checksum;
>
> + }
>
> + }
>
> + }
>
> +
>
> + //
>
> + // pFADT table not found.
>
> + //
>
> + if (pFADT == NULL) {
>
> + return;
>
> + }
>
> +
>
> + //
>
> + // Calculate CRC value.
>
> + //
>
> + Status = gBS->CalculateCrc32 (AcpiTable, AcpiTableCount, &CRC);
>
> + DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
>
> +
>
> + //
>
> + // Set HardwareSignature value based on CRC value.
>
> + //
>
> + FacsPtr =
> (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)pFADT-
> >FirmwareCtrl;
>
> + FacsPtr->HardwareSignature = CRC;
>
> + FreePool (AcpiTable);
>
> + DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
>
> +}
>
> +
>
> VOID
>
> UpdateLocalTable (
>
> VOID
>
> @@ -1329,7 +1431,13 @@ AcpiEndOfDxeEvent (
> //
>
> // Calculate Hardware Signature value based on current platform
> configurations
>
> //
>
> - IsHardwareChange ();
>
> + if ((PcdGet8 (PcdFadtMajorVersion) <=
> EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) &&
>
> + (PcdGet8 (PcdFadtMinorVersion) <=
> EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION))
>
> + {
>
> + IsHardwareChange ();
>
> + } else {
>
> + 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 (#103734):
> https://edk2.groups.io/g/devel/message/103734
> Mute This Topic: https://groups.io/mt/98551423/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] 7+ messages in thread
* Re: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
2023-04-28 3:30 ` VincentX Ke
@ 2023-04-28 3:46 ` VincentX Ke
2023-04-28 3:54 ` Ni, Ray
[not found] ` <1759FCFBF7AAA148.29517@groups.io>
2 siblings, 0 replies; 7+ messages in thread
From: VincentX Ke @ 2023-04-28 3:46 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
Dong, Eric
Dear All,
Patch v4 updated. Please let me know if there is any suggestion. Thanks
[PATCH v4] https://edk2.groups.io/g/devel/message/103738
BR,
Vincent
-----Original Message-----
From: Ke, VincentX
Sent: Friday, April 28, 2023 11:30 AM
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
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>
Subject: RE: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
Hi, Ray
Based on ACPI Spec 6.5, "Hardware Signature" filed changed the description.
Here is the new description.
"The only thing that determines the hardware signature is the ACPI tables.
If any content or structure of the ACPI tables has changed, including adding or removing of tables, then the hardware signature must change."
I keep version 6.3 only for Intel code base test. It should be updated to 6.4.
Thanks for the reminder.
BR,
Vincent
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Friday, April 28, 2023 11:08 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>
Subject: RE: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
Vincent,
It's an interesting patch.
The original logic is to use the HardwareSignature field to indicate any changes in adding/removing PCI devices.
Your new logic is to expand this field to indicate any changes in any tables when FADT version is > 6.3.
Why?
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> VincentX Ke
> Sent: Friday, April 28, 2023 10:57 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 v3] MinPlatformPkg: Update HWSignature
> filed in FADT
>
> From: VincentX Ke <vincentx.ke@intel.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4428
>
> Calculating CRC based on checksum from all ACPI tables.
> Update HWSignature filed in FADT based on CRC while ACPI table changed.
>
> 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>
> ---
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 110
> +++++++++++++++++++-
> Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
> 2 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git
> a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index e967031a3b..d84c1d4f6d 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1285,6 +1285,108 @@ IsHardwareChange (
> FreePool (HWChange);
>
> }
>
>
>
> +/**
>
> + This function calculates RCR based on Checksum from all ACPI tables.
>
> + It also calculates CRC and provides as HWSignature filed in FADT table.
>
> +**/
>
> +VOID
>
> +IsAcpiTableChange (
>
> + VOID
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> + UINTN Index;
>
> + UINTN AcpiTableCount;
>
> + UINT32 Table;
>
> + UINT32 CRC;
>
> + UINT32 *AcpiTable;
>
> + 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;
>
> + AcpiTable = 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;
>
> + }
>
> +
>
> + //
>
> + // ACPI table count starts with 2 as RSDT and XSDT are already located.
>
> + // Then add ACPI tables found by XSDT and FADT.
>
> + //
>
> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> >RsdtAddress;
>
> + Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> >XsdtAddress;
>
> + AcpiTableCount = AcpiTableCount + 2;
>
> + AcpiTableCount = AcpiTableCount + (Xsdt->Length - sizeof
> (EFI_ACPI_DESCRIPTION_HEADER))/sizeof (UINT64);
>
> +
>
> + 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->XDsdt != 0) || (pFADT->Dsdt != 0)) {
>
> + AcpiTableCount = AcpiTableCount + 1;
>
> + }
>
> + }
>
> + }
>
> +
>
> + //
>
> + // Allocate memory for founded ACPI tables.
>
> + //
>
> + AcpiTable = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
>
> + if (AcpiTable == NULL) {
>
> + return;
>
> + }
>
> +
>
> + AcpiTableCount = 0;
>
> + AcpiTable[AcpiTableCount++] = Rsdt->Checksum;
>
> + AcpiTable[AcpiTableCount++] = Xsdt->Checksum;
>
> +
>
> + for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < (Xsdt-
> >Length); Index = Index + sizeof (UINT64)) {
>
> + Table = *((UINT32 *)((UINT8 *)Xsdt + Index));
>
> + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> *)(UINTN)Table)->Checksum;
>
> + 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->XDsdt != 0) {
>
> + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> *)(UINTN)pFADT->XDsdt)->Checksum;
>
> + } else {
>
> + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> *)(UINTN)pFADT->Dsdt)->Checksum;
>
> + }
>
> + }
>
> + }
>
> +
>
> + //
>
> + // pFADT table not found.
>
> + //
>
> + if (pFADT == NULL) {
>
> + return;
>
> + }
>
> +
>
> + //
>
> + // Calculate CRC value.
>
> + //
>
> + Status = gBS->CalculateCrc32 (AcpiTable, AcpiTableCount, &CRC);
>
> + DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
>
> +
>
> + //
>
> + // Set HardwareSignature value based on CRC value.
>
> + //
>
> + FacsPtr =
> (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)pFADT-
> >FirmwareCtrl;
>
> + FacsPtr->HardwareSignature = CRC;
>
> + FreePool (AcpiTable);
>
> + DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
>
> +}
>
> +
>
> VOID
>
> UpdateLocalTable (
>
> VOID
>
> @@ -1329,7 +1431,13 @@ AcpiEndOfDxeEvent (
> //
>
> // Calculate Hardware Signature value based on current platform
> configurations
>
> //
>
> - IsHardwareChange ();
>
> + if ((PcdGet8 (PcdFadtMajorVersion) <=
> EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) &&
>
> + (PcdGet8 (PcdFadtMinorVersion) <=
> EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION))
>
> + {
>
> + IsHardwareChange ();
>
> + } else {
>
> + 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 (#103734):
> https://edk2.groups.io/g/devel/message/103734
> Mute This Topic: https://groups.io/mt/98551423/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] 7+ messages in thread
* Re: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
2023-04-28 3:30 ` VincentX Ke
2023-04-28 3:46 ` VincentX Ke
@ 2023-04-28 3:54 ` Ni, Ray
2023-05-03 2:44 ` VincentX Ke
[not found] ` <1759FCFBF7AAA148.29517@groups.io>
2 siblings, 1 reply; 7+ messages in thread
From: Ni, Ray @ 2023-04-28 3:54 UTC (permalink / raw)
To: Ke, VincentX, devel@edk2.groups.io
Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
Dong, Eric
RSDP points to RSDT or XSDT.
RSDT/XSDT contains multiple tables, one of which is FADT, others are SSDTs.
FADT contains two pointers, one pointing to FACS, the other pointing to DSDT.
So:
1. Your code assume if Table->Checksum is not changed, the Table is not changed. I don't think it's a valid assumption.
2. Your code "measures" the DSDT change which is good. (still only reading Checksum is not good.)
3. FACS table is not "measured".
Thanks,
Ray
> -----Original Message-----
> From: Ke, VincentX <vincentx.ke@intel.com>
> Sent: Friday, April 28, 2023 11:30 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> 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>
> Subject: RE: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature
> filed in FADT
>
> Hi, Ray
>
> Based on ACPI Spec 6.5, "Hardware Signature" filed changed the description.
>
> Here is the new description.
> "The only thing that determines the hardware signature is the ACPI tables.
> If any content or structure of the ACPI tables has changed,
> including adding or removing of tables, then the hardware signature must
> change."
>
> I keep version 6.3 only for Intel code base test. It should be updated to 6.4.
> Thanks for the reminder.
>
> BR,
> Vincent
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Friday, April 28, 2023 11:08 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>
> Subject: RE: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature
> filed in FADT
>
> Vincent,
> It's an interesting patch.
>
> The original logic is to use the HardwareSignature field to indicate any
> changes in adding/removing PCI devices.
> Your new logic is to expand this field to indicate any changes in any tables
> when FADT version is > 6.3.
>
> Why?
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > VincentX Ke
> > Sent: Friday, April 28, 2023 10:57 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 v3] MinPlatformPkg: Update HWSignature
> > filed in FADT
> >
> > From: VincentX Ke <vincentx.ke@intel.com>
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4428
> >
> > Calculating CRC based on checksum from all ACPI tables.
> > Update HWSignature filed in FADT based on CRC while ACPI table changed.
> >
> > 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>
> > ---
> > Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 110
> > +++++++++++++++++++-
> > Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
> > 2 files changed, 110 insertions(+), 1 deletion(-)
> >
> > diff --git
> > a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > index e967031a3b..d84c1d4f6d 100644
> > --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > @@ -1285,6 +1285,108 @@ IsHardwareChange (
> > FreePool (HWChange);
> >
> > }
> >
> >
> >
> > +/**
> >
> > + This function calculates RCR based on Checksum from all ACPI tables.
> >
> > + It also calculates CRC and provides as HWSignature filed in FADT table.
> >
> > +**/
> >
> > +VOID
> >
> > +IsAcpiTableChange (
> >
> > + VOID
> >
> > + )
> >
> > +{
> >
> > + EFI_STATUS Status;
> >
> > + UINTN Index;
> >
> > + UINTN AcpiTableCount;
> >
> > + UINT32 Table;
> >
> > + UINT32 CRC;
> >
> > + UINT32 *AcpiTable;
> >
> > + 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;
> >
> > + AcpiTable = 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;
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // ACPI table count starts with 2 as RSDT and XSDT are already located.
> >
> > + // Then add ACPI tables found by XSDT and FADT.
> >
> > + //
> >
> > + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> > >RsdtAddress;
> >
> > + Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> > >XsdtAddress;
> >
> > + AcpiTableCount = AcpiTableCount + 2;
> >
> > + AcpiTableCount = AcpiTableCount + (Xsdt->Length - sizeof
> > (EFI_ACPI_DESCRIPTION_HEADER))/sizeof (UINT64);
> >
> > +
> >
> > + 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->XDsdt != 0) || (pFADT->Dsdt != 0)) {
> >
> > + AcpiTableCount = AcpiTableCount + 1;
> >
> > + }
> >
> > + }
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // Allocate memory for founded ACPI tables.
> >
> > + //
> >
> > + AcpiTable = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
> >
> > + if (AcpiTable == NULL) {
> >
> > + return;
> >
> > + }
> >
> > +
> >
> > + AcpiTableCount = 0;
> >
> > + AcpiTable[AcpiTableCount++] = Rsdt->Checksum;
> >
> > + AcpiTable[AcpiTableCount++] = Xsdt->Checksum;
> >
> > +
> >
> > + for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < (Xsdt-
> > >Length); Index = Index + sizeof (UINT64)) {
> >
> > + Table = *((UINT32 *)((UINT8 *)Xsdt + Index));
> >
> > + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> > *)(UINTN)Table)->Checksum;
> >
> > + 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->XDsdt != 0) {
> >
> > + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> > *)(UINTN)pFADT->XDsdt)->Checksum;
> >
> > + } else {
> >
> > + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> > *)(UINTN)pFADT->Dsdt)->Checksum;
> >
> > + }
> >
> > + }
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // pFADT table not found.
> >
> > + //
> >
> > + if (pFADT == NULL) {
> >
> > + return;
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // Calculate CRC value.
> >
> > + //
> >
> > + Status = gBS->CalculateCrc32 (AcpiTable, AcpiTableCount, &CRC);
> >
> > + DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
> >
> > +
> >
> > + //
> >
> > + // Set HardwareSignature value based on CRC value.
> >
> > + //
> >
> > + FacsPtr =
> > (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)pFADT-
> > >FirmwareCtrl;
> >
> > + FacsPtr->HardwareSignature = CRC;
> >
> > + FreePool (AcpiTable);
> >
> > + DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
> >
> > +}
> >
> > +
> >
> > VOID
> >
> > UpdateLocalTable (
> >
> > VOID
> >
> > @@ -1329,7 +1431,13 @@ AcpiEndOfDxeEvent (
> > //
> >
> > // Calculate Hardware Signature value based on current platform
> > configurations
> >
> > //
> >
> > - IsHardwareChange ();
> >
> > + if ((PcdGet8 (PcdFadtMajorVersion) <=
> > EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) &&
> >
> > + (PcdGet8 (PcdFadtMinorVersion) <=
> > EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION))
> >
> > + {
> >
> > + IsHardwareChange ();
> >
> > + } else {
> >
> > + 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 (#103734):
> > https://edk2.groups.io/g/devel/message/103734
> > Mute This Topic: https://groups.io/mt/98551423/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] 7+ messages in thread
* Re: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
[not found] ` <1759FCFBF7AAA148.29517@groups.io>
@ 2023-04-28 5:02 ` Ni, Ray
0 siblings, 0 replies; 7+ messages in thread
From: Ni, Ray @ 2023-04-28 5:02 UTC (permalink / raw)
To: devel@edk2.groups.io, Ni, Ray, Ke, VincentX
Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
Dong, Eric
"Measure" checksum is not sufficient to detect ACPI table change because if one field in the table is changed
from value x to x+1, and another field is changed from y to y-1, the Checksum doesn't change.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Friday, April 28, 2023 11:55 AM
> To: Ke, VincentX <vincentx.ke@intel.com>; devel@edk2.groups.io
> 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>
> Subject: Re: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature
> filed in FADT
>
> RSDP points to RSDT or XSDT.
> RSDT/XSDT contains multiple tables, one of which is FADT, others are SSDTs.
> FADT contains two pointers, one pointing to FACS, the other pointing to DSDT.
>
> So:
> 1. Your code assume if Table->Checksum is not changed, the Table is not
> changed. I don't think it's a valid assumption.
> 2. Your code "measures" the DSDT change which is good. (still only reading
> Checksum is not good.)
> 3. FACS table is not "measured".
>
> Thanks,
> Ray
>
> > -----Original Message-----
> > From: Ke, VincentX <vincentx.ke@intel.com>
> > Sent: Friday, April 28, 2023 11:30 AM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > 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>
> > Subject: RE: [edk2-devel] [PATCH v3] MinPlatformPkg: Update
> HWSignature
> > filed in FADT
> >
> > Hi, Ray
> >
> > Based on ACPI Spec 6.5, "Hardware Signature" filed changed the
> description.
> >
> > Here is the new description.
> > "The only thing that determines the hardware signature is the ACPI tables.
> > If any content or structure of the ACPI tables has changed,
> > including adding or removing of tables, then the hardware signature must
> > change."
> >
> > I keep version 6.3 only for Intel code base test. It should be updated to 6.4.
> > Thanks for the reminder.
> >
> > BR,
> > Vincent
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Friday, April 28, 2023 11:08 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>
> > Subject: RE: [edk2-devel] [PATCH v3] MinPlatformPkg: Update
> HWSignature
> > filed in FADT
> >
> > Vincent,
> > It's an interesting patch.
> >
> > The original logic is to use the HardwareSignature field to indicate any
> > changes in adding/removing PCI devices.
> > Your new logic is to expand this field to indicate any changes in any tables
> > when FADT version is > 6.3.
> >
> > Why?
> >
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > VincentX Ke
> > > Sent: Friday, April 28, 2023 10:57 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 v3] MinPlatformPkg: Update HWSignature
> > > filed in FADT
> > >
> > > From: VincentX Ke <vincentx.ke@intel.com>
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4428
> > >
> > > Calculating CRC based on checksum from all ACPI tables.
> > > Update HWSignature filed in FADT based on CRC while ACPI table
> changed.
> > >
> > > 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>
> > > ---
> > > Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 110
> > > +++++++++++++++++++-
> > > Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
> > > 2 files changed, 110 insertions(+), 1 deletion(-)
> > >
> > > diff --git
> > > a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > > b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > > index e967031a3b..d84c1d4f6d 100644
> > > --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > > +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > > @@ -1285,6 +1285,108 @@ IsHardwareChange (
> > > FreePool (HWChange);
> > >
> > > }
> > >
> > >
> > >
> > > +/**
> > >
> > > + This function calculates RCR based on Checksum from all ACPI tables.
> > >
> > > + It also calculates CRC and provides as HWSignature filed in FADT table.
> > >
> > > +**/
> > >
> > > +VOID
> > >
> > > +IsAcpiTableChange (
> > >
> > > + VOID
> > >
> > > + )
> > >
> > > +{
> > >
> > > + EFI_STATUS Status;
> > >
> > > + UINTN Index;
> > >
> > > + UINTN AcpiTableCount;
> > >
> > > + UINT32 Table;
> > >
> > > + UINT32 CRC;
> > >
> > > + UINT32 *AcpiTable;
> > >
> > > + 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;
> > >
> > > + AcpiTable = 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;
> > >
> > > + }
> > >
> > > +
> > >
> > > + //
> > >
> > > + // ACPI table count starts with 2 as RSDT and XSDT are already located.
> > >
> > > + // Then add ACPI tables found by XSDT and FADT.
> > >
> > > + //
> > >
> > > + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> > > >RsdtAddress;
> > >
> > > + Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> > > >XsdtAddress;
> > >
> > > + AcpiTableCount = AcpiTableCount + 2;
> > >
> > > + AcpiTableCount = AcpiTableCount + (Xsdt->Length - sizeof
> > > (EFI_ACPI_DESCRIPTION_HEADER))/sizeof (UINT64);
> > >
> > > +
> > >
> > > + 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->XDsdt != 0) || (pFADT->Dsdt != 0)) {
> > >
> > > + AcpiTableCount = AcpiTableCount + 1;
> > >
> > > + }
> > >
> > > + }
> > >
> > > + }
> > >
> > > +
> > >
> > > + //
> > >
> > > + // Allocate memory for founded ACPI tables.
> > >
> > > + //
> > >
> > > + AcpiTable = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
> > >
> > > + if (AcpiTable == NULL) {
> > >
> > > + return;
> > >
> > > + }
> > >
> > > +
> > >
> > > + AcpiTableCount = 0;
> > >
> > > + AcpiTable[AcpiTableCount++] = Rsdt->Checksum;
> > >
> > > + AcpiTable[AcpiTableCount++] = Xsdt->Checksum;
> > >
> > > +
> > >
> > > + for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < (Xsdt-
> > > >Length); Index = Index + sizeof (UINT64)) {
> > >
> > > + Table = *((UINT32 *)((UINT8 *)Xsdt + Index));
> > >
> > > + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> > > *)(UINTN)Table)->Checksum;
> > >
> > > + 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->XDsdt != 0) {
> > >
> > > + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> > > *)(UINTN)pFADT->XDsdt)->Checksum;
> > >
> > > + } else {
> > >
> > > + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> > > *)(UINTN)pFADT->Dsdt)->Checksum;
> > >
> > > + }
> > >
> > > + }
> > >
> > > + }
> > >
> > > +
> > >
> > > + //
> > >
> > > + // pFADT table not found.
> > >
> > > + //
> > >
> > > + if (pFADT == NULL) {
> > >
> > > + return;
> > >
> > > + }
> > >
> > > +
> > >
> > > + //
> > >
> > > + // Calculate CRC value.
> > >
> > > + //
> > >
> > > + Status = gBS->CalculateCrc32 (AcpiTable, AcpiTableCount, &CRC);
> > >
> > > + DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
> > >
> > > +
> > >
> > > + //
> > >
> > > + // Set HardwareSignature value based on CRC value.
> > >
> > > + //
> > >
> > > + FacsPtr =
> > > (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)pFADT-
> > > >FirmwareCtrl;
> > >
> > > + FacsPtr->HardwareSignature = CRC;
> > >
> > > + FreePool (AcpiTable);
> > >
> > > + DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
> > >
> > > +}
> > >
> > > +
> > >
> > > VOID
> > >
> > > UpdateLocalTable (
> > >
> > > VOID
> > >
> > > @@ -1329,7 +1431,13 @@ AcpiEndOfDxeEvent (
> > > //
> > >
> > > // Calculate Hardware Signature value based on current platform
> > > configurations
> > >
> > > //
> > >
> > > - IsHardwareChange ();
> > >
> > > + if ((PcdGet8 (PcdFadtMajorVersion) <=
> > > EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) &&
> > >
> > > + (PcdGet8 (PcdFadtMinorVersion) <=
> > > EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION))
> > >
> > > + {
> > >
> > > + IsHardwareChange ();
> > >
> > > + } else {
> > >
> > > + 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 (#103734):
> > > https://edk2.groups.io/g/devel/message/103734
> > > Mute This Topic: https://groups.io/mt/98551423/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] 7+ messages in thread
* Re: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
2023-04-28 3:54 ` Ni, Ray
@ 2023-05-03 2:44 ` VincentX Ke
0 siblings, 0 replies; 7+ messages in thread
From: VincentX Ke @ 2023-05-03 2:44 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
Dong, Eric
Hi, Ray
Update patch and fix your concern in the patch v5.
Please let me know if there is any suggestion. Thanks.
[Patch v5] https://edk2.groups.io/g/devel/message/103877
BR,
Vincent
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Friday, April 28, 2023 11:55 AM
To: Ke, VincentX <vincentx.ke@intel.com>; devel@edk2.groups.io
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>
Subject: RE: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
RSDP points to RSDT or XSDT.
RSDT/XSDT contains multiple tables, one of which is FADT, others are SSDTs.
FADT contains two pointers, one pointing to FACS, the other pointing to DSDT.
So:
1. Your code assume if Table->Checksum is not changed, the Table is not changed. I don't think it's a valid assumption.
2. Your code "measures" the DSDT change which is good. (still only reading Checksum is not good.) 3. FACS table is not "measured".
Thanks,
Ray
> -----Original Message-----
> From: Ke, VincentX <vincentx.ke@intel.com>
> Sent: Friday, April 28, 2023 11:30 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> 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>
> Subject: RE: [edk2-devel] [PATCH v3] MinPlatformPkg: Update
> HWSignature filed in FADT
>
> Hi, Ray
>
> Based on ACPI Spec 6.5, "Hardware Signature" filed changed the description.
>
> Here is the new description.
> "The only thing that determines the hardware signature is the ACPI tables.
> If any content or structure of the ACPI tables has changed, including
> adding or removing of tables, then the hardware signature must
> change."
>
> I keep version 6.3 only for Intel code base test. It should be updated to 6.4.
> Thanks for the reminder.
>
> BR,
> Vincent
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Friday, April 28, 2023 11:08 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>
> Subject: RE: [edk2-devel] [PATCH v3] MinPlatformPkg: Update
> HWSignature filed in FADT
>
> Vincent,
> It's an interesting patch.
>
> The original logic is to use the HardwareSignature field to indicate
> any changes in adding/removing PCI devices.
> Your new logic is to expand this field to indicate any changes in any
> tables when FADT version is > 6.3.
>
> Why?
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > VincentX Ke
> > Sent: Friday, April 28, 2023 10:57 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 v3] MinPlatformPkg: Update HWSignature
> > filed in FADT
> >
> > From: VincentX Ke <vincentx.ke@intel.com>
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4428
> >
> > Calculating CRC based on checksum from all ACPI tables.
> > Update HWSignature filed in FADT based on CRC while ACPI table changed.
> >
> > 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>
> > ---
> > Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 110
> > +++++++++++++++++++-
> > Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf | 1 +
> > 2 files changed, 110 insertions(+), 1 deletion(-)
> >
> > diff --git
> > a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > index e967031a3b..d84c1d4f6d 100644
> > --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > @@ -1285,6 +1285,108 @@ IsHardwareChange (
> > FreePool (HWChange);
> >
> > }
> >
> >
> >
> > +/**
> >
> > + This function calculates RCR based on Checksum from all ACPI tables.
> >
> > + It also calculates CRC and provides as HWSignature filed in FADT table.
> >
> > +**/
> >
> > +VOID
> >
> > +IsAcpiTableChange (
> >
> > + VOID
> >
> > + )
> >
> > +{
> >
> > + EFI_STATUS Status;
> >
> > + UINTN Index;
> >
> > + UINTN AcpiTableCount;
> >
> > + UINT32 Table;
> >
> > + UINT32 CRC;
> >
> > + UINT32 *AcpiTable;
> >
> > + 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;
> >
> > + AcpiTable = 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;
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // ACPI table count starts with 2 as RSDT and XSDT are already located.
> >
> > + // Then add ACPI tables found by XSDT and FADT.
> >
> > + //
> >
> > + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> > >RsdtAddress;
> >
> > + Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> > >XsdtAddress;
> >
> > + AcpiTableCount = AcpiTableCount + 2;
> >
> > + AcpiTableCount = AcpiTableCount + (Xsdt->Length - sizeof
> > (EFI_ACPI_DESCRIPTION_HEADER))/sizeof (UINT64);
> >
> > +
> >
> > + 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->XDsdt != 0) || (pFADT->Dsdt != 0)) {
> >
> > + AcpiTableCount = AcpiTableCount + 1;
> >
> > + }
> >
> > + }
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // Allocate memory for founded ACPI tables.
> >
> > + //
> >
> > + AcpiTable = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
> >
> > + if (AcpiTable == NULL) {
> >
> > + return;
> >
> > + }
> >
> > +
> >
> > + AcpiTableCount = 0;
> >
> > + AcpiTable[AcpiTableCount++] = Rsdt->Checksum;
> >
> > + AcpiTable[AcpiTableCount++] = Xsdt->Checksum;
> >
> > +
> >
> > + for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < (Xsdt-
> > >Length); Index = Index + sizeof (UINT64)) {
> >
> > + Table = *((UINT32 *)((UINT8 *)Xsdt + Index));
> >
> > + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> > *)(UINTN)Table)->Checksum;
> >
> > + 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->XDsdt != 0) {
> >
> > + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> > *)(UINTN)pFADT->XDsdt)->Checksum;
> >
> > + } else {
> >
> > + AcpiTable[AcpiTableCount++] = ((EFI_ACPI_DESCRIPTION_HEADER
> > *)(UINTN)pFADT->Dsdt)->Checksum;
> >
> > + }
> >
> > + }
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // pFADT table not found.
> >
> > + //
> >
> > + if (pFADT == NULL) {
> >
> > + return;
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // Calculate CRC value.
> >
> > + //
> >
> > + Status = gBS->CalculateCrc32 (AcpiTable, AcpiTableCount, &CRC);
> >
> > + DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
> >
> > +
> >
> > + //
> >
> > + // Set HardwareSignature value based on CRC value.
> >
> > + //
> >
> > + FacsPtr =
> > (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)pFADT-
> > >FirmwareCtrl;
> >
> > + FacsPtr->HardwareSignature = CRC;
> >
> > + FreePool (AcpiTable);
> >
> > + DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
> >
> > +}
> >
> > +
> >
> > VOID
> >
> > UpdateLocalTable (
> >
> > VOID
> >
> > @@ -1329,7 +1431,13 @@ AcpiEndOfDxeEvent (
> > //
> >
> > // Calculate Hardware Signature value based on current platform
> > configurations
> >
> > //
> >
> > - IsHardwareChange ();
> >
> > + if ((PcdGet8 (PcdFadtMajorVersion) <=
> > EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) &&
> >
> > + (PcdGet8 (PcdFadtMinorVersion) <=
> > EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_MINOR_REVISION))
> >
> > + {
> >
> > + IsHardwareChange ();
> >
> > + } else {
> >
> > + 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 (#103734):
> > https://edk2.groups.io/g/devel/message/103734
> > Mute This Topic: https://groups.io/mt/98551423/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] 7+ messages in thread
end of thread, other threads:[~2023-05-03 2:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-28 2:56 [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT VincentX Ke
2023-04-28 3:08 ` [edk2-devel] " Ni, Ray
2023-04-28 3:30 ` VincentX Ke
2023-04-28 3:46 ` VincentX Ke
2023-04-28 3:54 ` Ni, Ray
2023-05-03 2:44 ` VincentX Ke
[not found] ` <1759FCFBF7AAA148.29517@groups.io>
2023-04-28 5:02 ` Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox