public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [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

* [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

* [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: [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

* 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

* 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