public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
@ 2023-05-11  9:59 VincentX Ke
  2023-05-12  9:32 ` [edk2-devel] " Ni, Ray
  2023-05-15 21:16 ` Ankit Sinha
  0 siblings, 2 replies; 4+ messages in thread
From: VincentX Ke @ 2023-05-11  9:59 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 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>
Cc: Ankit Sinha<ankit.sinha@intel.com>
Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
---
 Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c   | 287 +++++++++++++++-----
 Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf |   1 +
 2 files changed, 223 insertions(+), 65 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index e967031a3b..3dca6f99f7 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -1191,98 +1191,255 @@ 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 ACPI table required to calculate CRC.
+
+  @retval CRC       A pointer to allocate UINT32 that
+                    contains the CRC32 data.
+**/
+UINT32
+AcpiTableCrcCalculator (
+  IN  EFI_ACPI_COMMON_HEADER  *Table
+  )
+{
+  EFI_STATUS  Status;
+  UINT32      CRC;
+
+  Status = EFI_SUCCESS;
+  CRC    = 0;
+
+  //
+  // Calculate CRC value.
+  //
+  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;
+  }
+
+  Status = gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length, &CRC);
+  return CRC;
+}
+
+/**
+  This function count ACPI tables in RSDT/XSDT and return the result.
+
+  @param[in] Sdt                ACPI XSDT/RSDT.
+  @param[in] TablePointerSize   Size of table pointer:
+                                4(RSDT) or 8(XSDT).
+
+  @retval TableCount            The total number of ACPI tables in
+                                RSDT or XSDT.
+**/
+UINTN
+CountTableInSDT (
+  IN  EFI_ACPI_DESCRIPTION_HEADER  *Sdt,
+  IN  UINTN                        TablePointerSize
+  )
+{
+  UINTN                   Index;
+  UINTN                   TableCount;
+  UINTN                   EntryCount;
+  UINT64                  EntryPtr;
+  UINTN                   BasePtr;
+  EFI_ACPI_COMMON_HEADER  *Table;
+
+  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / TablePointerSize;
+  BasePtr    = (UINTN)(Sdt + 1);
+
+  for (Index = 0, TableCount = 0; Index < EntryCount; Index++) {
+    EntryPtr = 0;
+    Table    = NULL;
+    CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize), TablePointerSize);
+    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
+    if (Table) {
+      TableCount++;
+    }
+
+    if (Table->Signature == EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
+      CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
+      if (Fadt.FirmwareCtrl || Fadt.XFirmwareCtrl) {
+        TableCount++;
+      }
+
+      if (Fadt.Dsdt || Fadt.XDsdt) {
+        TableCount++;
+      }
+    }
+  }
+
+  return TableCount;
+}
+
+/**
+  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;
+  UINTN                                         EntryCount;
+  UINTN                                         BasePtr;
+  UINT64                                        EntryPtr;
+  UINT32                                        *TableCrcRecord;
+  UINT32                                        HWSignature;
+  EFI_ACPI_COMMON_HEADER                        *Table;
+  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;
+
+  Index          = 0;
+  AcpiTableCount = 0;
+  EntryCount     = 0;
+  BasePtr        = 0;
+  EntryPtr       = 0;
+  HWSignature    = 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->Signature != EFI_ACPI_6_5_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
+    if (Rsdt->Signature != EFI_ACPI_6_5_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
+      return;
+    }
   }
 
   //
-  // Allocate memory for HWChange and add additional entrie for
-  // pFADT->XDsdt
+  // ACPI table count starts with 0.
+  // Then add ACPI tables found by RSDT/XSDT and FADT.
   //
-  HWChangeSize = HandleCount + 1;
-  HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
-  ASSERT(HWChange != NULL);
+  AcpiTableCount = CountTableInSDT (Xsdt, sizeof (UINT64));
+  if (AcpiTableCount == 0) {
+    AcpiTableCount = CountTableInSDT (Rsdt, sizeof (UINT32));
+  }
 
-  if (HWChange == NULL) return;
+  //
+  // Allocate memory for founded ACPI tables.
+  //
+  TableCrcRecord = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
+  if (TableCrcRecord == NULL) {
+    return;
+  }
 
   //
-  // add HWChange inputs: PCI devices
+  // Search XSDT
   //
-  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;
+  AcpiTableCount = 0;
+  EntryCount     = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / sizeof (UINT64);
+  BasePtr        = (UINTN)(Xsdt + 1);
+  for (Index = 0; Index < EntryCount; Index++) {
+    EntryPtr = 0;
+    Table    = NULL;
+    CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof (UINT64)), sizeof (UINT64));
+    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
+    if (Table) {
+      TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
+    }
+
+    if (Table->Signature == EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
+      CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
+      //
+      // Locate FACS in FADT
+      //
+      CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XFirmwareCtrl, sizeof (UINT64));
+      if (EntryPtr != 0) {
+        FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)->XFirmwareCtrl;
+
+        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
+      } else {
+        FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)->FirmwareCtrl;
+
+        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
+      }
+
+      //
+      // Locate DSDT in FADT
+      //
+      CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XDsdt, sizeof (UINT64));
+      if (EntryPtr != 0) {
+        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
+      } else {
+        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
       }
-      HWChange[Index++] = PciId;
     }
   }
 
-  //
-  // Locate FACP Table
-  //
-  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
+  if (FacsPtr->Signature != EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
+    //
+    // Search RSDT
+    //
+    AcpiTableCount = 0;
+    EntryCount     = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / sizeof (UINT32);
+    BasePtr        = (UINTN)(Rsdt + 1);
+    for (Index = 0; Index < EntryCount; Index++) {
+      EntryPtr = 0;
+      Table    = NULL;
+      CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof (UINT32)), sizeof (UINT32));
+      Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
+      if (Table) {
+        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
+      }
+
+      if (Table->Signature == EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
+        CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
+        //
+        // Locate FACS in FADT
+        //
+        if (Fadt.FirmwareCtrl) {
+          FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)->FirmwareCtrl;
+
+          TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
+        }
+
+        //
+        // Locate DSDT in FADT
+        //
+        if (Fadt.Dsdt) {
+          TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
+        }
+      }
+    }
   }
 
   //
-  // add HWChange inputs: others
+  // FACS not found
   //
-  HWChange[Index++] = (UINT32)pFADT->XDsdt;
+  if (FacsPtr->Signature != EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
+    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 (TableCrcRecord, 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->HardwareSignature = HWSignature;
+  FreePool (TableCrcRecord);
+  DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
 }
 
 VOID
@@ -1329,7 +1486,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: [edk2-devel] [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
  2023-05-11  9:59 [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS VincentX Ke
@ 2023-05-12  9:32 ` Ni, Ray
  2023-05-16 10:06   ` VincentX Ke
  2023-05-15 21:16 ` Ankit Sinha
  1 sibling, 1 reply; 4+ messages in thread
From: Ni, Ray @ 2023-05-12  9:32 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



> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of VincentX Ke
> Sent: Thursday, May 11, 2023 6:00 PM
> 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: [edk2-devel] [PATCH v7] 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>
> Cc: Ankit Sinha<ankit.sinha@intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c   | 287
> +++++++++++++++-----
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf |   1 +
>  2 files changed, 223 insertions(+), 65 deletions(-)
> 
> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index e967031a3b..3dca6f99f7 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1191,98 +1191,255 @@ 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 ACPI table required to calculate CRC.
> 
> +
> 
> +  @retval CRC       A pointer to allocate UINT32 that
> 
> +                    contains the CRC32 data.
> 
> +**/
> 
> +UINT32
> 
> +AcpiTableCrcCalculator (
> 
> +  IN  EFI_ACPI_COMMON_HEADER  *Table
> 
> +  )
> 
> +{
> 
> +  EFI_STATUS  Status;
> 
> +  UINT32      CRC;
> 
> +
> 
> +  Status = EFI_SUCCESS;
> 
> +  CRC    = 0;
> 
> +
> 
> +  //
> 
> +  // Calculate CRC value.
> 
> +  //
> 
> +  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;
> 
> +  }
> 
> +
> 
> +  Status = gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length, &CRC);
> 
> +  return CRC;
> 
> +}
> 
> +
> 
> +/**
> 
> +  This function count ACPI tables in RSDT/XSDT and return the result.
> 
> +
> 
> +  @param[in] Sdt                ACPI XSDT/RSDT.
> 
> +  @param[in] TablePointerSize   Size of table pointer:
> 
> +                                4(RSDT) or 8(XSDT).
> 
> +
> 
> +  @retval TableCount            The total number of ACPI tables in
> 
> +                                RSDT or XSDT.
> 
> +**/
> 
> +UINTN
> 
> +CountTableInSDT (
> 
> +  IN  EFI_ACPI_DESCRIPTION_HEADER  *Sdt,
> 
> +  IN  UINTN                        TablePointerSize
> 
> +  )
> 
> +{
> 
> +  UINTN                   Index;
> 
> +  UINTN                   TableCount;
> 
> +  UINTN                   EntryCount;
> 
> +  UINT64                  EntryPtr;
> 
> +  UINTN                   BasePtr;
> 
> +  EFI_ACPI_COMMON_HEADER  *Table;
> 
> +
> 
> +  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> TablePointerSize;
> 
> +  BasePtr    = (UINTN)(Sdt + 1);
> 
> +
> 
> +  for (Index = 0, TableCount = 0; Index < EntryCount; Index++) {
> 
> +    EntryPtr = 0;
> 
> +    Table    = NULL;
> 
> +    CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize),
> TablePointerSize);
> 
> +    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> 
> +    if (Table) {

1. if (Table != NULL)

> 
> +      TableCount++;
> 
> +    }
> 
> +
> 
> +    if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +      CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));

2. Different versions of FADT have different sizes. This CopyMem is dangerous.
And why do you CopyMem()?

> 
> +      if (Fadt.FirmwareCtrl || Fadt.XFirmwareCtrl) {

3. if (Fadt.FirmwareCtrl != 0 || ...)

> 
> +        TableCount++;
> 
> +      }
> 
> +
> 
> +      if (Fadt.Dsdt || Fadt.XDsdt) {

4. if (Fadt. Dsdt != 0 || ...)

> 
> +        TableCount++;
> 
> +      }
> 
> +    }
> 
> +  }
> 
> +
> 
> +  return TableCount;
> 
> +}
> 
> +
> 
> +/**
> 
> +  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;
> 
> +  UINTN                                         EntryCount;
> 
> +  UINTN                                         BasePtr;
> 
> +  UINT64                                        EntryPtr;
> 
> +  UINT32                                        *TableCrcRecord;
> 
> +  UINT32                                        HWSignature;
> 
> +  EFI_ACPI_COMMON_HEADER                        *Table;
> 
> +  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;
> 
> +
> 
> +  Index          = 0;
> 
> +  AcpiTableCount = 0;
> 
> +  EntryCount     = 0;
> 
> +  BasePtr        = 0;
> 
> +  EntryPtr       = 0;
> 
> +  HWSignature    = 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->Signature !=
> EFI_ACPI_6_5_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +    if (Rsdt->Signature !=
> EFI_ACPI_6_5_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +      return;
> 
> +    }
> 
>    }

5. You doesn't need to check the Signature. You can check if Xsdt is not 0, use XSDT, otherwise use RSDT.

> 
> 
> 
>    //
> 
> -  // Allocate memory for HWChange and add additional entrie for
> 
> -  // pFADT->XDsdt
> 
> +  // ACPI table count starts with 0.
> 
> +  // Then add ACPI tables found by RSDT/XSDT and FADT.
> 
>    //
> 
> -  HWChangeSize = HandleCount + 1;
> 
> -  HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
> 
> -  ASSERT(HWChange != NULL);
> 
> +  AcpiTableCount = CountTableInSDT (Xsdt, sizeof (UINT64));
> 
> +  if (AcpiTableCount == 0) {
> 
> +    AcpiTableCount = CountTableInSDT (Rsdt, sizeof (UINT32));
> 
> +  }

6. No need to choose between XSDT and RSDT again. Use #5 comment to choose between the two.

> 
> 
> 
> -  if (HWChange == NULL) return;

7. Why not return earlier but here?

8. I will stop reviewing. Can you please review within your team first then send out?


> 
> +  //
> 
> +  // Allocate memory for founded ACPI tables.
> 
> +  //
> 
> +  TableCrcRecord = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
> 
> +  if (TableCrcRecord == NULL) {
> 
> +    return;
> 
> +  }
> 
> 
> 
>    //
> 
> -  // add HWChange inputs: PCI devices
> 
> +  // Search XSDT
> 
>    //
> 
> -  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;
> 
> +  AcpiTableCount = 0;
> 
> +  EntryCount     = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof (UINT64);
> 
> +  BasePtr        = (UINTN)(Xsdt + 1);
> 
> +  for (Index = 0; Index < EntryCount; Index++) {
> 
> +    EntryPtr = 0;
> 
> +    Table    = NULL;
> 
> +    CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof (UINT64)),
> sizeof (UINT64));
> 
> +    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> 
> +    if (Table) {
> 
> +      TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
> 
> +    }
> 
> +
> 
> +    if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +      CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
> 
> +      //
> 
> +      // Locate FACS in FADT
> 
> +      //
> 
> +      CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XFirmwareCtrl, sizeof
> (UINT64));
> 
> +      if (EntryPtr != 0) {
> 
> +        FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >XFirmwareCtrl;
> 
> +
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
> 
> +      } else {
> 
> +        FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
> 
> +
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
> 
> +      }
> 
> +
> 
> +      //
> 
> +      // Locate DSDT in FADT
> 
> +      //
> 
> +      CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XDsdt, sizeof (UINT64));
> 
> +      if (EntryPtr != 0) {
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
> 
> +      } else {
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
> 
>        }
> 
> -      HWChange[Index++] = PciId;
> 
>      }
> 
>    }
> 
> 
> 
> -  //
> 
> -  // Locate FACP Table
> 
> -  //
> 
> -  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
> 
> +  if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> 
> +    //
> 
> +    // Search RSDT
> 
> +    //
> 
> +    AcpiTableCount = 0;
> 
> +    EntryCount     = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof (UINT32);
> 
> +    BasePtr        = (UINTN)(Rsdt + 1);
> 
> +    for (Index = 0; Index < EntryCount; Index++) {
> 
> +      EntryPtr = 0;
> 
> +      Table    = NULL;
> 
> +      CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof (UINT32)),
> sizeof (UINT32));
> 
> +      Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> 
> +      if (Table) {
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
> 
> +      }
> 
> +
> 
> +      if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +        CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
> 
> +        //
> 
> +        // Locate FACS in FADT
> 
> +        //
> 
> +        if (Fadt.FirmwareCtrl) {
> 
> +          FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
> 
> +
> 
> +          TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
> 
> +        }
> 
> +
> 
> +        //
> 
> +        // Locate DSDT in FADT
> 
> +        //
> 
> +        if (Fadt.Dsdt) {
> 
> +          TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
> 
> +        }
> 
> +      }
> 
> +    }
> 
>    }
> 
> 
> 
>    //
> 
> -  // add HWChange inputs: others
> 
> +  // FACS not found
> 
>    //
> 
> -  HWChange[Index++] = (UINT32)pFADT->XDsdt;
> 
> +  if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> 
> +    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 (TableCrcRecord, 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->HardwareSignature = HWSignature;
> 
> +  FreePool (TableCrcRecord);
> 
> +  DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
> 
>  }
> 
> 
> 
>  VOID
> 
> @@ -1329,7 +1486,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 (#104685): https://edk2.groups.io/g/devel/message/104685
> Mute This Topic: https://groups.io/mt/98824282/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
> 


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

* Re: [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
  2023-05-11  9:59 [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS VincentX Ke
  2023-05-12  9:32 ` [edk2-devel] " Ni, Ray
@ 2023-05-15 21:16 ` Ankit Sinha
  1 sibling, 0 replies; 4+ messages in thread
From: Ankit Sinha @ 2023-05-15 21:16 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>

> -----Original Message-----
> From: Ke, VincentX <vincentx.ke@intel.com>
> Sent: Thursday, May 11, 2023 3:00 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 v7] 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>
> Cc: Ankit Sinha<ankit.sinha@intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c   | 287
> +++++++++++++++-----
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf |   1 +
>  2 files changed, 223 insertions(+), 65 deletions(-)
> 
> diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index e967031a3b..3dca6f99f7 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1191,98 +1191,255 @@ 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 ACPI table required to calculate CRC.
> 
> +
> 
> +  @retval CRC       A pointer to allocate UINT32 that
> 
> +                    contains the CRC32 data.
> 
> +**/
> 
> +UINT32
> 
> +AcpiTableCrcCalculator (
> 
> +  IN  EFI_ACPI_COMMON_HEADER  *Table
> 
> +  )
> 
> +{
> 
> +  EFI_STATUS  Status;
> 
> +  UINT32      CRC;
> 
> +
> 
> +  Status = EFI_SUCCESS;
> 
> +  CRC    = 0;
> 
> +
> 
> +  //
> 
> +  // Calculate CRC value.
> 
> +  //
> 
> +  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;
> 
> +  }
> 
> +
> 
> +  Status = gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length,
> &CRC);
> 
> +  return CRC;
> 
> +}
> 
> +
> 
> +/**
> 
> +  This function count ACPI tables in RSDT/XSDT and return the result.
> 
> +
> 
> +  @param[in] Sdt                ACPI XSDT/RSDT.
> 
> +  @param[in] TablePointerSize   Size of table pointer:
> 
> +                                4(RSDT) or 8(XSDT).
> 
> +
> 
> +  @retval TableCount            The total number of ACPI tables in
> 
> +                                RSDT or XSDT.
> 
> +**/
> 
> +UINTN
> 
> +CountTableInSDT (
> 
> +  IN  EFI_ACPI_DESCRIPTION_HEADER  *Sdt,
> 
> +  IN  UINTN                        TablePointerSize
> 
> +  )
> 
> +{
> 
> +  UINTN                   Index;
> 
> +  UINTN                   TableCount;
> 
> +  UINTN                   EntryCount;
> 
> +  UINT64                  EntryPtr;
> 
> +  UINTN                   BasePtr;
> 
> +  EFI_ACPI_COMMON_HEADER  *Table;
> 
> +
> 
> +  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> TablePointerSize;
> 
> +  BasePtr    = (UINTN)(Sdt + 1);
> 
> +
> 
> +  for (Index = 0, TableCount = 0; Index < EntryCount; Index++) {
> 
> +    EntryPtr = 0;
> 
> +    Table    = NULL;
> 
> +    CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize),
> TablePointerSize);
> 
> +    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> 
> +    if (Table) {
> 
> +      TableCount++;
> 
> +    }
> 
> +
> 
> +    if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +      CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
> 
> +      if (Fadt.FirmwareCtrl || Fadt.XFirmwareCtrl) {
> 
> +        TableCount++;
> 
> +      }
> 
> +
> 
> +      if (Fadt.Dsdt || Fadt.XDsdt) {
> 
> +        TableCount++;
> 
> +      }
> 
> +    }
> 
> +  }
> 
> +
> 
> +  return TableCount;
> 
> +}
> 
> +
> 
> +/**
> 
> +  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;
> 
> +  UINTN                                         EntryCount;
> 
> +  UINTN                                         BasePtr;
> 
> +  UINT64                                        EntryPtr;
> 
> +  UINT32                                        *TableCrcRecord;
> 
> +  UINT32                                        HWSignature;
> 
> +  EFI_ACPI_COMMON_HEADER                        *Table;
> 
> +  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;
> 
> +
> 
> +  Index          = 0;
> 
> +  AcpiTableCount = 0;
> 
> +  EntryCount     = 0;
> 
> +  BasePtr        = 0;
> 
> +  EntryPtr       = 0;
> 
> +  HWSignature    = 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->Signature !=
> EFI_ACPI_6_5_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +    if (Rsdt->Signature !=
> EFI_ACPI_6_5_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +      return;
> 
> +    }
> 
>    }
> 
> 
> 
>    //
> 
> -  // Allocate memory for HWChange and add additional entrie for
> 
> -  // pFADT->XDsdt
> 
> +  // ACPI table count starts with 0.
> 
> +  // Then add ACPI tables found by RSDT/XSDT and FADT.
> 
>    //
> 
> -  HWChangeSize = HandleCount + 1;
> 
> -  HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
> 
> -  ASSERT(HWChange != NULL);
> 
> +  AcpiTableCount = CountTableInSDT (Xsdt, sizeof (UINT64));
> 
> +  if (AcpiTableCount == 0) {
> 
> +    AcpiTableCount = CountTableInSDT (Rsdt, sizeof (UINT32));
> 
> +  }
> 
> 
> 
> -  if (HWChange == NULL) return;
> 
> +  //
> 
> +  // Allocate memory for founded ACPI tables.
> 
> +  //
> 
> +  TableCrcRecord = AllocateZeroPool (sizeof (UINT32) * AcpiTableCount);
> 
> +  if (TableCrcRecord == NULL) {
> 
> +    return;
> 
> +  }
> 
> 
> 
>    //
> 
> -  // add HWChange inputs: PCI devices
> 
> +  // Search XSDT
> 
>    //
> 
> -  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;
> 
> +  AcpiTableCount = 0;
> 
> +  EntryCount     = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER))
> / sizeof (UINT64);
> 
> +  BasePtr        = (UINTN)(Xsdt + 1);
> 
> +  for (Index = 0; Index < EntryCount; Index++) {
> 
> +    EntryPtr = 0;
> 
> +    Table    = NULL;
> 
> +    CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof
> (UINT64)), sizeof (UINT64));
> 
> +    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> 
> +    if (Table) {
> 
> +      TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
> 
> +    }
> 
> +
> 
> +    if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +      CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
> 
> +      //
> 
> +      // Locate FACS in FADT
> 
> +      //
> 
> +      CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XFirmwareCtrl, sizeof
> (UINT64));
> 
> +      if (EntryPtr != 0) {
> 
> +        FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >XFirmwareCtrl;
> 
> +
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
> 
> +      } else {
> 
> +        FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
> 
> +
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
> 
> +      }
> 
> +
> 
> +      //
> 
> +      // Locate DSDT in FADT
> 
> +      //
> 
> +      CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XDsdt, sizeof (UINT64));
> 
> +      if (EntryPtr != 0) {
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
> 
> +      } else {
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
> 
>        }
> 
> -      HWChange[Index++] = PciId;
> 
>      }
> 
>    }
> 
> 
> 
> -  //
> 
> -  // Locate FACP Table
> 
> -  //
> 
> -  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
> 
> +  if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> 
> +    //
> 
> +    // Search RSDT
> 
> +    //
> 
> +    AcpiTableCount = 0;
> 
> +    EntryCount     = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER))
> / sizeof (UINT32);
> 
> +    BasePtr        = (UINTN)(Rsdt + 1);
> 
> +    for (Index = 0; Index < EntryCount; Index++) {
> 
> +      EntryPtr = 0;
> 
> +      Table    = NULL;
> 
> +      CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof
> (UINT32)), sizeof (UINT32));
> 
> +      Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> 
> +      if (Table) {
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator (Table);
> 
> +      }
> 
> +
> 
> +      if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +        CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
> 
> +        //
> 
> +        // Locate FACS in FADT
> 
> +        //
> 
> +        if (Fadt.FirmwareCtrl) {
> 
> +          FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
> 
> +
> 
> +          TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
> 
> +        }
> 
> +
> 
> +        //
> 
> +        // Locate DSDT in FADT
> 
> +        //
> 
> +        if (Fadt.Dsdt) {
> 
> +          TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
> 
> +        }
> 
> +      }
> 
> +    }
> 
>    }
> 
> 
> 
>    //
> 
> -  // add HWChange inputs: others
> 
> +  // FACS not found
> 
>    //
> 
> -  HWChange[Index++] = (UINT32)pFADT->XDsdt;
> 
> +  if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> 
> +    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 (TableCrcRecord, 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->HardwareSignature = HWSignature;
> 
> +  FreePool (TableCrcRecord);
> 
> +  DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
> 
>  }
> 
> 
> 
>  VOID
> 
> @@ -1329,7 +1486,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 v7] MinPlatformPkg: Update HWSignature filed in FACS
  2023-05-12  9:32 ` [edk2-devel] " Ni, Ray
@ 2023-05-16 10:06   ` VincentX Ke
  0 siblings, 0 replies; 4+ messages in thread
From: VincentX Ke @ 2023-05-16 10:06 UTC (permalink / raw)
  To: Ni, Ray, devel@edk2.groups.io
  Cc: Chiu, Chasel, Desimone, Nathaniel L, Oram, Isaac W, Gao, Liming,
	Dong, Eric, Sinha, Ankit

Hi, Ray

Please see below replies. And thanks for the code review.

#2. Different versions of FADT have different sizes. This CopyMem is dangerous. And why do you CopyMem()?
Reply: Based on https://edk2.groups.io/g/devel/topic/98821097#104899, The extern variable "Facs" will only keep the unique one in AcpiPlatform.c, "extern EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE Facs" should be follow up if version changed. And since the purpose is calculating CRC, we only need data of FACS rather than physical pointer to FACS.

#5. You don't need to check the Signature. You can check if Xsdt is not 0, use XSDT, otherwise use RSDT.
Reply: To avoid the case that XSDT is garbage in RSDT only platform. Only checking with "Xsdt is not 0" cannot meet the requirement.

 #7. Why not return earlier but here?
Reply: The reason to return here is AllocateZeroPool() failed or XSDT/RSDT doesn't find out any pointer entry. This return should be kept.

Sincerely,
Vincent
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Friday, May 12, 2023 5:33 PM
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 v7] MinPlatformPkg: Update HWSignature filed in FACS



> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
> VincentX Ke
> Sent: Thursday, May 11, 2023 6:00 PM
> 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: [edk2-devel] [PATCH v7] 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>
> Cc: Ankit Sinha<ankit.sinha@intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c   | 287
> +++++++++++++++-----
>  Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf |   1 +
>  2 files changed, 223 insertions(+), 65 deletions(-)
> 
> diff --git 
> a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> index e967031a3b..3dca6f99f7 100644
> --- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
> @@ -1191,98 +1191,255 @@ 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 ACPI table required to calculate CRC.
> 
> +
> 
> +  @retval CRC       A pointer to allocate UINT32 that
> 
> +                    contains the CRC32 data.
> 
> +**/
> 
> +UINT32
> 
> +AcpiTableCrcCalculator (
> 
> +  IN  EFI_ACPI_COMMON_HEADER  *Table
> 
> +  )
> 
> +{
> 
> +  EFI_STATUS  Status;
> 
> +  UINT32      CRC;
> 
> +
> 
> +  Status = EFI_SUCCESS;
> 
> +  CRC    = 0;
> 
> +
> 
> +  //
> 
> +  // Calculate CRC value.
> 
> +  //
> 
> +  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;
> 
> +  }
> 
> +
> 
> +  Status = gBS->CalculateCrc32 ((UINT8 *)Table, (UINTN)Table->Length, 
> + &CRC);
> 
> +  return CRC;
> 
> +}
> 
> +
> 
> +/**
> 
> +  This function count ACPI tables in RSDT/XSDT and return the result.
> 
> +
> 
> +  @param[in] Sdt                ACPI XSDT/RSDT.
> 
> +  @param[in] TablePointerSize   Size of table pointer:
> 
> +                                4(RSDT) or 8(XSDT).
> 
> +
> 
> +  @retval TableCount            The total number of ACPI tables in
> 
> +                                RSDT or XSDT.
> 
> +**/
> 
> +UINTN
> 
> +CountTableInSDT (
> 
> +  IN  EFI_ACPI_DESCRIPTION_HEADER  *Sdt,
> 
> +  IN  UINTN                        TablePointerSize
> 
> +  )
> 
> +{
> 
> +  UINTN                   Index;
> 
> +  UINTN                   TableCount;
> 
> +  UINTN                   EntryCount;
> 
> +  UINT64                  EntryPtr;
> 
> +  UINTN                   BasePtr;
> 
> +  EFI_ACPI_COMMON_HEADER  *Table;
> 
> +
> 
> +  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> TablePointerSize;
> 
> +  BasePtr    = (UINTN)(Sdt + 1);
> 
> +
> 
> +  for (Index = 0, TableCount = 0; Index < EntryCount; Index++) {
> 
> +    EntryPtr = 0;
> 
> +    Table    = NULL;
> 
> +    CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize),
> TablePointerSize);
> 
> +    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> 
> +    if (Table) {

1. if (Table != NULL)

> 
> +      TableCount++;
> 
> +    }
> 
> +
> 
> +    if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +      CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));

2. Different versions of FADT have different sizes. This CopyMem is dangerous.
And why do you CopyMem()?

> 
> +      if (Fadt.FirmwareCtrl || Fadt.XFirmwareCtrl) {

3. if (Fadt.FirmwareCtrl != 0 || ...)

> 
> +        TableCount++;
> 
> +      }
> 
> +
> 
> +      if (Fadt.Dsdt || Fadt.XDsdt) {

4. if (Fadt. Dsdt != 0 || ...)

> 
> +        TableCount++;
> 
> +      }
> 
> +    }
> 
> +  }
> 
> +
> 
> +  return TableCount;
> 
> +}
> 
> +
> 
> +/**
> 
> +  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;
> 
> +  UINTN                                         EntryCount;
> 
> +  UINTN                                         BasePtr;
> 
> +  UINT64                                        EntryPtr;
> 
> +  UINT32                                        *TableCrcRecord;
> 
> +  UINT32                                        HWSignature;
> 
> +  EFI_ACPI_COMMON_HEADER                        *Table;
> 
> +  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;
> 
> +
> 
> +  Index          = 0;
> 
> +  AcpiTableCount = 0;
> 
> +  EntryCount     = 0;
> 
> +  BasePtr        = 0;
> 
> +  EntryPtr       = 0;
> 
> +  HWSignature    = 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->Signature !=
> EFI_ACPI_6_5_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +    if (Rsdt->Signature !=
> EFI_ACPI_6_5_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +      return;
> 
> +    }
> 
>    }

5. You doesn't need to check the Signature. You can check if Xsdt is not 0, use XSDT, otherwise use RSDT.

> 
> 
> 
>    //
> 
> -  // Allocate memory for HWChange and add additional entrie for
> 
> -  // pFADT->XDsdt
> 
> +  // ACPI table count starts with 0.
> 
> +  // Then add ACPI tables found by RSDT/XSDT and FADT.
> 
>    //
> 
> -  HWChangeSize = HandleCount + 1;
> 
> -  HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
> 
> -  ASSERT(HWChange != NULL);
> 
> +  AcpiTableCount = CountTableInSDT (Xsdt, sizeof (UINT64));
> 
> +  if (AcpiTableCount == 0) {
> 
> +    AcpiTableCount = CountTableInSDT (Rsdt, sizeof (UINT32));
> 
> +  }

6. No need to choose between XSDT and RSDT again. Use #5 comment to choose between the two.

> 
> 
> 
> -  if (HWChange == NULL) return;

7. Why not return earlier but here?

8. I will stop reviewing. Can you please review within your team first then send out?


> 
> +  //
> 
> +  // Allocate memory for founded ACPI tables.
> 
> +  //
> 
> +  TableCrcRecord = AllocateZeroPool (sizeof (UINT32) * 
> + AcpiTableCount);
> 
> +  if (TableCrcRecord == NULL) {
> 
> +    return;
> 
> +  }
> 
> 
> 
>    //
> 
> -  // add HWChange inputs: PCI devices
> 
> +  // Search XSDT
> 
>    //
> 
> -  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;
> 
> +  AcpiTableCount = 0;
> 
> +  EntryCount     = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof (UINT64);
> 
> +  BasePtr        = (UINTN)(Xsdt + 1);
> 
> +  for (Index = 0; Index < EntryCount; Index++) {
> 
> +    EntryPtr = 0;
> 
> +    Table    = NULL;
> 
> +    CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof 
> + (UINT64)),
> sizeof (UINT64));
> 
> +    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> 
> +    if (Table) {
> 
> +      TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator 
> + (Table);
> 
> +    }
> 
> +
> 
> +    if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +      CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
> 
> +      //
> 
> +      // Locate FACS in FADT
> 
> +      //
> 
> +      CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XFirmwareCtrl, sizeof
> (UINT64));
> 
> +      if (EntryPtr != 0) {
> 
> +        FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >XFirmwareCtrl;
> 
> +
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
> 
> +      } else {
> 
> +        FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
> 
> +
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
> 
> +      }
> 
> +
> 
> +      //
> 
> +      // Locate DSDT in FADT
> 
> +      //
> 
> +      CopyMem ((VOID *)&EntryPtr, (VOID *)&Fadt.XDsdt, sizeof 
> + (UINT64));
> 
> +      if (EntryPtr != 0) {
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)EntryPtr);
> 
> +      } else {
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
> 
>        }
> 
> -      HWChange[Index++] = PciId;
> 
>      }
> 
>    }
> 
> 
> 
> -  //
> 
> -  // Locate FACP Table
> 
> -  //
> 
> -  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
> 
> +  if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> 
> +    //
> 
> +    // Search RSDT
> 
> +    //
> 
> +    AcpiTableCount = 0;
> 
> +    EntryCount     = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof (UINT32);
> 
> +    BasePtr        = (UINTN)(Rsdt + 1);
> 
> +    for (Index = 0; Index < EntryCount; Index++) {
> 
> +      EntryPtr = 0;
> 
> +      Table    = NULL;
> 
> +      CopyMem ((VOID *)&EntryPtr, (VOID *)(BasePtr + Index * sizeof 
> + (UINT32)),
> sizeof (UINT32));
> 
> +      Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> 
> +      if (Table) {
> 
> +        TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator 
> + (Table);
> 
> +      }
> 
> +
> 
> +      if (Table->Signature ==
> EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> 
> +        CopyMem ((VOID *)&Fadt, (VOID *)Table, sizeof
> (EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE));
> 
> +        //
> 
> +        // Locate FACS in FADT
> 
> +        //
> 
> +        if (Fadt.FirmwareCtrl) {
> 
> +          FacsPtr = (EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)((EFI_ACPI_6_5_FIXED_ACPI_DESCRIPTION_TABLE *)Table)-
> >FirmwareCtrl;
> 
> +
> 
> +          TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.FirmwareCtrl);
> 
> +        }
> 
> +
> 
> +        //
> 
> +        // Locate DSDT in FADT
> 
> +        //
> 
> +        if (Fadt.Dsdt) {
> 
> +          TableCrcRecord[AcpiTableCount++] = AcpiTableCrcCalculator
> ((EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt.Dsdt);
> 
> +        }
> 
> +      }
> 
> +    }
> 
>    }
> 
> 
> 
>    //
> 
> -  // add HWChange inputs: others
> 
> +  // FACS not found
> 
>    //
> 
> -  HWChange[Index++] = (UINT32)pFADT->XDsdt;
> 
> +  if (FacsPtr->Signature !=
> EFI_ACPI_6_5_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> 
> +    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 (TableCrcRecord, 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->HardwareSignature = HWSignature;
> 
> +  FreePool (TableCrcRecord);
> 
> +  DEBUG ((DEBUG_INFO, "%a() - End\n", __FUNCTION__));
> 
>  }
> 
> 
> 
>  VOID
> 
> @@ -1329,7 +1486,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 (#104685): 
> https://edk2.groups.io/g/devel/message/104685
> Mute This Topic: https://groups.io/mt/98824282/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com] 
> -=-=-=-=-=-=
> 


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

end of thread, other threads:[~2023-05-16 10:06 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-11  9:59 [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS VincentX Ke
2023-05-12  9:32 ` [edk2-devel] " Ni, Ray
2023-05-16 10:06   ` VincentX Ke
2023-05-15 21:16 ` Ankit Sinha

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