[edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT
Ni, Ray
ray.ni at intel.com
Fri Apr 28 05:02:30 UTC 2023
"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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Friday, April 28, 2023 11:55 AM
> To: Ke, VincentX <vincentx.ke at intel.com>; devel at edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu at intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone at intel.com>; Oram, Isaac W
> <isaac.w.oram at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>; Dong,
> Eric <eric.dong at intel.com>
> Subject: 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 at intel.com>
> > Sent: Friday, April 28, 2023 11:30 AM
> > To: Ni, Ray <ray.ni at intel.com>; devel at edk2.groups.io
> > Cc: Chiu, Chasel <chasel.chiu at intel.com>; Desimone, Nathaniel L
> > <nathaniel.l.desimone at intel.com>; Oram, Isaac W
> > <isaac.w.oram at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>;
> Dong,
> > Eric <eric.dong at intel.com>
> > Subject: 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 at intel.com>
> > Sent: Friday, April 28, 2023 11:08 AM
> > To: devel at edk2.groups.io; Ke, VincentX <vincentx.ke at intel.com>
> > Cc: Chiu, Chasel <chasel.chiu at intel.com>; Desimone, Nathaniel L
> > <nathaniel.l.desimone at intel.com>; Oram, Isaac W
> > <isaac.w.oram at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>;
> Dong,
> > Eric <eric.dong at intel.com>
> > Subject: 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> > > VincentX Ke
> > > Sent: Friday, April 28, 2023 10:57 AM
> > > To: devel at edk2.groups.io
> > > Cc: Ke, VincentX <vincentx.ke at intel.com>; Chiu, Chasel
> > > <chasel.chiu at intel.com>; Desimone, Nathaniel L
> > > <nathaniel.l.desimone at intel.com>; Oram, Isaac W
> > > <isaac.w.oram at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>;
> > > Dong, Eric <eric.dong at intel.com>
> > > Subject: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature
> > > filed in FADT
> > >
> > > From: VincentX Ke <vincentx.ke at 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 at intel.com>
> > > Cc: Chasel Chiu <chasel.chiu at intel.com>
> > > Cc: Nate DeSimone <nathaniel.l.desimone at intel.com>
> > > Cc: Isaac Oram <isaac.w.oram at intel.com>
> > > Cc: Liming Gao <gaoliming at byosoft.com.cn>
> > > Cc: Eric Dong <eric.dong at intel.com>
> > > ---
> > > 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 at edk2.groups.io
> > > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni at intel.com]
> > > -=-=-=-=-=-=
> > >
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103748): https://edk2.groups.io/g/devel/message/103748
Mute This Topic: https://groups.io/mt/98551423/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list