public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v9] MinPlatformPkg: Update HWSignature field in FACS
@ 2023-05-24  8:39 VincentX Ke
  2023-05-30 15:52 ` Ankit Sinha
  2023-05-31  2:01 ` [edk2-devel] " Ni, Ray
  0 siblings, 2 replies; 4+ messages in thread
From: VincentX Ke @ 2023-05-24  8:39 UTC (permalink / raw)
  To: devel
  Cc: VincentX Ke, Chasel Chiu, Nate DeSimone, Isaac Oram, Liming Gao,
	Eric Dong, Ankit Sinha

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 field 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>
Cc: Ankit Sinha<ankit.sinha@intel.com>
Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
---
 Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c   | 282 +++++++++++++++-----
 Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf |   1 +
 2 files changed, 215 insertions(+), 68 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index 2f2c96f907..ca1c73f6fe 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -1191,98 +1191,244 @@ 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.
+  Function prototype for GetAcpiTableCount/CalculateAcpiTableCrc.
+
+  @param[in] Table        The pointer to ACPI table.
+  @param[in] TableIndex   The ACPI table index.
+  @param[in] Context      The pointer to UINTN for GetAcpiTableCount.
+                          The pointer to UINT32 array for CalculateAcpiTableCrc.
 **/
+typedef
 VOID
-IsHardwareChange (
-  VOID
+(EFIAPI *ACPI_TABLE_CALLBACK)(
+  IN  EFI_ACPI_COMMON_HEADER  *Table,
+  IN  UINTN                   TableIndex,
+  IN  VOID                    *Context
+  );
+
+/**
+  Enumerate all ACPI tables in RSDT/XSDT.
+
+  @param[in] Sdt                ACPI XSDT/RSDT.
+  @param[in] TablePointerSize   Size of table pointer:
+                                4(RSDT) or 8(XSDT).
+  @param[in] CallbackFunction   The pointer to GetAcpiTableCount/CalculateAcpiTableCrc.
+  @param[in] Context            The pointer to UINTN for GetAcpiTableCount.
+                                The pointer to UINT32 array for CalculateAcpiTableCrc.
+**/
+VOID
+EnumerateAllAcpiTables (
+  IN  EFI_ACPI_DESCRIPTION_HEADER  *Sdt,
+  IN  UINTN                        TablePointerSize,
+  IN  ACPI_TABLE_CALLBACK          CallbackFunction,
+  IN  VOID                         *Context
   )
 {
-  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_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *FacsPtr;
-  EFI_ACPI_6_5_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!
+  UINTN                                      Index;
+  UINTN                                      TableIndex;
+  UINTN                                      EntryCount;
+  UINT64                                     EntryPtr;
+  UINTN                                      BasePtr;
+  EFI_ACPI_COMMON_HEADER                     *Table;
+  EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE  *FadtPtr;
+
+  Index      = 0;
+  TableIndex = 0;
+  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / TablePointerSize;
+  EntryPtr   = 0;
+  BasePtr    = (UINTN)(Sdt + 1);
+  Table      = NULL;
+  FadtPtr    = NULL;
+
+  if (Sdt == NULL) {
+    ASSERT (Sdt != NULL);
+    return;
   }
 
-  //
-  // Allocate memory for HWChange and add additional entrie for
-  // pFADT->XDsdt
-  //
-  HWChangeSize = HandleCount + 1;
-  HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
-  ASSERT(HWChange != NULL);
+  for (Index = 0; Index < EntryCount; Index++) {
+    EntryPtr = 0;
+    Table    = NULL;
+    CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize), TablePointerSize);
+    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
+    if (Table != NULL) {
+      CallbackFunction (Table, TableIndex++, Context);
+    }
+
+    if (Table->Signature == EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
+      FadtPtr = (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table;
+      if (FadtPtr->Header.Revision < EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) {
+        //
+        // Locate FACS/DSDT in FADT
+        //
+        CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr->FirmwareCtrl, TableIndex++, Context);
+        CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr->Dsdt, TableIndex++, Context);
+      } else {
+        //
+        // Locate FACS in FADT
+        //
+        if (FadtPtr->XFirmwareCtrl) {
+          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr->XFirmwareCtrl, TableIndex++, Context);
+        } else {
+          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr->FirmwareCtrl, TableIndex++, Context);
+        }
+
+        //
+        // Locate DSDT in FADT
+        //
+        if (FadtPtr->XDsdt) {
+          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr->XDsdt, TableIndex++, Context);
+        } else {
+          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr->Dsdt, TableIndex++, Context);
+        }
+      }
+    }
+  }
+}
+
+/**
+  Count the number of ACPI tables.
+
+  @param[in] Table        The pointer to ACPI table.
+  @param[in] TableIndex   The ACPI table index.
+  @param[in] Context      The pointer to UINTN.
+**/
+VOID
+GetAcpiTableCount (
+  IN  EFI_ACPI_COMMON_HEADER  *Table,
+  IN  UINTN                   TableIndex,
+  IN  VOID                    *Context
+  )
+{
+  UINTN  *TableCount;
+
+  TableCount = (UINTN *)Context;
+
+  if (Table == NULL) {
+    ASSERT (Table != NULL);
+    return;
+  }
+
+  (*TableCount)++;
+}
 
-  if (HWChange == NULL) return;
+/**
+  Calculate CRC based on each offset in the ACPI table.
+
+  @param[in] Table        The pointer to ACPI table.
+  @param[in] TableIndex   The ACPI table index.
+  @param[in] Context      The pointer to UINT32 array.
+**/
+VOID
+CalculateAcpiTableCrc (
+  IN  EFI_ACPI_COMMON_HEADER  *Table,
+  IN  UINTN                   TableIndex,
+  IN  VOID                    *Context
+  )
+{
+  UINT32  *TableCrcRecord;
+
+  TableCrcRecord = (UINT32 *)Context;
+
+  if (Table == NULL) {
+    ASSERT (Table != NULL);
+    return;
+  }
 
   //
-  // add HWChange inputs: PCI devices
+  // Calculate CRC value.
   //
-  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;
-      }
-      HWChange[Index++] = PciId;
+  if (Table->Signature == EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
+    //
+    // Zero HardwareSignature field before Calculating FACS CRC
+    //
+    ((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)Table)->HardwareSignature = 0;
+  }
+
+  gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length, &TableCrcRecord[TableIndex]);
+}
+
+/**
+  This function calculates CRC based on each ACPI table.
+  It also calculates CRC and provides as HardwareSignature field in FACS.
+**/
+VOID
+IsAcpiTableChange (
+  VOID
+  )
+{
+  EFI_STATUS                                    Status;
+  BOOLEAN                                       IsRsdt;
+  UINTN                                         AcpiTableCount;
+  UINT32                                        *TableCrcRecord;
+  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;
+
+  IsRsdt         = FALSE;
+  AcpiTableCount = 0;
+  TableCrcRecord = NULL;
+  Rsdp           = NULL;
+  Rsdt           = NULL;
+  Xsdt           = NULL;
+  FacsPtr        = NULL;
+
+  DEBUG ((DEBUG_INFO, "%a() - Start\n", __FUNCTION__));
+
+  Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, (VOID **)&Rsdp);
+  if (EFI_ERROR (Status) || (Rsdp == NULL)) {
+    return;
+  }
+
+  Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->RsdtAddress;
+  Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->XsdtAddress;
+  if (Xsdt == NULL) {
+    if (Rsdt != NULL) {
+      IsRsdt = TRUE;
+    } else {
+      return;
     }
   }
 
+  FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)EfiLocateFirstAcpiTable (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE);
+  if (FacsPtr == NULL) {
+    return;
+  }
+
   //
-  // Locate FACP Table
+  // Count the ACPI tables found by RSDT/XSDT and FADT.
   //
-  Handle = 0;
-  Status = LocateAcpiTableBySignature (
-              EFI_ACPI_6_5_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
+  if (IsRsdt) {
+    EnumerateAllAcpiTables (Rsdt, sizeof (UINT32), GetAcpiTableCount, (VOID *)&AcpiTableCount);
+  } else {
+    EnumerateAllAcpiTables (Xsdt, sizeof (UINT64), GetAcpiTableCount, (VOID *)&AcpiTableCount);
   }
 
   //
-  // add HWChange inputs: others
+  // Allocate memory for founded ACPI tables.
   //
-  HWChange[Index++] = (UINT32)pFADT->XDsdt;
+  TableCrcRecord = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
+  if (TableCrcRecord == NULL) {
+    return;
+  }
 
   //
-  // Calculate CRC value with HWChange data.
+  // Calculate CRC for each ACPI table and set record.
   //
-  Status = gBS->CalculateCrc32(HWChange, HWChangeSize, &CRC);
-  DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
+  if (IsRsdt) {
+    EnumerateAllAcpiTables (Rsdt, sizeof (UINT32), CalculateAcpiTableCrc, (VOID *)TableCrcRecord);
+  } else {
+    EnumerateAllAcpiTables (Xsdt, sizeof (UINT64), CalculateAcpiTableCrc, (VOID *)TableCrcRecord);
+  }
 
   //
-  // Set HardwareSignature value based on CRC value.
+  // Calculate and set HardwareSignature data.
   //
-  FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN) pFADT->FirmwareCtrl;
-  FacsPtr->HardwareSignature = CRC;
-  FreePool (HWChange);
+  Status = gBS->CalculateCrc32 ((UINT8 *)TableCrcRecord, AcpiTableCount, &(FacsPtr->HardwareSignature));
+  DEBUG ((DEBUG_INFO, "HardwareSignature = %x and Status = %r\n", FacsPtr->HardwareSignature, Status));
+
+  FreePool (TableCrcRecord);
+  DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
 }
 
 VOID
@@ -1329,7 +1475,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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v9] MinPlatformPkg: Update HWSignature field in FACS
  2023-05-24  8:39 [PATCH v9] MinPlatformPkg: Update HWSignature field in FACS VincentX Ke
@ 2023-05-30 15:52 ` Ankit Sinha
  2023-05-30 17:12   ` Chiu, Chasel
  2023-05-31  2:01 ` [edk2-devel] " Ni, Ray
  1 sibling, 1 reply; 4+ messages in thread
From: Ankit Sinha @ 2023-05-30 15:52 UTC (permalink / raw)
  To: Ke, VincentX, devel@edk2.groups.io
  Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
	Dong, Eric

Reviewed-by: Ankit Sinha <ankit.sinha@intel.com>

Thank you,
Ankit

> -----Original Message-----
> From: Ke, VincentX <vincentx.ke@intel.com>
> Sent: Wednesday, May 24, 2023 1:39 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>; Sinha, Ankit <ankit.sinha@intel.com>
> Subject: [PATCH v9] MinPlatformPkg: Update HWSignature field 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 field 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>
> Cc: Ankit Sinha<ankit.sinha@intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c   | 282
> +++++++++++++++-----
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf |   1 +
>  2 files changed, 215 insertions(+), 68 deletions(-)
> 
> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index 2f2c96f907..ca1c73f6fe 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1191,98 +1191,244 @@ 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.
> 
> +  Function prototype for GetAcpiTableCount/CalculateAcpiTableCrc.
> 
> +
> 
> +  @param[in] Table        The pointer to ACPI table.
> 
> +  @param[in] TableIndex   The ACPI table index.
> 
> +  @param[in] Context      The pointer to UINTN for GetAcpiTableCount.
> 
> +                          The pointer to UINT32 array for CalculateAcpiTableCrc.
> 
>  **/
> 
> +typedef
> 
>  VOID
> 
> -IsHardwareChange (
> 
> -  VOID
> 
> +(EFIAPI *ACPI_TABLE_CALLBACK)(
> 
> +  IN  EFI_ACPI_COMMON_HEADER  *Table,
> 
> +  IN  UINTN                   TableIndex,
> 
> +  IN  VOID                    *Context
> 
> +  );
> 
> +
> 
> +/**
> 
> +  Enumerate all ACPI tables in RSDT/XSDT.
> 
> +
> 
> +  @param[in] Sdt                ACPI XSDT/RSDT.
> 
> +  @param[in] TablePointerSize   Size of table pointer:
> 
> +                                4(RSDT) or 8(XSDT).
> 
> +  @param[in] CallbackFunction   The pointer to
> GetAcpiTableCount/CalculateAcpiTableCrc.
> 
> +  @param[in] Context            The pointer to UINTN for GetAcpiTableCount.
> 
> +                                The pointer to UINT32 array for CalculateAcpiTableCrc.
> 
> +**/
> 
> +VOID
> 
> +EnumerateAllAcpiTables (
> 
> +  IN  EFI_ACPI_DESCRIPTION_HEADER  *Sdt,
> 
> +  IN  UINTN                        TablePointerSize,
> 
> +  IN  ACPI_TABLE_CALLBACK          CallbackFunction,
> 
> +  IN  VOID                         *Context
> 
>    )
> 
>  {
> 
> -  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_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *FacsPtr;
> 
> -  EFI_ACPI_6_5_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!
> 
> +  UINTN                                      Index;
> 
> +  UINTN                                      TableIndex;
> 
> +  UINTN                                      EntryCount;
> 
> +  UINT64                                     EntryPtr;
> 
> +  UINTN                                      BasePtr;
> 
> +  EFI_ACPI_COMMON_HEADER                     *Table;
> 
> +  EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE  *FadtPtr;
> 
> +
> 
> +  Index      = 0;
> 
> +  TableIndex = 0;
> 
> +  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> TablePointerSize;
> 
> +  EntryPtr   = 0;
> 
> +  BasePtr    = (UINTN)(Sdt + 1);
> 
> +  Table      = NULL;
> 
> +  FadtPtr    = NULL;
> 
> +
> 
> +  if (Sdt == NULL) {
> 
> +    ASSERT (Sdt != NULL);
> 
> +    return;
> 
>    }
> 
> 
> 
> -  //
> 
> -  // Allocate memory for HWChange and add additional entrie for
> 
> -  // pFADT->XDsdt
> 
> -  //
> 
> -  HWChangeSize = HandleCount + 1;
> 
> -  HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
> 
> -  ASSERT(HWChange != NULL);
> 
> +  for (Index = 0; Index < EntryCount; Index++) {
> 
> +    EntryPtr = 0;
> 
> +    Table    = NULL;
> 
> +    CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize),
> TablePointerSize);
> 
> +    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> 
> +    if (Table != NULL) {
> 
> +      CallbackFunction (Table, TableIndex++, Context);
> 
> +    }
> 
> +
> 
> +    if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +      FadtPtr = (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table;
> 
> +      if (FadtPtr->Header.Revision <
> EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) {
> 
> +        //
> 
> +        // Locate FACS/DSDT in FADT
> 
> +        //
> 
> +        CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> >FirmwareCtrl, TableIndex++, Context);
> 
> +        CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> >Dsdt, TableIndex++, Context);
> 
> +      } else {
> 
> +        //
> 
> +        // Locate FACS in FADT
> 
> +        //
> 
> +        if (FadtPtr->XFirmwareCtrl) {
> 
> +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> >XFirmwareCtrl, TableIndex++, Context);
> 
> +        } else {
> 
> +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> >FirmwareCtrl, TableIndex++, Context);
> 
> +        }
> 
> +
> 
> +        //
> 
> +        // Locate DSDT in FADT
> 
> +        //
> 
> +        if (FadtPtr->XDsdt) {
> 
> +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> >XDsdt, TableIndex++, Context);
> 
> +        } else {
> 
> +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> >Dsdt, TableIndex++, Context);
> 
> +        }
> 
> +      }
> 
> +    }
> 
> +  }
> 
> +}
> 
> +
> 
> +/**
> 
> +  Count the number of ACPI tables.
> 
> +
> 
> +  @param[in] Table        The pointer to ACPI table.
> 
> +  @param[in] TableIndex   The ACPI table index.
> 
> +  @param[in] Context      The pointer to UINTN.
> 
> +**/
> 
> +VOID
> 
> +GetAcpiTableCount (
> 
> +  IN  EFI_ACPI_COMMON_HEADER  *Table,
> 
> +  IN  UINTN                   TableIndex,
> 
> +  IN  VOID                    *Context
> 
> +  )
> 
> +{
> 
> +  UINTN  *TableCount;
> 
> +
> 
> +  TableCount = (UINTN *)Context;
> 
> +
> 
> +  if (Table == NULL) {
> 
> +    ASSERT (Table != NULL);
> 
> +    return;
> 
> +  }
> 
> +
> 
> +  (*TableCount)++;
> 
> +}
> 
> 
> 
> -  if (HWChange == NULL) return;
> 
> +/**
> 
> +  Calculate CRC based on each offset in the ACPI table.
> 
> +
> 
> +  @param[in] Table        The pointer to ACPI table.
> 
> +  @param[in] TableIndex   The ACPI table index.
> 
> +  @param[in] Context      The pointer to UINT32 array.
> 
> +**/
> 
> +VOID
> 
> +CalculateAcpiTableCrc (
> 
> +  IN  EFI_ACPI_COMMON_HEADER  *Table,
> 
> +  IN  UINTN                   TableIndex,
> 
> +  IN  VOID                    *Context
> 
> +  )
> 
> +{
> 
> +  UINT32  *TableCrcRecord;
> 
> +
> 
> +  TableCrcRecord = (UINT32 *)Context;
> 
> +
> 
> +  if (Table == NULL) {
> 
> +    ASSERT (Table != NULL);
> 
> +    return;
> 
> +  }
> 
> 
> 
>    //
> 
> -  // add HWChange inputs: PCI devices
> 
> +  // Calculate CRC value.
> 
>    //
> 
> -  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;
> 
> -      }
> 
> -      HWChange[Index++] = PciId;
> 
> +  if (Table->Signature ==
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> 
> +    //
> 
> +    // Zero HardwareSignature field before Calculating FACS CRC
> 
> +    //
> 
> +    ((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)Table)-
> >HardwareSignature = 0;
> 
> +  }
> 
> +
> 
> +  gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length,
> &TableCrcRecord[TableIndex]);
> 
> +}
> 
> +
> 
> +/**
> 
> +  This function calculates CRC based on each ACPI table.
> 
> +  It also calculates CRC and provides as HardwareSignature field in FACS.
> 
> +**/
> 
> +VOID
> 
> +IsAcpiTableChange (
> 
> +  VOID
> 
> +  )
> 
> +{
> 
> +  EFI_STATUS                                    Status;
> 
> +  BOOLEAN                                       IsRsdt;
> 
> +  UINTN                                         AcpiTableCount;
> 
> +  UINT32                                        *TableCrcRecord;
> 
> +  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;
> 
> +
> 
> +  IsRsdt         = FALSE;
> 
> +  AcpiTableCount = 0;
> 
> +  TableCrcRecord = NULL;
> 
> +  Rsdp           = NULL;
> 
> +  Rsdt           = NULL;
> 
> +  Xsdt           = NULL;
> 
> +  FacsPtr        = NULL;
> 
> +
> 
> +  DEBUG ((DEBUG_INFO, "%a() - Start\n", __FUNCTION__));
> 
> +
> 
> +  Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, (VOID
> **)&Rsdp);
> 
> +  if (EFI_ERROR (Status) || (Rsdp == NULL)) {
> 
> +    return;
> 
> +  }
> 
> +
> 
> +  Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->RsdtAddress;
> 
> +  Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->XsdtAddress;
> 
> +  if (Xsdt == NULL) {
> 
> +    if (Rsdt != NULL) {
> 
> +      IsRsdt = TRUE;
> 
> +    } else {
> 
> +      return;
> 
>      }
> 
>    }
> 
> 
> 
> +  FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)EfiLocateFirstAcpiTable
> (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE);
> 
> +  if (FacsPtr == NULL) {
> 
> +    return;
> 
> +  }
> 
> +
> 
>    //
> 
> -  // Locate FACP Table
> 
> +  // Count the ACPI tables found by RSDT/XSDT and FADT.
> 
>    //
> 
> -  Handle = 0;
> 
> -  Status = LocateAcpiTableBySignature (
> 
> -              EFI_ACPI_6_5_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
> 
> +  if (IsRsdt) {
> 
> +    EnumerateAllAcpiTables (Rsdt, sizeof (UINT32), GetAcpiTableCount,
> (VOID *)&AcpiTableCount);
> 
> +  } else {
> 
> +    EnumerateAllAcpiTables (Xsdt, sizeof (UINT64), GetAcpiTableCount,
> (VOID *)&AcpiTableCount);
> 
>    }
> 
> 
> 
>    //
> 
> -  // add HWChange inputs: others
> 
> +  // Allocate memory for founded ACPI tables.
> 
>    //
> 
> -  HWChange[Index++] = (UINT32)pFADT->XDsdt;
> 
> +  TableCrcRecord = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
> 
> +  if (TableCrcRecord == NULL) {
> 
> +    return;
> 
> +  }
> 
> 
> 
>    //
> 
> -  // Calculate CRC value with HWChange data.
> 
> +  // Calculate CRC for each ACPI table and set record.
> 
>    //
> 
> -  Status = gBS->CalculateCrc32(HWChange, HWChangeSize, &CRC);
> 
> -  DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
> 
> +  if (IsRsdt) {
> 
> +    EnumerateAllAcpiTables (Rsdt, sizeof (UINT32), CalculateAcpiTableCrc,
> (VOID *)TableCrcRecord);
> 
> +  } else {
> 
> +    EnumerateAllAcpiTables (Xsdt, sizeof (UINT64), CalculateAcpiTableCrc,
> (VOID *)TableCrcRecord);
> 
> +  }
> 
> 
> 
>    //
> 
> -  // Set HardwareSignature value based on CRC value.
> 
> +  // Calculate and set HardwareSignature data.
> 
>    //
> 
> -  FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN) pFADT->FirmwareCtrl;
> 
> -  FacsPtr->HardwareSignature = CRC;
> 
> -  FreePool (HWChange);
> 
> +  Status = gBS->CalculateCrc32 ((UINT8 *)TableCrcRecord, AcpiTableCount,
> &(FacsPtr->HardwareSignature));
> 
> +  DEBUG ((DEBUG_INFO, "HardwareSignature = %x and Status = %r\n",
> FacsPtr->HardwareSignature, Status));
> 
> +
> 
> +  FreePool (TableCrcRecord);
> 
> +  DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
> 
>  }
> 
> 
> 
>  VOID
> 
> @@ -1329,7 +1475,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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v9] MinPlatformPkg: Update HWSignature field in FACS
  2023-05-30 15:52 ` Ankit Sinha
@ 2023-05-30 17:12   ` Chiu, Chasel
  0 siblings, 0 replies; 4+ messages in thread
From: Chiu, Chasel @ 2023-05-30 17:12 UTC (permalink / raw)
  To: Sinha, Ankit, Ke, VincentX, devel@edk2.groups.io
  Cc: Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming, Dong, Eric,
	Ni, Ray, Chiu, Chasel


Thanks for the reviewing Ankit!
It looks to me current version is good enough for merging, if no other feedbacks I will help to merge it tomorrow.

Thanks,
Chasel


> -----Original Message-----
> From: Sinha, Ankit <ankit.sinha@intel.com>
> Sent: Tuesday, May 30, 2023 8:53 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: [PATCH v9] MinPlatformPkg: Update HWSignature field in FACS
> 
> Reviewed-by: Ankit Sinha <ankit.sinha@intel.com>
> 
> Thank you,
> Ankit
> 
> > -----Original Message-----
> > From: Ke, VincentX <vincentx.ke@intel.com>
> > Sent: Wednesday, May 24, 2023 1:39 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>; Sinha, Ankit <ankit.sinha@intel.com>
> > Subject: [PATCH v9] MinPlatformPkg: Update HWSignature field 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 field 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>
> > Cc: Ankit Sinha<ankit.sinha@intel.com>
> > Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> > ---
> >  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c   | 282
> > +++++++++++++++-----
> >  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf |   1 +
> >  2 files changed, 215 insertions(+), 68 deletions(-)
> >
> > diff --git
> > a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > index 2f2c96f907..ca1c73f6fe 100644
> > --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> > @@ -1191,98 +1191,244 @@ 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.
> >
> > +  Function prototype for GetAcpiTableCount/CalculateAcpiTableCrc.
> >
> > +
> >
> > +  @param[in] Table        The pointer to ACPI table.
> >
> > +  @param[in] TableIndex   The ACPI table index.
> >
> > +  @param[in] Context      The pointer to UINTN for GetAcpiTableCount.
> >
> > +                          The pointer to UINT32 array for CalculateAcpiTableCrc.
> >
> >  **/
> >
> > +typedef
> >
> >  VOID
> >
> > -IsHardwareChange (
> >
> > -  VOID
> >
> > +(EFIAPI *ACPI_TABLE_CALLBACK)(
> >
> > +  IN  EFI_ACPI_COMMON_HEADER  *Table,
> >
> > +  IN  UINTN                   TableIndex,
> >
> > +  IN  VOID                    *Context
> >
> > +  );
> >
> > +
> >
> > +/**
> >
> > +  Enumerate all ACPI tables in RSDT/XSDT.
> >
> > +
> >
> > +  @param[in] Sdt                ACPI XSDT/RSDT.
> >
> > +  @param[in] TablePointerSize   Size of table pointer:
> >
> > +                                4(RSDT) or 8(XSDT).
> >
> > +  @param[in] CallbackFunction   The pointer to
> > GetAcpiTableCount/CalculateAcpiTableCrc.
> >
> > +  @param[in] Context            The pointer to UINTN for GetAcpiTableCount.
> >
> > +                                The pointer to UINT32 array for CalculateAcpiTableCrc.
> >
> > +**/
> >
> > +VOID
> >
> > +EnumerateAllAcpiTables (
> >
> > +  IN  EFI_ACPI_DESCRIPTION_HEADER  *Sdt,
> >
> > +  IN  UINTN                        TablePointerSize,
> >
> > +  IN  ACPI_TABLE_CALLBACK          CallbackFunction,
> >
> > +  IN  VOID                         *Context
> >
> >    )
> >
> >  {
> >
> > -  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_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *FacsPtr;
> >
> > -  EFI_ACPI_6_5_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!
> >
> > +  UINTN                                      Index;
> >
> > +  UINTN                                      TableIndex;
> >
> > +  UINTN                                      EntryCount;
> >
> > +  UINT64                                     EntryPtr;
> >
> > +  UINTN                                      BasePtr;
> >
> > +  EFI_ACPI_COMMON_HEADER                     *Table;
> >
> > +  EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE  *FadtPtr;
> >
> > +
> >
> > +  Index      = 0;
> >
> > +  TableIndex = 0;
> >
> > +  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> > TablePointerSize;
> >
> > +  EntryPtr   = 0;
> >
> > +  BasePtr    = (UINTN)(Sdt + 1);
> >
> > +  Table      = NULL;
> >
> > +  FadtPtr    = NULL;
> >
> > +
> >
> > +  if (Sdt == NULL) {
> >
> > +    ASSERT (Sdt != NULL);
> >
> > +    return;
> >
> >    }
> >
> >
> >
> > -  //
> >
> > -  // Allocate memory for HWChange and add additional entrie for
> >
> > -  // pFADT->XDsdt
> >
> > -  //
> >
> > -  HWChangeSize = HandleCount + 1;
> >
> > -  HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
> >
> > -  ASSERT(HWChange != NULL);
> >
> > +  for (Index = 0; Index < EntryCount; Index++) {
> >
> > +    EntryPtr = 0;
> >
> > +    Table    = NULL;
> >
> > +    CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize),
> > TablePointerSize);
> >
> > +    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> >
> > +    if (Table != NULL) {
> >
> > +      CallbackFunction (Table, TableIndex++, Context);
> >
> > +    }
> >
> > +
> >
> > +    if (Table->Signature ==
> > EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> >
> > +      FadtPtr = (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table;
> >
> > +      if (FadtPtr->Header.Revision <
> > EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) {
> >
> > +        //
> >
> > +        // Locate FACS/DSDT in FADT
> >
> > +        //
> >
> > +        CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> > >FirmwareCtrl, TableIndex++, Context);
> >
> > +        CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> > >Dsdt, TableIndex++, Context);
> >
> > +      } else {
> >
> > +        //
> >
> > +        // Locate FACS in FADT
> >
> > +        //
> >
> > +        if (FadtPtr->XFirmwareCtrl) {
> >
> > +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> > >XFirmwareCtrl, TableIndex++, Context);
> >
> > +        } else {
> >
> > +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> > >FirmwareCtrl, TableIndex++, Context);
> >
> > +        }
> >
> > +
> >
> > +        //
> >
> > +        // Locate DSDT in FADT
> >
> > +        //
> >
> > +        if (FadtPtr->XDsdt) {
> >
> > +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> > >XDsdt, TableIndex++, Context);
> >
> > +        } else {
> >
> > +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr-
> > >Dsdt, TableIndex++, Context);
> >
> > +        }
> >
> > +      }
> >
> > +    }
> >
> > +  }
> >
> > +}
> >
> > +
> >
> > +/**
> >
> > +  Count the number of ACPI tables.
> >
> > +
> >
> > +  @param[in] Table        The pointer to ACPI table.
> >
> > +  @param[in] TableIndex   The ACPI table index.
> >
> > +  @param[in] Context      The pointer to UINTN.
> >
> > +**/
> >
> > +VOID
> >
> > +GetAcpiTableCount (
> >
> > +  IN  EFI_ACPI_COMMON_HEADER  *Table,
> >
> > +  IN  UINTN                   TableIndex,
> >
> > +  IN  VOID                    *Context
> >
> > +  )
> >
> > +{
> >
> > +  UINTN  *TableCount;
> >
> > +
> >
> > +  TableCount = (UINTN *)Context;
> >
> > +
> >
> > +  if (Table == NULL) {
> >
> > +    ASSERT (Table != NULL);
> >
> > +    return;
> >
> > +  }
> >
> > +
> >
> > +  (*TableCount)++;
> >
> > +}
> >
> >
> >
> > -  if (HWChange == NULL) return;
> >
> > +/**
> >
> > +  Calculate CRC based on each offset in the ACPI table.
> >
> > +
> >
> > +  @param[in] Table        The pointer to ACPI table.
> >
> > +  @param[in] TableIndex   The ACPI table index.
> >
> > +  @param[in] Context      The pointer to UINT32 array.
> >
> > +**/
> >
> > +VOID
> >
> > +CalculateAcpiTableCrc (
> >
> > +  IN  EFI_ACPI_COMMON_HEADER  *Table,
> >
> > +  IN  UINTN                   TableIndex,
> >
> > +  IN  VOID                    *Context
> >
> > +  )
> >
> > +{
> >
> > +  UINT32  *TableCrcRecord;
> >
> > +
> >
> > +  TableCrcRecord = (UINT32 *)Context;
> >
> > +
> >
> > +  if (Table == NULL) {
> >
> > +    ASSERT (Table != NULL);
> >
> > +    return;
> >
> > +  }
> >
> >
> >
> >    //
> >
> > -  // add HWChange inputs: PCI devices
> >
> > +  // Calculate CRC value.
> >
> >    //
> >
> > -  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;
> >
> > -      }
> >
> > -      HWChange[Index++] = PciId;
> >
> > +  if (Table->Signature ==
> > EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> >
> > +    //
> >
> > +    // Zero HardwareSignature field before Calculating FACS CRC
> >
> > +    //
> >
> > +    ((EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)Table)-
> > >HardwareSignature = 0;
> >
> > +  }
> >
> > +
> >
> > +  gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length,
> > &TableCrcRecord[TableIndex]);
> >
> > +}
> >
> > +
> >
> > +/**
> >
> > +  This function calculates CRC based on each ACPI table.
> >
> > +  It also calculates CRC and provides as HardwareSignature field in FACS.
> >
> > +**/
> >
> > +VOID
> >
> > +IsAcpiTableChange (
> >
> > +  VOID
> >
> > +  )
> >
> > +{
> >
> > +  EFI_STATUS                                    Status;
> >
> > +  BOOLEAN                                       IsRsdt;
> >
> > +  UINTN                                         AcpiTableCount;
> >
> > +  UINT32                                        *TableCrcRecord;
> >
> > +  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;
> >
> > +
> >
> > +  IsRsdt         = FALSE;
> >
> > +  AcpiTableCount = 0;
> >
> > +  TableCrcRecord = NULL;
> >
> > +  Rsdp           = NULL;
> >
> > +  Rsdt           = NULL;
> >
> > +  Xsdt           = NULL;
> >
> > +  FacsPtr        = NULL;
> >
> > +
> >
> > +  DEBUG ((DEBUG_INFO, "%a() - Start\n", __FUNCTION__));
> >
> > +
> >
> > +  Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, (VOID
> > **)&Rsdp);
> >
> > +  if (EFI_ERROR (Status) || (Rsdp == NULL)) {
> >
> > +    return;
> >
> > +  }
> >
> > +
> >
> > +  Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->RsdtAddress;
> >
> > +  Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)Rsdp->XsdtAddress;
> >
> > +  if (Xsdt == NULL) {
> >
> > +    if (Rsdt != NULL) {
> >
> > +      IsRsdt = TRUE;
> >
> > +    } else {
> >
> > +      return;
> >
> >      }
> >
> >    }
> >
> >
> >
> > +  FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> > *)EfiLocateFirstAcpiTable
> > (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE);
> >
> > +  if (FacsPtr == NULL) {
> >
> > +    return;
> >
> > +  }
> >
> > +
> >
> >    //
> >
> > -  // Locate FACP Table
> >
> > +  // Count the ACPI tables found by RSDT/XSDT and FADT.
> >
> >    //
> >
> > -  Handle = 0;
> >
> > -  Status = LocateAcpiTableBySignature (
> >
> > -              EFI_ACPI_6_5_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
> >
> > +  if (IsRsdt) {
> >
> > +    EnumerateAllAcpiTables (Rsdt, sizeof (UINT32), GetAcpiTableCount,
> > (VOID *)&AcpiTableCount);
> >
> > +  } else {
> >
> > +    EnumerateAllAcpiTables (Xsdt, sizeof (UINT64), GetAcpiTableCount,
> > (VOID *)&AcpiTableCount);
> >
> >    }
> >
> >
> >
> >    //
> >
> > -  // add HWChange inputs: others
> >
> > +  // Allocate memory for founded ACPI tables.
> >
> >    //
> >
> > -  HWChange[Index++] = (UINT32)pFADT->XDsdt;
> >
> > +  TableCrcRecord = AllocateZeroPool (sizeof (UINT32) *
> > + AcpiTableCount);
> >
> > +  if (TableCrcRecord == NULL) {
> >
> > +    return;
> >
> > +  }
> >
> >
> >
> >    //
> >
> > -  // Calculate CRC value with HWChange data.
> >
> > +  // Calculate CRC for each ACPI table and set record.
> >
> >    //
> >
> > -  Status = gBS->CalculateCrc32(HWChange, HWChangeSize, &CRC);
> >
> > -  DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
> >
> > +  if (IsRsdt) {
> >
> > +    EnumerateAllAcpiTables (Rsdt, sizeof (UINT32),
> > + CalculateAcpiTableCrc,
> > (VOID *)TableCrcRecord);
> >
> > +  } else {
> >
> > +    EnumerateAllAcpiTables (Xsdt, sizeof (UINT64),
> > + CalculateAcpiTableCrc,
> > (VOID *)TableCrcRecord);
> >
> > +  }
> >
> >
> >
> >    //
> >
> > -  // Set HardwareSignature value based on CRC value.
> >
> > +  // Calculate and set HardwareSignature data.
> >
> >    //
> >
> > -  FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> > *)(UINTN) pFADT->FirmwareCtrl;
> >
> > -  FacsPtr->HardwareSignature = CRC;
> >
> > -  FreePool (HWChange);
> >
> > +  Status = gBS->CalculateCrc32 ((UINT8 *)TableCrcRecord,
> > + AcpiTableCount,
> > &(FacsPtr->HardwareSignature));
> >
> > +  DEBUG ((DEBUG_INFO, "HardwareSignature = %x and Status = %r\n",
> > FacsPtr->HardwareSignature, Status));
> >
> > +
> >
> > +  FreePool (TableCrcRecord);
> >
> > +  DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
> >
> >  }
> >
> >
> >
> >  VOID
> >
> > @@ -1329,7 +1475,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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [PATCH v9] MinPlatformPkg: Update HWSignature field in FACS
  2023-05-24  8:39 [PATCH v9] MinPlatformPkg: Update HWSignature field in FACS VincentX Ke
  2023-05-30 15:52 ` Ankit Sinha
@ 2023-05-31  2:01 ` Ni, Ray
  1 sibling, 0 replies; 4+ messages in thread
From: Ni, Ray @ 2023-05-31  2:01 UTC (permalink / raw)
  To: devel@edk2.groups.io, Ke, VincentX
  Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
	Dong, Eric, Sinha, Ankit

4 minor comments.

> +        if (FadtPtr->XFirmwareCtrl) {
1. you should use "if (FadtPtr->XFirmwareCtrl != 0)" to align with edk2 coding style.

> 
> +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr->XFirmwareCtrl, TableIndex++, Context);
> 
> +        } else {
> 
> +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr->FirmwareCtrl, TableIndex++, Context);
> 
> +        }
> 
> +
> 
> +        //
> 
> +        // Locate DSDT in FADT
> 
> +        //
> 
> +        if (FadtPtr->XDsdt) {
2. Same comment as #1.

> 
> +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr->XDsdt, TableIndex++, Context);
> 
> +        } else {
> 
> +          CallbackFunction ((EFI_ACPI_COMMON_HEADER *)(UINTN)FadtPtr->Dsdt, TableIndex++, Context);
> 
> +        }
> 
> +      }
> 
> +VOID
> 
> +GetAcpiTableCount (
3. "EFIAPI" is missed here but the CALLBACK prototype contains "EFIAPI". You need to match them two.

> 
> +VOID
> 
> +CalculateAcpiTableCrc (
4. Similar comments as #3.


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-05-31  2:02 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-24  8:39 [PATCH v9] MinPlatformPkg: Update HWSignature field in FACS VincentX Ke
2023-05-30 15:52 ` Ankit Sinha
2023-05-30 17:12   ` Chiu, Chasel
2023-05-31  2:01 ` [edk2-devel] " Ni, Ray

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox