[edk2-devel] [PATCH edk2-platforms 00/10] MTCP-over-KCS support

[edk2-devel] [PATCH edk2-platforms 00/10] MTCP-over-KCS support

[Konstantin Aladyshev]

The Manageability KCS transport library needs to support requests both
from MCTP and IPMI transports. Currently the code only handles IPMI
case correctly.
In the MCTP case the communication should be based on the MCTP-over-KCS
specification (DSP0254). This specification defines a special KCS
binding header and trailer structures that need to be present in every
MCTP message.
The header structure contains a length field, therefore response packet
size is not needed to be known beforehand.
The trailer structure contains a PEC checksum that can be used to check
itegrity of the response message.
Modify Manageability KCS transport library code to check which message
is processed (IPMI or MCTP) and handle each case correctly based on its
own specification.
This patch is a result of a joint effort from the Konstantin Aladyshev
<aladyshev22@gmail.com> and Abner Chang <abner.chang@amd.com>.

Tested:
PLDM communication between the HOST and BMC was tested with both
components implemented via open-source software:
- The HOST (UEFI firmware) part was based one the edk2 [1] and
edk2-platforms [2] code,
- The BMC part was based on the openbmc [3] distribution.

The testing process and all the necessary utilities are described in
the [4] repository.

The provided changes keep IPMI over KCS stack working as reported by
Abner Chang.

[1]: https://github.com/tianocore/edk2
[2]: https://github.com/tianocore/edk2-platforms
[3]: https://github.com/openbmc/openbmc
[4]: https://github.com/Kostr/PLDM

Konstantin Aladyshev (10):
  ManageabilityPkg: Add definition for the MCTP KCS TRAILER structure
  ManageabilityPkg: Check MCTP EIDs for reserved values
  ManageabilityPkg: Support both MCTP and IPMI in KCS tranport library
  ManageabilityPkg: Check header fields in the MCTP response
  ManageabilityPkg: Correct typo in MCTP destination EID field
  ManageabilityPkg: Update the algorithm of using MCTP endpoint ID PCD
  ManageabilityPkg: Correct value for the MCTP TAG_OWNER response bit
  ManageabilityPkg: Don't check MCTP header fields if transfer has
    failed
  ManageabilityPkg: Use correct constants for PLDM header checks
  ManageabilityPkg: Return error on multiple-packet MCTP responses

 .../Library/ManageabilityTransportMctpLib.h   |   9 +-
 .../Include/Protocol/MctpProtocol.h           |  12 +-
 .../Common/KcsCommon.c                        | 284 +++++++++++++++---
 .../Dxe/ManageabilityTransportMctp.c          |   4 +-
 .../MctpProtocol/Common/MctpProtocolCommon.c  | 129 +++++++-
 .../Universal/MctpProtocol/Dxe/MctpProtocol.c |  51 +++-
 .../PldmProtocol/Common/PldmProtocolCommon.c  |  10 +-
 7 files changed, 430 insertions(+), 69 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109633): https://edk2.groups.io/g/devel/message/109633
Mute This Topic: https://groups.io/mt/101994937/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms 01/10] ManageabilityPkg: Add definition for the MCTP KCS TRAILER structure

[Konstantin Aladyshev]

Currently there is only a definition for the MCTP KCS HEADER structure.
Add definition for the MCTP KCS TRAILER structure as well.

Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
Signed-off-by: Abner Chang <abner.chang@amd.com>
---
 .../Library/ManageabilityTransportMctpLib.h        |  5 +++++
 .../MctpProtocol/Common/MctpProtocolCommon.c       | 14 +++++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportMctpLib.h b/Features/ManageabilityPkg/Include/Library/ManageabilityTransportMctpLib.h
index 43bd142f4c..462e7436e6 100644
--- a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportMctpLib.h
+++ b/Features/ManageabilityPkg/Include/Library/ManageabilityTransportMctpLib.h
@@ -39,6 +39,11 @@ typedef struct {
   UINT8    DefiningBody; ///< Message type.
   UINT8    ByteCount;    ///< Byte count of payload.
 } MANAGEABILITY_MCTP_KCS_HEADER;
+
+typedef struct {
+  UINT8    Pec;  ///< MCTP over KCS Packet Error Code.
+} MANAGEABILITY_MCTP_KCS_TRAILER;
+
 #define MCTP_KCS_NETFN_LUN                       0xb0
 #define DEFINING_BODY_DMTF_PRE_OS_WORKING_GROUP  0x01
 
diff --git a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
index 1ad48efdc7..7576007f77 100644
--- a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
+++ b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
@@ -132,7 +132,7 @@ SetupMctpRequestTransportPacket (
   MANAGEABILITY_MCTP_KCS_HEADER  *MctpKcsHeader;
   MCTP_TRANSPORT_HEADER          *MctpTransportHeader;
   MCTP_MESSAGE_HEADER            *MctpMessageHeader;
-  UINT8                          *Pec;
+  MANAGEABILITY_MCTP_KCS_TRAILER *MctpKcsTrailer;
   UINT8                          *ThisPackage;
 
   if ((PacketHeader == NULL) || (PacketHeaderSize == NULL) ||
@@ -151,8 +151,8 @@ SetupMctpRequestTransportPacket (
       return EFI_OUT_OF_RESOURCES;
     }
 
-    Pec = (UINT8 *)AllocateZeroPool (sizeof (UINT8));
-    if (Pec == NULL) {
+    MctpKcsTrailer = (MANAGEABILITY_MCTP_KCS_TRAILER *)AllocateZeroPool (sizeof (MANAGEABILITY_MCTP_KCS_TRAILER));
+    if (MctpKcsTrailer == NULL) {
       DEBUG ((DEBUG_ERROR, "%a: Not enough resource for PEC.\n", __func__));
       FreePool (MctpKcsHeader);
       return EFI_OUT_OF_RESOURCES;
@@ -167,7 +167,7 @@ SetupMctpRequestTransportPacket (
     if (ThisPackage == NULL) {
       DEBUG ((DEBUG_ERROR, "%a: Not enough resource for package.\n", __func__));
       FreePool (MctpKcsHeader);
-      FreePool (Pec);
+      FreePool (MctpKcsTrailer);
       return EFI_OUT_OF_RESOURCES;
     }
 
@@ -193,14 +193,14 @@ SetupMctpRequestTransportPacket (
 
     //
     // Generate PEC follow SMBUS 2.0 specification.
-    *Pec = HelperManageabilityGenerateCrc8 (MCTP_KCS_PACKET_ERROR_CODE_POLY, 0, ThisPackage, MctpKcsHeader->ByteCount);
+    MctpKcsTrailer->Pec = HelperManageabilityGenerateCrc8 (MCTP_KCS_PACKET_ERROR_CODE_POLY, 0, ThisPackage, MctpKcsHeader->ByteCount);
 
     *PacketBody        = (UINT8 *)ThisPackage;
     *PacketBodySize    = MctpKcsHeader->ByteCount;
-    *PacketTrailer     = (MANAGEABILITY_TRANSPORT_TRAILER)Pec;
+    *PacketTrailer     = (MANAGEABILITY_TRANSPORT_TRAILER)MctpKcsTrailer;
     *PacketHeader      = (MANAGEABILITY_TRANSPORT_HEADER)MctpKcsHeader;
     *PacketHeaderSize  = sizeof (MANAGEABILITY_MCTP_KCS_HEADER);
-    *PacketTrailerSize = 1;
+    *PacketTrailerSize = sizeof (MANAGEABILITY_MCTP_KCS_TRAILER);
     return EFI_SUCCESS;
   } else {
     DEBUG ((DEBUG_ERROR, "%a: No implementation of building up packet.", __func__));
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109634): https://edk2.groups.io/g/devel/message/109634
Mute This Topic: https://groups.io/mt/101994938/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms 02/10] ManageabilityPkg: Check MCTP EIDs for reserved values

[Konstantin Aladyshev]

MTCP base specification marks EIDs 1-7 as reserved. Therefore return
EFI_INVALID_PARAMETER if such EIDs were provided to the
MctpSubmitMessage function.

Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
Signed-off-by: Abner Chang <abner.chang@amd.com>
---
 .../Universal/MctpProtocol/Dxe/MctpProtocol.c   | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Features/ManageabilityPkg/Universal/MctpProtocol/Dxe/MctpProtocol.c b/Features/ManageabilityPkg/Universal/MctpProtocol/Dxe/MctpProtocol.c
index 88bfd9b7e7..d0f49a1abb 100644
--- a/Features/ManageabilityPkg/Universal/MctpProtocol/Dxe/MctpProtocol.c
+++ b/Features/ManageabilityPkg/Universal/MctpProtocol/Dxe/MctpProtocol.c
@@ -78,6 +78,23 @@ MctpSubmitMessage (
     return EFI_INVALID_PARAMETER;
   }
 
+  //
+  // Check source EID and destination EID
+  //
+  if ((MctpSourceEndpointId >= MCTP_RESERVED_ENDPOINT_START_ID) &&
+      (MctpSourceEndpointId <= MCTP_RESERVED_ENDPOINT_END_ID)
+      ) {
+    DEBUG ((DEBUG_ERROR, "%a: The value of MCTP source EID (%x) is reserved.\n", __func__, MctpSourceEndpointId));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if ((MctpDestinationEndpointId >= MCTP_RESERVED_ENDPOINT_START_ID) &&
+      (MctpDestinationEndpointId <= MCTP_RESERVED_ENDPOINT_END_ID)
+      ) {
+    DEBUG ((DEBUG_ERROR, "%a: The value of MCTP destination EID (%x) is reserved.\n", __func__, MctpDestinationEndpointId));
+    return EFI_INVALID_PARAMETER;
+  }
+
   Status = CommonMctpSubmitMessage (
              mTransportToken,
              MctpType,
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109635): https://edk2.groups.io/g/devel/message/109635
Mute This Topic: https://groups.io/mt/101994939/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms 03/10] ManageabilityPkg: Support both MCTP and IPMI in KCS tranport library

[Konstantin Aladyshev]

The Manageability KCS transport library needs to support requests both
from MCTP and IPMI transports. Currently the code only handles IPMI
case correctly.
In the MCTP case the communication should be based on the MCTP-over-KCS
specification (DSP0254). This specification defines a special KCS
binding header and trailer structures that need to be present in every
MCTP message.
The header structure contains a length field, therefore response packet
size is not needed to be known beforehand.
The trailer structure contains a PEC checksum that can be used to check
itegrity of the response message.
Modify Manageability KCS transport library code to check which message
is processed (IPMI or MCTP) and handle each case correctly based on its
own specification.

Tested:
- The IPMI KCS communication is tested by Abner Chang,
- The MCTP KCS communication is tested by Konstantin Aladyshev on the
AMD EthanolX CRB.

Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
Signed-off-by: Abner Chang <abner.chang@amd.com>
---
 .../Common/KcsCommon.c                        | 284 +++++++++++++++---
 .../MctpProtocol/Common/MctpProtocolCommon.c  |  14 +-
 2 files changed, 260 insertions(+), 38 deletions(-)

diff --git a/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Common/KcsCommon.c b/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Common/KcsCommon.c
index d5b54c04be..4f7e7d450f 100644
--- a/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Common/KcsCommon.c
+++ b/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Common/KcsCommon.c
@@ -8,16 +8,19 @@
 **/
 #include <Uefi.h>
 #include <IndustryStandard/IpmiKcs.h>
+#include <IndustryStandard/Mctp.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/IoLib.h>
 #include <Library/DebugLib.h>
 #include <Library/ManageabilityTransportHelperLib.h>
+#include <Library/ManageabilityTransportMctpLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/TimerLib.h>
 
 #include "ManageabilityTransportKcs.h"
 
 extern MANAGEABILITY_TRANSPORT_KCS_HARDWARE_INFO  mKcsHardwareInfo;
+extern MANAGEABILITY_TRANSPORT_KCS                *mSingleSessionToken;
 
 /**
   This function waits for parameter Flag to set.
@@ -379,6 +382,218 @@ KcsTransportRead (
   return EFI_SUCCESS;
 }
 
+/**
+  This funciton checks the KCS response data according to
+  manageability protocol.
+
+  @param[in]      ResponseData        Pointer to response data.
+  @param[in]      ResponseDataSize    Size of response data.
+  @param[out]     AdditionalStatus    Pointer to receive the additional status.
+
+  @retval         EFI_SUCCESS         KCS response header is checked without error
+  @retval         EFI_DEVICE_ERROR    KCS response header has problem.
+**/
+EFI_STATUS
+KcsCheckResponseData (
+  IN UINT8                                       *ResponseData,
+  IN UINT32                                      ResponseDataSize,
+  OUT MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS  *AdditionalStatus
+  )
+{
+  EFI_STATUS                      Status;
+  MANAGEABILITY_MCTP_KCS_TRAILER  MctpKcsPec;
+  UINT32                          PecSize;
+  UINT8                           CalculatedPec;
+  CHAR16                          *CompletionCodeStr;
+
+  Status            = EFI_SUCCESS;
+  *AdditionalStatus = MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS_NO_ERRORS;
+  if (CompareGuid (&gManageabilityProtocolMctpGuid, mSingleSessionToken->Token.ManageabilityProtocolSpecification)) {
+    //
+    // For MCTP over KCS, check PEC
+    //
+    PecSize = sizeof (MANAGEABILITY_MCTP_KCS_TRAILER) + 1;  // +1 to read last dummy byte that finishes KCS transfer
+    Status  = KcsTransportRead (&MctpKcsPec.Pec, &PecSize);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: Error! Failed to read PEC with Status(%r)\n",
+        __func__,
+        Status
+        ));
+      *AdditionalStatus = MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS_ERROR;
+      return Status;
+    }
+
+    if (PecSize != sizeof (MctpKcsPec)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: Error! Received PEC size is %d instead of %d\n",
+        __func__,
+        PecSize,
+        sizeof (MctpKcsPec)
+        ));
+      *AdditionalStatus = MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS_ERROR;
+      return EFI_DEVICE_ERROR;
+    }
+
+    HelperManageabilityDebugPrint ((VOID *)&MctpKcsPec.Pec, PecSize - 1, "MCTP over KCS Response PEC:\n");
+    CalculatedPec = HelperManageabilityGenerateCrc8 (MCTP_KCS_PACKET_ERROR_CODE_POLY, 0, ResponseData, ResponseDataSize);
+    if (CalculatedPec != MctpKcsPec.Pec) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: Error! Received PEC is 0x%02x instead of 0x%02x\n",
+        __func__,
+        MctpKcsPec.Pec,
+        CalculatedPec
+        ));
+      Status = EFI_DEVICE_ERROR;
+    }
+  } else if (CompareGuid (&gManageabilityProtocolIpmiGuid, mSingleSessionToken->Token.ManageabilityProtocolSpecification)) {
+    //
+    // For IPMI over KCS
+    // Check and print Completion Code
+    //
+    Status = IpmiHelperCheckCompletionCode (*ResponseData, &CompletionCodeStr, AdditionalStatus);
+    if (!EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_MANAGEABILITY_INFO, "Cc: %02x %s.\n", *((UINT8 *)ResponseData), CompletionCodeStr));
+    } else if (Status == EFI_NOT_FOUND) {
+      DEBUG ((DEBUG_ERROR, "Cc: %02x not defined in IpmiCompletionCodeMapping or invalid.\n", *((UINT8 *)ResponseData)));
+    }
+  }
+
+  return Status;
+}
+
+/**
+  This funciton reads the KCS response header according to
+  manageability protocol. Caller has to free the memory
+  allocated for response header.
+
+  @param[in]      ResponseHeader         Pointer to receive the response header.
+  @param[out]     AdditionalStatus       Pointer to receive the additional status.
+
+  @retval         EFI_SUCCESS            KCS response header is checked and returned
+                                         to caller.
+  @retval         EFI_INVALID_PARAMETER  One of the given parameter is incorrect.
+  @retval         EFI_OUT_OF_RESOURCE    Memory allocation is failed for ResponseHeader.
+  @retval         EFI_DEVICE_ERROR       Incorrect response header.
+**/
+EFI_STATUS
+KcsReadResponseHeader (
+  IN   UINT8                                      **ResponseHeader,
+  OUT  MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS  *AdditionalStatus
+  )
+{
+  EFI_STATUS  Status;
+  UINT32      RspHeaderSize;
+  UINT32      ExpectedHeaderSize;
+  UINT8       *RspHeader;
+
+  if ((ResponseHeader == NULL) || (AdditionalStatus == NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  *ResponseHeader = NULL;
+  if (CompareGuid (&gManageabilityProtocolMctpGuid, mSingleSessionToken->Token.ManageabilityProtocolSpecification)) {
+
+    // For MCTP over KCS
+    ExpectedHeaderSize = sizeof (MANAGEABILITY_MCTP_KCS_HEADER);
+    DEBUG ((
+      DEBUG_MANAGEABILITY_INFO,
+      "%a: Reading MCTP over KCS response header.\n",
+      __func__
+      ));
+  } else if (CompareGuid (&gManageabilityProtocolIpmiGuid, mSingleSessionToken->Token.ManageabilityProtocolSpecification)) {
+    // For IPMI over KCS
+
+    ExpectedHeaderSize = sizeof (IPMI_KCS_RESPONSE_HEADER);
+    DEBUG ((
+      DEBUG_MANAGEABILITY_INFO,
+      "%a: Reading IPMI over KCS response header.\n",
+      __func__
+      ));
+  } else {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Error! Unsupportted manageability protocol over KCS: %g.\n",
+      __func__,
+      mSingleSessionToken->Token.ManageabilityProtocolSpecification
+      ));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  RspHeader = (UINT8 *)AllocateZeroPool (ExpectedHeaderSize);
+  if (RspHeader == NULL) {
+    DEBUG ((DEBUG_ERROR, "Memory allocation failed for KCS response header!\n"));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  RspHeaderSize = ExpectedHeaderSize;
+  Status        = KcsTransportRead (RspHeader, &RspHeaderSize);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Error! Failed to read KCS response header Status(%r)\n",
+      __func__,
+      Status
+      ));
+    FreePool (RspHeader);
+    *AdditionalStatus = MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS_ERROR;
+    return Status;
+  }
+
+  if (RspHeaderSize != 0) {
+    HelperManageabilityDebugPrint ((VOID *)RspHeader, RspHeaderSize, "KCS Response Header:\n");
+  }
+
+  if (ExpectedHeaderSize != RspHeaderSize) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "The size (%d bytes) of returned resposne header is not the same as expection (%d bytes)!\n",
+      RspHeaderSize,
+      ExpectedHeaderSize
+      ));
+    FreePool (RspHeader);
+    return EFI_DEVICE_ERROR;
+  }
+
+  if (CompareGuid (&gManageabilityProtocolMctpGuid, mSingleSessionToken->Token.ManageabilityProtocolSpecification)) {
+    //
+    // MCTP over KCS
+    //
+    if (((MANAGEABILITY_MCTP_KCS_HEADER *)RspHeader)->NetFunc != MCTP_KCS_NETFN_LUN) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: Error! MANAGEABILITY_MCTP_KCS_HEADER.NetFunc is equal 0x%02x instead of 0x%02x\n",
+        __func__,
+        ((MANAGEABILITY_MCTP_KCS_HEADER *)RspHeader)->NetFunc,
+        MCTP_KCS_NETFN_LUN
+        ));
+      FreePool (RspHeader);
+      *AdditionalStatus = MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS_ERROR;
+      return EFI_DEVICE_ERROR;
+    }
+
+    if (((MANAGEABILITY_MCTP_KCS_HEADER *)RspHeader)->DefiningBody != DEFINING_BODY_DMTF_PRE_OS_WORKING_GROUP) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: Error! MANAGEABILITY_MCTP_KCS_HEADER.DefiningBody is equal 0x%02x instead of 0x%02x\n",
+        __func__,
+        ((MANAGEABILITY_MCTP_KCS_HEADER *)RspHeader)->DefiningBody,
+        DEFINING_BODY_DMTF_PRE_OS_WORKING_GROUP
+        ));
+      FreePool (RspHeader);
+      *AdditionalStatus = MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS_ERROR;
+      return EFI_DEVICE_ERROR;
+    }
+  }
+
+  *ResponseHeader   = RspHeader;
+  *AdditionalStatus = MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS_NO_ERRORS;
+  return EFI_SUCCESS;
+}
+
 /**
   This service communicates with BMC using KCS protocol.
 
@@ -423,11 +638,9 @@ KcsTransportSendCommand (
   OUT  MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS  *AdditionalStatus
   )
 {
-  EFI_STATUS                Status;
-  UINT32                    RspHeaderSize;
-  IPMI_KCS_RESPONSE_HEADER  RspHeader;
-  UINT32                    ExpectedResponseDataSize;
-  CHAR16                    *CompletionCodeStr;
+  EFI_STATUS  Status;
+  UINT8       *RspHeader;
+  UINT32      ExpectedResponseDataSize;
 
   if ((RequestData != NULL) && (RequestDataSize == 0)) {
     DEBUG ((DEBUG_ERROR, "%a: Mismatched values of RequestData and RequestDataSize\n", __func__));
@@ -467,56 +680,59 @@ KcsTransportSendCommand (
                RequestDataSize
                );
     if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_ERROR, "KCS Write Failed with Status(%r)", Status));
+      DEBUG ((DEBUG_ERROR, "KCS Write Failed with Status(%r)\n", Status));
       return Status;
     }
+  }
 
+  if ((ResponseData != NULL) && (ResponseDataSize != NULL) && (*ResponseDataSize != 0)) {
     //
     // Read the response header
-    RspHeaderSize = sizeof (IPMI_KCS_RESPONSE_HEADER);
-    Status        = KcsTransportRead ((UINT8 *)&RspHeader, &RspHeaderSize);
+    //
+    Status = KcsReadResponseHeader (&RspHeader, AdditionalStatus);
     if (EFI_ERROR (Status)) {
-      DEBUG ((
-        DEBUG_ERROR,
-        "KCS read response header failed Status(%r), " \
-        "RspNetFunctionLun = 0x%x, " \
-        "Comamnd = 0x%x \n",
-        Status,
-        RspHeader.NetFunc,
-        RspHeader.Command
-        ));
       return (Status);
     }
 
     //
-    // Print out the response payloads.
-    HelperManageabilityDebugPrint ((VOID *)&RspHeader, RspHeaderSize, "KCS Response Header:\n");
-  }
+    // Override ResposeDataSize if the manageability protocol is MCTP.
+    //
+    if (CompareGuid (&gManageabilityProtocolMctpGuid, mSingleSessionToken->Token.ManageabilityProtocolSpecification)) {
+      if (*ResponseDataSize < ((MANAGEABILITY_MCTP_KCS_HEADER *)RspHeader)->ByteCount) {
+        DEBUG ((
+          DEBUG_ERROR,
+          "%a: Error! MANAGEABILITY_MCTP_KCS_HEADER.ByteCount (0x%02x) is bigger than provided buffer (0x%02x)\n",
+          __func__,
+          ((MANAGEABILITY_MCTP_KCS_HEADER *)RspHeader)->ByteCount,
+          *ResponseDataSize
+          ));
+        return EFI_INVALID_PARAMETER;
+      }
+
+      *ResponseDataSize = ((MANAGEABILITY_MCTP_KCS_HEADER *)RspHeader)->ByteCount;
+    }
+    FreePool (RspHeader);
 
-  if ((ResponseData != NULL) && (ResponseDataSize != NULL) && (*ResponseDataSize != 0)) {
     ExpectedResponseDataSize = *ResponseDataSize;
-    Status                   = KcsTransportRead ((UINT8 *)ResponseData, ResponseDataSize);
+    Status                   = KcsTransportRead (ResponseData, ResponseDataSize);
     if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_ERROR, "KCS response read Failed with Status(%r)", Status));
+      DEBUG ((DEBUG_ERROR, "KCS response read Failed with Status(%r)\n", Status));
     }
 
-    //
     // Print out the response payloads.
     if (*ResponseDataSize != 0) {
       if (ExpectedResponseDataSize != *ResponseDataSize) {
-        DEBUG ((DEBUG_ERROR, "Expected KCS response size : %d is not matched to returned size : %d.\n", ExpectedResponseDataSize, *ResponseDataSize));
-        Status = EFI_DEVICE_ERROR;
+        DEBUG ((
+          DEBUG_ERROR,
+          "Expected KCS response size : %d is not matched to returned size : %d.\n",
+          ExpectedResponseDataSize,
+          *ResponseDataSize
+          ));
+        return EFI_DEVICE_ERROR;
       }
 
       HelperManageabilityDebugPrint ((VOID *)ResponseData, (UINT32)*ResponseDataSize, "KCS Response Data:\n");
-
-      // Print Completion Code
-      Status = IpmiHelperCheckCompletionCode (*((UINT8 *)ResponseData), &CompletionCodeStr, AdditionalStatus);
-      if (!EFI_ERROR (Status)) {
-        DEBUG ((DEBUG_MANAGEABILITY_INFO, "Cc: %02x %s.\n", *((UINT8 *)ResponseData), CompletionCodeStr));
-      } else if (Status == EFI_NOT_FOUND) {
-        DEBUG ((DEBUG_MANAGEABILITY_INFO, "Cc: %02x not defined in IpmiCompletionCodeMapping or invalid.\n", *((UINT8 *)ResponseData)));
-      }
+      Status = KcsCheckResponseData (ResponseData, *ResponseDataSize, AdditionalStatus);
     } else {
       DEBUG ((DEBUG_ERROR, "No response, can't determine Completion Code.\n"));
     }
diff --git a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
index 7576007f77..e560c638d5 100644
--- a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
+++ b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
@@ -267,6 +267,9 @@ CommonMctpSubmitMessage (
   MANAGEABILITY_TRANSPORT_TRAILER            MctpTransportTrailer;
   MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES  *MultiPackages;
   MANAGEABILITY_TRANSMISSION_PACKAGE_ATTR    *ThisPackage;
+  UINT8                                      *ResponseBuffer;
+  MCTP_TRANSPORT_HEADER                      *MctpTransportResponseHeader;
+  MCTP_MESSAGE_HEADER                        *MctpMessageResponseHeader;
 
   if (TransportToken == NULL) {
     DEBUG ((DEBUG_ERROR, "%a: No transport toke for MCTP\n", __func__));
@@ -435,11 +438,12 @@ CommonMctpSubmitMessage (
     ThisPackage++;
   }
 
+  ResponseBuffer = (UINT8 *)AllocatePool (*ResponseDataSize + sizeof (MCTP_TRANSPORT_HEADER) + sizeof (MCTP_MESSAGE_HEADER));
   // Receive packet.
   TransferToken.TransmitPackage.TransmitPayload             = NULL;
   TransferToken.TransmitPackage.TransmitSizeInByte          = 0;
-  TransferToken.ReceivePackage.ReceiveBuffer                = ResponseData;
-  TransferToken.ReceivePackage.ReceiveSizeInByte            = *ResponseDataSize;
+  TransferToken.ReceivePackage.ReceiveBuffer                = ResponseBuffer;
+  TransferToken.ReceivePackage.ReceiveSizeInByte            = *ResponseDataSize + sizeof (MCTP_TRANSPORT_HEADER) + sizeof (MCTP_MESSAGE_HEADER);
   TransferToken.TransmitHeader                              = NULL;
   TransferToken.TransmitHeaderSize                          = 0;
   TransferToken.TransmitTrailer                             = NULL;
@@ -461,8 +465,10 @@ CommonMctpSubmitMessage (
   // Return transfer status.
   //
   *AdditionalTransferError = TransferToken.TransportAdditionalStatus;
-  *ResponseDataSize        = TransferToken.ReceivePackage.ReceiveSizeInByte;
-  Status                   = TransferToken.TransferStatus;
+  *ResponseDataSize        = TransferToken.ReceivePackage.ReceiveSizeInByte - sizeof (MCTP_TRANSPORT_HEADER) - sizeof (MCTP_MESSAGE_HEADER);
+  CopyMem (ResponseData, ResponseBuffer + sizeof (MCTP_TRANSPORT_HEADER) + sizeof (MCTP_MESSAGE_HEADER), *ResponseDataSize);
+  FreePool (ResponseBuffer);
+  Status = TransferToken.TransferStatus;
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "%a: Failed to send MCTP command over %s: %r\n", __func__, mTransportName, Status));
     return Status;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109636): https://edk2.groups.io/g/devel/message/109636
Mute This Topic: https://groups.io/mt/101994941/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms 04/10] ManageabilityPkg: Check header fields in the MCTP response

[Konstantin Aladyshev]

Add checks for the MCTP header fields in the MCTP response.

Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
---
 .../MctpProtocol/Common/MctpProtocolCommon.c  | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
index e560c638d5..5844d54eb2 100644
--- a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
+++ b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
@@ -461,6 +461,88 @@ CommonMctpSubmitMessage (
                                                     &TransferToken
                                                     );
 
+  MctpTransportResponseHeader = (MCTP_TRANSPORT_HEADER *)ResponseBuffer;
+  if (MctpTransportResponseHeader->Bits.HeaderVersion != MCTP_KCS_HEADER_VERSION) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Error! Response HeaderVersion (0x%02x) doesn't match MCTP_KCS_HEADER_VERSION (0x%02x)\n",
+      __func__,
+      MctpTransportResponseHeader->Bits.HeaderVersion,
+      MCTP_KCS_HEADER_VERSION
+      ));
+    FreePool (ResponseBuffer);
+    return EFI_DEVICE_ERROR;
+  }
+  if (MctpTransportResponseHeader->Bits.MessageTag != MCTP_MESSAGE_TAG) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Error! Response MessageTag (0x%02x) doesn't match MCTP_MESSAGE_TAG (0x%02x)\n",
+      __func__,
+      MctpTransportResponseHeader->Bits.MessageTag,
+      MCTP_MESSAGE_TAG
+      ));
+    FreePool (ResponseBuffer);
+    return EFI_DEVICE_ERROR;
+  }
+  if (MctpTransportResponseHeader->Bits.TagOwner != MCTP_MESSAGE_TAG_OWNER_RESPONSE) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Error! Response TagOwner (0x%02x) doesn't match MCTP_MESSAGE_TAG_OWNER_RESPONSE (0x%02x)\n",
+      __func__,
+      MctpTransportResponseHeader->Bits.TagOwner,
+      MCTP_MESSAGE_TAG_OWNER_RESPONSE
+      ));
+    FreePool (ResponseBuffer);
+    return EFI_DEVICE_ERROR;
+  }
+  if (MctpTransportResponseHeader->Bits.SourceEndpointId != MctpDestinationEndpointId) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Error! Response SrcEID (0x%02x) doesn't match sent EID (0x%02x)\n",
+      __func__,
+      MctpTransportResponseHeader->Bits.SourceEndpointId,
+      MctpDestinationEndpointId
+      ));
+    FreePool (ResponseBuffer);
+    return EFI_DEVICE_ERROR;
+  }
+  if (MctpTransportResponseHeader->Bits.DestinationEndpointId != MctpSourceEndpointId) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Error! Response DestEID (0x%02x) doesn't match local EID (0x%02x)\n",
+      __func__,
+      MctpTransportResponseHeader->Bits.DestinationEndpointId,
+      MctpSourceEndpointId
+      ));
+    FreePool (ResponseBuffer);
+    return EFI_DEVICE_ERROR;
+  }
+
+  MctpMessageResponseHeader = (MCTP_MESSAGE_HEADER *)(MctpTransportResponseHeader + 1);
+  if (MctpMessageResponseHeader->Bits.MessageType != MctpType) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Error! Response MessageType (0x%02x) doesn't match sent MessageType (0x%02x)\n",
+      __func__,
+      MctpMessageResponseHeader->Bits.MessageType,
+      MctpType
+      ));
+    FreePool (ResponseBuffer);
+    return EFI_DEVICE_ERROR;
+  }
+
+  if (MctpMessageResponseHeader->Bits.IntegrityCheck != (UINT8)RequestDataIntegrityCheck) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Error! Response IntegrityCheck (%d) doesn't match sent IntegrityCheck (%d)\n",
+      __func__,
+      MctpMessageResponseHeader->Bits.IntegrityCheck,
+      (UINT8)RequestDataIntegrityCheck
+      ));
+    FreePool (ResponseBuffer);
+    return EFI_DEVICE_ERROR;
+  }
+
   //
   // Return transfer status.
   //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109637): https://edk2.groups.io/g/devel/message/109637
Mute This Topic: https://groups.io/mt/101994942/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms 05/10] ManageabilityPkg: Correct typo in MCTP destination EID field

[Konstantin Aladyshev]

Correct wrong structure member used for MCTP destination EID.

Signed-off-by: Abner Chang <abner.chang@amd.com>
Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
---
 .../Universal/PldmProtocol/Common/PldmProtocolCommon.c          | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Features/ManageabilityPkg/Universal/PldmProtocol/Common/PldmProtocolCommon.c b/Features/ManageabilityPkg/Universal/PldmProtocol/Common/PldmProtocolCommon.c
index ce1e2cba95..1c4506d87f 100644
--- a/Features/ManageabilityPkg/Universal/PldmProtocol/Common/PldmProtocolCommon.c
+++ b/Features/ManageabilityPkg/Universal/PldmProtocol/Common/PldmProtocolCommon.c
@@ -119,7 +119,7 @@ SetupPldmRequestTransportPacket (
     }
 
     MctpHeader->SourceEndpointId             = PcdGet8 (PcdMctpSourceEndpointId);
-    MctpHeader->SourceEndpointId             = PcdGet8 (PcdMctpDestinationEndpointId);
+    MctpHeader->DestinationEndpointId        = PcdGet8 (PcdMctpDestinationEndpointId);
     MctpHeader->MessageHeader.IntegrityCheck = FALSE;
     MctpHeader->MessageHeader.MessageType    = MCTP_MESSAGE_TYPE_PLDM;
     *PacketHeader                            = (MANAGEABILITY_TRANSPORT_HEADER *)MctpHeader;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109638): https://edk2.groups.io/g/devel/message/109638
Mute This Topic: https://groups.io/mt/101994943/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms 06/10] ManageabilityPkg: Update the algorithm of using MCTP endpoint ID PCD

[Konstantin Aladyshev]

Although MtcpSubmit function receives source and destination MCTP EID
arguments these value are not used in any way currently. Instead the
code always uses EID values from the PCDs.
To correct this issue modify function interface to receive source and
destination MCTP EIDs via pointers and use PCD values only if the
pointers are NULL.

Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
Signed-off-by: Abner Chang <abner.chang@amd.com>
---
 .../Include/Protocol/MctpProtocol.h           | 12 ++++--
 .../Dxe/ManageabilityTransportMctp.c          |  4 +-
 .../MctpProtocol/Common/MctpProtocolCommon.c  |  4 +-
 .../Universal/MctpProtocol/Dxe/MctpProtocol.c | 42 ++++++++++++++-----
 4 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/Features/ManageabilityPkg/Include/Protocol/MctpProtocol.h b/Features/ManageabilityPkg/Include/Protocol/MctpProtocol.h
index 85e42f157d..c96b986c44 100644
--- a/Features/ManageabilityPkg/Include/Protocol/MctpProtocol.h
+++ b/Features/ManageabilityPkg/Include/Protocol/MctpProtocol.h
@@ -28,8 +28,12 @@ typedef struct  _EDKII_MCTP_PROTOCOL EDKII_MCTP_PROTOCOL;
 
   @param[in]         This                       EDKII_MCTP_PROTOCOL instance.
   @param[in]         MctpType                   MCTP message type.
-  @param[in]         MctpSourceEndpointId       MCTP source endpoint ID.
-  @param[in]         MctpDestinationEndpointId  MCTP source endpoint ID.
+  @param[in]         MctpSourceEndpointId       Pointer of MCTP source endpoint ID.
+                                                Set to NULL means use platform PCD value
+                                                (PcdMctpSourceEndpointId).
+  @param[in]         MctpDestinationEndpointId  Pointer of MCTP destination endpoint ID.
+                                                Set to NULL means use platform PCD value
+                                                (PcdMctpDestinationEndpointId).
   @param[in]         RequestDataIntegrityCheck  Indicates whether MCTP message has
                                                 integrity check byte.
   @param[in]         RequestData                Message Data.
@@ -58,8 +62,8 @@ EFI_STATUS
 (EFIAPI *MCTP_SUBMIT_COMMAND)(
   IN     EDKII_MCTP_PROTOCOL  *This,
   IN     UINT8                MctpType,
-  IN     UINT8                MctpSourceEndpointId,
-  IN     UINT8                MctpDestinationEndpointId,
+  IN     UINT8                *MctpSourceEndpointId,
+  IN     UINT8                *MctpDestinationEndpointId,
   IN     BOOLEAN              RequestDataIntegrityCheck,
   IN     UINT8                *RequestData,
   IN     UINT32               RequestDataSize,
diff --git a/Features/ManageabilityPkg/Library/ManageabilityTransportMctpLib/Dxe/ManageabilityTransportMctp.c b/Features/ManageabilityPkg/Library/ManageabilityTransportMctpLib/Dxe/ManageabilityTransportMctp.c
index c520e2302d..249104c873 100644
--- a/Features/ManageabilityPkg/Library/ManageabilityTransportMctpLib/Dxe/ManageabilityTransportMctp.c
+++ b/Features/ManageabilityPkg/Library/ManageabilityTransportMctpLib/Dxe/ManageabilityTransportMctp.c
@@ -205,8 +205,8 @@ MctpTransportTransmitReceive (
   Status = mMctpProtocol->Functions.Version1_0->MctpSubmitCommand (
                                                   mMctpProtocol,
                                                   TransmitHeader->MessageHeader.MessageType,
-                                                  TransmitHeader->SourceEndpointId,
-                                                  TransmitHeader->DestinationEndpointId,
+                                                  &TransmitHeader->SourceEndpointId,
+                                                  &TransmitHeader->DestinationEndpointId,
                                                   (BOOLEAN)TransmitHeader->MessageHeader.IntegrityCheck,
                                                   TransferToken->TransmitPackage.TransmitPayload,
                                                   TransferToken->TransmitPackage.TransmitSizeInByte,
diff --git a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
index 5844d54eb2..3128aadd15 100644
--- a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
+++ b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
@@ -175,8 +175,8 @@ SetupMctpRequestTransportPacket (
     MctpTransportHeader                             = (MCTP_TRANSPORT_HEADER *)ThisPackage;
     MctpTransportHeader->Bits.Reserved              = 0;
     MctpTransportHeader->Bits.HeaderVersion         = MCTP_KCS_HEADER_VERSION;
-    MctpTransportHeader->Bits.DestinationEndpointId = PcdGet8 (PcdMctpDestinationEndpointId);
-    MctpTransportHeader->Bits.SourceEndpointIdId    = PcdGet8 (PcdMctpSourceEndpointId);
+    MctpTransportHeader->Bits.DestinationEndpointId = MctpDestinationEndpointId;
+    MctpTransportHeader->Bits.SourceEndpointId      = MctpSourceEndpointId;
     MctpTransportHeader->Bits.MessageTag            = MCTP_MESSAGE_TAG;
     MctpTransportHeader->Bits.TagOwner              = MCTP_MESSAGE_TAG_OWNER_REQUEST;
     MctpTransportHeader->Bits.PacketSequence        = mMctpPacketSequence & MCTP_PACKET_SEQUENCE_MASK;
diff --git a/Features/ManageabilityPkg/Universal/MctpProtocol/Dxe/MctpProtocol.c b/Features/ManageabilityPkg/Universal/MctpProtocol/Dxe/MctpProtocol.c
index d0f49a1abb..73445bf816 100644
--- a/Features/ManageabilityPkg/Universal/MctpProtocol/Dxe/MctpProtocol.c
+++ b/Features/ManageabilityPkg/Universal/MctpProtocol/Dxe/MctpProtocol.c
@@ -29,8 +29,12 @@ UINT32                         mTransportMaximumPayload;
 
   @param[in]         This                       EDKII_MCTP_PROTOCOL instance.
   @param[in]         MctpType                   MCTP message type.
-  @param[in]         MctpSourceEndpointId       MCTP source endpoint ID.
-  @param[in]         MctpDestinationEndpointId  MCTP source endpoint ID.
+  @param[in]         MctpSourceEndpointId       Pointer of MCTP source endpoint ID.
+                                                Set to NULL means use platform PCD value
+                                                (PcdMctpSourceEndpointId).
+  @param[in]         MctpDestinationEndpointId  Pointer of MCTP destination endpoint ID.
+                                                Set to NULL means use platform PCD value
+                                                (PcdMctpDestinationEndpointId).
   @param[in]         RequestDataIntegrityCheck  Indicates whether MCTP message has
                                                 integrity check byte.
   @param[in]         RequestData                Message Data.
@@ -59,8 +63,8 @@ EFIAPI
 MctpSubmitMessage (
   IN     EDKII_MCTP_PROTOCOL                        *This,
   IN     UINT8                                      MctpType,
-  IN     UINT8                                      MctpSourceEndpointId,
-  IN     UINT8                                      MctpDestinationEndpointId,
+  IN     UINT8                                      *MctpSourceEndpointId,
+  IN     UINT8                                      *MctpDestinationEndpointId,
   IN     BOOLEAN                                    RequestDataIntegrityCheck,
   IN     UINT8                                      *RequestData,
   IN     UINT32                                     RequestDataSize,
@@ -72,24 +76,42 @@ MctpSubmitMessage (
   )
 {
   EFI_STATUS  Status;
+  UINT8       SourceEid;
+  UINT8       DestinationEid;
 
   if ((RequestData == NULL) && (ResponseData == NULL)) {
     DEBUG ((DEBUG_ERROR, "%a: Both RequestData and ResponseData are NULL\n", __func__));
     return EFI_INVALID_PARAMETER;
   }
 
+  if (MctpSourceEndpointId == NULL) {
+    SourceEid = PcdGet8 (PcdMctpSourceEndpointId);
+    DEBUG ((DEBUG_MANAGEABILITY, "%a: Use PcdMctpSourceEndpointId for MCTP source EID: %x\n", __func__, SourceEid));
+  } else {
+    SourceEid = *MctpSourceEndpointId;
+    DEBUG ((DEBUG_MANAGEABILITY, "%a: MCTP source EID: %x\n", __func__, SourceEid));
+  }
+
+  if (MctpDestinationEndpointId == NULL) {
+    DestinationEid = PcdGet8 (PcdMctpDestinationEndpointId);
+    DEBUG ((DEBUG_MANAGEABILITY, "%a: Use PcdMctpDestinationEndpointId for MCTP destination EID: %x\n", __func__, DestinationEid));
+  } else {
+    DestinationEid = *MctpDestinationEndpointId;
+    DEBUG ((DEBUG_MANAGEABILITY, "%a: MCTP destination EID: %x\n", __func__, DestinationEid));
+  }
+
   //
   // Check source EID and destination EID
   //
-  if ((MctpSourceEndpointId >= MCTP_RESERVED_ENDPOINT_START_ID) &&
-      (MctpSourceEndpointId <= MCTP_RESERVED_ENDPOINT_END_ID)
+  if ((SourceEid >= MCTP_RESERVED_ENDPOINT_START_ID) &&
+      (SourceEid <= MCTP_RESERVED_ENDPOINT_END_ID)
       ) {
     DEBUG ((DEBUG_ERROR, "%a: The value of MCTP source EID (%x) is reserved.\n", __func__, MctpSourceEndpointId));
     return EFI_INVALID_PARAMETER;
   }
 
-  if ((MctpDestinationEndpointId >= MCTP_RESERVED_ENDPOINT_START_ID) &&
-      (MctpDestinationEndpointId <= MCTP_RESERVED_ENDPOINT_END_ID)
+  if ((DestinationEid >= MCTP_RESERVED_ENDPOINT_START_ID) &&
+      (DestinationEid <= MCTP_RESERVED_ENDPOINT_END_ID)
       ) {
     DEBUG ((DEBUG_ERROR, "%a: The value of MCTP destination EID (%x) is reserved.\n", __func__, MctpDestinationEndpointId));
     return EFI_INVALID_PARAMETER;
@@ -98,8 +120,8 @@ MctpSubmitMessage (
   Status = CommonMctpSubmitMessage (
              mTransportToken,
              MctpType,
-             MctpSourceEndpointId,
-             MctpDestinationEndpointId,
+             SourceEid,
+             DestinationEid,
              RequestDataIntegrityCheck,
              RequestData,
              RequestDataSize,
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109639): https://edk2.groups.io/g/devel/message/109639
Mute This Topic: https://groups.io/mt/101994944/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms 07/10] ManageabilityPkg: Correct value for the MCTP TAG_OWNER response bit

[Konstantin Aladyshev]

Currently the MCTP TAG_OWNER bit is checked against 1 both in MTCP
request and response.
According to the MTCP Base specification in case of the MCTP response
the TAG_OWNER bit should be equal to 0.
Correct MCTP_MESSAGE_TAG_OWNER_RESPONSE flag value to fix the issue.

Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
---
 .../Include/Library/ManageabilityTransportMctpLib.h           | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportMctpLib.h b/Features/ManageabilityPkg/Include/Library/ManageabilityTransportMctpLib.h
index 462e7436e6..a8dc8a8519 100644
--- a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportMctpLib.h
+++ b/Features/ManageabilityPkg/Include/Library/ManageabilityTransportMctpLib.h
@@ -51,8 +51,8 @@ typedef struct {
 // is not defined by the specification.
 #define MCTP_MESSAGE_TAG  0x1
 
-#define MCTP_MESSAGE_TAG_OWNER_REQUEST   0x01
-#define MCTP_MESSAGE_TAG_OWNER_RESPONSE  0x01
+#define MCTP_MESSAGE_TAG_OWNER_REQUEST   1
+#define MCTP_MESSAGE_TAG_OWNER_RESPONSE  0
 
 #define MCTP_PACKET_SEQUENCE_MASK  0x3
 
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109640): https://edk2.groups.io/g/devel/message/109640
Mute This Topic: https://groups.io/mt/101994945/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms 08/10] ManageabilityPkg: Don't check MCTP header fields if transfer has failed

[Konstantin Aladyshev]

If MCTP KCS communication has failed we need to abort MCTP transfer
function before checking any MCTP header data.

Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
---
 .../MctpProtocol/Common/MctpProtocolCommon.c     | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
index 3128aadd15..4aae4fcba9 100644
--- a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
+++ b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
@@ -461,6 +461,13 @@ CommonMctpSubmitMessage (
                                                     &TransferToken
                                                     );
 
+  *AdditionalTransferError = TransferToken.TransportAdditionalStatus;
+  Status = TransferToken.TransferStatus;
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "%a: Failed to send MCTP command over %s: %r\n", __func__, mTransportName, Status));
+    return Status;
+  }
+
   MctpTransportResponseHeader = (MCTP_TRANSPORT_HEADER *)ResponseBuffer;
   if (MctpTransportResponseHeader->Bits.HeaderVersion != MCTP_KCS_HEADER_VERSION) {
     DEBUG ((
@@ -543,18 +550,9 @@ CommonMctpSubmitMessage (
     return EFI_DEVICE_ERROR;
   }
 
-  //
-  // Return transfer status.
-  //
-  *AdditionalTransferError = TransferToken.TransportAdditionalStatus;
   *ResponseDataSize        = TransferToken.ReceivePackage.ReceiveSizeInByte - sizeof (MCTP_TRANSPORT_HEADER) - sizeof (MCTP_MESSAGE_HEADER);
   CopyMem (ResponseData, ResponseBuffer + sizeof (MCTP_TRANSPORT_HEADER) + sizeof (MCTP_MESSAGE_HEADER), *ResponseDataSize);
   FreePool (ResponseBuffer);
-  Status = TransferToken.TransferStatus;
-  if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "%a: Failed to send MCTP command over %s: %r\n", __func__, mTransportName, Status));
-    return Status;
-  }
 
   return Status;
 }
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109641): https://edk2.groups.io/g/devel/message/109641
Mute This Topic: https://groups.io/mt/101994946/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms 09/10] ManageabilityPkg: Use correct constants for PLDM header checks

[Konstantin Aladyshev]

Currently PldmProtocol code uses magic numbers in the PLDM header
checks. Since PLDM headers have all the necessary definitions replace
magic numbers with the appropriate defines.

Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
---
 .../Universal/PldmProtocol/Common/PldmProtocolCommon.c    | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Features/ManageabilityPkg/Universal/PldmProtocol/Common/PldmProtocolCommon.c b/Features/ManageabilityPkg/Universal/PldmProtocol/Common/PldmProtocolCommon.c
index 1c4506d87f..3deded541a 100644
--- a/Features/ManageabilityPkg/Universal/PldmProtocol/Common/PldmProtocolCommon.c
+++ b/Features/ManageabilityPkg/Universal/PldmProtocol/Common/PldmProtocolCommon.c
@@ -321,15 +321,15 @@ CommonPldmSubmitCommand (
   //
   // Check the integrity of response. data.
   ResponseHeader = (PLDM_RESPONSE_HEADER *)FullPacketResponseData;
-  if ((ResponseHeader->PldmHeader.DatagramBit != 0) ||
-      (ResponseHeader->PldmHeader.RequestBit != 0) ||
+  if ((ResponseHeader->PldmHeader.DatagramBit != PLDM_MESSAGE_HEADER_IS_DATAGRAM) ||
+      (ResponseHeader->PldmHeader.RequestBit != PLDM_MESSAGE_HEADER_IS_RESPONSE) ||
       (ResponseHeader->PldmHeader.InstanceId != mPldmRequestInstanceId) ||
       (ResponseHeader->PldmHeader.PldmType != PldmType) ||
       (ResponseHeader->PldmHeader.PldmTypeCommandCode != PldmCommand))
   {
     DEBUG ((DEBUG_ERROR, "PLDM integrity check of response data is failed.\n"));
-    DEBUG ((DEBUG_ERROR, "    Request bit  = %d (Expected value: 0)\n"));
-    DEBUG ((DEBUG_ERROR, "    Datagram     = %d (Expected value: 0)\n"));
+    DEBUG ((DEBUG_ERROR, "    Request bit  = %d (Expected value: %d)\n", PLDM_MESSAGE_HEADER_IS_RESPONSE));
+    DEBUG ((DEBUG_ERROR, "    Datagram     = %d (Expected value: %d)\n", PLDM_MESSAGE_HEADER_IS_DATAGRAM));
     DEBUG ((DEBUG_ERROR, "    Instance ID  = %d (Expected value: %d)\n", ResponseHeader->PldmHeader.InstanceId, mPldmRequestInstanceId));
     DEBUG ((DEBUG_ERROR, "    Pldm Type    = %d (Expected value: %d)\n", ResponseHeader->PldmHeader.PldmType, PldmType));
     DEBUG ((DEBUG_ERROR, "    Pldm Command = %d (Expected value: %d)\n", ResponseHeader->PldmHeader.PldmTypeCommandCode, PldmCommand));
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109642): https://edk2.groups.io/g/devel/message/109642
Mute This Topic: https://groups.io/mt/101994947/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms 10/10] ManageabilityPkg: Return error on multiple-packet MCTP responses

[Konstantin Aladyshev]

Since the current driver doesn't yet support handling of
multiple-packet MCTP responses, return EFI_UNSUPPORTED error in such
cases.

Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
---
 .../MctpProtocol/Common/MctpProtocolCommon.c          | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
index 4aae4fcba9..7cf2d916f4 100644
--- a/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
+++ b/Features/ManageabilityPkg/Universal/MctpProtocol/Common/MctpProtocolCommon.c
@@ -524,6 +524,17 @@ CommonMctpSubmitMessage (
     FreePool (ResponseBuffer);
     return EFI_DEVICE_ERROR;
   }
+  if ((MctpTransportResponseHeader->Bits.StartOfMessage != 1) ||
+      (MctpTransportResponseHeader->Bits.EndOfMessage != 1) ||
+      (MctpTransportResponseHeader->Bits.PacketSequence != 0)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Error! Multiple-packet MCTP responses are not supported by the current driver\n",
+      __func__,
+      ));
+    FreePool (ResponseBuffer);
+    return EFI_UNSUPPORTED;
+  }
 
   MctpMessageResponseHeader = (MCTP_MESSAGE_HEADER *)(MctpTransportResponseHeader + 1);
   if (MctpMessageResponseHeader->Bits.MessageType != MctpType) {
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109643): https://edk2.groups.io/g/devel/message/109643
Mute This Topic: https://groups.io/mt/101994949/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms 00/10] MTCP-over-KCS support

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

For entire series, Reviewed-by: Abner Chang <abner.chang@amd.com>

Hi Aladyshev,
Could you please provide the corresponding branch somewhere that contains the latest version of this patch set? Then I can move forward to merge it after edk2 portion is merged.

Thanks for this contribution.
Abner

> -----Original Message-----
> From: Konstantin Aladyshev <aladyshev22@gmail.com>
> Sent: Monday, October 16, 2023 9:18 PM
> To: devel@edk2.groups.io
> Cc: Chang, Abner <Abner.Chang@amd.com>; isaac.w.oram@intel.com; Attar,
> AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>;
> nicklew@nvidia.com; Konstantin Aladyshev <aladyshev22@gmail.com>
> Subject: [PATCH edk2-platforms 00/10] MTCP-over-KCS support
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> The Manageability KCS transport library needs to support requests both
> from MCTP and IPMI transports. Currently the code only handles IPMI
> case correctly.
> In the MCTP case the communication should be based on the MCTP-over-KCS
> specification (DSP0254). This specification defines a special KCS
> binding header and trailer structures that need to be present in every
> MCTP message.
> The header structure contains a length field, therefore response packet
> size is not needed to be known beforehand.
> The trailer structure contains a PEC checksum that can be used to check
> itegrity of the response message.
> Modify Manageability KCS transport library code to check which message
> is processed (IPMI or MCTP) and handle each case correctly based on its
> own specification.
> This patch is a result of a joint effort from the Konstantin Aladyshev
> <aladyshev22@gmail.com> and Abner Chang <abner.chang@amd.com>.
>
> Tested:
> PLDM communication between the HOST and BMC was tested with both
> components implemented via open-source software:
> - The HOST (UEFI firmware) part was based one the edk2 [1] and
> edk2-platforms [2] code,
> - The BMC part was based on the openbmc [3] distribution.
>
> The testing process and all the necessary utilities are described in
> the [4] repository.
>
> The provided changes keep IPMI over KCS stack working as reported by
> Abner Chang.
>
> [1]: https://github.com/tianocore/edk2
> [2]: https://github.com/tianocore/edk2-platforms
> [3]: https://github.com/openbmc/openbmc
> [4]: https://github.com/Kostr/PLDM
>
> Konstantin Aladyshev (10):
>   ManageabilityPkg: Add definition for the MCTP KCS TRAILER structure
>   ManageabilityPkg: Check MCTP EIDs for reserved values
>   ManageabilityPkg: Support both MCTP and IPMI in KCS tranport library
>   ManageabilityPkg: Check header fields in the MCTP response
>   ManageabilityPkg: Correct typo in MCTP destination EID field
>   ManageabilityPkg: Update the algorithm of using MCTP endpoint ID PCD
>   ManageabilityPkg: Correct value for the MCTP TAG_OWNER response bit
>   ManageabilityPkg: Don't check MCTP header fields if transfer has
>     failed
>   ManageabilityPkg: Use correct constants for PLDM header checks
>   ManageabilityPkg: Return error on multiple-packet MCTP responses
>
>  .../Library/ManageabilityTransportMctpLib.h   |   9 +-
>  .../Include/Protocol/MctpProtocol.h           |  12 +-
>  .../Common/KcsCommon.c                        | 284 +++++++++++++++---
>  .../Dxe/ManageabilityTransportMctp.c          |   4 +-
>  .../MctpProtocol/Common/MctpProtocolCommon.c  | 129 +++++++-
>  .../Universal/MctpProtocol/Dxe/MctpProtocol.c |  51 +++-
>  .../PldmProtocol/Common/PldmProtocolCommon.c  |  10 +-
>  7 files changed, 430 insertions(+), 69 deletions(-)
>
> --
> 2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109710): https://edk2.groups.io/g/devel/message/109710
Mute This Topic: https://groups.io/mt/101994937/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-