Re: [edk2-devel] [PATCH v5] MinPlatformPkg: Update HWSignature filed in FACS

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

Hi, Ray

To fix the concern you mentioned.
[Patch V6] updated with https://edk2.groups.io/g/devel/message/104537

Please kindly code review and let me know your suggestion. 
Thanks a lot.
Vincent
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Monday, May 8, 2023 11:23 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>; Sinha, Ankit <ankit.sinha@intel.com>
Subject: RE: [edk2-devel] [PATCH v5] MinPlatformPkg: Update HWSignature filed in FACS

Vincent,
I don't quite understand your code logic.
For example:
1. Why you use XSDT only? What if there is only RSDT?
2. Why there is a do-while in AcpiTableCrcCalculator()?
3. Why  (VOID *) is converted to (UINTN) then to (EFI_ACPI_DESCRIPTION_HEADER*)?
4. Why you "measure" the XSDT and RSDT table? The two tables only contain pointers.
5.  Can you please review " MdePkg/Library/UefiLib/Acpi.c" which contains logic that iterates the acpi tables?

Thanks,
Ray


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
> VincentX Ke
> Sent: Wednesday, May 3, 2023 10:40 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 v5] MinPlatformPkg: Update HWSignature 
> filed in FACS
> 
> From: VincentX Ke <vincentx.ke@intel.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4428
> 
> Calculating CRC based on each ACPI table.
> Update HWSignature filed in FACS based on CRC while ACPI table changed.
> 
> Change-Id: Ic0ca66ff10cda0fbcd0683020fab1bc9aea9b78c
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Eric Dong <eric.dong@intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c   | 182
> +++++++++++++-------
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf |   1 +
>  2 files changed, 118 insertions(+), 65 deletions(-)
> 
> diff --git 
> a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index e967031a3b..a940424ced 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1191,98 +1191,150 @@ PlatformUpdateTables (  }
> 
> 
> 
>  /**
> 
> -  This function calculates RCR based on PCI Device ID and Vendor ID 
> from the devices
> 
> -  available on the platform.
> 
> -  It also includes other instances of BIOS change to calculate CRC 
> and provides as
> 
> -  HWSignature filed in FADT table.
> 
> +  This function calculates CRC based on each offset in the ACPI table.
> 
> +
> 
> +  @param[in] Table          The pointer to ACPI table that
> 
> +                            required to calculate CRC.
> 
> +
> 
> +  @retval CRC               A pointer to allocate UINT32 that
> 
> +                            contains the CRC32 data.
> 
> +**/
> 
> +UINT32
> 
> +AcpiTableCrcCalculator (
> 
> +  IN  VOID  *Table
> 
> +  )
> 
> +{
> 
> +  UINT32  CRC;
> 
> +
> 
> +  CRC = 0;
> 
> +
> 
> +  //
> 
> +  // Calculate CRC value.
> 
> +  //
> 
> +  if (((EFI_ACPI_6_5_COMMON_HEADER *)(UINTN)Table)->Signature ==
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> 
> +    //
> 
> +    // Zero HardwareSignature field before Calculating FACS CRC
> 
> +    //
> 
> +    do {
> 
> +      ((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)Table)->HardwareSignature = 0;
> 
> +    } while (((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)Table)->HardwareSignature);
> 
> +
> 
> +    gBS->CalculateCrc32 ((UINT8 *)Table,
> (UINTN)((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)Table)->Length, &CRC);
> 
> +  } else {
> 
> +    gBS->CalculateCrc32 ((UINT8 *)Table,
> (UINTN)((EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Table)->Length, &CRC);
> 
> +  }
> 
> +
> 
> +  return CRC;
> 
> +}
> 
> +
> 
> +/**
> 
> +  This function calculates CRC based on each ACPI table.
> 
> +  It also calculates CRC and provides as HWSignature filed in FACS.
> 
>  **/
> 
>  VOID
> 
> -IsHardwareChange (
> 
> +IsAcpiTableChange (
> 
>    VOID
> 
>    )
> 
>  {
> 
> -  EFI_STATUS                    Status;
> 
> -  UINTN                         Index;
> 
> -  UINTN                         HandleCount;
> 
> -  EFI_HANDLE                    *HandleBuffer;
> 
> -  EFI_PCI_IO_PROTOCOL           *PciIo;
> 
> -  UINT32                        CRC;
> 
> -  UINT32                        *HWChange;
> 
> -  UINTN                         HWChangeSize;
> 
> -  UINT32                        PciId;
> 
> -  UINTN                         Handle;
> 
> -  EFI_ACPI_6_3_FIRMWARE_ACPI_CONTROL_STRUCTURE *FacsPtr;
> 
> -  EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE    *pFADT;
> 
> -
> 
> -  HandleCount  = 0;
> 
> -  HandleBuffer = NULL;
> 
> -
> 
> -  Status = gBS->LocateHandleBuffer (
> 
> -                  ByProtocol,
> 
> -                  &gEfiPciIoProtocolGuid,
> 
> -                  NULL,
> 
> -                  &HandleCount,
> 
> -                  &HandleBuffer
> 
> -                  );
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    return; // PciIO protocol not installed yet!
> 
> +  EFI_STATUS                                    Status;
> 
> +  UINTN                                         Index;
> 
> +  UINTN                                         AcpiTableCount;
> 
> +  UINT32                                        Table;
> 
> +  UINT32                                        HWSignature;
> 
> +  UINT32                                        *AcpiTableCrc;
> 
> +  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;
> 
> +  AcpiTableCrc   = 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;
> 
>    }
> 
> 
> 
>    //
> 
> -  // Allocate memory for HWChange and add additional entrie for
> 
> -  // pFADT->XDsdt
> 
> +  // ACPI table count starts with 2 as RSDT and XSDT are already located.
> 
> +  // Then add ACPI tables found by XSDT and FADT.
> 
>    //
> 
> -  HWChangeSize = HandleCount + 1;
> 
> -  HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
> 
> -  ASSERT(HWChange != NULL);
> 
> +  Rsdt            = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> >RsdtAddress;
> 
> +  Xsdt            = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp-
> >XsdtAddress;
> 
> +  AcpiTableCount += 2;
> 
> +  AcpiTableCount  = AcpiTableCount + (Xsdt->Length - sizeof
> (EFI_ACPI_DESCRIPTION_HEADER))/sizeof (UINT64);
> 
> 
> 
> -  if (HWChange == NULL) return;
> 
> +  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->FirmwareCtrl) {
> 
> +        AcpiTableCount++;
> 
> +      }
> 
> 
> 
> -  //
> 
> -  // add HWChange inputs: PCI devices
> 
> -  //
> 
> -  for (Index = 0; HandleCount > 0; HandleCount--) {
> 
> -    PciId = 0;
> 
> -    Status = gBS->HandleProtocol (HandleBuffer[Index],
> &gEfiPciIoProtocolGuid, (VOID **) &PciIo);
> 
> -    if (!EFI_ERROR (Status)) {
> 
> -      Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint32, 0, 1, &PciId);
> 
> -      if (EFI_ERROR (Status)) {
> 
> -        continue;
> 
> +      if ((pFADT->XDsdt) || (pFADT->Dsdt)) {
> 
> +        AcpiTableCount++;
> 
>        }
> 
> -      HWChange[Index++] = PciId;
> 
>      }
> 
>    }
> 
> 
> 
>    //
> 
> -  // Locate FACP Table
> 
> +  // Allocate memory for founded ACPI tables.
> 
>    //
> 
> -  Handle = 0;
> 
> -  Status = LocateAcpiTableBySignature (
> 
> -              EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
> 
> -              (EFI_ACPI_DESCRIPTION_HEADER **) &pFADT,
> 
> -              &Handle
> 
> -              );
> 
> -  if (EFI_ERROR (Status) || (pFADT == NULL)) {
> 
> -    return;  //Table not found or out of memory resource for pFADT table
> 
> +  AcpiTableCrc = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
> 
> +  if (AcpiTableCrc == NULL) {
> 
> +    return;
> 
> +  }
> 
> +
> 
> +  AcpiTableCount                 = 0;
> 
> +  AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator ((VOID
> *)(UINTN)Rsdt);
> 
> +  AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator ((VOID
> *)(UINTN)Xsdt);
> 
> +
> 
> +  for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < (Xsdt-
> >Length); Index = Index + sizeof (UINT64)) {
> 
> +    Table                          = *((UINT32 *)((UINT8 *)Xsdt + Index));
> 
> +    AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator ((VOID
> *)(UINTN)Table);
> 
> +    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->FirmwareCtrl) {
> 
> +        AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator 
> + ((VOID
> *)(UINTN)pFADT->FirmwareCtrl);
> 
> +      }
> 
> +
> 
> +      if (pFADT->XDsdt) {
> 
> +        AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator 
> + ((VOID
> *)(UINTN)pFADT->XDsdt);
> 
> +      } else {
> 
> +        AcpiTableCrc[AcpiTableCount++] = AcpiTableCrcCalculator 
> + ((VOID
> *)(UINTN)pFADT->Dsdt);
> 
> +      }
> 
> +    }
> 
>    }
> 
> 
> 
>    //
> 
> -  // add HWChange inputs: others
> 
> +  // pFADT table not found.
> 
>    //
> 
> -  HWChange[Index++] = (UINT32)pFADT->XDsdt;
> 
> +  if (pFADT == NULL) {
> 
> +    return;
> 
> +  }
> 
> 
> 
>    //
> 
> -  // Calculate CRC value with HWChange data.
> 
> +  // Calculate HWSignature data.
> 
>    //
> 
> -  Status = gBS->CalculateCrc32(HWChange, HWChangeSize, &CRC);
> 
> -  DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
> 
> +  Status = gBS->CalculateCrc32 (AcpiTableCrc, AcpiTableCount,
> &HWSignature);
> 
> +  DEBUG ((DEBUG_INFO, "HardwareSignature = %x and Status = %r\n",
> HWSignature, Status));
> 
> 
> 
>    //
> 
>    // Set HardwareSignature value based on CRC value.
> 
>    //
> 
> -  FacsPtr = (EFI_ACPI_6_3_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)pFADT->FirmwareCtrl;
> 
> -  FacsPtr->HardwareSignature = CRC;
> 
> -  FreePool (HWChange);
> 
> +  FacsPtr                    =
> (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)pFADT-
> >FirmwareCtrl;
> 
> +  FacsPtr->HardwareSignature = HWSignature;
> 
> +  FreePool (AcpiTableCrc);
> 
> +  DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
> 
>  }
> 
> 
> 
>  VOID
> 
> @@ -1329,7 +1381,7 @@ AcpiEndOfDxeEvent (
>    //
> 
>    // Calculate Hardware Signature value based on current platform 
> configurations
> 
>    //
> 
> -  IsHardwareChange ();
> 
> +  IsAcpiTableChange ();
> 
>  }
> 
> 
> 
>  /**
> 
> diff --git 
> a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> index 694492112b..f47cc3908d 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
> @@ -128,6 +128,7 @@
>    gEfiGlobalVariableGuid                        ## CONSUMES
> 
>    gEfiHobListGuid                               ## CONSUMES
> 
>    gEfiEndOfDxeEventGroupGuid                    ## CONSUMES
> 
> +  gEfiAcpiTableGuid                             ## CONSUMES
> 
> 
> 
>  [Depex]
> 
>    gEfiAcpiTableProtocolGuid           AND
> 
> --
> 2.39.2.windows.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#103877):
> https://edk2.groups.io/g/devel/message/103877
> Mute This Topic: https://groups.io/mt/98654282/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com] 
> -=-=-=-=-=-=
>