Series comparison

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>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100524): https://edk2.groups.io/g/devel/message/100524
Mute This Topic: https://groups.io/mt/97279446/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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(-)

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100525): https://edk2.groups.io/g/devel/message/100525
Mute This Topic: https://groups.io/mt/97279448/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 XXXXXXX..XXXXXXX 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c
@@ -XXX,XX +XXX,XX @@ 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);
 
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
@@ -XXX,XX +XXX,XX @@ Returns:
   UINT8       *TempPtr;
   UINT32      Retries;
   BOOLEAN     bResultFlag = FALSE;
+  UINT8       TempData[MAX_TEMP_DATA];
 
   //
   // Get the SELF TEST Results.
@@ -XXX,XX +XXX,XX @@ Returns:
     Retries = PcdGet8 (PcdIpmiBmcReadyDelayTimer);
   }
 
-  DataSize = sizeof (IpmiInstance->TempData);
+  DataSize = sizeof (TempData);
 
-  IpmiInstance->TempData[1] = 0;
+  TempData[1] = 0;
 
   do {
     Status = IpmiSendCommand (
@@ -XXX,XX +XXX,XX @@ 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:
@@ -XXX,XX +XXX,XX @@ 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.
@@ -XXX,XX +XXX,XX @@ 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;
@@ -XXX,XX +XXX,XX @@ 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;
@@ -XXX,XX +XXX,XX @@ 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)++;
@@ -XXX,XX +XXX,XX @@ 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;
@@ -XXX,XX +XXX,XX @@ 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;
@@ -XXX,XX +XXX,XX @@ 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:
@@ -XXX,XX +XXX,XX @@ 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)) {
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c
@@ -XXX,XX +XXX,XX @@ 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
@@ -XXX,XX +XXX,XX @@ 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,
@@ -XXX,XX +XXX,XX @@ 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",
@@ -XXX,XX +XXX,XX @@ 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));
   //
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c
@@ -XXX,XX +XXX,XX @@ Returns:
   IPMI_COMMAND                *IpmiCommand;
   IPMI_RESPONSE               *IpmiResponse;
   UINT8                       Index;
+  UINT8                       TempData[MAX_TEMP_DATA];
 
   IpmiInstance = INSTANCE_FROM_PEI_SM_IPMI_BMC_THIS (This);
 
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h
@@ -XXX,XX +XXX,XX @@ typedef struct {
   UINTN                  Signature;
   UINT64                 KcsTimeoutPeriod;
   UINT8                  SlaveAddress;
-  UINT8                  TempData[MAX_TEMP_DATA];
   BMC_STATUS             BmcStatus;
   UINT64                 ErrorStatus;
   UINT8                  SoftErrorCount;
-- 
2.35.3

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100526): https://edk2.groups.io/g/devel/message/100526
Mute This Topic: https://groups.io/mt/97279449/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 XXXXXXX..XXXXXXX 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
@@ -XXX,XX +XXX,XX @@ Returns:
   BOOLEAN     bResultFlag = FALSE;
   UINT8       TempData[MAX_TEMP_DATA];
 
+  IPMI_SELF_TEST_RESULT_RESPONSE *pSelfTestResult;
+
   //
   // Get the SELF TEST Results.
   //
@@ -XXX,XX +XXX,XX @@ Returns:
 
   DataSize = sizeof (TempData);
 
-  TempData[1] = 0;
+  pSelfTestResult = (IPMI_SELF_TEST_RESULT_RESPONSE*)&TempData[0];
+  pSelfTestResult->CompletionCode = 0;
 
   do {
     Status = IpmiSendCommand (
@@ -XXX,XX +XXX,XX @@ 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:
@@ -XXX,XX +XXX,XX @@ 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.
@@ -XXX,XX +XXX,XX @@ 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;
@@ -XXX,XX +XXX,XX @@ 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;
@@ -XXX,XX +XXX,XX @@ 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.

v2:
  patch 1:
    added R-b
  patch 2:
    added changes to SmmGenericIpmi.c
  patch 3:
    renamed pSelfTestResult to SelfTestResult,
    fixed initialization of SelfTestResult->CompletionCode before
    IpmiSendCommand call.
   
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>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100881): https://edk2.groups.io/g/devel/message/100881
Mute This Topic: https://groups.io/mt/97485240/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.

Signed-off-by: Mike Maslenkin <mike.maslenkin@gmail.com>
Reviewed-by: Isaac Oram <Isaac.w.oram@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
---
 .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c               | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100882): https://edk2.groups.io/g/devel/message/100882
Mute This Topic: https://groups.io/mt/97485241/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
---
 .../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 -
 .../GenericIpmi/Smm/SmmGenericIpmi.c          |  5 ++-
 7 files changed, 36 insertions(+), 31 deletions(-)

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100883): https://edk2.groups.io/g/devel/message/100883
Mute This Topic: https://groups.io/mt/97485242/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Use predefined type while accessing IPMI command returned data
instead of raw byte array.

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>
---
 .../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 XXXXXXX..XXXXXXX 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
@@ -XXX,XX +XXX,XX @@ Returns:
   BOOLEAN     bResultFlag = FALSE;
   UINT8       TempData[MAX_TEMP_DATA];
 
+  IPMI_SELF_TEST_RESULT_RESPONSE *SelfTestResult;
+
   //
   // Get the SELF TEST Results.
   //
@@ -XXX,XX +XXX,XX @@ Returns:
 
   DataSize = sizeof (TempData);
 
-  TempData[1] = 0;
+  SelfTestResult = (IPMI_SELF_TEST_RESULT_RESPONSE *) &TempData[0];
+  SelfTestResult->Result = 0;
 
   do {
     Status = IpmiSendCommand (
@@ -XXX,XX +XXX,XX @@ Returns:
                &DataSize
                );
     if (Status == EFI_SUCCESS) {
-      switch (TempData[1]) {
+      switch (SelfTestResult->Result) {
         case IPMI_APP_SELFTEST_NO_ERROR:
         case IPMI_APP_SELFTEST_NOT_IMPLEMENTED:
         case IPMI_APP_SELFTEST_ERROR:
@@ -XXX,XX +XXX,XX @@ 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", SelfTestResult->Result, SelfTestResult->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.
@@ -XXX,XX +XXX,XX @@ Returns:
     // Check the IPMI defined self test results.
     // Additional Cases are device specific test results.
     //
-    switch (TempData[1]) {
+    switch (SelfTestResult->Result) {
       case IPMI_APP_SELFTEST_NO_ERROR:
       case IPMI_APP_SELFTEST_NOT_IMPLEMENTED:
         IpmiInstance->BmcStatus = BMC_OK;
@@ -XXX,XX +XXX,XX @@ 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 ((SelfTestResult->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;
@@ -XXX,XX +XXX,XX @@ Returns:
         //
         // Check if SDR repository is empty and report it if it is.
         //
-        if ((TempData[2] & IPMI_APP_SELFTEST_SDR_REPOSITORY_EMPTY) != 0) {
+        if ((SelfTestResult->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 (#100884): https://edk2.groups.io/g/devel/message/100884
Mute This Topic: https://groups.io/mt/97485243/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-