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, > + &CRC); > > + 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 UINTN
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, > &CRC); > > + 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 (&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_STATUSStatus; > > - UINTN Index; > > - UINTN HandleCount; > > - EFI_HANDLE*HandleBuffer; > > - EFI_PCI_IO_PROTOCOL *PciIo; > > - UINT32CRC; > > - UINT32*HWChange; > > -
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, &CRC); > > + 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 (&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.XFirmwareC
[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, &CRC); + 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 (&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_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, - &gEfiPciIoProtocolGuid, - NULL, - &HandleCount, - &HandleBuffer - ); - if (EFI_ERROR (Status)) { -return; // PciIO protocol not installed yet! + EFI_STATUSStatus; + UINTN Index; + UINTN AcpiTableCount; + UINTN EntryCount; + UINTN BasePtr; + UINT64