Re: [edk2-devel] [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
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
Sent: Friday, May 12, 2023 5:33 PM
To: devel@edk2.groups.io; Ke, VincentX
Cc: Chiu, Chasel ; Desimone, Nathaniel L
; Oram, Isaac W ; Gao,
Liming ; Dong, Eric ; Sinha,
Ankit
Subject: RE: [edk2-devel] [PATCH v7] MinPlatformPkg: Update HWSignature filed
in FACS
> -Original Message-
> From: 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 ; Chiu, Chasel
> ; Desimone, Nathaniel L
> ; Oram, Isaac W
> ; Gao, Liming ;
> Dong, Eric ; Sinha, Ankit
> Subject: [edk2-devel] [PATCH v7] MinPlatformPkg: Update HWSignature
> filed in FACS
>
> From: VincentX Ke
>
> 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
> Cc: Chasel Chiu
> Cc: Nate DeSimone
> Cc: Isaac Oram
> Cc: Liming Gao
> Cc: Eric Dong
> Cc: Ankit Sinha
> Signed-off-by: VincentX Ke
> ---
> 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,
> + );
>
> + return CRC;
>
> +}
>
> +
>
> +/**
>
> + This function count ACPI tables in RSDT/XSDT and return the result.
>
> +
>
> + @param[in] SdtACPI XSDT/RSDT.
>
> + @param[in] TablePointerSize Size of table pointer:
>
> +4(RSDT) or 8(XSDT).
>
> +
>
> + @retval TableCountThe total number of ACPI tables in
>
> +RSDT or XSDT.
>
> +**/
>
> +UINTN
>
> +CountTableInSDT (
>
> + IN EFI_ACPI_DESCRIPTION_HEADER *Sdt,
>
> + IN UINTNTablePointerSize
>
>
Re: [edk2-devel] [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
Reviewed-by: Ankit Sinha
> -Original Message-
> From: Ke, VincentX
> Sent: Thursday, May 11, 2023 3:00 AM
> To: devel@edk2.groups.io
> Cc: Ke, VincentX ; Chiu, Chasel
> ; Desimone, Nathaniel L
> ; Oram, Isaac W
> ; Gao, Liming ;
> Dong, Eric ; Sinha, Ankit
> Subject: [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
>
> From: VincentX Ke
>
> 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
> Cc: Chasel Chiu
> Cc: Nate DeSimone
> Cc: Isaac Oram
> Cc: Liming Gao
> Cc: Eric Dong
> Cc: Ankit Sinha
> Signed-off-by: VincentX Ke
> ---
> 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,
> );
>
> + return CRC;
>
> +}
>
> +
>
> +/**
>
> + This function count ACPI tables in RSDT/XSDT and return the result.
>
> +
>
> + @param[in] SdtACPI XSDT/RSDT.
>
> + @param[in] TablePointerSize Size of table pointer:
>
> +4(RSDT) or 8(XSDT).
>
> +
>
> + @retval TableCountThe total number of ACPI tables in
>
> +RSDT or XSDT.
>
> +**/
>
> +UINTN
>
> +CountTableInSDT (
>
> + IN EFI_ACPI_DESCRIPTION_HEADER *Sdt,
>
> + IN UINTNTablePointerSize
>
> + )
>
> +{
>
> + 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 (, (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 *), (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_STATUSStatus;
>
> - UINTN Index;
>
> - UINTN HandleCount;
>
> - EFI_HANDLE*HandleBuffer;
>
> - EFI_PCI_IO_PROTOCOL *PciIo;
>
> - UINT32CRC;
>
> - UINT32*HWChange;
>
> - UINTN
Re: [edk2-devel] [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
> -Original Message-
> From: 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 ; Chiu, Chasel
> ; Desimone, Nathaniel L
> ; Oram, Isaac W ;
> Gao, Liming ; Dong, Eric ;
> Sinha, Ankit
> Subject: [edk2-devel] [PATCH v7] MinPlatformPkg: Update HWSignature filed in
> FACS
>
> From: VincentX Ke
>
> 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
> Cc: Chasel Chiu
> Cc: Nate DeSimone
> Cc: Isaac Oram
> Cc: Liming Gao
> Cc: Eric Dong
> Cc: Ankit Sinha
> Signed-off-by: VincentX Ke
> ---
> 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, );
>
> + return CRC;
>
> +}
>
> +
>
> +/**
>
> + This function count ACPI tables in RSDT/XSDT and return the result.
>
> +
>
> + @param[in] SdtACPI XSDT/RSDT.
>
> + @param[in] TablePointerSize Size of table pointer:
>
> +4(RSDT) or 8(XSDT).
>
> +
>
> + @retval TableCountThe total number of ACPI tables in
>
> +RSDT or XSDT.
>
> +**/
>
> +UINTN
>
> +CountTableInSDT (
>
> + IN EFI_ACPI_DESCRIPTION_HEADER *Sdt,
>
> + IN UINTNTablePointerSize
>
> + )
>
> +{
>
> + 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 (, (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 *), (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. i
[edk2-devel] [PATCH v7] MinPlatformPkg: Update HWSignature filed in FACS
From: VincentX Ke
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
Cc: Chasel Chiu
Cc: Nate DeSimone
Cc: Isaac Oram
Cc: Liming Gao
Cc: Eric Dong
Cc: Ankit Sinha
Signed-off-by: VincentX Ke
---
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, );
+ return CRC;
+}
+
+/**
+ This function count ACPI tables in RSDT/XSDT and return the result.
+
+ @param[in] SdtACPI XSDT/RSDT.
+ @param[in] TablePointerSize Size of table pointer:
+4(RSDT) or 8(XSDT).
+
+ @retval TableCountThe total number of ACPI tables in
+RSDT or XSDT.
+**/
+UINTN
+CountTableInSDT (
+ IN EFI_ACPI_DESCRIPTION_HEADER *Sdt,
+ IN UINTNTablePointerSize
+ )
+{
+ 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 (, (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 *), (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_STATUSStatus;
- UINTN Index;
- UINTN HandleCount;
- EFI_HANDLE*HandleBuffer;
- EFI_PCI_IO_PROTOCOL *PciIo;
- UINT32CRC;
- UINT32*HWChange;
- UINTN HWChangeSize;
- UINT32PciId;
- 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,
- ,
- NULL,
- ,
-
- );
- if (EFI_ERROR (Status)) {
-return; // PciIO protocol not installed yet!
+ EFI_STATUSStatus;
+ UINTN Index;
+ UINTN AcpiTableCount;
+ UINTN EntryCount;
+ UINTN BasePtr;
+ UINT64EntryPtr;
+ UINT32
