"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...@intel.com>; devel@edk2.groups.io > Cc: Chiu, Chasel <chasel.c...@intel.com>; Desimone, Nathaniel L > <nathaniel.l.desim...@intel.com>; Oram, Isaac W > <isaac.w.o...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Dong, > Eric <eric.d...@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...@intel.com> > > Sent: Friday, April 28, 2023 11:30 AM > > To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io > > Cc: Chiu, Chasel <chasel.c...@intel.com>; Desimone, Nathaniel L > > <nathaniel.l.desim...@intel.com>; Oram, Isaac W > > <isaac.w.o...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; > Dong, > > Eric <eric.d...@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...@intel.com> > > Sent: Friday, April 28, 2023 11:08 AM > > To: devel@edk2.groups.io; Ke, VincentX <vincentx...@intel.com> > > Cc: Chiu, Chasel <chasel.c...@intel.com>; Desimone, Nathaniel L > > <nathaniel.l.desim...@intel.com>; Oram, Isaac W > > <isaac.w.o...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; > Dong, > > Eric <eric.d...@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...@intel.com>; Chiu, Chasel > > > <chasel.c...@intel.com>; Desimone, Nathaniel L > > > <nathaniel.l.desim...@intel.com>; Oram, Isaac W > > > <isaac.w.o...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; > > > Dong, Eric <eric.d...@intel.com> > > > Subject: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature > > > filed in FADT > > > > > > From: VincentX Ke <vincentx...@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...@intel.com> > > > Cc: Chasel Chiu <chasel.c...@intel.com> > > > Cc: Nate DeSimone <nathaniel.l.desim...@intel.com> > > > Cc: Isaac Oram <isaac.w.o...@intel.com> > > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > > Cc: Eric Dong <eric.d...@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+ow...@edk2.groups.io > > > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray...@intel.com] > > > -=-=-=-=-=-= > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103748): https://edk2.groups.io/g/devel/message/103748 Mute This Topic: https://groups.io/mt/98551423/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
