* [PATCH edk2-platforms 0/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response
@ 2023-02-27 23:27 Mike Maslenkin
2023-02-27 23:27 ` [PATCH edk2-platforms 1/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response parsing Mike Maslenkin
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Mike Maslenkin @ 2023-02-27 23:27 UTC (permalink / raw)
To: devel; +Cc: Mike Maslenkin, Isaac Oram, Nate DeSimone, Liming Gao
This patchset fixes "Get Self Test Results" IPMI command response processing.
The first patch just makes a fix.
The second patch removes a transfer buffer from IPMI instance data as a
preparation of further improvement. It's not clear why a buffer of
a maximum size used for all commands. For the command mentioned above
response contains only 3 bytes.
The third patch drops raw byte array usage while parsing command
response because structure for this response is already defined in edk2.
Checked compilation for the Intel’s Aowanda platform using GCC5
toolchain.
Signed-off-by: Mike Maslenkin <mike.maslenkin@gmail.com>
CC: Isaac Oram <isaac.w.oram@intel.com>
CC: Nate DeSimone <nathaniel.l.desimone@intel.com>
CC: Liming Gao <gaoliming@byosoft.com.cn>
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH edk2-platforms 1/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response parsing 2023-02-27 23:27 [PATCH edk2-platforms 0/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response Mike Maslenkin @ 2023-02-27 23:27 ` Mike Maslenkin 2023-03-07 22:09 ` Isaac Oram 2023-02-27 23:27 ` [PATCH edk2-platforms 2/3] IpmiFeaturePkg: remove buffer temporary buffer from BMC instance structure Mike Maslenkin 2023-02-27 23:27 ` [PATCH edk2-platforms 3/3] IpmiFeaturePkg: refine GetSelfTest function Mike Maslenkin 2 siblings, 1 reply; 7+ messages in thread From: Mike Maslenkin @ 2023-02-27 23:27 UTC (permalink / raw) To: devel; +Cc: Mike Maslenkin, Isaac Oram, Nate DeSimone, Liming Gao Byte 0 of a response contains completion code for the command. So, the examined data starts from byte 1. It's easy to make a mistake here since specification counts response data from 1. For the "Get Self Test Results" command Intelligent Platform Management Interface Specification v2.0 rev 1.1 paragraph 20.4 defines response as: +-----+---------------------------------------------------------------+ |byte | data field | +-----+---------------------------------------------------------------+ | 1 | Completion Code | | | | | 2 | 55h = No error. All Self Tests Passed. | | | 56h = Self Test function not implemented in this controller. | | | 57h = Corrupted or inaccessible data or devices | | | 58h = Fatal hardware error | | | | | 3 | For byte 2 = 55h, 56h, FFh: 00h | | | For byte 2 = 58h, all other: Device-specific | | | For byte 2 = 57h: self-test error bitfield. | +-----+---------------------------------------------------------------+ Signed-off-by: Mike Maslenkin <mike.maslenkin@gmail.com> --- .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c index d788b4886723..aeaefaad642e 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c @@ -161,7 +161,7 @@ Returns: // Check the IPMI defined self test results. // Additional Cases are device specific test results. // - switch (IpmiInstance->TempData[0]) { + switch (IpmiInstance->TempData[1]) { case IPMI_APP_SELFTEST_NO_ERROR: case IPMI_APP_SELFTEST_NOT_IMPLEMENTED: IpmiInstance->BmcStatus = BMC_OK; @@ -173,7 +173,7 @@ Returns: // BootBlock Firmware corruption, and Operational Firmware Corruption. All // other errors are BMC soft failures. // - if ((IpmiInstance->TempData[1] & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) { + if ((IpmiInstance->TempData[2] & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) { IpmiInstance->BmcStatus = BMC_HARDFAIL; } else { IpmiInstance->BmcStatus = BMC_SOFTFAIL; @@ -181,7 +181,7 @@ Returns: // // Check if SDR repository is empty and report it if it is. // - if ((IpmiInstance->TempData[1] & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) { + if ((IpmiInstance->TempData[2] & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) { if (*ErrorCount < MAX_SOFT_COUNT) { StatusCodeValue[*ErrorCount] = EFI_COMPUTING_UNIT_FIRMWARE_PROCESSOR | CU_FP_EC_SDR_EMPTY; (*ErrorCount)++; -- 2.35.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH edk2-platforms 1/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response parsing 2023-02-27 23:27 ` [PATCH edk2-platforms 1/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response parsing Mike Maslenkin @ 2023-03-07 22:09 ` Isaac Oram 0 siblings, 0 replies; 7+ messages in thread From: Isaac Oram @ 2023-03-07 22:09 UTC (permalink / raw) To: Mike Maslenkin, devel@edk2.groups.io; +Cc: Desimone, Nathaniel L, Gao, Liming Reviewed-by: Isaac Oram <Isaac.w.oram@intel.com> -----Original Message----- From: Mike Maslenkin <mike.maslenkin@gmail.com> Sent: Monday, February 27, 2023 3:28 PM To: devel@edk2.groups.io Cc: Mike Maslenkin <mike.maslenkin@gmail.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn> Subject: [PATCH edk2-platforms 1/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response parsing Byte 0 of a response contains completion code for the command. So, the examined data starts from byte 1. It's easy to make a mistake here since specification counts response data from 1. For the "Get Self Test Results" command Intelligent Platform Management Interface Specification v2.0 rev 1.1 paragraph 20.4 defines response as: +-----+---------------------------------------------------------------+ |byte | data field | +-----+---------------------------------------------------------------+ | 1 | Completion Code | | | | | 2 | 55h = No error. All Self Tests Passed. | | | 56h = Self Test function not implemented in this controller. | | | 57h = Corrupted or inaccessible data or devices | | | 58h = Fatal hardware error | | | | | 3 | For byte 2 = 55h, 56h, FFh: 00h | | | For byte 2 = 58h, all other: Device-specific | | | For byte 2 = 57h: self-test error bitfield. | +-----+---------------------------------------------------------------+ Signed-off-by: Mike Maslenkin <mike.maslenkin@gmail.com> --- .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c index d788b4886723..aeaefaad642e 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/ +++ IpmiInit.c @@ -161,7 +161,7 @@ Returns: // Check the IPMI defined self test results. // Additional Cases are device specific test results. //- switch (IpmiInstance->TempData[0]) {+ switch (IpmiInstance->TempData[1]) { case IPMI_APP_SELFTEST_NO_ERROR: case IPMI_APP_SELFTEST_NOT_IMPLEMENTED: IpmiInstance->BmcStatus = BMC_OK;@@ -173,7 +173,7 @@ Returns: // BootBlock Firmware corruption, and Operational Firmware Corruption. All // other errors are BMC soft failures. //- if ((IpmiInstance->TempData[1] & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) {+ if ((IpmiInstance->TempData[2] & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) { IpmiInstance->BmcStatus = BMC_HARDFAIL; } else { IpmiInstance->BmcStatus = BMC_SOFTFAIL;@@ -181,7 +181,7 @@ Returns: // // Check if SDR repository is empty and report it if it is. //- if ((IpmiInstance->TempData[1] & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) {+ if ((IpmiInstance->TempData[2] & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) { if (*ErrorCount < MAX_SOFT_COUNT) { StatusCodeValue[*ErrorCount] = EFI_COMPUTING_UNIT_FIRMWARE_PROCESSOR | CU_FP_EC_SDR_EMPTY; (*ErrorCount)++;-- 2.35.3 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH edk2-platforms 2/3] IpmiFeaturePkg: remove buffer temporary buffer from BMC instance structure 2023-02-27 23:27 [PATCH edk2-platforms 0/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response Mike Maslenkin 2023-02-27 23:27 ` [PATCH edk2-platforms 1/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response parsing Mike Maslenkin @ 2023-02-27 23:27 ` Mike Maslenkin 2023-03-07 22:45 ` Isaac Oram 2023-02-27 23:27 ` [PATCH edk2-platforms 3/3] IpmiFeaturePkg: refine GetSelfTest function Mike Maslenkin 2 siblings, 1 reply; 7+ messages in thread From: Mike Maslenkin @ 2023-02-27 23:27 UTC (permalink / raw) To: devel; +Cc: Mike Maslenkin, Isaac Oram, Nate DeSimone, Liming Gao There is no point to have temporary buffer in BMC instance data used only for synchronous IPMI command as a transfer buffer. Using local variables make things much simpler. Signed-off-by: Mike Maslenkin <mike.maslenkin@gmail.com> --- .../GenericIpmi/Common/IpmiBmc.c | 5 ++- .../GenericIpmi/Common/IpmiBmcCommon.h | 1 - .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 39 ++++++++++--------- .../GenericIpmi/Pei/PeiGenericIpmi.c | 11 +++--- .../GenericIpmi/Pei/PeiIpmiBmc.c | 5 ++- .../GenericIpmi/Pei/PeiIpmiBmcDef.h | 1 - 6 files changed, 33 insertions(+), 29 deletions(-) diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c index 03b8174e3786..a6be2f46e84e 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c @@ -101,6 +101,7 @@ Returns: IPMI_RESPONSE *IpmiResponse; UINT8 RetryCnt = IPMI_SEND_COMMAND_MAX_RETRY; UINT8 Index; + UINT8 TempData[MAX_TEMP_DATA]; IpmiInstance = INSTANCE_FROM_SM_IPMI_BMC_THIS (This); @@ -110,8 +111,8 @@ Returns: // response data. Since the command format is different from the response // format, the buffer is cast to both structure definitions. // - IpmiCommand = (IPMI_COMMAND*) IpmiInstance->TempData; - IpmiResponse = (IPMI_RESPONSE*) IpmiInstance->TempData; + IpmiCommand = (IPMI_COMMAND*) TempData; + IpmiResponse = (IPMI_RESPONSE*) TempData; // // Send IPMI command to BMC diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h index 1e5dfd81f1fb..06eab62aaec9 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h @@ -50,7 +50,6 @@ typedef struct { UINTN Signature; UINT64 KcsTimeoutPeriod; UINT8 SlaveAddress; - UINT8 TempData[MAX_TEMP_DATA]; BMC_STATUS BmcStatus; UINT64 ErrorStatus; UINT8 SoftErrorCount; diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c index aeaefaad642e..8a0c596a6434 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c @@ -84,6 +84,7 @@ Returns: UINT8 *TempPtr; UINT32 Retries; BOOLEAN bResultFlag = FALSE; + UINT8 TempData[MAX_TEMP_DATA]; // // Get the SELF TEST Results. @@ -97,9 +98,9 @@ Returns: Retries = PcdGet8 (PcdIpmiBmcReadyDelayTimer); } - DataSize = sizeof (IpmiInstance->TempData); + DataSize = sizeof (TempData); - IpmiInstance->TempData[1] = 0; + TempData[1] = 0; do { Status = IpmiSendCommand ( @@ -109,11 +110,11 @@ Returns: IPMI_APP_GET_SELFTEST_RESULTS, NULL, 0, - IpmiInstance->TempData, + TempData, &DataSize ); if (Status == EFI_SUCCESS) { - switch (IpmiInstance->TempData[1]) { + switch (TempData[1]) { case IPMI_APP_SELFTEST_NO_ERROR: case IPMI_APP_SELFTEST_NOT_IMPLEMENTED: case IPMI_APP_SELFTEST_ERROR: @@ -146,7 +147,7 @@ Returns: IpmiInstance->BmcStatus = BMC_HARDFAIL; return Status; } else { - DEBUG ((EFI_D_INFO, "[IPMI] BMC self-test result: %02X-%02X\n", IpmiInstance->TempData[1], IpmiInstance->TempData[2])); + DEBUG ((DEBUG_INFO, "[IPMI] BMC self-test result: %02X-%02X\n", TempData[1], TempData[2])); // // Copy the Self test results to Error Status. Data will be copied as long as it // does not exceed the size of the ErrorStatus variable. @@ -155,13 +156,13 @@ Returns: (Index < DataSize) && (Index < sizeof (IpmiInstance->ErrorStatus)); Index++, TempPtr++ ) { - *TempPtr = IpmiInstance->TempData[Index]; + *TempPtr = TempData[Index]; } // // Check the IPMI defined self test results. // Additional Cases are device specific test results. // - switch (IpmiInstance->TempData[1]) { + switch (TempData[1]) { case IPMI_APP_SELFTEST_NO_ERROR: case IPMI_APP_SELFTEST_NOT_IMPLEMENTED: IpmiInstance->BmcStatus = BMC_OK; @@ -173,7 +174,7 @@ Returns: // BootBlock Firmware corruption, and Operational Firmware Corruption. All // other errors are BMC soft failures. // - if ((IpmiInstance->TempData[2] & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) { + if ((TempData[2] & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) { IpmiInstance->BmcStatus = BMC_HARDFAIL; } else { IpmiInstance->BmcStatus = BMC_SOFTFAIL; @@ -181,7 +182,7 @@ Returns: // // Check if SDR repository is empty and report it if it is. // - if ((IpmiInstance->TempData[2] & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) { + if ((TempData[2] & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) { if (*ErrorCount < MAX_SOFT_COUNT) { StatusCodeValue[*ErrorCount] = EFI_COMPUTING_UNIT_FIRMWARE_PROCESSOR | CU_FP_EC_SDR_EMPTY; (*ErrorCount)++; @@ -244,6 +245,8 @@ Returns: SM_CTRL_INFO *pBmcInfo; IPMI_MSG_GET_BMC_EXEC_RSP *pBmcExecContext; UINT32 Retries; + UINT8 TempData[MAX_TEMP_DATA]; + #ifdef FAST_VIDEO_SUPPORT EFI_VIDEOPRINT_PROTOCOL *VideoPrintProtocol; EFI_STATUS VideoPrintStatus; @@ -266,16 +269,16 @@ Returns: // // Get the device ID information for the BMC. // - DataSize = sizeof (IpmiInstance->TempData); + DataSize = sizeof (TempData); while (EFI_ERROR (Status = IpmiSendCommand ( &IpmiInstance->IpmiTransport, IPMI_NETFN_APP, 0, IPMI_APP_GET_DEVICE_ID, NULL, 0, - IpmiInstance->TempData, &DataSize)) + TempData, &DataSize)) ) { DEBUG ((DEBUG_ERROR, "[IPMI] BMC does not respond by Get BMC DID (status: %r), %d retries left, ResponseData: 0x%lx\n", - Status, Retries, IpmiInstance->TempData)); + Status, Retries, TempData)); if (Retries-- == 0) { IpmiInstance->BmcStatus = BMC_HARDFAIL; @@ -287,7 +290,7 @@ Returns: MicroSecondDelay (1*1000*1000); } - pBmcInfo = (SM_CTRL_INFO*)&IpmiInstance->TempData[0]; + pBmcInfo = (SM_CTRL_INFO*)&TempData[0]; DEBUG ((EFI_D_ERROR, "[IPMI] BMC Device ID: 0x%02X, firmware version: %d.%02X UpdateMode:%x\n", pBmcInfo->DeviceId, pBmcInfo->MajorFirmwareRev, pBmcInfo->MinorFirmwareRev,pBmcInfo->UpdateMode)); // // In OpenBMC, UpdateMode: the bit 7 of byte 4 in get device id command is used for the BMC status: @@ -303,10 +306,10 @@ Returns: IPMI_NETFN_FIRMWARE, 0, IPMI_GET_BMC_EXECUTION_CONTEXT, NULL, 0, - IpmiInstance->TempData, &DataSize + TempData, &DataSize ); - pBmcExecContext = (IPMI_MSG_GET_BMC_EXEC_RSP*)&IpmiInstance->TempData[0]; + pBmcExecContext = (IPMI_MSG_GET_BMC_EXEC_RSP*)&TempData[0]; DEBUG ((DEBUG_INFO, "[IPMI] Operational status of BMC: 0x%x\n", pBmcExecContext->CurrentExecutionContext)); if ((pBmcExecContext->CurrentExecutionContext == IPMI_BMC_IN_FORCED_UPDATE_MODE) && !EFI_ERROR (Status)) { @@ -324,12 +327,12 @@ Returns: IPMI_NETFN_APP, 0, IPMI_APP_GET_DEVICE_ID, NULL, 0, - IpmiInstance->TempData, &DataSize + TempData, &DataSize ); if (!EFI_ERROR (Status)) { - pBmcInfo = (SM_CTRL_INFO*)&IpmiInstance->TempData[0]; - DEBUG ((EFI_D_ERROR, "[IPMI] UpdateMode Retries: %d pBmcInfo->UpdateMode:%x, Status: %r, Response Data: 0x%lx\n",Retries, pBmcInfo->UpdateMode, Status, IpmiInstance->TempData)); + pBmcInfo = (SM_CTRL_INFO*)&TempData[0]; + DEBUG ((DEBUG_ERROR, "[IPMI] UpdateMode Retries: %d pBmcInfo->UpdateMode:%x, Status: %r, Response Data: 0x%lx\n",Retries, pBmcInfo->UpdateMode, Status, TempData)); if (pBmcInfo->UpdateMode == BMC_READY) { mIpmiInstance->BmcStatus = BMC_OK; return EFI_SUCCESS; diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c index 3efb772b684f..e8b99b6900c5 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c @@ -288,6 +288,7 @@ Returns: UINT32 DataSize; SM_CTRL_INFO *pBmcInfo; UINTN Retries; + UINT8 TempData[MAX_TEMP_DATA]; // // Set up a loop to retry for up to PcdIpmiBmcReadyDelayTimer seconds. Calculate retries not timeout @@ -298,7 +299,7 @@ Returns: // // Get the device ID information for the BMC. // - DataSize = sizeof (mIpmiInstance->TempData); + DataSize = sizeof (TempData); while (EFI_ERROR (Status = PeiIpmiSendCommand ( &mIpmiInstance->IpmiTransportPpi, IPMI_NETFN_APP, @@ -306,7 +307,7 @@ Returns: IPMI_APP_GET_DEVICE_ID, NULL, 0, - mIpmiInstance->TempData, + TempData, &DataSize ))) { DEBUG ((EFI_D_ERROR, "[IPMI] BMC does not respond (status: %r), %d retries left\n", @@ -322,7 +323,7 @@ Returns: // MicroSecondDelay (1*1000*1000); } - pBmcInfo = (SM_CTRL_INFO*) &mIpmiInstance->TempData[0]; + pBmcInfo = (SM_CTRL_INFO*) &TempData[0]; DEBUG ((DEBUG_INFO, "[IPMI PEI] BMC Device ID: 0x%02X, firmware version: %d.%02X UpdateMode:%x\n", pBmcInfo->DeviceId, pBmcInfo->MajorFirmwareRev, pBmcInfo->MinorFirmwareRev, pBmcInfo->UpdateMode)); // @@ -347,11 +348,11 @@ Returns: IPMI_APP_GET_DEVICE_ID, NULL, 0, - mIpmiInstance->TempData, + TempData, &DataSize ); if (!EFI_ERROR (Status)) { - pBmcInfo = (SM_CTRL_INFO*) &mIpmiInstance->TempData[0]; + pBmcInfo = (SM_CTRL_INFO*) &TempData[0]; DEBUG ((DEBUG_INFO, "[IPMI PEI] UpdateMode Retries:%x pBmcInfo->UpdateMode:%x\n", Retries, pBmcInfo->UpdateMode)); if (pBmcInfo->UpdateMode == BMC_READY) { mIpmiInstance->BmcStatus = BMC_OK; diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c index 32665b3e2244..dbe25421ae49 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c @@ -99,6 +99,7 @@ Returns: IPMI_COMMAND *IpmiCommand; IPMI_RESPONSE *IpmiResponse; UINT8 Index; + UINT8 TempData[MAX_TEMP_DATA]; IpmiInstance = INSTANCE_FROM_PEI_SM_IPMI_BMC_THIS (This); @@ -107,8 +108,8 @@ Returns: // response data. Since the command format is different from the response // format, the buffer is cast to both structure definitions. // - IpmiCommand = (IPMI_COMMAND*) IpmiInstance->TempData; - IpmiResponse = (IPMI_RESPONSE*) IpmiInstance->TempData; + IpmiCommand = (IPMI_COMMAND*) TempData; + IpmiResponse = (IPMI_RESPONSE*) TempData; // // Send IPMI command to BMC diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h index 3fbe70ce629d..fc9fbacf1a2b 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h @@ -49,7 +49,6 @@ typedef struct { UINTN Signature; UINT64 KcsTimeoutPeriod; UINT8 SlaveAddress; - UINT8 TempData[MAX_TEMP_DATA]; BMC_STATUS BmcStatus; UINT64 ErrorStatus; UINT8 SoftErrorCount; -- 2.35.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH edk2-platforms 2/3] IpmiFeaturePkg: remove buffer temporary buffer from BMC instance structure 2023-02-27 23:27 ` [PATCH edk2-platforms 2/3] IpmiFeaturePkg: remove buffer temporary buffer from BMC instance structure Mike Maslenkin @ 2023-03-07 22:45 ` Isaac Oram 0 siblings, 0 replies; 7+ messages in thread From: Isaac Oram @ 2023-03-07 22:45 UTC (permalink / raw) To: Mike Maslenkin, devel@edk2.groups.io; +Cc: Desimone, Nathaniel L, Gao, Liming Mike, Thanks for the fixes. There is one more instance using TempData in SmmGenericIpmi.c. Could you please fix that as well? I noted that it isn't currently built/included by the IpmiFeaturePkg.dsc. I will fix that. Also, please add maintainers and reviewers for the package to commit messages, e.g.: Cc: Isaac Oram <isaac.w.oram@intel.com> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Regards, Isaac -----Original Message----- From: Mike Maslenkin <mike.maslenkin@gmail.com> Sent: Monday, February 27, 2023 3:28 PM To: devel@edk2.groups.io Cc: Mike Maslenkin <mike.maslenkin@gmail.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn> Subject: [PATCH edk2-platforms 2/3] IpmiFeaturePkg: remove buffer temporary buffer from BMC instance structure There is no point to have temporary buffer in BMC instance data used only for synchronous IPMI command as a transfer buffer. Using local variables make things much simpler. Signed-off-by: Mike Maslenkin <mike.maslenkin@gmail.com> --- .../GenericIpmi/Common/IpmiBmc.c | 5 ++- .../GenericIpmi/Common/IpmiBmcCommon.h | 1 - .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 39 ++++++++++--------- .../GenericIpmi/Pei/PeiGenericIpmi.c | 11 +++--- .../GenericIpmi/Pei/PeiIpmiBmc.c | 5 ++- .../GenericIpmi/Pei/PeiIpmiBmcDef.h | 1 - 6 files changed, 33 insertions(+), 29 deletions(-) diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c index 03b8174e3786..a6be2f46e84e 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Comm +++ on/IpmiBmc.c @@ -101,6 +101,7 @@ Returns: IPMI_RESPONSE *IpmiResponse; UINT8 RetryCnt = IPMI_SEND_COMMAND_MAX_RETRY; UINT8 Index;+ UINT8 TempData[MAX_TEMP_DATA]; IpmiInstance = INSTANCE_FROM_SM_IPMI_BMC_THIS (This); @@ -110,8 +111,8 @@ Returns: // response data. Since the command format is different from the response // format, the buffer is cast to both structure definitions. //- IpmiCommand = (IPMI_COMMAND*) IpmiInstance->TempData;- IpmiResponse = (IPMI_RESPONSE*) IpmiInstance->TempData;+ IpmiCommand = (IPMI_COMMAND*) TempData;+ IpmiResponse = (IPMI_RESPONSE*) TempData; // // Send IPMI command to BMCdiff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h index 1e5dfd81f1fb..06eab62aaec9 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Comm +++ on/IpmiBmcCommon.h @@ -50,7 +50,6 @@ typedef struct { UINTN Signature; UINT64 KcsTimeoutPeriod; UINT8 SlaveAddress;- UINT8 TempData[MAX_TEMP_DATA]; BMC_STATUS BmcStatus; UINT64 ErrorStatus; UINT8 SoftErrorCount;diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c index aeaefaad642e..8a0c596a6434 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/ +++ IpmiInit.c @@ -84,6 +84,7 @@ Returns: UINT8 *TempPtr; UINT32 Retries; BOOLEAN bResultFlag = FALSE;+ UINT8 TempData[MAX_TEMP_DATA]; // // Get the SELF TEST Results.@@ -97,9 +98,9 @@ Returns: Retries = PcdGet8 (PcdIpmiBmcReadyDelayTimer); } - DataSize = sizeof (IpmiInstance->TempData);+ DataSize = sizeof (TempData); - IpmiInstance->TempData[1] = 0;+ TempData[1] = 0; do { Status = IpmiSendCommand (@@ -109,11 +110,11 @@ Returns: IPMI_APP_GET_SELFTEST_RESULTS, NULL, 0,- IpmiInstance->TempData,+ TempData, &DataSize ); if (Status == EFI_SUCCESS) {- switch (IpmiInstance->TempData[1]) {+ switch (TempData[1]) { case IPMI_APP_SELFTEST_NO_ERROR: case IPMI_APP_SELFTEST_NOT_IMPLEMENTED: case IPMI_APP_SELFTEST_ERROR:@@ -146,7 +147,7 @@ Returns: IpmiInstance->BmcStatus = BMC_HARDFAIL; return Status; } else {- DEBUG ((EFI_D_INFO, "[IPMI] BMC self-test result: %02X-%02X\n", IpmiInstance->TempData[1], IpmiInstance->TempData[2]));+ DEBUG ((DEBUG_INFO, "[IPMI] BMC self-test result: %02X-%02X\n", TempData[1], TempData[2])); // // Copy the Self test results to Error Status. Data will be copied as long as it // does not exceed the size of the ErrorStatus variable.@@ -155,13 +156,13 @@ Returns: (Index < DataSize) && (Index < sizeof (IpmiInstance->ErrorStatus)); Index++, TempPtr++ ) {- *TempPtr = IpmiInstance->TempData[Index];+ *TempPtr = TempData[Index]; } // // Check the IPMI defined self test results. // Additional Cases are device specific test results. //- switch (IpmiInstance->TempData[1]) {+ switch (TempData[1]) { case IPMI_APP_SELFTEST_NO_ERROR: case IPMI_APP_SELFTEST_NOT_IMPLEMENTED: IpmiInstance->BmcStatus = BMC_OK;@@ -173,7 +174,7 @@ Returns: // BootBlock Firmware corruption, and Operational Firmware Corruption. All // other errors are BMC soft failures. //- if ((IpmiInstance->TempData[2] & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) {+ if ((TempData[2] & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) { IpmiInstance->BmcStatus = BMC_HARDFAIL; } else { IpmiInstance->BmcStatus = BMC_SOFTFAIL;@@ -181,7 +182,7 @@ Returns: // // Check if SDR repository is empty and report it if it is. //- if ((IpmiInstance->TempData[2] & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) {+ if ((TempData[2] & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) { if (*ErrorCount < MAX_SOFT_COUNT) { StatusCodeValue[*ErrorCount] = EFI_COMPUTING_UNIT_FIRMWARE_PROCESSOR | CU_FP_EC_SDR_EMPTY; (*ErrorCount)++;@@ -244,6 +245,8 @@ Returns: SM_CTRL_INFO *pBmcInfo; IPMI_MSG_GET_BMC_EXEC_RSP *pBmcExecContext; UINT32 Retries;+ UINT8 TempData[MAX_TEMP_DATA];+ #ifdef FAST_VIDEO_SUPPORT EFI_VIDEOPRINT_PROTOCOL *VideoPrintProtocol; EFI_STATUS VideoPrintStatus;@@ -266,16 +269,16 @@ Returns: // // Get the device ID information for the BMC. //- DataSize = sizeof (IpmiInstance->TempData);+ DataSize = sizeof (TempData); while (EFI_ERROR (Status = IpmiSendCommand ( &IpmiInstance->IpmiTransport, IPMI_NETFN_APP, 0, IPMI_APP_GET_DEVICE_ID, NULL, 0,- IpmiInstance->TempData, &DataSize))+ TempData, &DataSize)) ) { DEBUG ((DEBUG_ERROR, "[IPMI] BMC does not respond by Get BMC DID (status: %r), %d retries left, ResponseData: 0x%lx\n",- Status, Retries, IpmiInstance->TempData));+ Status, Retries, TempData)); if (Retries-- == 0) { IpmiInstance->BmcStatus = BMC_HARDFAIL;@@ -287,7 +290,7 @@ Returns: MicroSecondDelay (1*1000*1000); } - pBmcInfo = (SM_CTRL_INFO*)&IpmiInstance->TempData[0];+ pBmcInfo = (SM_CTRL_INFO*)&TempData[0]; DEBUG ((EFI_D_ERROR, "[IPMI] BMC Device ID: 0x%02X, firmware version: %d.%02X UpdateMode:%x\n", pBmcInfo->DeviceId, pBmcInfo->MajorFirmwareRev, pBmcInfo->MinorFirmwareRev,pBmcInfo->UpdateMode)); // // In OpenBMC, UpdateMode: the bit 7 of byte 4 in get device id command is used for the BMC status:@@ -303,10 +306,10 @@ Returns: IPMI_NETFN_FIRMWARE, 0, IPMI_GET_BMC_EXECUTION_CONTEXT, NULL, 0,- IpmiInstance->TempData, &DataSize+ TempData, &DataSize ); - pBmcExecContext = (IPMI_MSG_GET_BMC_EXEC_RSP*)&IpmiInstance->TempData[0];+ pBmcExecContext = (IPMI_MSG_GET_BMC_EXEC_RSP*)&TempData[0]; DEBUG ((DEBUG_INFO, "[IPMI] Operational status of BMC: 0x%x\n", pBmcExecContext->CurrentExecutionContext)); if ((pBmcExecContext->CurrentExecutionContext == IPMI_BMC_IN_FORCED_UPDATE_MODE) && !EFI_ERROR (Status)) {@@ -324,12 +327,12 @@ Returns: IPMI_NETFN_APP, 0, IPMI_APP_GET_DEVICE_ID, NULL, 0,- IpmiInstance->TempData, &DataSize+ TempData, &DataSize ); if (!EFI_ERROR (Status)) {- pBmcInfo = (SM_CTRL_INFO*)&IpmiInstance->TempData[0];- DEBUG ((EFI_D_ERROR, "[IPMI] UpdateMode Retries: %d pBmcInfo->UpdateMode:%x, Status: %r, Response Data: 0x%lx\n",Retries, pBmcInfo->UpdateMode, Status, IpmiInstance->TempData));+ pBmcInfo = (SM_CTRL_INFO*)&TempData[0];+ DEBUG ((DEBUG_ERROR, "[IPMI] UpdateMode Retries: %d pBmcInfo->UpdateMode:%x, Status: %r, Response Data: 0x%lx\n",Retries, pBmcInfo->UpdateMode, Status, TempData)); if (pBmcInfo->UpdateMode == BMC_READY) { mIpmiInstance->BmcStatus = BMC_OK; return EFI_SUCCESS;diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c index 3efb772b684f..e8b99b6900c5 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/ +++ PeiGenericIpmi.c @@ -288,6 +288,7 @@ Returns: UINT32 DataSize; SM_CTRL_INFO *pBmcInfo; UINTN Retries;+ UINT8 TempData[MAX_TEMP_DATA]; // // Set up a loop to retry for up to PcdIpmiBmcReadyDelayTimer seconds. Calculate retries not timeout@@ -298,7 +299,7 @@ Returns: // // Get the device ID information for the BMC. //- DataSize = sizeof (mIpmiInstance->TempData);+ DataSize = sizeof (TempData); while (EFI_ERROR (Status = PeiIpmiSendCommand ( &mIpmiInstance->IpmiTransportPpi, IPMI_NETFN_APP,@@ -306,7 +307,7 @@ Returns: IPMI_APP_GET_DEVICE_ID, NULL, 0,- mIpmiInstance->TempData,+ TempData, &DataSize ))) { DEBUG ((EFI_D_ERROR, "[IPMI] BMC does not respond (status: %r), %d retries left\n",@@ -322,7 +323,7 @@ Returns: // MicroSecondDelay (1*1000*1000); }- pBmcInfo = (SM_CTRL_INFO*) &mIpmiInstance->TempData[0];+ pBmcInfo = (SM_CTRL_INFO*) &TempData[0]; DEBUG ((DEBUG_INFO, "[IPMI PEI] BMC Device ID: 0x%02X, firmware version: %d.%02X UpdateMode:%x\n", pBmcInfo->DeviceId, pBmcInfo->MajorFirmwareRev, pBmcInfo->MinorFirmwareRev, pBmcInfo->UpdateMode)); //@@ -347,11 +348,11 @@ Returns: IPMI_APP_GET_DEVICE_ID, NULL, 0,- mIpmiInstance->TempData,+ TempData, &DataSize ); if (!EFI_ERROR (Status)) {- pBmcInfo = (SM_CTRL_INFO*) &mIpmiInstance->TempData[0];+ pBmcInfo = (SM_CTRL_INFO*) &TempData[0]; DEBUG ((DEBUG_INFO, "[IPMI PEI] UpdateMode Retries:%x pBmcInfo->UpdateMode:%x\n", Retries, pBmcInfo->UpdateMode)); if (pBmcInfo->UpdateMode == BMC_READY) { mIpmiInstance->BmcStatus = BMC_OK;diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c index 32665b3e2244..dbe25421ae49 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/ +++ PeiIpmiBmc.c @@ -99,6 +99,7 @@ Returns: IPMI_COMMAND *IpmiCommand; IPMI_RESPONSE *IpmiResponse; UINT8 Index;+ UINT8 TempData[MAX_TEMP_DATA]; IpmiInstance = INSTANCE_FROM_PEI_SM_IPMI_BMC_THIS (This); @@ -107,8 +108,8 @@ Returns: // response data. Since the command format is different from the response // format, the buffer is cast to both structure definitions. //- IpmiCommand = (IPMI_COMMAND*) IpmiInstance->TempData;- IpmiResponse = (IPMI_RESPONSE*) IpmiInstance->TempData;+ IpmiCommand = (IPMI_COMMAND*) TempData;+ IpmiResponse = (IPMI_RESPONSE*) TempData; // // Send IPMI command to BMCdiff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h index 3fbe70ce629d..fc9fbacf1a2b 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/ +++ PeiIpmiBmcDef.h @@ -49,7 +49,6 @@ typedef struct { UINTN Signature; UINT64 KcsTimeoutPeriod; UINT8 SlaveAddress;- UINT8 TempData[MAX_TEMP_DATA]; BMC_STATUS BmcStatus; UINT64 ErrorStatus; UINT8 SoftErrorCount;-- 2.35.3 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH edk2-platforms 3/3] IpmiFeaturePkg: refine GetSelfTest function 2023-02-27 23:27 [PATCH edk2-platforms 0/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response Mike Maslenkin 2023-02-27 23:27 ` [PATCH edk2-platforms 1/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response parsing Mike Maslenkin 2023-02-27 23:27 ` [PATCH edk2-platforms 2/3] IpmiFeaturePkg: remove buffer temporary buffer from BMC instance structure Mike Maslenkin @ 2023-02-27 23:27 ` Mike Maslenkin 2023-03-07 23:07 ` [edk2-devel] " Isaac Oram 2 siblings, 1 reply; 7+ messages in thread From: Mike Maslenkin @ 2023-02-27 23:27 UTC (permalink / raw) To: devel; +Cc: Mike Maslenkin, Isaac Oram, Nate DeSimone, Liming Gao Use predefined type while accessing IPMI command returned data instead of raw byte array. Signed-off-by: Mike Maslenkin <mike.maslenkin@gmail.com> --- .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c index 8a0c596a6434..1db47e28c54e 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c @@ -86,6 +86,8 @@ Returns: BOOLEAN bResultFlag = FALSE; UINT8 TempData[MAX_TEMP_DATA]; + IPMI_SELF_TEST_RESULT_RESPONSE *pSelfTestResult; + // // Get the SELF TEST Results. // @@ -100,7 +102,8 @@ Returns: DataSize = sizeof (TempData); - TempData[1] = 0; + pSelfTestResult = (IPMI_SELF_TEST_RESULT_RESPONSE*)&TempData[0]; + pSelfTestResult->CompletionCode = 0; do { Status = IpmiSendCommand ( @@ -114,7 +117,7 @@ Returns: &DataSize ); if (Status == EFI_SUCCESS) { - switch (TempData[1]) { + switch (pSelfTestResult->Result) { case IPMI_APP_SELFTEST_NO_ERROR: case IPMI_APP_SELFTEST_NOT_IMPLEMENTED: case IPMI_APP_SELFTEST_ERROR: @@ -147,7 +150,7 @@ Returns: IpmiInstance->BmcStatus = BMC_HARDFAIL; return Status; } else { - DEBUG ((DEBUG_INFO, "[IPMI] BMC self-test result: %02X-%02X\n", TempData[1], TempData[2])); + DEBUG ((DEBUG_INFO, "[IPMI] BMC self-test result: %02X-%02X\n", pSelfTestResult->Result, pSelfTestResult->Param)); // // Copy the Self test results to Error Status. Data will be copied as long as it // does not exceed the size of the ErrorStatus variable. @@ -162,7 +165,7 @@ Returns: // Check the IPMI defined self test results. // Additional Cases are device specific test results. // - switch (TempData[1]) { + switch (pSelfTestResult->Result) { case IPMI_APP_SELFTEST_NO_ERROR: case IPMI_APP_SELFTEST_NOT_IMPLEMENTED: IpmiInstance->BmcStatus = BMC_OK; @@ -174,7 +177,7 @@ Returns: // BootBlock Firmware corruption, and Operational Firmware Corruption. All // other errors are BMC soft failures. // - if ((TempData[2] & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) { + if ((pSelfTestResult->Param & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) { IpmiInstance->BmcStatus = BMC_HARDFAIL; } else { IpmiInstance->BmcStatus = BMC_SOFTFAIL; @@ -182,7 +185,7 @@ Returns: // // Check if SDR repository is empty and report it if it is. // - if ((TempData[2] & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) { + if ((pSelfTestResult->Param & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) { if (*ErrorCount < MAX_SOFT_COUNT) { StatusCodeValue[*ErrorCount] = EFI_COMPUTING_UNIT_FIRMWARE_PROCESSOR | CU_FP_EC_SDR_EMPTY; (*ErrorCount)++; -- 2.35.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH edk2-platforms 3/3] IpmiFeaturePkg: refine GetSelfTest function 2023-02-27 23:27 ` [PATCH edk2-platforms 3/3] IpmiFeaturePkg: refine GetSelfTest function Mike Maslenkin @ 2023-03-07 23:07 ` Isaac Oram 0 siblings, 0 replies; 7+ messages in thread From: Isaac Oram @ 2023-03-07 23:07 UTC (permalink / raw) To: devel@edk2.groups.io, mike.maslenkin@gmail.com Cc: Desimone, Nathaniel L, Gao, Liming Coding convention does not allow Hungarian notation, https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/v/release-2.20/4_naming_conventions/43_identifiers#4.3.3-hungarian-prefixes Please change pSelfTestResult to SelfTestResult. Thanks, Isaac -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Mike Maslenkin Sent: Monday, February 27, 2023 3:28 PM To: devel@edk2.groups.io Cc: Mike Maslenkin <mike.maslenkin@gmail.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn> Subject: [edk2-devel] [PATCH edk2-platforms 3/3] IpmiFeaturePkg: refine GetSelfTest function Use predefined type while accessing IPMI command returned data instead of raw byte array. Signed-off-by: Mike Maslenkin <mike.maslenkin@gmail.com> --- .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c index 8a0c596a6434..1db47e28c54e 100644 --- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c +++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/ +++ IpmiInit.c @@ -86,6 +86,8 @@ Returns: BOOLEAN bResultFlag = FALSE; UINT8 TempData[MAX_TEMP_DATA]; + IPMI_SELF_TEST_RESULT_RESPONSE *pSelfTestResult;+ // // Get the SELF TEST Results. //@@ -100,7 +102,8 @@ Returns: DataSize = sizeof (TempData); - TempData[1] = 0;+ pSelfTestResult = (IPMI_SELF_TEST_RESULT_RESPONSE*)&TempData[0];+ pSelfTestResult->CompletionCode = 0; do { Status = IpmiSendCommand (@@ -114,7 +117,7 @@ Returns: &DataSize ); if (Status == EFI_SUCCESS) {- switch (TempData[1]) {+ switch (pSelfTestResult->Result) { case IPMI_APP_SELFTEST_NO_ERROR: case IPMI_APP_SELFTEST_NOT_IMPLEMENTED: case IPMI_APP_SELFTEST_ERROR:@@ -147,7 +150,7 @@ Returns: IpmiInstance->BmcStatus = BMC_HARDFAIL; return Status; } else {- DEBUG ((DEBUG_INFO, "[IPMI] BMC self-test result: %02X-%02X\n", TempData[1], TempData[2]));+ DEBUG ((DEBUG_INFO, "[IPMI] BMC self-test result: %02X-%02X\n", pSelfTestResult->Result, pSelfTestResult->Param)); // // Copy the Self test results to Error Status. Data will be copied as long as it // does not exceed the size of the ErrorStatus variable.@@ -162,7 +165,7 @@ Returns: // Check the IPMI defined self test results. // Additional Cases are device specific test results. //- switch (TempData[1]) {+ switch (pSelfTestResult->Result) { case IPMI_APP_SELFTEST_NO_ERROR: case IPMI_APP_SELFTEST_NOT_IMPLEMENTED: IpmiInstance->BmcStatus = BMC_OK;@@ -174,7 +177,7 @@ Returns: // BootBlock Firmware corruption, and Operational Firmware Corruption. All // other errors are BMC soft failures. //- if ((TempData[2] & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) {+ if ((pSelfTestResult->Param & (IPMI_APP_SELFTEST_FRU_CORRUPT | IPMI_APP_SELFTEST_FW_BOOTBLOCK_CORRUPT | IPMI_APP_SELFTEST_FW_CORRUPT)) != 0) { IpmiInstance->BmcStatus = BMC_HARDFAIL; } else { IpmiInstance->BmcStatus = BMC_SOFTFAIL;@@ -182,7 +185,7 @@ Returns: // // Check if SDR repository is empty and report it if it is. //- if ((TempData[2] & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) {+ if ((pSelfTestResult->Param & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) { if (*ErrorCount < MAX_SOFT_COUNT) { StatusCodeValue[*ErrorCount] = EFI_COMPUTING_UNIT_FIRMWARE_PROCESSOR | CU_FP_EC_SDR_EMPTY; (*ErrorCount)++;-- 2.35.3 -=-=-=-=-=-= Groups.io Links: You receive all messages sent to this group. View/Reply Online (#100527): https://edk2.groups.io/g/devel/message/100527 Mute This Topic: https://groups.io/mt/97279450/1492418 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [isaac.w.oram@intel.com] -=-=-=-=-=-= ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-07 23:07 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-27 23:27 [PATCH edk2-platforms 0/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response Mike Maslenkin 2023-02-27 23:27 ` [PATCH edk2-platforms 1/3] IpmiFeaturePkg: fix IPMI GetSelfTest command response parsing Mike Maslenkin 2023-03-07 22:09 ` Isaac Oram 2023-02-27 23:27 ` [PATCH edk2-platforms 2/3] IpmiFeaturePkg: remove buffer temporary buffer from BMC instance structure Mike Maslenkin 2023-03-07 22:45 ` Isaac Oram 2023-02-27 23:27 ` [PATCH edk2-platforms 3/3] IpmiFeaturePkg: refine GetSelfTest function Mike Maslenkin 2023-03-07 23:07 ` [edk2-devel] " Isaac Oram
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox