Re: [edk2-devel] [PATCH v3] MinPlatformPkg: Update HWSignature filed in FADT

["VincentX Ke" <vincentx.ke@intel.com>]

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] 
> > -=-=-=-=-=-=
> >