public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel][edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM
@ 2023-06-12 12:52 Arun K
  2023-06-13  0:27 ` Isaac Oram
  0 siblings, 1 reply; 6+ messages in thread
From: Arun K @ 2023-06-12 12:52 UTC (permalink / raw)
  To: devel@edk2.groups.io, Arun K
  Cc: isaac.w.oram@intel.com, nathaniel.l.desimone@intel.com,
	Ramkumar Krishnamoorthi, Liming Gao

Created IpmiTransport2 PPI/Protocol to support multiple
IPMI BMC Interface support such as KCS/BT/SSIF with 2 API's
IpmiSubmitCommand2 & IpmiSubmitCommand2Ex.
IpmiSubmitCommand2 - This API use the default interface
(PcdDefaultSystemInterface) to send IPMI command.
IpmiSubmitCommand2Ex - This API use the specific interface type
to send IPMI command which is passed as an argument.

Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>

Signed-off-by: Arun K <arunk@ami.com>
---
 .../GenericIpmi/Common/IpmiBmc.h              |  10 +
 .../GenericIpmi/Common/IpmiBmcCommon.h        |   2 +
 .../GenericIpmi/Common/IpmiHooks.c            | 256 ++++++++++++++++++
 .../GenericIpmi/Common/IpmiHooks.h            |  93 ++++++-
 .../GenericIpmi/Dxe/GenericIpmi.inf           |  14 +-
 .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 205 ++++++++++++++
 .../GenericIpmi/Smm/SmmGenericIpmi.c          | 200 +++++++++++++-
 .../GenericIpmi/Smm/SmmGenericIpmi.inf        |  12 +
 .../IpmiFeaturePkg/IpmiFeaturePkg.dec         |  45 +++
 9 files changed, 832 insertions(+), 5 deletions(-)

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.h b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.h
index d306a085e5..19fb2a26a3 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.h
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.h
@@ -3,6 +3,7 @@


   @copyright

   Copyright 1999 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



@@ -21,6 +22,7 @@
 #include <Library/ReportStatusCodeLib.h>

 #include <Library/IpmiBaseLib.h>

 #include <Protocol/IpmiTransportProtocol.h>

+#include <Protocol/IpmiTransport2Protocol.h>



 #include "IpmiBmcCommon.h"

 #include "KcsBmc.h"

@@ -41,4 +43,12 @@
   SM_IPMI_BMC_SIGNATURE \

   )



+#define INSTANCE_FROM_IPMI_TRANSPORT2_THIS(a) \

+  CR ( \

+  a, \

+  IPMI_BMC_INSTANCE_DATA, \

+  IpmiTransport2, \

+  SM_IPMI_BMC_SIGNATURE \

+  )

+

 #endif // _IPMI_BMC_H_

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h
index 06eab62aae..3b252f5f1c 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h
@@ -3,6 +3,7 @@


   @copyright

   Copyright 1999 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



@@ -55,6 +56,7 @@ typedef struct {
   UINT8               SoftErrorCount;

   UINT16              IpmiIoBase;

   IPMI_TRANSPORT      IpmiTransport;

+  IPMI_TRANSPORT2     IpmiTransport2;

   EFI_HANDLE          IpmiSmmHandle;

 } IPMI_BMC_INSTANCE_DATA;



diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.c
index b2788e5a4c..19e5c1c04b 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.c
@@ -3,6 +3,7 @@


   @copyright

   Copyright 2014 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



@@ -48,6 +49,11 @@ Returns:


 --*/

 {

+

+  if (This == NULL) {

+      return EFI_INVALID_PARAMETER;

+  }

+

   //

   // This Will be unchanged ( BMC/KCS style )

   //

@@ -64,6 +70,251 @@ Returns:
            );

 } // IpmiSendCommand()



+EFI_STATUS

+EFIAPI

+IpmiSendCommand2 (

+  IN      IPMI_TRANSPORT2              *This,

+  IN      UINT8                        NetFunction,

+  IN      UINT8                        Lun,

+  IN      UINT8                        Command,

+  IN      UINT8                        *CommandData,

+  IN      UINT32                       CommandDataSize,

+  IN OUT  UINT8                        *ResponseData,

+  IN OUT  UINT32                       *ResponseDataSize

+  )

+/*++

+

+Routine Description:

+

+  This API use the default interface (PcdDefaultSystemInterface) to send IPMI command

+  in the right mode to the appropiate device, ME or BMC.

+

+Arguments:

+

+  This              - Pointer to IPMI protocol instance

+  NetFunction       - Net Function of command to send

+  Lun               - LUN of command to send

+  Command           - IPMI command to send

+  CommandData       - Pointer to command data buffer, if needed

+  CommandDataSize   - Size of command data buffer

+  ResponseData      - Pointer to response data buffer

+  ResponseDataSize  - Pointer to response data buffer size

+

+Returns:

+

+  EFI_INVALID_PARAMETER - One of the input values is bad

+  EFI_DEVICE_ERROR      - IPMI command failed

+  EFI_BUFFER_TOO_SMALL  - Response buffer is too small

+  EFI_UNSUPPORTED       - Command is not supported by BMC

+  EFI_SUCCESS           - Command completed successfully

+

+--*/

+{

+

+  IPMI_BMC_INSTANCE_DATA  *IpmiInstance;

+

+  if (This == NULL) {

+      return EFI_INVALID_PARAMETER;

+  }

+

+  IpmiInstance = INSTANCE_FROM_IPMI_TRANSPORT2_THIS(This);

+

+#if KcsInterfaceSupport

+  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceKcs) &&

+      (IpmiInstance->IpmiTransport2.Interface.KcsInterfaceState == IpmiInterfaceInitialized)) {

+

+      return IpmiSendCommand (

+               &IpmiInstance->IpmiTransport,

+               NetFunction,

+               Lun,

+               Command,

+               CommandData,

+               CommandDataSize,

+               ResponseData,

+               ResponseDataSize

+               );

+  }

+#endif

+

+#if BtInterfaceSupport

+  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceBt) &&

+      (IpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState == IpmiInterfaceInitialized)) {

+

+      return IpmiBtSendCommandToBmc (

+               &IpmiInstance->IpmiTransport2,

+               NetFunction,

+               Lun,

+               Command,

+               CommandData,

+               (UINT8) CommandDataSize,

+               ResponseData,

+               (UINT8*) ResponseDataSize,

+               NULL

+               );

+  }

+#endif

+

+#if SsifInterfaceSupport

+  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceSsif) &&

+      (IpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == IpmiInterfaceInitialized)) {

+

+      return IpmiSsifSendCommandToBmc (

+               &IpmiInstance->IpmiTransport2,

+               NetFunction,

+               Lun,

+               Command,

+               CommandData,

+               (UINT8) CommandDataSize,

+               ResponseData,

+               (UINT8*) ResponseDataSize,

+               NULL

+               );

+  }

+#endif

+

+#if IpmbInterfaceSupport

+  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceIpmb) &&

+      (IpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == IpmiInterfaceInitialized)) {

+

+      return IpmiIpmbSendCommandToBmc (

+               &IpmiInstance->IpmiTransport2,

+               NetFunction,

+               Lun,

+               Command,

+               CommandData,

+               (UINT8) CommandDataSize,

+               ResponseData,

+               (UINT8*) ResponseDataSize,

+               NULL

+               );

+  }

+#endif

+  return EFI_UNSUPPORTED;

+} // IpmiSendCommand2()

+

+EFI_STATUS

+EFIAPI

+IpmiSendCommand2Ex (

+  IN      IPMI_TRANSPORT2              *This,

+  IN      UINT8                        NetFunction,

+  IN      UINT8                        Lun,

+  IN      UINT8                        Command,

+  IN      UINT8                        *CommandData,

+  IN      UINT32                       CommandDataSize,

+  IN OUT  UINT8                        *ResponseData,

+  IN OUT  UINT32                       *ResponseDataSize,

+  IN      SYSTEM_INTERFACE_TYPE        InterfaceType

+  )

+{

+/*++

+Routine Description:

+

+  This API use the specific interface type to send IPMI command

+  in the right mode to the appropiate device, ME or BMC.

+

+Arguments:

+

+  This              - Pointer to IPMI protocol instance

+  NetFunction       - Net Function of command to send

+  Lun               - LUN of command to send

+  Command           - IPMI command to send

+  CommandData       - Pointer to command data buffer, if needed

+  CommandDataSize   - Size of command data buffer

+  ResponseData      - Pointer to response data buffer

+  ResponseDataSize  - Pointer to response data buffer size

+  InterfaceType     - BMC Interface type.

+

+Returns:

+

+  EFI_INVALID_PARAMETER - One of the input values is bad

+  EFI_DEVICE_ERROR      - IPMI command failed

+  EFI_BUFFER_TOO_SMALL  - Response buffer is too small

+  EFI_UNSUPPORTED       - Command is not supported by BMC

+  EFI_SUCCESS           - Command completed successfully

+

+--*/

+

+  IPMI_BMC_INSTANCE_DATA  *IpmiInstance;

+

+  if (This == NULL) {

+      return EFI_INVALID_PARAMETER;

+  }

+

+  IpmiInstance = INSTANCE_FROM_IPMI_TRANSPORT2_THIS(This);

+

+#if KcsInterfaceSupport

+  if ((InterfaceType == SysInterfaceKcs) &&

+      (IpmiInstance->IpmiTransport2.Interface.KcsInterfaceState == IpmiInterfaceInitialized)) {

+

+      return IpmiSendCommand (

+                &IpmiInstance->IpmiTransport,

+                NetFunction,

+                Lun,

+                Command,

+                CommandData,

+                CommandDataSize,

+                ResponseData,

+                ResponseDataSize

+                );

+     }

+#endif

+

+#if BtInterfaceSupport

+  if ((InterfaceType == SysInterfaceBt) &&

+      (IpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState == IpmiInterfaceInitialized)) {

+

+      return IpmiBtSendCommandToBmc (

+                &IpmiInstance->IpmiTransport2,

+                NetFunction,

+                Lun,

+                Command,

+                CommandData,

+                (UINT8) CommandDataSize,

+                ResponseData,

+                (UINT8*) ResponseDataSize,

+                NULL

+                );

+  }

+#endif

+

+#if SsifInterfaceSupport

+  if ((InterfaceType == SysInterfaceSsif) &&

+      (IpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == IpmiInterfaceInitialized)) {

+

+      return IpmiSsifSendCommandToBmc (

+                &IpmiInstance->IpmiTransport2,

+                NetFunction,

+                Lun,

+                Command,

+                CommandData,

+                (UINT8) CommandDataSize,

+                ResponseData,

+                (UINT8*) ResponseDataSize,

+                NULL

+                );

+  }

+#endif

+

+#if IpmbInterfaceSupport

+  if ((InterfaceType == SysInterfaceIpmb) &&

+      (IpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == IpmiInterfaceInitialized)) {

+

+      return IpmiIpmbSendCommandToBmc (

+                &IpmiInstance->IpmiTransport2,

+                NetFunction,

+                Lun,

+                Command,

+                CommandData,

+                (UINT8) CommandDataSize,

+                ResponseData,

+                (UINT8*) ResponseDataSize,

+                NULL

+                );

+  }

+#endif

+  return EFI_UNSUPPORTED;

+}

+

 EFI_STATUS

 EFIAPI

 IpmiGetBmcStatus (

@@ -89,6 +340,11 @@ Returns:


 --*/

 {

+

+  if ((This == NULL) || (BmcStatus == NULL) || (ComAddress == NULL)) {

+      return EFI_INVALID_PARAMETER;

+  }

+

   return IpmiBmcStatus (

            This,

            BmcStatus,

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.h b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.h
index 823cc08c61..3933e07443 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.h
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.h
@@ -3,13 +3,21 @@


   @copyright

   Copyright 2014 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



 #ifndef _IPMI_HOOKS_H

 #define _IPMI_HOOKS_H



-#include "IpmiBmc.h"

+#include <Protocol/IpmiTransportProtocol.h>

+#include <Protocol/IpmiTransport2Protocol.h>

+#include <Library/BtInterfaceLib.h>

+#include <Library/SsifInterfaceLib.h>

+#include <Library/IpmbInterfaceLib.h>

+#include <Library/DebugLib.h>

+#include<IpmiBmcCommon.h>

+#include<IpmiBmc.h>



 //

 // Internal(hook) function list

@@ -54,6 +62,89 @@ Returns:
 --*/

 ;



+EFI_STATUS

+EFIAPI

+IpmiSendCommand2 (

+  IN      IPMI_TRANSPORT2              *This,

+  IN      UINT8                        NetFunction,

+  IN      UINT8                        Lun,

+  IN      UINT8                        Command,

+  IN      UINT8                        *CommandData,

+  IN      UINT32                       CommandDataSize,

+  IN OUT  UINT8                        *ResponseData,

+  IN OUT  UINT32                       *ResponseDataSize

+  )

+/*++

+

+Routine Description:

+

+  This API use the default interface (PcdDefaultSystemInterface) to send IPMI command

+  in the right mode to the appropiate device, ME or BMC.

+

+Arguments:

+

+  This              - Pointer to IPMI protocol instance

+  NetFunction       - Net Function of command to send

+  Lun               - LUN of command to send

+  Command           - IPMI command to send

+  CommandData       - Pointer to command data buffer, if needed

+  CommandDataSize   - Size of command data buffer

+  ResponseData      - Pointer to response data buffer

+  ResponseDataSize  - Pointer to response data buffer size

+

+Returns:

+

+  EFI_INVALID_PARAMETER - One of the input values is bad

+  EFI_DEVICE_ERROR      - IPMI command failed

+  EFI_BUFFER_TOO_SMALL  - Response buffer is too small

+  EFI_UNSUPPORTED       - Command is not supported by BMC

+  EFI_SUCCESS           - Command completed successfully

+

+--*/

+;

+

+EFI_STATUS

+EFIAPI

+IpmiSendCommand2Ex (

+  IN      IPMI_TRANSPORT2              *This,

+  IN      UINT8                        NetFunction,

+  IN      UINT8                        Lun,

+  IN      UINT8                        Command,

+  IN      UINT8                        *CommandData,

+  IN      UINT32                       CommandDataSize,

+  IN OUT  UINT8                        *ResponseData,

+  IN OUT  UINT32                       *ResponseDataSize,

+  IN      SYSTEM_INTERFACE_TYPE        InterfaceType

+  )

+/*++

+Routine Description:

+

+  This API use the specific interface type to send IPMI command

+  in the right mode to the appropiate device, ME or BMC.

+

+Arguments:

+

+  This              - Pointer to IPMI protocol instance

+  NetFunction       - Net Function of command to send

+  Lun               - LUN of command to send

+  Command           - IPMI command to send

+  CommandData       - Pointer to command data buffer, if needed

+  CommandDataSize   - Size of command data buffer

+  ResponseData      - Pointer to response data buffer

+  ResponseDataSize  - Pointer to response data buffer size

+  InterfaceType     - BMC Interface type.

+

+Returns:

+

+  EFI_INVALID_PARAMETER - One of the input values is bad

+  EFI_DEVICE_ERROR      - IPMI command failed

+  EFI_BUFFER_TOO_SMALL  - Response buffer is too small

+  EFI_UNSUPPORTED       - Command is not supported by BMC

+  EFI_SUCCESS           - Command completed successfully

+

+--*/

+;

+

 EFI_STATUS

 EFIAPI

 IpmiGetBmcStatus (

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf
index 8d80aeb6b5..1564ceb08a 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf
@@ -3,6 +3,7 @@
 #

 # @copyright

 # Copyright 2010 - 2021 Intel Corporation. <BR>

+# Copyright (c) 1985 - 2023, American Megatrends International LLC. <BR>

 # SPDX-License-Identifier: BSD-2-Clause-Patent

 ##



@@ -49,16 +50,25 @@
   IoLib

   ReportStatusCodeLib

   TimerLib

+  BmcCommonInterfaceLib

+  BtInterfaceLib

+  SsifInterfaceLib

+  IpmbInterfaceLib



 [Protocols]

   gIpmiTransportProtocolGuid               # PROTOCOL ALWAYS_PRODUCED

   gEfiVideoPrintProtocolGuid

-

-[Guids]

+  gIpmiTransport2ProtocolGuid



 [Pcd]

   gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiIoBaseAddress

   gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiBmcReadyDelayTimer

+  gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort



 [Depex]

   gEfiRuntimeArchProtocolGuid AND

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
index c333ca2e06..74d96f8684 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
@@ -3,6 +3,7 @@


   @copyright

   Copyright 1999 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



@@ -13,6 +14,7 @@
 #include "IpmiBmc.h"

 #include "IpmiPhysicalLayer.h"

 #include <Library/TimerLib.h>

+#include <Library/BmcCommonInterfaceLib.h>

 #ifdef FAST_VIDEO_SUPPORT

   #include <Protocol/VideoPrint.h>

 #endif

@@ -351,6 +353,130 @@ Returns:
   return Status;

 } // GetDeviceId()



+/**

+    Initialize the API and parameters for IPMI Transport2 Instance

+

+    @param[in] IpmiInstance         Pointer to IPMI Instance

+

+    @return VOID    Nothing.

+

+**/

+VOID

+InitIpmiTransport2 (

+  IN  IPMI_BMC_INSTANCE_DATA *IpmiInstance

+  )

+{

+

+  IpmiInstance->IpmiTransport2.InterfaceType            = FixedPcdGet8 (PcdDefaultSystemInterface);

+  IpmiInstance->IpmiTransport2.IpmiTransport2BmcStatus  = BmcStatusOk;

+  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2       = IpmiSendCommand2;

+  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2Ex     = IpmiSendCommand2Ex;

+

+#if BtInterfaceSupport

+  InitBtInterfaceData(&IpmiInstance->IpmiTransport2);

+#endif

+

+#if SsifInterfaceSupport

+  InitSsifInterfaceData(&IpmiInstance->IpmiTransport2);

+#endif

+

+#if IpmbInterfaceSupport

+  InitIpmbInterfaceData(&IpmiInstance->IpmiTransport2);

+#endif

+}

+

+/**

+    Notify call back function.

+

+    @param[in] Event    Event which caused this handler.

+    @param[in] Context  Context passed during Event Handler registration.

+

+    @return VOID    Nothing.

+

+**/

+VOID

+EFIAPI

+DxeNotifyCallback (

+  IN EFI_EVENT  Event,

+  IN VOID       *Context

+  )

+{

+  EFI_STATUS             Status;

+  IPMI_INTERFACE_STATE   InterfaceState;

+  EFI_HANDLE             Handle;

+

+  InterfaceState = IpmiInterfaceNotReady;

+

+#if SsifInterfaceSupport

+  InitSsifInterfaceData(&mIpmiInstance->IpmiTransport2);

+

+  if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == IpmiInterfaceInitialized) {

+      InterfaceState = IpmiInterfaceInitialized;

+  }

+#endif

+

+#if IpmbInterfaceSupport

+  InitIpmbInterfaceData(&mIpmiInstance->IpmiTransport2);

+

+  if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == IpmiInterfaceInitialized) {

+      InterfaceState = IpmiInterfaceInitialized;

+  }

+#endif

+  // Default Interface data should be initialized to install Ipmi Transport2 Protocol.

+  if (InterfaceState != IpmiInterfaceInitialized) {

+      return;

+  }

+

+  Handle = NULL;

+  Status = gBS->InstallProtocolInterface (

+                  &Handle,

+                  &gIpmiTransport2ProtocolGuid,

+                  EFI_NATIVE_INTERFACE,

+                  &mIpmiInstance->IpmiTransport2

+                  );

+  ASSERT_EFI_ERROR (Status);

+}

+

+/**

+    Registers protocol notify call back.

+

+    @param[in] ProtocolGuid     Pointer to Protocol Guid to register

+                                call back.

+

+    @retval EFI_INVALID_PARAMETER   If the ProtocolGuid is 0 or NULL.

+    @retval Others                  Status of call back registration.

+

+**/

+EFI_STATUS

+DxeRegisterProtocolCallback (

+  IN EFI_GUID   *ProtocolGuid

+  )

+{

+  EFI_STATUS  Status;

+  EFI_EVENT   NotifyEvent;

+  VOID        *Registration;

+

+  if ((ProtocolGuid == NULL) ||

+      ((ProtocolGuid != NULL) && IsZeroBuffer (ProtocolGuid, sizeof (EFI_GUID)))) {

+      return EFI_INVALID_PARAMETER;

+  }

+

+  Status = gBS->CreateEvent (

+                  EVT_NOTIFY_SIGNAL,

+                  TPL_NOTIFY,

+                  DxeNotifyCallback,

+                  NULL,

+                  &NotifyEvent);

+

+  if (!EFI_ERROR (Status)) {

+      Status = gBS->RegisterProtocolNotify (

+                      ProtocolGuid,

+                      NotifyEvent,

+                      &Registration);

+  }

+

+  return Status;

+}



 /**

   This function initializes KCS interface to BMC.

@@ -376,7 +502,10 @@ InitializeIpmiKcsPhysicalLayer (
   UINT8                  ErrorCount;

   EFI_HANDLE             Handle;

   UINT8                  Index;

+  IPMI_INTERFACE_STATE   InterfaceState = IpmiInterfaceNotReady;

+#if KcsInterfaceSupport

   EFI_STATUS_CODE_VALUE  StatusCodeValue[MAX_SOFT_COUNT];

+#endif



   ErrorCount = 0;

   mImageHandle = ImageHandle;

@@ -405,6 +534,8 @@ InitializeIpmiKcsPhysicalLayer (
     mIpmiInstance->Signature                        = SM_IPMI_BMC_SIGNATURE;

     mIpmiInstance->SlaveAddress                     = BMC_SLAVE_ADDRESS;

     mIpmiInstance->BmcStatus                        = BMC_NOTREADY;

+

+#if KcsInterfaceSupport

     mIpmiInstance->IpmiTransport.IpmiSubmitCommand  = IpmiSendCommand;

     mIpmiInstance->IpmiTransport.GetBmcStatus       = IpmiGetBmcStatus;



@@ -454,7 +585,81 @@ InitializeIpmiKcsPhysicalLayer (
                       );

       ASSERT_EFI_ERROR (Status);

     }

+#endif

+

+    // Initialise the IPMI transport2

+    InitIpmiTransport2(mIpmiInstance);

+

+    // Check interface data initialized successfully else register notify protocol.

+    for (Index = SysInterfaceKcs; Index < SysInterfaceMax; Index++) {

+

+        switch (Index) {

+#if KcsInterfaceSupport

+            case SysInterfaceKcs:

+                if ((mIpmiInstance->BmcStatus != BMC_HARDFAIL) && (mIpmiInstance->BmcStatus != BMC_UPDATE_IN_PROGRESS)) {

+                    BMC_INTERFACE_STATUS  BmcStatus;

+                    mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState = IpmiInterfaceInitialized;

+                    Status = CheckSelfTestByInterfaceType(

+                                        &mIpmiInstance->IpmiTransport2,

+                                        &BmcStatus,

+                                        SysInterfaceKcs);

+                    if (!EFI_ERROR (Status) && (BmcStatus != BmcStatusHardFail)) {

+                        InterfaceState = IpmiInterfaceInitialized;

+                    } else {

+                        mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState = IpmiInterfaceInitError;

+                    }

+                }

+                break;

+#endif

+

+#if BtInterfaceSupport

+            case SysInterfaceBt:

+                if (mIpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState == IpmiInterfaceInitialized){

+                    InterfaceState = IpmiInterfaceInitialized;

+                }

+                break;

+#endif

+

+#if SsifInterfaceSupport

+            case SysInterfaceSsif:

+                if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == IpmiInterfaceInitialized) {

+                    InterfaceState = IpmiInterfaceInitialized;

+                } else if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == IpmiInterfaceInitError) {

+                    // Register protocol notify for SMBUS Protocol.

+                    Status = DxeRegisterProtocolCallback (

+                                &mIpmiInstance->IpmiTransport2.Interface.Ssif.SsifInterfaceApiGuid);

+                }

+                break;

+#endif

+#if IpmbInterfaceSupport

+            case SysInterfaceIpmb:

+                if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == IpmiInterfaceInitialized) {

+                    InterfaceState = IpmiInterfaceInitialized;

+                } else if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == IpmiInterfaceInitError) {

+                    // Register Protocol notify for I2C Protocol.

+                    Status = DxeRegisterProtocolCallback (

+                                &mIpmiInstance->IpmiTransport2.Interface.Ipmb.IpmbInterfaceApiGuid);

+                }

+                break;

+#endif

+            default:

+                break;

+        }

+    }

+

+    // Any one of the Interface data should be initialized to install IPMI Transport2 Protocol.

+    if (InterfaceState != IpmiInterfaceInitialized) {

+        return EFI_SUCCESS;

+    }



+    Handle = NULL;

+    Status = gBS->InstallProtocolInterface (

+                    &Handle,

+                    &gIpmiTransport2ProtocolGuid,

+                    EFI_NATIVE_INTERFACE,

+                    &mIpmiInstance->IpmiTransport2

+                    );

+    ASSERT_EFI_ERROR (Status);

     return EFI_SUCCESS;

   }

 } // InitializeIpmiKcsPhysicalLayer()

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.c
index 1af2d18f79..adf59374b3 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.c
@@ -3,6 +3,7 @@


   @copyright

   Copyright 1999 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



@@ -24,6 +25,7 @@
 #include "IpmiBmcCommon.h"

 #include "IpmiBmc.h"

 #include <Library/TimerLib.h>

+#include <Library/BmcCommonInterfaceLib.h>



 IPMI_BMC_INSTANCE_DATA             *mIpmiInstance;

 EFI_HANDLE                         mImageHandle;

@@ -115,6 +117,123 @@ Returns:
   return Status;

 }



+/**

+    Initialize the API and parameters for IPMI Transport2 Instance

+

+    @param[in] IpmiInstance         Pointer to IPMI Instance

+

+    @return VOID    Nothing.

+

+**/

+VOID

+InitIpmiTransport2 (

+  IN  IPMI_BMC_INSTANCE_DATA    *IpmiInstance

+  )

+{

+

+  IpmiInstance->IpmiTransport2.InterfaceType           = FixedPcdGet8 (PcdDefaultSystemInterface);

+  IpmiInstance->IpmiTransport2.IpmiTransport2BmcStatus = BmcStatusOk;

+  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2      = IpmiSendCommand2;

+  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2Ex    = IpmiSendCommand2Ex;

+

+#if BtInterfaceSupport

+  InitBtInterfaceData (&IpmiInstance->IpmiTransport2);

+#endif

+

+#if SsifInterfaceSupport

+  InitSsifInterfaceData (&IpmiInstance->IpmiTransport2);

+#endif

+

+#if IpmbInterfaceSupport

+  InitIpmbInterfaceData (&IpmiInstance->IpmiTransport2);

+#endif

+}

+

+/**

+    Notify call back to initialize the interfaces and install Smm Ipmi

+    protocol.

+

+    @param[in] Protocol     Pointer to the protocol guid.

+    @param[in] Interface    Pointer to the protocol instance.

+    @param[in] Handle       Handle on which the protocol is installed.

+

+    @return EFI_STATUS  Status of Notify call back.

+

+**/

+EFI_STATUS

+EFIAPI

+SmmNotifyCallback (

+  IN CONST  EFI_GUID    *Protocol,

+  IN        VOID        *Interface,

+  IN        EFI_HANDLE  Handle

+  )

+{

+

+  EFI_STATUS             Status;

+  IPMI_INTERFACE_STATE   InterfaceState;

+

+  InterfaceState = IpmiInterfaceNotReady;

+

+#if SsifInterfaceSupport

+  InitSsifInterfaceData(&mIpmiInstance->IpmiTransport2);

+

+  if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == IpmiInterfaceInitialized){

+      InterfaceState = IpmiInterfaceInitialized;

+  }

+#endif

+

+#if IpmbInterfaceSupport

+  InitIpmbInterfaceData(&mIpmiInstance->IpmiTransport2);

+#endif

+

+  if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == IpmiInterfaceInitialized){

+      InterfaceState = IpmiInterfaceInitialized;

+  }

+  if (InterfaceState != IpmiInterfaceInitialized) {

+      return EFI_SUCCESS;

+  }

+

+  // Default Interface data should be initialized to install Ipmi Transport2 Protocol.

+  if (InterfaceState == IpmiInterfaceInitialized) {

+      Handle = NULL;

+      Status = gSmst->SmmInstallProtocolInterface (

+                          &Handle,

+                          &gSmmIpmiTransport2ProtocolGuid,

+                          EFI_NATIVE_INTERFACE,

+                          &mIpmiInstance->IpmiTransport2

+                          );

+  }

+  return EFI_SUCCESS;

+}

+

+/**

+    Registers Protocol call back

+

+    @param ProtocolGuid       Pointer to Protocol GUID to register call back

+

+    @retval EFI_INVALID_PARAMETER   If the ProtocolGuid is 0 or NULL.

+    @retval Others                  Status of Notify registration.

+**/

+EFI_STATUS

+SmmRegisterProtocolCallback (

+  IN  EFI_GUID  *ProtocolGuid

+)

+{

+    EFI_STATUS  Status;

+    VOID        *Registration;

+

+    if ((ProtocolGuid == NULL) ||

+        ((ProtocolGuid != NULL) && IsZeroBuffer (ProtocolGuid, sizeof (EFI_GUID)))) {

+        return EFI_INVALID_PARAMETER;

+    }

+

+    Status = gSmst->SmmRegisterProtocolNotify (

+                        ProtocolGuid,

+                        SmmNotifyCallback,

+                        &Registration );

+    return Status;

+}

+

 EFI_STATUS

 SmmInitializeIpmiKcsPhysicalLayer (

   IN EFI_HANDLE             ImageHandle,

@@ -142,10 +261,13 @@ Returns:
   UINT8                            ErrorCount;

   EFI_HANDLE                       Handle;

   EFI_STATUS_CODE_VALUE            StatusCodeValue[MAX_SOFT_COUNT];

+  IPMI_INTERFACE_STATE             InterfaceState;

+  UINT8                            Index;



   DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer entry \n"));

-  ErrorCount = 0;

-  mImageHandle = ImageHandle;

+  ErrorCount     = 0;

+  mImageHandle   = ImageHandle;

+  InterfaceState = IpmiInterfaceNotReady;



   mIpmiInstance = AllocateZeroPool (sizeof (IPMI_BMC_INSTANCE_DATA));

   ASSERT (mIpmiInstance != NULL);

@@ -170,6 +292,7 @@ Returns:
     mIpmiInstance->IpmiTransport.IpmiSubmitCommand  = IpmiSendCommand;

     mIpmiInstance->IpmiTransport.GetBmcStatus       = IpmiGetBmcStatus;



+#if KcsInterfaceSupport

     DEBUG ((DEBUG_INFO,"IPMI: Waiting for Getting BMC DID in SMM \n"));

     //

     // Get the Device ID and check if the system is in Force Update mode.

@@ -191,6 +314,79 @@ Returns:
                       &mIpmiInstance->IpmiTransport

                       );

     ASSERT_EFI_ERROR (Status);

+#endif

+

+    InitIpmiTransport2(mIpmiInstance);

+

+    // Check interface data initialized successfully else register notify protocol.

+    for (Index = SysInterfaceKcs; Index < SysInterfaceMax; Index++) {

+

+        switch (Index) {

+#if KcsInterfaceSupport

+            case SysInterfaceKcs:

+                if ((mIpmiInstance->BmcStatus != BMC_HARDFAIL) && (mIpmiInstance->BmcStatus != BMC_UPDATE_IN_PROGRESS)) {

+                    BMC_INTERFACE_STATUS  BmcStatus;

+                    mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState = IpmiInterfaceInitialized;

+                    Status = CheckSelfTestByInterfaceType(

+                                               &mIpmiInstance->IpmiTransport2,

+                                               &BmcStatus,

+                                               SysInterfaceKcs);

+                    if (!EFI_ERROR (Status) && (BmcStatus != BmcStatusHardFail)) {

+                        InterfaceState = IpmiInterfaceInitialized;

+                    } else {

+                        mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState = IpmiInterfaceInitError;

+                    }

+                }

+                break;

+#endif

+

+#if BtInterfaceSupport

+            case SysInterfaceBt:

+                if (mIpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState == IpmiInterfaceInitialized){

+                    InterfaceState = IpmiInterfaceInitialized;

+                }

+                break;

+#endif

+

+#if SsifInterfaceSupport

+            case SysInterfaceSsif:

+                if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == IpmiInterfaceInitialized){

+                    InterfaceState = IpmiInterfaceInitialized;

+                } else if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == IpmiInterfaceInitError) {

+                    // Register protocol notify for SMBUS Protocol.

+                    Status = SmmRegisterProtocolCallback (&mIpmiInstance->IpmiTransport2.Interface.Ssif.SsifInterfaceApiGuid);

+                }

+                break;

+#endif

+

+#if IpmbInterfaceSupport

+            case SysInterfaceIpmb:

+                if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == IpmiInterfaceInitialized){

+                    InterfaceState = IpmiInterfaceInitialized;

+                } else if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == IpmiInterfaceInitError) {

+                    // Register protocol notify for SMBUS Protocol.

+                    Status = SmmRegisterProtocolCallback (&mIpmiInstance->IpmiTransport2.Interface.Ipmb.IpmbInterfaceApiGuid);

+                }

+                break;

+#endif

+            default:

+                break;

+        }

+    }

+

+    // Default Interface data should be initialized to install Ipmi Transport2 Protocol.

+    if (InterfaceState == IpmiInterfaceInitialized) {

+        Handle = NULL;

+        Status = gSmst->SmmInstallProtocolInterface (

+                          &Handle,

+                          &gSmmIpmiTransport2ProtocolGuid,

+                          EFI_NATIVE_INTERFACE,

+                          &mIpmiInstance->IpmiTransport2

+                          );

+        if (EFI_ERROR(Status)) {

+            DEBUG ((DEBUG_ERROR,"IPMI Transport2 protocol install Status = %r \n",Status));

+        }

+    }



     DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer exit \n"));



diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf
index f430195d1e..12dc17ae84 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf
@@ -3,6 +3,7 @@
 #

 # @copyright

 # Copyright 2010 - 2021 Intel Corporation. <BR>

+# Copyright (c) 1985 - 2023, American Megatrends International LLC. <BR>

 # SPDX-License-Identifier: BSD-2-Clause-Patent

 ##



@@ -39,15 +40,26 @@
   IoLib

   ReportStatusCodeLib

   TimerLib

+  BmcCommonInterfaceLib

+  BtInterfaceLib

+  SsifInterfaceLib

+  IpmbInterfaceLib



 [Protocols]

   gSmmIpmiTransportProtocolGuid                     # PROTOCOL ALWAYS_PRODUCED

+  gSmmIpmiTransport2ProtocolGuid                    # PROTOCOL ALWAYS_PRODUCED



 [Guids]



 [Pcd]

   gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiSmmIoBaseAddress

   gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiBmcReadyDelayTimer

+  gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport



 [Depex]

  gIpmiTransportProtocolGuid

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.dec b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.dec
index 8c1b902446..2131ec475b 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.dec
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.dec
@@ -9,6 +9,7 @@
 #

 # Copyright (c) 2019-2021, Intel Corporation. All rights reserved.<BR>

 # Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.<BR>

+# Copyright (c) 1985 - 2023, American Megatrends International LLC. <BR>

 #

 # SPDX-License-Identifier: BSD-2-Clause-Patent

 #

@@ -43,16 +44,26 @@
   #

   IpmiBaseLib|Include/Library/IpmiBaseLib.h



+  ##  @libraryclass  Provides generic functions among all interfaces.

+  BmcCommonInterfaceLib|Include/Library/BmcCommonInterfaceLib.h

+  BtInterfaceLib|Include/Library/BtInterfaceLib.h

+  SsifInterfaceLib|Include/Library/SsifInterfaceLib.h

+  IpmbInterfaceLib|Include/Library/IpmbInterfaceLib.h

+

 [Guids]

   gIpmiFeaturePkgTokenSpaceGuid  =  {0xc05283f6, 0xd6a8, 0x48f3, {0x9b, 0x59, 0xfb, 0xca, 0x71, 0x32, 0x0f, 0x12}}

+  gPeiIpmiHobGuid                = {0xcb4d3e13, 0x1e34, 0x4373, {0x8a, 0x81, 0xe9, 0x0, 0x10, 0xf1, 0xdb, 0xa4}}



 [Ppis]

   gPeiIpmiTransportPpiGuid = {0x7bf5fecc, 0xc5b5, 0x4b25, {0x81, 0x1b, 0xb4, 0xb5, 0xb, 0x28, 0x79, 0xf7}}

+  gPeiIpmiTransport2PpiGuid = {0x8122CEBD, 0xF4FD, 0x4EA8, { 0x97, 0x6C, 0xF0, 0x30, 0xAD, 0xDC, 0x4C, 0xB4 }}



 [Protocols]

   gIpmiTransportProtocolGuid  = {0x6bb945e8, 0x3743, 0x433e, {0xb9, 0x0e, 0x29, 0xb3, 0x0d, 0x5d, 0xc6, 0x30}}

   gSmmIpmiTransportProtocolGuid  = {0x8bb070f1, 0xa8f3, 0x471d, {0x86, 0x16, 0x77, 0x4b, 0xa3, 0xf4, 0x30, 0xa0}}

   gEfiVideoPrintProtocolGuid     = {0x3dbf3e06, 0x9d0c, 0x40d3, {0xb2, 0x17, 0x45, 0x5f, 0x33, 0x9e, 0x29, 0x09}}

+  gIpmiTransport2ProtocolGuid = { 0x4A1D0E66, 0x5271, 0x4E22, { 0x83, 0xFE, 0x90, 0x92, 0x1B, 0x74, 0x82, 0x13 }}

+  gSmmIpmiTransport2ProtocolGuid = { 0x1DBD1503, 0x0A60, 0x4230, { 0xAA, 0xA3, 0x80, 0x16, 0xD8, 0xC3, 0xDE, 0x2F }}



 [PcdsFeatureFlag]

   gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiFeatureEnable|FALSE|BOOLEAN|0xA0000001

@@ -61,6 +72,40 @@
   gIpmiFeaturePkgTokenSpaceGuid.PcdMaxSOLChannels|3|UINT8|0xF0000001

   #When True, BIOS will send a Pre-Boot signal to BMC

   gIpmiFeaturePkgTokenSpaceGuid.PcdSignalPreBootToBmc|FALSE|BOOLEAN|0xF0000002

+  #  typedef enum {

+  #      SysInterfaceUnknown, // Unknown interface type.

+  #      SysInterfaceKcs,     // Kcs interface = 1.

+  #      SysInterfaceSmic,    // Smic interface = 2.

+  #      SysInterfaceBt,      // Bt interface = 3.

+  #      SysInterfaceSsif,    // Ssif interface = 4.

+  #      SysInterfaceMax   // Maximum interface type.

+  #  } SYSTEM_INTERFACE_TYPE;

+  gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface|1|UINT8|0xF0000003

+

+  #BT Base address, retry counter and delay parameters

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtCommandRetryCounter|0x0004E400|UINT32|0xF0000004

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort|0xE4|UINT16|0xF0000005

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtBufferPort|0xE5|UINT16|0xF0000006

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtDelayPerRetry|15|UINT32|0xF0000007

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterruptMaskPort|0xE6|UINT16|0xF0000008

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtBufferSize|0x40|UINT8|0xF0000009

+

+  #SSIF slave address, retry counter and delay parameters

+  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifSlaveAddress|0x10|UINT16|0xF000000A

+  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifRequestRetriesDelay|0xCB20|UINT32|0xF000000B

+  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifCommandtRetryCounter|0x5|UINT16|0xF000000C

+

+   #Interface access type for BMC communication. 0-MMIO, 1-IO

+  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiDefaultAccessType|1|UINT8|0xF000000D

+  gIpmiFeaturePkgTokenSpaceGuid.PcdMmioBaseAddress|0x0|UINT32|0xF000000E

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBaseAddressRange|0x0|UINT32|0xF000000F

+

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBmcSlaveAddress|0x20|UINT32|0xF0000010

+

+  gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport|1|UINT8|0xF0000011

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport|1|UINT8|0xF0000012

+  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport|1|UINT8|0xF0000013

+  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport|1|UINT8|0xF0000014



 [PcdsDynamic, PcdsDynamicEx]

   gIpmiFeaturePkgTokenSpaceGuid.PcdFRB2EnabledFlag|TRUE|BOOLEAN|0xD0000001

--
2.38.1.windows.1
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [edk2-devel][edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM
  2023-06-12 12:52 [edk2-devel][edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM Arun K
@ 2023-06-13  0:27 ` Isaac Oram
  2023-06-13  4:35   ` Chang, Abner
  0 siblings, 1 reply; 6+ messages in thread
From: Isaac Oram @ 2023-06-13  0:27 UTC (permalink / raw)
  To: Arun K, devel@edk2.groups.io
  Cc: Desimone, Nathaniel L, Ramkumar Krishnamoorthi, Gao, Liming

Comments inline, prefaced with "[Isaac]".  Mostly style things that you could probably fix with uncrustify also.

The main concern is the controls seem confused with #defines and PCD.  Simplifying to just use the PCD directly seems clear to me and eliminating the #if logic in favor of regular C logic is also preferred.

Thanks,
Isaac

-----Original Message-----
From: Arun K <arunk@ami.com> 
Sent: Monday, June 12, 2023 5:52 AM
To: devel@edk2.groups.io; Arun K <arunk@ami.com>
Cc: Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Ramkumar Krishnamoorthi <ramkumark@ami.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: [edk2-devel][edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM

Created IpmiTransport2 PPI/Protocol to support multiple IPMI BMC Interface support such as KCS/BT/SSIF with 2 API's
IpmiSubmitCommand2 & IpmiSubmitCommand2Ex.
IpmiSubmitCommand2 - This API use the default interface
(PcdDefaultSystemInterface) to send IPMI command.
IpmiSubmitCommand2Ex - This API use the specific interface type to send IPMI command which is passed as an argument.

Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>

Signed-off-by: Arun K <arunk@ami.com>
---
 .../GenericIpmi/Common/IpmiBmc.h              |  10 +
 .../GenericIpmi/Common/IpmiBmcCommon.h        |   2 +
 .../GenericIpmi/Common/IpmiHooks.c            | 256 ++++++++++++++++++
 .../GenericIpmi/Common/IpmiHooks.h            |  93 ++++++-
 .../GenericIpmi/Dxe/GenericIpmi.inf           |  14 +-
 .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 205 ++++++++++++++
 .../GenericIpmi/Smm/SmmGenericIpmi.c          | 200 +++++++++++++-
 .../GenericIpmi/Smm/SmmGenericIpmi.inf        |  12 +
 .../IpmiFeaturePkg/IpmiFeaturePkg.dec         |  45 +++
 9 files changed, 832 insertions(+), 5 deletions(-)

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.h b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.h
index d306a085e5..19fb2a26a3 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.h
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Comm
+++ on/IpmiBmc.h
@@ -3,6 +3,7 @@


   @copyright

   Copyright 1999 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. 
+ <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



@@ -21,6 +22,7 @@
 #include <Library/ReportStatusCodeLib.h>

 #include <Library/IpmiBaseLib.h>

 #include <Protocol/IpmiTransportProtocol.h>

+#include <Protocol/IpmiTransport2Protocol.h>



 #include "IpmiBmcCommon.h"

 #include "KcsBmc.h"

@@ -41,4 +43,12 @@
   SM_IPMI_BMC_SIGNATURE \

   )



+#define INSTANCE_FROM_IPMI_TRANSPORT2_THIS(a) \

+  CR ( \

+  a, \

+  IPMI_BMC_INSTANCE_DATA, \

+  IpmiTransport2, \

+  SM_IPMI_BMC_SIGNATURE \

+  )

+

 #endif // _IPMI_BMC_H_

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h
index 06eab62aae..3b252f5f1c 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Comm
+++ on/IpmiBmcCommon.h
@@ -3,6 +3,7 @@


   @copyright

   Copyright 1999 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. 
+ <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



@@ -55,6 +56,7 @@ typedef struct {
   UINT8               SoftErrorCount;

   UINT16              IpmiIoBase;

   IPMI_TRANSPORT      IpmiTransport;

+  IPMI_TRANSPORT2     IpmiTransport2;

   EFI_HANDLE          IpmiSmmHandle;

 } IPMI_BMC_INSTANCE_DATA;



diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.c
index b2788e5a4c..19e5c1c04b 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Comm
+++ on/IpmiHooks.c
@@ -3,6 +3,7 @@


   @copyright

   Copyright 2014 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. 
+ <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



@@ -48,6 +49,11 @@ Returns:


 --*/

 {

+

+  if (This == NULL) {

+      return EFI_INVALID_PARAMETER;
[Isaac] The preceding line is improperly indented.

+  }

+

   //

   // This Will be unchanged ( BMC/KCS style )

   //

@@ -64,6 +70,251 @@ Returns:
            );

 } // IpmiSendCommand()



+EFI_STATUS

+EFIAPI

+IpmiSendCommand2 (

+  IN      IPMI_TRANSPORT2              *This,

+  IN      UINT8                        NetFunction,

+  IN      UINT8                        Lun,

+  IN      UINT8                        Command,

+  IN      UINT8                        *CommandData,

+  IN      UINT32                       CommandDataSize,

+  IN OUT  UINT8                        *ResponseData,

+  IN OUT  UINT32                       *ResponseDataSize

+  )

+/*++

+

+Routine Description:

+

+  This API use the default interface (PcdDefaultSystemInterface) to 
+ send IPMI command

+  in the right mode to the appropiate device, ME or BMC.

+

+Arguments:

+

+  This              - Pointer to IPMI protocol instance

+  NetFunction       - Net Function of command to send

+  Lun               - LUN of command to send

+  Command           - IPMI command to send

+  CommandData       - Pointer to command data buffer, if needed

+  CommandDataSize   - Size of command data buffer

+  ResponseData      - Pointer to response data buffer

+  ResponseDataSize  - Pointer to response data buffer size

+

+Returns:

+

+  EFI_INVALID_PARAMETER - One of the input values is bad

+  EFI_DEVICE_ERROR      - IPMI command failed

+  EFI_BUFFER_TOO_SMALL  - Response buffer is too small

+  EFI_UNSUPPORTED       - Command is not supported by BMC

+  EFI_SUCCESS           - Command completed successfully

+

+--*/

+{

+

+  IPMI_BMC_INSTANCE_DATA  *IpmiInstance;

+

+  if (This == NULL) {

+      return EFI_INVALID_PARAMETER;
[Isaac] The preceding line is improperly indented.

+  }

+

+  IpmiInstance = INSTANCE_FROM_IPMI_TRANSPORT2_THIS(This);

+

+#if KcsInterfaceSupport
[Isaac] I don't think that using #defines helps the code.  I think obscuring the PCD makes the code harder to understand.  Also not following the MACRO_NAMING_CONVENTION.
If you really prefer the macro, change it to something like IS_KCS_INTERFACE_PCD_ENABLED.
I also prefer using regular C code instead of c preprocessors.  All major toolchains support optimization that should remove any benefit to using #if.  If we use regular if statements, then code is compiled and thus tested for build more regularly.

+  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceKcs) 
+ &&

+      (IpmiInstance->IpmiTransport2.Interface.KcsInterfaceState == 
+ IpmiInterfaceInitialized)) {

+

+      return IpmiSendCommand (

+               &IpmiInstance->IpmiTransport,

+               NetFunction,

+               Lun,

+               Command,

+               CommandData,

+               CommandDataSize,

+               ResponseData,

+               ResponseDataSize

+               );

+  }

+#endif

+

+#if BtInterfaceSupport

+  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceBt) &&

+      (IpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState == 
+ IpmiInterfaceInitialized)) {

+

+      return IpmiBtSendCommandToBmc (

+               &IpmiInstance->IpmiTransport2,

+               NetFunction,

+               Lun,

+               Command,

+               CommandData,

+               (UINT8) CommandDataSize,

+               ResponseData,

+               (UINT8*) ResponseDataSize,

+               NULL

+               );

+  }

+#endif

+

+#if SsifInterfaceSupport

+  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceSsif) 
+ &&

+      (IpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == 
+ IpmiInterfaceInitialized)) {

+

+      return IpmiSsifSendCommandToBmc (

+               &IpmiInstance->IpmiTransport2,

+               NetFunction,

+               Lun,

+               Command,

+               CommandData,

+               (UINT8) CommandDataSize,

+               ResponseData,

+               (UINT8*) ResponseDataSize,

+               NULL

+               );

+  }

+#endif

+

+#if IpmbInterfaceSupport

+  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceIpmb) 
+ &&

+      (IpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == 
+ IpmiInterfaceInitialized)) {

+

+      return IpmiIpmbSendCommandToBmc (

+               &IpmiInstance->IpmiTransport2,

+               NetFunction,

+               Lun,

+               Command,

+               CommandData,

+               (UINT8) CommandDataSize,

+               ResponseData,

+               (UINT8*) ResponseDataSize,

+               NULL

+               );

+  }

+#endif

+  return EFI_UNSUPPORTED;
[Isaac] Should we comment this a bit more?  It isn't really obvious if hitting this is a valid configuration.  Should there be an assert here to indicate that one of the interfaces must be enabled?  Are the function comments about default interface correct?  Same for next function.

+} // IpmiSendCommand2()

+

+EFI_STATUS

+EFIAPI

+IpmiSendCommand2Ex (

+  IN      IPMI_TRANSPORT2              *This,

+  IN      UINT8                        NetFunction,

+  IN      UINT8                        Lun,

+  IN      UINT8                        Command,

+  IN      UINT8                        *CommandData,

+  IN      UINT32                       CommandDataSize,

+  IN OUT  UINT8                        *ResponseData,

+  IN OUT  UINT32                       *ResponseDataSize,

+  IN      SYSTEM_INTERFACE_TYPE        InterfaceType

+  )

+{

+/*++

+Routine Description:

+

+  This API use the specific interface type to send IPMI command

+  in the right mode to the appropiate device, ME or BMC.

+

+Arguments:

+

+  This              - Pointer to IPMI protocol instance

+  NetFunction       - Net Function of command to send

+  Lun               - LUN of command to send

+  Command           - IPMI command to send

+  CommandData       - Pointer to command data buffer, if needed

+  CommandDataSize   - Size of command data buffer

+  ResponseData      - Pointer to response data buffer

+  ResponseDataSize  - Pointer to response data buffer size

+  InterfaceType     - BMC Interface type.

+

+Returns:

+

+  EFI_INVALID_PARAMETER - One of the input values is bad

+  EFI_DEVICE_ERROR      - IPMI command failed

+  EFI_BUFFER_TOO_SMALL  - Response buffer is too small

+  EFI_UNSUPPORTED       - Command is not supported by BMC

+  EFI_SUCCESS           - Command completed successfully

+

+--*/

+

+  IPMI_BMC_INSTANCE_DATA  *IpmiInstance;

+

+  if (This == NULL) {

+      return EFI_INVALID_PARAMETER;

+  }

+

+  IpmiInstance = INSTANCE_FROM_IPMI_TRANSPORT2_THIS(This);

+

+#if KcsInterfaceSupport

+  if ((InterfaceType == SysInterfaceKcs) &&

+      (IpmiInstance->IpmiTransport2.Interface.KcsInterfaceState == 
+ IpmiInterfaceInitialized)) {

+

+      return IpmiSendCommand (

+                &IpmiInstance->IpmiTransport,

+                NetFunction,

+                Lun,

+                Command,

+                CommandData,

+                CommandDataSize,

+                ResponseData,

+                ResponseDataSize

+                );

+     }

+#endif

+

+#if BtInterfaceSupport

+  if ((InterfaceType == SysInterfaceBt) &&

+      (IpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState == 
+ IpmiInterfaceInitialized)) {

+

+      return IpmiBtSendCommandToBmc (

+                &IpmiInstance->IpmiTransport2,

+                NetFunction,

+                Lun,

+                Command,

+                CommandData,

+                (UINT8) CommandDataSize,

+                ResponseData,

+                (UINT8*) ResponseDataSize,

+                NULL

+                );

+  }

+#endif

+

+#if SsifInterfaceSupport

+  if ((InterfaceType == SysInterfaceSsif) &&

+      (IpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == 
+ IpmiInterfaceInitialized)) {

+

+      return IpmiSsifSendCommandToBmc (

+                &IpmiInstance->IpmiTransport2,

+                NetFunction,

+                Lun,

+                Command,

+                CommandData,

+                (UINT8) CommandDataSize,

+                ResponseData,

+                (UINT8*) ResponseDataSize,

+                NULL

+                );

+  }

+#endif

+

+#if IpmbInterfaceSupport

+  if ((InterfaceType == SysInterfaceIpmb) &&

+      (IpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == 
+ IpmiInterfaceInitialized)) {

+

+      return IpmiIpmbSendCommandToBmc (

+                &IpmiInstance->IpmiTransport2,

+                NetFunction,

+                Lun,

+                Command,

+                CommandData,

+                (UINT8) CommandDataSize,

+                ResponseData,

+                (UINT8*) ResponseDataSize,

+                NULL

+                );

+  }

+#endif

+  return EFI_UNSUPPORTED;

+}

+

 EFI_STATUS

 EFIAPI

 IpmiGetBmcStatus (

@@ -89,6 +340,11 @@ Returns:


 --*/

 {

+

+  if ((This == NULL) || (BmcStatus == NULL) || (ComAddress == NULL)) {

+      return EFI_INVALID_PARAMETER;

+  }

+

   return IpmiBmcStatus (

            This,

            BmcStatus,

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.h b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.h
index 823cc08c61..3933e07443 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.h
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Comm
+++ on/IpmiHooks.h
@@ -3,13 +3,21 @@


   @copyright

   Copyright 2014 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. 
+ <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



 #ifndef _IPMI_HOOKS_H

 #define _IPMI_HOOKS_H



-#include "IpmiBmc.h"

+#include <Protocol/IpmiTransportProtocol.h>

+#include <Protocol/IpmiTransport2Protocol.h>

+#include <Library/BtInterfaceLib.h>

+#include <Library/SsifInterfaceLib.h>

+#include <Library/IpmbInterfaceLib.h>

+#include <Library/DebugLib.h>

+#include<IpmiBmcCommon.h>

+#include<IpmiBmc.h>
[Isaac] Add a space before < in the two preceding lines.



 //

 // Internal(hook) function list

@@ -54,6 +62,89 @@ Returns:
 --*/

 ;



+EFI_STATUS

+EFIAPI

+IpmiSendCommand2 (

+  IN      IPMI_TRANSPORT2              *This,

+  IN      UINT8                        NetFunction,

+  IN      UINT8                        Lun,

+  IN      UINT8                        Command,

+  IN      UINT8                        *CommandData,

+  IN      UINT32                       CommandDataSize,

+  IN OUT  UINT8                        *ResponseData,

+  IN OUT  UINT32                       *ResponseDataSize

+  )

+/*++

+

+Routine Description:

+

+  This API use the default interface (PcdDefaultSystemInterface) to 
+ send IPMI command

+  in the right mode to the appropiate device, ME or BMC.

+

+Arguments:

+

+  This              - Pointer to IPMI protocol instance

+  NetFunction       - Net Function of command to send

+  Lun               - LUN of command to send

+  Command           - IPMI command to send

+  CommandData       - Pointer to command data buffer, if needed

+  CommandDataSize   - Size of command data buffer

+  ResponseData      - Pointer to response data buffer

+  ResponseDataSize  - Pointer to response data buffer size

+

+Returns:

+

+  EFI_INVALID_PARAMETER - One of the input values is bad

+  EFI_DEVICE_ERROR      - IPMI command failed

+  EFI_BUFFER_TOO_SMALL  - Response buffer is too small

+  EFI_UNSUPPORTED       - Command is not supported by BMC

+  EFI_SUCCESS           - Command completed successfully

+

+--*/

+;

+

+EFI_STATUS

+EFIAPI

+IpmiSendCommand2Ex (

+  IN      IPMI_TRANSPORT2              *This,

+  IN      UINT8                        NetFunction,

+  IN      UINT8                        Lun,

+  IN      UINT8                        Command,

+  IN      UINT8                        *CommandData,

+  IN      UINT32                       CommandDataSize,

+  IN OUT  UINT8                        *ResponseData,

+  IN OUT  UINT32                       *ResponseDataSize,

+  IN      SYSTEM_INTERFACE_TYPE        InterfaceType

+  )

+/*++

+Routine Description:

+

+  This API use the specific interface type to send IPMI command

+  in the right mode to the appropiate device, ME or BMC.

+

+Arguments:

+

+  This              - Pointer to IPMI protocol instance

+  NetFunction       - Net Function of command to send

+  Lun               - LUN of command to send

+  Command           - IPMI command to send

+  CommandData       - Pointer to command data buffer, if needed

+  CommandDataSize   - Size of command data buffer

+  ResponseData      - Pointer to response data buffer

+  ResponseDataSize  - Pointer to response data buffer size

+  InterfaceType     - BMC Interface type.

+

+Returns:

+

+  EFI_INVALID_PARAMETER - One of the input values is bad

+  EFI_DEVICE_ERROR      - IPMI command failed

+  EFI_BUFFER_TOO_SMALL  - Response buffer is too small

+  EFI_UNSUPPORTED       - Command is not supported by BMC

+  EFI_SUCCESS           - Command completed successfully

+

+--*/

+;

+

 EFI_STATUS

 EFIAPI

 IpmiGetBmcStatus (

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf
index 8d80aeb6b5..1564ceb08a 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
+++ GenericIpmi.inf
@@ -3,6 +3,7 @@
 #

 # @copyright

 # Copyright 2010 - 2021 Intel Corporation. <BR>

+# Copyright (c) 1985 - 2023, American Megatrends International LLC. 
+<BR>

 # SPDX-License-Identifier: BSD-2-Clause-Patent

 ##



@@ -49,16 +50,25 @@
   IoLib

   ReportStatusCodeLib

   TimerLib

+  BmcCommonInterfaceLib

+  BtInterfaceLib

+  SsifInterfaceLib

+  IpmbInterfaceLib



 [Protocols]

   gIpmiTransportProtocolGuid               # PROTOCOL ALWAYS_PRODUCED

   gEfiVideoPrintProtocolGuid

-

-[Guids]

+  gIpmiTransport2ProtocolGuid



 [Pcd]

   gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiIoBaseAddress

   gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiBmcReadyDelayTimer

+  gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort



 [Depex]

   gEfiRuntimeArchProtocolGuid AND

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
index c333ca2e06..74d96f8684 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
+++ IpmiInit.c
@@ -3,6 +3,7 @@


   @copyright

   Copyright 1999 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. 
+ <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



@@ -13,6 +14,7 @@
 #include "IpmiBmc.h"

 #include "IpmiPhysicalLayer.h"

 #include <Library/TimerLib.h>

+#include <Library/BmcCommonInterfaceLib.h>

 #ifdef FAST_VIDEO_SUPPORT

   #include <Protocol/VideoPrint.h>

 #endif

@@ -351,6 +353,130 @@ Returns:
   return Status;

 } // GetDeviceId()



+/**

+    Initialize the API and parameters for IPMI Transport2 Instance

+

+    @param[in] IpmiInstance         Pointer to IPMI Instance

+

+    @return VOID    Nothing.

+

+**/

+VOID

+InitIpmiTransport2 (

+  IN  IPMI_BMC_INSTANCE_DATA *IpmiInstance

+  )

+{

+

+  IpmiInstance->IpmiTransport2.InterfaceType            = FixedPcdGet8 (PcdDefaultSystemInterface);

+  IpmiInstance->IpmiTransport2.IpmiTransport2BmcStatus  = BmcStatusOk;

+  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2       = IpmiSendCommand2;

+  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2Ex     = IpmiSendCommand2Ex;

+

+#if BtInterfaceSupport

+  InitBtInterfaceData(&IpmiInstance->IpmiTransport2);

+#endif

+

+#if SsifInterfaceSupport

+  InitSsifInterfaceData(&IpmiInstance->IpmiTransport2);

+#endif

+

+#if IpmbInterfaceSupport

+  InitIpmbInterfaceData(&IpmiInstance->IpmiTransport2);

+#endif

+}

+

+/**

+    Notify call back function.

+

+    @param[in] Event    Event which caused this handler.

+    @param[in] Context  Context passed during Event Handler registration.

+

+    @return VOID    Nothing.

+

+**/

+VOID

+EFIAPI

+DxeNotifyCallback (

+  IN EFI_EVENT  Event,

+  IN VOID       *Context

+  )

+{

+  EFI_STATUS             Status;

+  IPMI_INTERFACE_STATE   InterfaceState;

+  EFI_HANDLE             Handle;

+

+  InterfaceState = IpmiInterfaceNotReady;

+

+#if SsifInterfaceSupport

+  InitSsifInterfaceData(&mIpmiInstance->IpmiTransport2);

+

+  if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == 
+ IpmiInterfaceInitialized) {

+      InterfaceState = IpmiInterfaceInitialized;

+  }

+#endif

+

+#if IpmbInterfaceSupport

+  InitIpmbInterfaceData(&mIpmiInstance->IpmiTransport2);

+

+  if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == 
+ IpmiInterfaceInitialized) {

+      InterfaceState = IpmiInterfaceInitialized;

+  }

+#endif

+  // Default Interface data should be initialized to install Ipmi Transport2 Protocol.

+  if (InterfaceState != IpmiInterfaceInitialized) {

+      return;

+  }

+

+  Handle = NULL;

+  Status = gBS->InstallProtocolInterface (

+                  &Handle,

+                  &gIpmiTransport2ProtocolGuid,

+                  EFI_NATIVE_INTERFACE,

+                  &mIpmiInstance->IpmiTransport2

+                  );

+  ASSERT_EFI_ERROR (Status);

+}

+

+/**

+    Registers protocol notify call back.

+

+    @param[in] ProtocolGuid     Pointer to Protocol Guid to register

+                                call back.

+

+    @retval EFI_INVALID_PARAMETER   If the ProtocolGuid is 0 or NULL.

+    @retval Others                  Status of call back registration.

+

+**/

+EFI_STATUS

+DxeRegisterProtocolCallback (

+  IN EFI_GUID   *ProtocolGuid

+  )

+{

+  EFI_STATUS  Status;

+  EFI_EVENT   NotifyEvent;

+  VOID        *Registration;

+

+  if ((ProtocolGuid == NULL) ||

+      ((ProtocolGuid != NULL) && IsZeroBuffer (ProtocolGuid, sizeof 
+ (EFI_GUID)))) {

+      return EFI_INVALID_PARAMETER;

+  }

+

+  Status = gBS->CreateEvent (

+                  EVT_NOTIFY_SIGNAL,

+                  TPL_NOTIFY,

+                  DxeNotifyCallback,

+                  NULL,

+                  &NotifyEvent);

+

+  if (!EFI_ERROR (Status)) {

+      Status = gBS->RegisterProtocolNotify (

+                      ProtocolGuid,

+                      NotifyEvent,

+                      &Registration);

+  }

+

+  return Status;

+}



 /**

   This function initializes KCS interface to BMC.

@@ -376,7 +502,10 @@ InitializeIpmiKcsPhysicalLayer (
   UINT8                  ErrorCount;

   EFI_HANDLE             Handle;

   UINT8                  Index;

+  IPMI_INTERFACE_STATE   InterfaceState = IpmiInterfaceNotReady;

+#if KcsInterfaceSupport

   EFI_STATUS_CODE_VALUE  StatusCodeValue[MAX_SOFT_COUNT];

+#endif



   ErrorCount = 0;

   mImageHandle = ImageHandle;

@@ -405,6 +534,8 @@ InitializeIpmiKcsPhysicalLayer (
     mIpmiInstance->Signature                        = SM_IPMI_BMC_SIGNATURE;

     mIpmiInstance->SlaveAddress                     = BMC_SLAVE_ADDRESS;

     mIpmiInstance->BmcStatus                        = BMC_NOTREADY;

+

+#if KcsInterfaceSupport

     mIpmiInstance->IpmiTransport.IpmiSubmitCommand  = IpmiSendCommand;

     mIpmiInstance->IpmiTransport.GetBmcStatus       = IpmiGetBmcStatus;



@@ -454,7 +585,81 @@ InitializeIpmiKcsPhysicalLayer (
                       );

       ASSERT_EFI_ERROR (Status);

     }

+#endif

+

+    // Initialise the IPMI transport2
[Isaac] I find the mismatch in spelling with some use of s and more use of z to be confusing.

+    InitIpmiTransport2(mIpmiInstance);
[Isaac] Space before (

+

+    // Check interface data initialized successfully else register notify protocol.

+    for (Index = SysInterfaceKcs; Index < SysInterfaceMax; Index++) {

+

+        switch (Index) {

+#if KcsInterfaceSupport
[Isaac] A lot of the following code is not indented properly.

+            case SysInterfaceKcs:

+                if ((mIpmiInstance->BmcStatus != BMC_HARDFAIL) && 
+ (mIpmiInstance->BmcStatus != BMC_UPDATE_IN_PROGRESS)) {

+                    BMC_INTERFACE_STATUS  BmcStatus;
[Isaac] I would prefer this at the beginning of the function with other local variables.

+                    
+ mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState = 
+ IpmiInterfaceInitialized;

+                    Status = CheckSelfTestByInterfaceType(
[Isaac] Please insert space before (.  Please add newline between the interesting code and the variable init.

+                                        &mIpmiInstance->IpmiTransport2,

+                                        &BmcStatus,

+                                        SysInterfaceKcs);
[Isaac] Put ); on its own line as per other code conventions.

+                    if (!EFI_ERROR (Status) && (BmcStatus != 
+ BmcStatusHardFail)) {

+                        InterfaceState = IpmiInterfaceInitialized;

+                    } else {

+                        
+ mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState = 
+ IpmiInterfaceInitError;

+                    }

+                }

+                break;

+#endif

+

+#if BtInterfaceSupport

+            case SysInterfaceBt:

+                if 
+ (mIpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState == 
+ IpmiInterfaceInitialized){
[Isaac] Space before {

+                    InterfaceState = IpmiInterfaceInitialized;

+                }

+                break;

+#endif

+

+#if SsifInterfaceSupport

+            case SysInterfaceSsif:

+                if 
+ (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == 
+ IpmiInterfaceInitialized) {

+                    InterfaceState = IpmiInterfaceInitialized;

+                } else if 
+ (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == 
+ IpmiInterfaceInitError) {

+                    // Register protocol notify for SMBUS Protocol.

+                    Status = DxeRegisterProtocolCallback (

+                                
+ &mIpmiInstance->IpmiTransport2.Interface.Ssif.SsifInterfaceApiGuid);
[Isaac] Indent this line 2 characters from the beginning of the function name on the prior line per coding style.  Also put ); on its own line.

+                }

+                break;

+#endif

+#if IpmbInterfaceSupport

+            case SysInterfaceIpmb:

+                if 
+ (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == 
+ IpmiInterfaceInitialized) {

+                    InterfaceState = IpmiInterfaceInitialized;

+                } else if 
+ (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == 
+ IpmiInterfaceInitError) {

+                    // Register Protocol notify for I2C Protocol.

+                    Status = DxeRegisterProtocolCallback (

+                                
+ &mIpmiInstance->IpmiTransport2.Interface.Ipmb.IpmbInterfaceApiGuid);
[Isaac] Put ); on its own line as per other code conventions.

+                }

+                break;

+#endif

+            default:

+                break;

+        }

+    }

+

+    // Any one of the Interface data should be initialized to install IPMI Transport2 Protocol.

+    if (InterfaceState != IpmiInterfaceInitialized) {

+        return EFI_SUCCESS;

+    }



+    Handle = NULL;

+    Status = gBS->InstallProtocolInterface (

+                    &Handle,

+                    &gIpmiTransport2ProtocolGuid,

+                    EFI_NATIVE_INTERFACE,

+                    &mIpmiInstance->IpmiTransport2

+                    );

+    ASSERT_EFI_ERROR (Status);

     return EFI_SUCCESS;

   }

 } // InitializeIpmiKcsPhysicalLayer()

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.c b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.c
index 1af2d18f79..adf59374b3 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.c
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/
+++ SmmGenericIpmi.c
@@ -3,6 +3,7 @@


   @copyright

   Copyright 1999 - 2021 Intel Corporation. <BR>

+  Copyright (c) 1985 - 2023, American Megatrends International LLC. 
+ <BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/



@@ -24,6 +25,7 @@
 #include "IpmiBmcCommon.h"

 #include "IpmiBmc.h"

 #include <Library/TimerLib.h>

+#include <Library/BmcCommonInterfaceLib.h>



 IPMI_BMC_INSTANCE_DATA             *mIpmiInstance;

 EFI_HANDLE                         mImageHandle;

@@ -115,6 +117,123 @@ Returns:
   return Status;

 }



+/**

+    Initialize the API and parameters for IPMI Transport2 Instance
[Isaac] There are a lot of four space indents.

+

+    @param[in] IpmiInstance         Pointer to IPMI Instance

+

+    @return VOID    Nothing.

+

+**/

+VOID

+InitIpmiTransport2 (

+  IN  IPMI_BMC_INSTANCE_DATA    *IpmiInstance

+  )

+{

+

+  IpmiInstance->IpmiTransport2.InterfaceType           = FixedPcdGet8 (PcdDefaultSystemInterface);

+  IpmiInstance->IpmiTransport2.IpmiTransport2BmcStatus = BmcStatusOk;

+  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2      = IpmiSendCommand2;

+  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2Ex    = IpmiSendCommand2Ex;

+

+#if BtInterfaceSupport

+  InitBtInterfaceData (&IpmiInstance->IpmiTransport2);

+#endif

+

+#if SsifInterfaceSupport

+  InitSsifInterfaceData (&IpmiInstance->IpmiTransport2);

+#endif

+

+#if IpmbInterfaceSupport

+  InitIpmbInterfaceData (&IpmiInstance->IpmiTransport2);

+#endif

+}

+

+/**

+    Notify call back to initialize the interfaces and install Smm Ipmi

+    protocol.

+

+    @param[in] Protocol     Pointer to the protocol guid.

+    @param[in] Interface    Pointer to the protocol instance.

+    @param[in] Handle       Handle on which the protocol is installed.

+

+    @return EFI_STATUS  Status of Notify call back.

+

+**/

+EFI_STATUS

+EFIAPI

+SmmNotifyCallback (

+  IN CONST  EFI_GUID    *Protocol,

+  IN        VOID        *Interface,

+  IN        EFI_HANDLE  Handle

+  )

+{

+

+  EFI_STATUS             Status;

+  IPMI_INTERFACE_STATE   InterfaceState;

+

+  InterfaceState = IpmiInterfaceNotReady;

+

+#if SsifInterfaceSupport

+  InitSsifInterfaceData(&mIpmiInstance->IpmiTransport2);

+

+  if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == 
+ IpmiInterfaceInitialized){
[Isaac] Space before {

+      InterfaceState = IpmiInterfaceInitialized;

+  }

+#endif

+

+#if IpmbInterfaceSupport

+  InitIpmbInterfaceData(&mIpmiInstance->IpmiTransport2);

+#endif

+

+  if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == 
+ IpmiInterfaceInitialized){
[Isaac] Space before {

+      InterfaceState = IpmiInterfaceInitialized;

+  }

+  if (InterfaceState != IpmiInterfaceInitialized) {

+      return EFI_SUCCESS;

+  }

+

+  // Default Interface data should be initialized to install Ipmi Transport2 Protocol.

+  if (InterfaceState == IpmiInterfaceInitialized) {

+      Handle = NULL;

+      Status = gSmst->SmmInstallProtocolInterface (

+                          &Handle,

+                          &gSmmIpmiTransport2ProtocolGuid,

+                          EFI_NATIVE_INTERFACE,

+                          &mIpmiInstance->IpmiTransport2

+                          );

+  }

+  return EFI_SUCCESS;

+}

+

+/**

+    Registers Protocol call back

+

+    @param ProtocolGuid       Pointer to Protocol GUID to register call back

+

+    @retval EFI_INVALID_PARAMETER   If the ProtocolGuid is 0 or NULL.

+    @retval Others                  Status of Notify registration.

+**/

+EFI_STATUS

+SmmRegisterProtocolCallback (

+  IN  EFI_GUID  *ProtocolGuid

+)
[Isaac] Not Indented the same way other functions are.

+{

+    EFI_STATUS  Status;

+    VOID        *Registration;

+

+    if ((ProtocolGuid == NULL) ||

+        ((ProtocolGuid != NULL) && IsZeroBuffer (ProtocolGuid, sizeof 
+ (EFI_GUID)))) {

+        return EFI_INVALID_PARAMETER;

+    }

+

+    Status = gSmst->SmmRegisterProtocolNotify (

+                        ProtocolGuid,

+                        SmmNotifyCallback,

+                        &Registration );

+    return Status;

+}

+

 EFI_STATUS

 SmmInitializeIpmiKcsPhysicalLayer (

   IN EFI_HANDLE             ImageHandle,

@@ -142,10 +261,13 @@ Returns:
   UINT8                            ErrorCount;

   EFI_HANDLE                       Handle;

   EFI_STATUS_CODE_VALUE            StatusCodeValue[MAX_SOFT_COUNT];

+  IPMI_INTERFACE_STATE             InterfaceState;

+  UINT8                            Index;



   DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer entry \n"));

-  ErrorCount = 0;

-  mImageHandle = ImageHandle;

+  ErrorCount     = 0;

+  mImageHandle   = ImageHandle;

+  InterfaceState = IpmiInterfaceNotReady;



   mIpmiInstance = AllocateZeroPool (sizeof (IPMI_BMC_INSTANCE_DATA));

   ASSERT (mIpmiInstance != NULL);

@@ -170,6 +292,7 @@ Returns:
     mIpmiInstance->IpmiTransport.IpmiSubmitCommand  = IpmiSendCommand;

     mIpmiInstance->IpmiTransport.GetBmcStatus       = IpmiGetBmcStatus;



+#if KcsInterfaceSupport

     DEBUG ((DEBUG_INFO,"IPMI: Waiting for Getting BMC DID in SMM \n"));

     //

     // Get the Device ID and check if the system is in Force Update mode.

@@ -191,6 +314,79 @@ Returns:
                       &mIpmiInstance->IpmiTransport

                       );

     ASSERT_EFI_ERROR (Status);

+#endif

+

+    InitIpmiTransport2(mIpmiInstance);
[Isaac] Space before (

+

+    // Check interface data initialized successfully else register notify protocol.

+    for (Index = SysInterfaceKcs; Index < SysInterfaceMax; Index++) {

+

+        switch (Index) {

+#if KcsInterfaceSupport

+            case SysInterfaceKcs:

+                if ((mIpmiInstance->BmcStatus != BMC_HARDFAIL) && 
+ (mIpmiInstance->BmcStatus != BMC_UPDATE_IN_PROGRESS)) {

+                    BMC_INTERFACE_STATUS  BmcStatus;
[Isaac] Better to be with other local variables at beginning of function.

+                    
+ mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState = 
+ IpmiInterfaceInitialized;

+                    Status = CheckSelfTestByInterfaceType(
[Isaac] Insert space before (

+                                               
+ &mIpmiInstance->IpmiTransport2,

+                                               &BmcStatus,

+                                               SysInterfaceKcs);
[Isaac] Place ); on own line.

+                    if (!EFI_ERROR (Status) && (BmcStatus != 
+ BmcStatusHardFail)) {

+                        InterfaceState = IpmiInterfaceInitialized;

+                    } else {

+                        
+ mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState = 
+ IpmiInterfaceInitError;

+                    }

+                }

+                break;

+#endif

+

+#if BtInterfaceSupport

+            case SysInterfaceBt:

+                if 
+ (mIpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState == 
+ IpmiInterfaceInitialized){
[Isaac] Insert space before {

+                    InterfaceState = IpmiInterfaceInitialized;

+                }

+                break;

+#endif

+

+#if SsifInterfaceSupport

+            case SysInterfaceSsif:

+                if 
+ (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == 
+ IpmiInterfaceInitialized){
[Isaac] Insert space before {

+                    InterfaceState = IpmiInterfaceInitialized;

+                } else if 
+ (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == 
+ IpmiInterfaceInitError) {

+                    // Register protocol notify for SMBUS Protocol.

+                    Status = SmmRegisterProtocolCallback 
+ (&mIpmiInstance->IpmiTransport2.Interface.Ssif.SsifInterfaceApiGuid);

+                }

+                break;

+#endif

+

+#if IpmbInterfaceSupport

+            case SysInterfaceIpmb:

+                if 
+ (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == 
+ IpmiInterfaceInitialized){

+                    InterfaceState = IpmiInterfaceInitialized;

+                } else if 
+ (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == 
+ IpmiInterfaceInitError) {

+                    // Register protocol notify for SMBUS Protocol.

+                    Status = SmmRegisterProtocolCallback 
+ (&mIpmiInstance->IpmiTransport2.Interface.Ipmb.IpmbInterfaceApiGuid);

+                }

+                break;

+#endif

+            default:

+                break;

+        }

+    }

+

+    // Default Interface data should be initialized to install Ipmi Transport2 Protocol.

+    if (InterfaceState == IpmiInterfaceInitialized) {

+        Handle = NULL;

+        Status = gSmst->SmmInstallProtocolInterface (

+                          &Handle,

+                          &gSmmIpmiTransport2ProtocolGuid,

+                          EFI_NATIVE_INTERFACE,

+                          &mIpmiInstance->IpmiTransport2

+                          );

+        if (EFI_ERROR(Status)) {

+            DEBUG ((DEBUG_ERROR,"IPMI Transport2 protocol install 
+ Status = %r \n",Status));

+        }

+    }



     DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer exit \n"));



diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf
index f430195d1e..12dc17ae84 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/
+++ SmmGenericIpmi.inf
@@ -3,6 +3,7 @@
 #

 # @copyright

 # Copyright 2010 - 2021 Intel Corporation. <BR>

+# Copyright (c) 1985 - 2023, American Megatrends International LLC. 
+<BR>

 # SPDX-License-Identifier: BSD-2-Clause-Patent

 ##



@@ -39,15 +40,26 @@
   IoLib

   ReportStatusCodeLib

   TimerLib

+  BmcCommonInterfaceLib

+  BtInterfaceLib

+  SsifInterfaceLib

+  IpmbInterfaceLib



 [Protocols]

   gSmmIpmiTransportProtocolGuid                     # PROTOCOL ALWAYS_PRODUCED

+  gSmmIpmiTransport2ProtocolGuid                    # PROTOCOL ALWAYS_PRODUCED



 [Guids]



 [Pcd]

   gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiSmmIoBaseAddress

   gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiBmcReadyDelayTimer

+  gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport

+  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport



 [Depex]

  gIpmiTransportProtocolGuid

diff --git a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.dec b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.dec
index 8c1b902446..2131ec475b 100644
--- a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.dec
+++ b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
+++ ec
@@ -9,6 +9,7 @@
 #

 # Copyright (c) 2019-2021, Intel Corporation. All rights reserved.<BR>

 # Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.<BR>

+# Copyright (c) 1985 - 2023, American Megatrends International LLC. 
+<BR>

 #

 # SPDX-License-Identifier: BSD-2-Clause-Patent

 #

@@ -43,16 +44,26 @@
   #

   IpmiBaseLib|Include/Library/IpmiBaseLib.h



+  ##  @libraryclass  Provides generic functions among all interfaces.

+  BmcCommonInterfaceLib|Include/Library/BmcCommonInterfaceLib.h

+  BtInterfaceLib|Include/Library/BtInterfaceLib.h

+  SsifInterfaceLib|Include/Library/SsifInterfaceLib.h

+  IpmbInterfaceLib|Include/Library/IpmbInterfaceLib.h

+

 [Guids]

   gIpmiFeaturePkgTokenSpaceGuid  =  {0xc05283f6, 0xd6a8, 0x48f3, {0x9b, 0x59, 0xfb, 0xca, 0x71, 0x32, 0x0f, 0x12}}

+  gPeiIpmiHobGuid                = {0xcb4d3e13, 0x1e34, 0x4373, {0x8a, 0x81, 0xe9, 0x0, 0x10, 0xf1, 0xdb, 0xa4}}



 [Ppis]

   gPeiIpmiTransportPpiGuid = {0x7bf5fecc, 0xc5b5, 0x4b25, {0x81, 0x1b, 0xb4, 0xb5, 0xb, 0x28, 0x79, 0xf7}}

+  gPeiIpmiTransport2PpiGuid = {0x8122CEBD, 0xF4FD, 0x4EA8, { 0x97, 
+ 0x6C, 0xF0, 0x30, 0xAD, 0xDC, 0x4C, 0xB4 }}



 [Protocols]

   gIpmiTransportProtocolGuid  = {0x6bb945e8, 0x3743, 0x433e, {0xb9, 0x0e, 0x29, 0xb3, 0x0d, 0x5d, 0xc6, 0x30}}

   gSmmIpmiTransportProtocolGuid  = {0x8bb070f1, 0xa8f3, 0x471d, {0x86, 0x16, 0x77, 0x4b, 0xa3, 0xf4, 0x30, 0xa0}}

   gEfiVideoPrintProtocolGuid     = {0x3dbf3e06, 0x9d0c, 0x40d3, {0xb2, 0x17, 0x45, 0x5f, 0x33, 0x9e, 0x29, 0x09}}

+  gIpmiTransport2ProtocolGuid = { 0x4A1D0E66, 0x5271, 0x4E22, { 0x83, 
+ 0xFE, 0x90, 0x92, 0x1B, 0x74, 0x82, 0x13 }}

+  gSmmIpmiTransport2ProtocolGuid = { 0x1DBD1503, 0x0A60, 0x4230, { 
+ 0xAA, 0xA3, 0x80, 0x16, 0xD8, 0xC3, 0xDE, 0x2F }}



 [PcdsFeatureFlag]

   gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiFeatureEnable|FALSE|BOOLEAN|0xA0000001

@@ -61,6 +72,40 @@
   gIpmiFeaturePkgTokenSpaceGuid.PcdMaxSOLChannels|3|UINT8|0xF0000001

   #When True, BIOS will send a Pre-Boot signal to BMC

   gIpmiFeaturePkgTokenSpaceGuid.PcdSignalPreBootToBmc|FALSE|BOOLEAN|0xF0000002

+  #  typedef enum {

+  #      SysInterfaceUnknown, // Unknown interface type.

+  #      SysInterfaceKcs,     // Kcs interface = 1.

+  #      SysInterfaceSmic,    // Smic interface = 2.

+  #      SysInterfaceBt,      // Bt interface = 3.

+  #      SysInterfaceSsif,    // Ssif interface = 4.

+  #      SysInterfaceMax   // Maximum interface type.

+  #  } SYSTEM_INTERFACE_TYPE;

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface|1|UINT8|0xF000
+ 0003

+

+  #BT Base address, retry counter and delay parameters

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdBtCommandRetryCounter|0x0004E400|UINT
+ 32|0xF0000004

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort|0xE4|UINT16|0xF0000005

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtBufferPort|0xE5|UINT16|0xF0000006

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtDelayPerRetry|15|UINT32|0xF0000007

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterruptMaskPort|0xE6|UINT16|0xF00
+ 00008

+  gIpmiFeaturePkgTokenSpaceGuid.PcdBtBufferSize|0x40|UINT8|0xF0000009

+

+  #SSIF slave address, retry counter and delay parameters

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdSsifSlaveAddress|0x10|UINT16|0xF00000
+ 0A

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdSsifRequestRetriesDelay|0xCB20|UINT32
+ |0xF000000B

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdSsifCommandtRetryCounter|0x5|UINT16|0
+ xF000000C

+

+   #Interface access type for BMC communication. 0-MMIO, 1-IO

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiDefaultAccessType|1|UINT8|0xF0000
+ 00D

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdMmioBaseAddress|0x0|UINT32|0xF000000E

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdBaseAddressRange|0x0|UINT32|0xF000000
+ F

+

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdBmcSlaveAddress|0x20|UINT32|0xF000001
+ 0

+

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport|1|UINT8|0xF000001
+ 1

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport|1|UINT8|0xF0000012

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport|1|UINT8|0xF00000
+ 13

+  
+ gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport|1|UINT8|0xF00000
+ 14



 [PcdsDynamic, PcdsDynamicEx]

   gIpmiFeaturePkgTokenSpaceGuid.PcdFRB2EnabledFlag|TRUE|BOOLEAN|0xD0000001

--
2.38.1.windows.1
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel][edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM
  2023-06-13  0:27 ` Isaac Oram
@ 2023-06-13  4:35   ` Chang, Abner
  2023-06-13 17:12     ` Isaac Oram
  2023-06-21  6:38     ` [edk2-devel] [edk2-platforms][PATCH " Arun K
  0 siblings, 2 replies; 6+ messages in thread
From: Chang, Abner @ 2023-06-13  4:35 UTC (permalink / raw)
  To: devel@edk2.groups.io, isaac.w.oram@intel.com, Arun K
  Cc: Desimone, Nathaniel L, Ramkumar Krishnamoorthi, Gao, Liming

[AMD Official Use Only - General]

Hi Isaac,
I had replied to AMI and had them to work on the ManageabilityPkg directly (I think it was few months ago) as we migrated all IPMI stuff from Intel IpmiFeaturePkg to ManageabilityPkg.
Do you think we still have to spend effort on IpmiFeaturePkg? AMD also has one IPMI feature waiting for upstream to edk2-platform, work on both packages will lead to divergency.
I suggest we just have AMI to revise their code to compliant with ManageabilityPkg.
Regards,
Abner

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Isaac
> Oram via groups.io
> Sent: Tuesday, June 13, 2023 8:27 AM
> To: Arun K <arunk@ami.com>; devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Ramkumar
> Krishnamoorthi <ramkumark@ami.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>
> Subject: Re: [edk2-devel][edk2-platforms][PATCH V3-1]
> IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Comments inline, prefaced with "[Isaac]".  Mostly style things that you could
> probably fix with uncrustify also.
>
> The main concern is the controls seem confused with #defines and PCD.
> Simplifying to just use the PCD directly seems clear to me and eliminating the
> #if logic in favor of regular C logic is also preferred.
>
> Thanks,
> Isaac
>
> -----Original Message-----
> From: Arun K <arunk@ami.com>
> Sent: Monday, June 12, 2023 5:52 AM
> To: devel@edk2.groups.io; Arun K <arunk@ami.com>
> Cc: Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Ramkumar Krishnamoorthi
> <ramkumark@ami.com>; Gao, Liming <gaoliming@byosoft.com.cn>
> Subject: [edk2-devel][edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided
> multiple IPMI interface support in DXE and SMM
>
> Created IpmiTransport2 PPI/Protocol to support multiple IPMI BMC Interface
> support such as KCS/BT/SSIF with 2 API's
> IpmiSubmitCommand2 & IpmiSubmitCommand2Ex.
> IpmiSubmitCommand2 - This API use the default interface
> (PcdDefaultSystemInterface) to send IPMI command.
> IpmiSubmitCommand2Ex - This API use the specific interface type to send
> IPMI command which is passed as an argument.
>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>
> Signed-off-by: Arun K <arunk@ami.com>
> ---
>  .../GenericIpmi/Common/IpmiBmc.h              |  10 +
>  .../GenericIpmi/Common/IpmiBmcCommon.h        |   2 +
>  .../GenericIpmi/Common/IpmiHooks.c            | 256 ++++++++++++++++++
>  .../GenericIpmi/Common/IpmiHooks.h            |  93 ++++++-
>  .../GenericIpmi/Dxe/GenericIpmi.inf           |  14 +-
>  .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 205 ++++++++++++++
>  .../GenericIpmi/Smm/SmmGenericIpmi.c          | 200 +++++++++++++-
>  .../GenericIpmi/Smm/SmmGenericIpmi.inf        |  12 +
>  .../IpmiFeaturePkg/IpmiFeaturePkg.dec         |  45 +++
>  9 files changed, 832 insertions(+), 5 deletions(-)
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmc.h
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmc.h
> index d306a085e5..19fb2a26a3 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmc.h
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> m
> +++ on/IpmiBmc.h
> @@ -3,6 +3,7 @@
>
>
>    @copyright
>
>    Copyright 1999 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
> @@ -21,6 +22,7 @@
>  #include <Library/ReportStatusCodeLib.h>
>
>  #include <Library/IpmiBaseLib.h>
>
>  #include <Protocol/IpmiTransportProtocol.h>
>
> +#include <Protocol/IpmiTransport2Protocol.h>
>
>
>
>  #include "IpmiBmcCommon.h"
>
>  #include "KcsBmc.h"
>
> @@ -41,4 +43,12 @@
>    SM_IPMI_BMC_SIGNATURE \
>
>    )
>
>
>
> +#define INSTANCE_FROM_IPMI_TRANSPORT2_THIS(a) \
>
> +  CR ( \
>
> +  a, \
>
> +  IPMI_BMC_INSTANCE_DATA, \
>
> +  IpmiTransport2, \
>
> +  SM_IPMI_BMC_SIGNATURE \
>
> +  )
>
> +
>
>  #endif // _IPMI_BMC_H_
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmcCommon.h
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmcCommon.h
> index 06eab62aae..3b252f5f1c 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmcCommon.h
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> m
> +++ on/IpmiBmcCommon.h
> @@ -3,6 +3,7 @@
>
>
>    @copyright
>
>    Copyright 1999 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
> @@ -55,6 +56,7 @@ typedef struct {
>    UINT8               SoftErrorCount;
>
>    UINT16              IpmiIoBase;
>
>    IPMI_TRANSPORT      IpmiTransport;
>
> +  IPMI_TRANSPORT2     IpmiTransport2;
>
>    EFI_HANDLE          IpmiSmmHandle;
>
>  } IPMI_BMC_INSTANCE_DATA;
>
>
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.c
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.c
> index b2788e5a4c..19e5c1c04b 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.c
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> m
> +++ on/IpmiHooks.c
> @@ -3,6 +3,7 @@
>
>
>    @copyright
>
>    Copyright 2014 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
> @@ -48,6 +49,11 @@ Returns:
>
>
>  --*/
>
>  {
>
> +
>
> +  if (This == NULL) {
>
> +      return EFI_INVALID_PARAMETER;
> [Isaac] The preceding line is improperly indented.
>
> +  }
>
> +
>
>    //
>
>    // This Will be unchanged ( BMC/KCS style )
>
>    //
>
> @@ -64,6 +70,251 @@ Returns:
>             );
>
>  } // IpmiSendCommand()
>
>
>
> +EFI_STATUS
>
> +EFIAPI
>
> +IpmiSendCommand2 (
>
> +  IN      IPMI_TRANSPORT2              *This,
>
> +  IN      UINT8                        NetFunction,
>
> +  IN      UINT8                        Lun,
>
> +  IN      UINT8                        Command,
>
> +  IN      UINT8                        *CommandData,
>
> +  IN      UINT32                       CommandDataSize,
>
> +  IN OUT  UINT8                        *ResponseData,
>
> +  IN OUT  UINT32                       *ResponseDataSize
>
> +  )
>
> +/*++
>
> +
>
> +Routine Description:
>
> +
>
> +  This API use the default interface (PcdDefaultSystemInterface) to
> + send IPMI command
>
> +  in the right mode to the appropiate device, ME or BMC.
>
> +
>
> +Arguments:
>
> +
>
> +  This              - Pointer to IPMI protocol instance
>
> +  NetFunction       - Net Function of command to send
>
> +  Lun               - LUN of command to send
>
> +  Command           - IPMI command to send
>
> +  CommandData       - Pointer to command data buffer, if needed
>
> +  CommandDataSize   - Size of command data buffer
>
> +  ResponseData      - Pointer to response data buffer
>
> +  ResponseDataSize  - Pointer to response data buffer size
>
> +
>
> +Returns:
>
> +
>
> +  EFI_INVALID_PARAMETER - One of the input values is bad
>
> +  EFI_DEVICE_ERROR      - IPMI command failed
>
> +  EFI_BUFFER_TOO_SMALL  - Response buffer is too small
>
> +  EFI_UNSUPPORTED       - Command is not supported by BMC
>
> +  EFI_SUCCESS           - Command completed successfully
>
> +
>
> +--*/
>
> +{
>
> +
>
> +  IPMI_BMC_INSTANCE_DATA  *IpmiInstance;
>
> +
>
> +  if (This == NULL) {
>
> +      return EFI_INVALID_PARAMETER;
> [Isaac] The preceding line is improperly indented.
>
> +  }
>
> +
>
> +  IpmiInstance = INSTANCE_FROM_IPMI_TRANSPORT2_THIS(This);
>
> +
>
> +#if KcsInterfaceSupport
> [Isaac] I don't think that using #defines helps the code.  I think obscuring the
> PCD makes the code harder to understand.  Also not following the
> MACRO_NAMING_CONVENTION.
> If you really prefer the macro, change it to something like
> IS_KCS_INTERFACE_PCD_ENABLED.
> I also prefer using regular C code instead of c preprocessors.  All major
> toolchains support optimization that should remove any benefit to using #if.
> If we use regular if statements, then code is compiled and thus tested for build
> more regularly.
>
> +  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceKcs)
> + &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.KcsInterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiSendCommand (
>
> +               &IpmiInstance->IpmiTransport,
>
> +               NetFunction,
>
> +               Lun,
>
> +               Command,
>
> +               CommandData,
>
> +               CommandDataSize,
>
> +               ResponseData,
>
> +               ResponseDataSize
>
> +               );
>
> +  }
>
> +#endif
>
> +
>
> +#if BtInterfaceSupport
>
> +  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceBt) &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiBtSendCommandToBmc (
>
> +               &IpmiInstance->IpmiTransport2,
>
> +               NetFunction,
>
> +               Lun,
>
> +               Command,
>
> +               CommandData,
>
> +               (UINT8) CommandDataSize,
>
> +               ResponseData,
>
> +               (UINT8*) ResponseDataSize,
>
> +               NULL
>
> +               );
>
> +  }
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceSsif)
> + &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiSsifSendCommandToBmc (
>
> +               &IpmiInstance->IpmiTransport2,
>
> +               NetFunction,
>
> +               Lun,
>
> +               Command,
>
> +               CommandData,
>
> +               (UINT8) CommandDataSize,
>
> +               ResponseData,
>
> +               (UINT8*) ResponseDataSize,
>
> +               NULL
>
> +               );
>
> +  }
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceIpmb)
> + &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiIpmbSendCommandToBmc (
>
> +               &IpmiInstance->IpmiTransport2,
>
> +               NetFunction,
>
> +               Lun,
>
> +               Command,
>
> +               CommandData,
>
> +               (UINT8) CommandDataSize,
>
> +               ResponseData,
>
> +               (UINT8*) ResponseDataSize,
>
> +               NULL
>
> +               );
>
> +  }
>
> +#endif
>
> +  return EFI_UNSUPPORTED;
> [Isaac] Should we comment this a bit more?  It isn't really obvious if hitting this
> is a valid configuration.  Should there be an assert here to indicate that one of
> the interfaces must be enabled?  Are the function comments about default
> interface correct?  Same for next function.
>
> +} // IpmiSendCommand2()
>
> +
>
> +EFI_STATUS
>
> +EFIAPI
>
> +IpmiSendCommand2Ex (
>
> +  IN      IPMI_TRANSPORT2              *This,
>
> +  IN      UINT8                        NetFunction,
>
> +  IN      UINT8                        Lun,
>
> +  IN      UINT8                        Command,
>
> +  IN      UINT8                        *CommandData,
>
> +  IN      UINT32                       CommandDataSize,
>
> +  IN OUT  UINT8                        *ResponseData,
>
> +  IN OUT  UINT32                       *ResponseDataSize,
>
> +  IN      SYSTEM_INTERFACE_TYPE        InterfaceType
>
> +  )
>
> +{
>
> +/*++
>
> +Routine Description:
>
> +
>
> +  This API use the specific interface type to send IPMI command
>
> +  in the right mode to the appropiate device, ME or BMC.
>
> +
>
> +Arguments:
>
> +
>
> +  This              - Pointer to IPMI protocol instance
>
> +  NetFunction       - Net Function of command to send
>
> +  Lun               - LUN of command to send
>
> +  Command           - IPMI command to send
>
> +  CommandData       - Pointer to command data buffer, if needed
>
> +  CommandDataSize   - Size of command data buffer
>
> +  ResponseData      - Pointer to response data buffer
>
> +  ResponseDataSize  - Pointer to response data buffer size
>
> +  InterfaceType     - BMC Interface type.
>
> +
>
> +Returns:
>
> +
>
> +  EFI_INVALID_PARAMETER - One of the input values is bad
>
> +  EFI_DEVICE_ERROR      - IPMI command failed
>
> +  EFI_BUFFER_TOO_SMALL  - Response buffer is too small
>
> +  EFI_UNSUPPORTED       - Command is not supported by BMC
>
> +  EFI_SUCCESS           - Command completed successfully
>
> +
>
> +--*/
>
> +
>
> +  IPMI_BMC_INSTANCE_DATA  *IpmiInstance;
>
> +
>
> +  if (This == NULL) {
>
> +      return EFI_INVALID_PARAMETER;
>
> +  }
>
> +
>
> +  IpmiInstance = INSTANCE_FROM_IPMI_TRANSPORT2_THIS(This);
>
> +
>
> +#if KcsInterfaceSupport
>
> +  if ((InterfaceType == SysInterfaceKcs) &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.KcsInterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiSendCommand (
>
> +                &IpmiInstance->IpmiTransport,
>
> +                NetFunction,
>
> +                Lun,
>
> +                Command,
>
> +                CommandData,
>
> +                CommandDataSize,
>
> +                ResponseData,
>
> +                ResponseDataSize
>
> +                );
>
> +     }
>
> +#endif
>
> +
>
> +#if BtInterfaceSupport
>
> +  if ((InterfaceType == SysInterfaceBt) &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiBtSendCommandToBmc (
>
> +                &IpmiInstance->IpmiTransport2,
>
> +                NetFunction,
>
> +                Lun,
>
> +                Command,
>
> +                CommandData,
>
> +                (UINT8) CommandDataSize,
>
> +                ResponseData,
>
> +                (UINT8*) ResponseDataSize,
>
> +                NULL
>
> +                );
>
> +  }
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +  if ((InterfaceType == SysInterfaceSsif) &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiSsifSendCommandToBmc (
>
> +                &IpmiInstance->IpmiTransport2,
>
> +                NetFunction,
>
> +                Lun,
>
> +                Command,
>
> +                CommandData,
>
> +                (UINT8) CommandDataSize,
>
> +                ResponseData,
>
> +                (UINT8*) ResponseDataSize,
>
> +                NULL
>
> +                );
>
> +  }
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  if ((InterfaceType == SysInterfaceIpmb) &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiIpmbSendCommandToBmc (
>
> +                &IpmiInstance->IpmiTransport2,
>
> +                NetFunction,
>
> +                Lun,
>
> +                Command,
>
> +                CommandData,
>
> +                (UINT8) CommandDataSize,
>
> +                ResponseData,
>
> +                (UINT8*) ResponseDataSize,
>
> +                NULL
>
> +                );
>
> +  }
>
> +#endif
>
> +  return EFI_UNSUPPORTED;
>
> +}
>
> +
>
>  EFI_STATUS
>
>  EFIAPI
>
>  IpmiGetBmcStatus (
>
> @@ -89,6 +340,11 @@ Returns:
>
>
>  --*/
>
>  {
>
> +
>
> +  if ((This == NULL) || (BmcStatus == NULL) || (ComAddress == NULL)) {
>
> +      return EFI_INVALID_PARAMETER;
>
> +  }
>
> +
>
>    return IpmiBmcStatus (
>
>             This,
>
>             BmcStatus,
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.h
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.h
> index 823cc08c61..3933e07443 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.h
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> m
> +++ on/IpmiHooks.h
> @@ -3,13 +3,21 @@
>
>
>    @copyright
>
>    Copyright 2014 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
>  #ifndef _IPMI_HOOKS_H
>
>  #define _IPMI_HOOKS_H
>
>
>
> -#include "IpmiBmc.h"
>
> +#include <Protocol/IpmiTransportProtocol.h>
>
> +#include <Protocol/IpmiTransport2Protocol.h>
>
> +#include <Library/BtInterfaceLib.h>
>
> +#include <Library/SsifInterfaceLib.h>
>
> +#include <Library/IpmbInterfaceLib.h>
>
> +#include <Library/DebugLib.h>
>
> +#include<IpmiBmcCommon.h>
>
> +#include<IpmiBmc.h>
> [Isaac] Add a space before < in the two preceding lines.
>
>
>
>  //
>
>  // Internal(hook) function list
>
> @@ -54,6 +62,89 @@ Returns:
>  --*/
>
>  ;
>
>
>
> +EFI_STATUS
>
> +EFIAPI
>
> +IpmiSendCommand2 (
>
> +  IN      IPMI_TRANSPORT2              *This,
>
> +  IN      UINT8                        NetFunction,
>
> +  IN      UINT8                        Lun,
>
> +  IN      UINT8                        Command,
>
> +  IN      UINT8                        *CommandData,
>
> +  IN      UINT32                       CommandDataSize,
>
> +  IN OUT  UINT8                        *ResponseData,
>
> +  IN OUT  UINT32                       *ResponseDataSize
>
> +  )
>
> +/*++
>
> +
>
> +Routine Description:
>
> +
>
> +  This API use the default interface (PcdDefaultSystemInterface) to
> + send IPMI command
>
> +  in the right mode to the appropiate device, ME or BMC.
>
> +
>
> +Arguments:
>
> +
>
> +  This              - Pointer to IPMI protocol instance
>
> +  NetFunction       - Net Function of command to send
>
> +  Lun               - LUN of command to send
>
> +  Command           - IPMI command to send
>
> +  CommandData       - Pointer to command data buffer, if needed
>
> +  CommandDataSize   - Size of command data buffer
>
> +  ResponseData      - Pointer to response data buffer
>
> +  ResponseDataSize  - Pointer to response data buffer size
>
> +
>
> +Returns:
>
> +
>
> +  EFI_INVALID_PARAMETER - One of the input values is bad
>
> +  EFI_DEVICE_ERROR      - IPMI command failed
>
> +  EFI_BUFFER_TOO_SMALL  - Response buffer is too small
>
> +  EFI_UNSUPPORTED       - Command is not supported by BMC
>
> +  EFI_SUCCESS           - Command completed successfully
>
> +
>
> +--*/
>
> +;
>
> +
>
> +EFI_STATUS
>
> +EFIAPI
>
> +IpmiSendCommand2Ex (
>
> +  IN      IPMI_TRANSPORT2              *This,
>
> +  IN      UINT8                        NetFunction,
>
> +  IN      UINT8                        Lun,
>
> +  IN      UINT8                        Command,
>
> +  IN      UINT8                        *CommandData,
>
> +  IN      UINT32                       CommandDataSize,
>
> +  IN OUT  UINT8                        *ResponseData,
>
> +  IN OUT  UINT32                       *ResponseDataSize,
>
> +  IN      SYSTEM_INTERFACE_TYPE        InterfaceType
>
> +  )
>
> +/*++
>
> +Routine Description:
>
> +
>
> +  This API use the specific interface type to send IPMI command
>
> +  in the right mode to the appropiate device, ME or BMC.
>
> +
>
> +Arguments:
>
> +
>
> +  This              - Pointer to IPMI protocol instance
>
> +  NetFunction       - Net Function of command to send
>
> +  Lun               - LUN of command to send
>
> +  Command           - IPMI command to send
>
> +  CommandData       - Pointer to command data buffer, if needed
>
> +  CommandDataSize   - Size of command data buffer
>
> +  ResponseData      - Pointer to response data buffer
>
> +  ResponseDataSize  - Pointer to response data buffer size
>
> +  InterfaceType     - BMC Interface type.
>
> +
>
> +Returns:
>
> +
>
> +  EFI_INVALID_PARAMETER - One of the input values is bad
>
> +  EFI_DEVICE_ERROR      - IPMI command failed
>
> +  EFI_BUFFER_TOO_SMALL  - Response buffer is too small
>
> +  EFI_UNSUPPORTED       - Command is not supported by BMC
>
> +  EFI_SUCCESS           - Command completed successfully
>
> +
>
> +--*/
>
> +;
>
> +
>
>  EFI_STATUS
>
>  EFIAPI
>
>  IpmiGetBmcStatus (
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> GenericIpmi.inf
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> GenericIpmi.inf
> index 8d80aeb6b5..1564ceb08a 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> GenericIpmi.inf
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> +++ GenericIpmi.inf
> @@ -3,6 +3,7 @@
>  #
>
>  # @copyright
>
>  # Copyright 2010 - 2021 Intel Corporation. <BR>
>
> +# Copyright (c) 1985 - 2023, American Megatrends International LLC.
> +<BR>
>
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  ##
>
>
>
> @@ -49,16 +50,25 @@
>    IoLib
>
>    ReportStatusCodeLib
>
>    TimerLib
>
> +  BmcCommonInterfaceLib
>
> +  BtInterfaceLib
>
> +  SsifInterfaceLib
>
> +  IpmbInterfaceLib
>
>
>
>  [Protocols]
>
>    gIpmiTransportProtocolGuid               # PROTOCOL ALWAYS_PRODUCED
>
>    gEfiVideoPrintProtocolGuid
>
> -
>
> -[Guids]
>
> +  gIpmiTransport2ProtocolGuid
>
>
>
>  [Pcd]
>
>    gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiIoBaseAddress
>
>    gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiBmcReadyDelayTimer
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort
>
>
>
>  [Depex]
>
>    gEfiRuntimeArchProtocolGuid AND
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> IpmiInit.c
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> IpmiInit.c
> index c333ca2e06..74d96f8684 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> IpmiInit.c
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> +++ IpmiInit.c
> @@ -3,6 +3,7 @@
>
>
>    @copyright
>
>    Copyright 1999 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
> @@ -13,6 +14,7 @@
>  #include "IpmiBmc.h"
>
>  #include "IpmiPhysicalLayer.h"
>
>  #include <Library/TimerLib.h>
>
> +#include <Library/BmcCommonInterfaceLib.h>
>
>  #ifdef FAST_VIDEO_SUPPORT
>
>    #include <Protocol/VideoPrint.h>
>
>  #endif
>
> @@ -351,6 +353,130 @@ Returns:
>    return Status;
>
>  } // GetDeviceId()
>
>
>
> +/**
>
> +    Initialize the API and parameters for IPMI Transport2 Instance
>
> +
>
> +    @param[in] IpmiInstance         Pointer to IPMI Instance
>
> +
>
> +    @return VOID    Nothing.
>
> +
>
> +**/
>
> +VOID
>
> +InitIpmiTransport2 (
>
> +  IN  IPMI_BMC_INSTANCE_DATA *IpmiInstance
>
> +  )
>
> +{
>
> +
>
> +  IpmiInstance->IpmiTransport2.InterfaceType            = FixedPcdGet8
> (PcdDefaultSystemInterface);
>
> +  IpmiInstance->IpmiTransport2.IpmiTransport2BmcStatus  = BmcStatusOk;
>
> +  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2       =
> IpmiSendCommand2;
>
> +  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2Ex     =
> IpmiSendCommand2Ex;
>
> +
>
> +#if BtInterfaceSupport
>
> +  InitBtInterfaceData(&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +  InitSsifInterfaceData(&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  InitIpmbInterfaceData(&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +}
>
> +
>
> +/**
>
> +    Notify call back function.
>
> +
>
> +    @param[in] Event    Event which caused this handler.
>
> +    @param[in] Context  Context passed during Event Handler registration.
>
> +
>
> +    @return VOID    Nothing.
>
> +
>
> +**/
>
> +VOID
>
> +EFIAPI
>
> +DxeNotifyCallback (
>
> +  IN EFI_EVENT  Event,
>
> +  IN VOID       *Context
>
> +  )
>
> +{
>
> +  EFI_STATUS             Status;
>
> +  IPMI_INTERFACE_STATE   InterfaceState;
>
> +  EFI_HANDLE             Handle;
>
> +
>
> +  InterfaceState = IpmiInterfaceNotReady;
>
> +
>
> +#if SsifInterfaceSupport
>
> +  InitSsifInterfaceData(&mIpmiInstance->IpmiTransport2);
>
> +
>
> +  if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized) {
>
> +      InterfaceState = IpmiInterfaceInitialized;
>
> +  }
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  InitIpmbInterfaceData(&mIpmiInstance->IpmiTransport2);
>
> +
>
> +  if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized) {
>
> +      InterfaceState = IpmiInterfaceInitialized;
>
> +  }
>
> +#endif
>
> +  // Default Interface data should be initialized to install Ipmi Transport2
> Protocol.
>
> +  if (InterfaceState != IpmiInterfaceInitialized) {
>
> +      return;
>
> +  }
>
> +
>
> +  Handle = NULL;
>
> +  Status = gBS->InstallProtocolInterface (
>
> +                  &Handle,
>
> +                  &gIpmiTransport2ProtocolGuid,
>
> +                  EFI_NATIVE_INTERFACE,
>
> +                  &mIpmiInstance->IpmiTransport2
>
> +                  );
>
> +  ASSERT_EFI_ERROR (Status);
>
> +}
>
> +
>
> +/**
>
> +    Registers protocol notify call back.
>
> +
>
> +    @param[in] ProtocolGuid     Pointer to Protocol Guid to register
>
> +                                call back.
>
> +
>
> +    @retval EFI_INVALID_PARAMETER   If the ProtocolGuid is 0 or NULL.
>
> +    @retval Others                  Status of call back registration.
>
> +
>
> +**/
>
> +EFI_STATUS
>
> +DxeRegisterProtocolCallback (
>
> +  IN EFI_GUID   *ProtocolGuid
>
> +  )
>
> +{
>
> +  EFI_STATUS  Status;
>
> +  EFI_EVENT   NotifyEvent;
>
> +  VOID        *Registration;
>
> +
>
> +  if ((ProtocolGuid == NULL) ||
>
> +      ((ProtocolGuid != NULL) && IsZeroBuffer (ProtocolGuid, sizeof
> + (EFI_GUID)))) {
>
> +      return EFI_INVALID_PARAMETER;
>
> +  }
>
> +
>
> +  Status = gBS->CreateEvent (
>
> +                  EVT_NOTIFY_SIGNAL,
>
> +                  TPL_NOTIFY,
>
> +                  DxeNotifyCallback,
>
> +                  NULL,
>
> +                  &NotifyEvent);
>
> +
>
> +  if (!EFI_ERROR (Status)) {
>
> +      Status = gBS->RegisterProtocolNotify (
>
> +                      ProtocolGuid,
>
> +                      NotifyEvent,
>
> +                      &Registration);
>
> +  }
>
> +
>
> +  return Status;
>
> +}
>
>
>
>  /**
>
>    This function initializes KCS interface to BMC.
>
> @@ -376,7 +502,10 @@ InitializeIpmiKcsPhysicalLayer (
>    UINT8                  ErrorCount;
>
>    EFI_HANDLE             Handle;
>
>    UINT8                  Index;
>
> +  IPMI_INTERFACE_STATE   InterfaceState = IpmiInterfaceNotReady;
>
> +#if KcsInterfaceSupport
>
>    EFI_STATUS_CODE_VALUE  StatusCodeValue[MAX_SOFT_COUNT];
>
> +#endif
>
>
>
>    ErrorCount = 0;
>
>    mImageHandle = ImageHandle;
>
> @@ -405,6 +534,8 @@ InitializeIpmiKcsPhysicalLayer (
>      mIpmiInstance->Signature                        = SM_IPMI_BMC_SIGNATURE;
>
>      mIpmiInstance->SlaveAddress                     = BMC_SLAVE_ADDRESS;
>
>      mIpmiInstance->BmcStatus                        = BMC_NOTREADY;
>
> +
>
> +#if KcsInterfaceSupport
>
>      mIpmiInstance->IpmiTransport.IpmiSubmitCommand  =
> IpmiSendCommand;
>
>      mIpmiInstance->IpmiTransport.GetBmcStatus       = IpmiGetBmcStatus;
>
>
>
> @@ -454,7 +585,81 @@ InitializeIpmiKcsPhysicalLayer (
>                        );
>
>        ASSERT_EFI_ERROR (Status);
>
>      }
>
> +#endif
>
> +
>
> +    // Initialise the IPMI transport2
> [Isaac] I find the mismatch in spelling with some use of s and more use of z to
> be confusing.
>
> +    InitIpmiTransport2(mIpmiInstance);
> [Isaac] Space before (
>
> +
>
> +    // Check interface data initialized successfully else register notify protocol.
>
> +    for (Index = SysInterfaceKcs; Index < SysInterfaceMax; Index++) {
>
> +
>
> +        switch (Index) {
>
> +#if KcsInterfaceSupport
> [Isaac] A lot of the following code is not indented properly.
>
> +            case SysInterfaceKcs:
>
> +                if ((mIpmiInstance->BmcStatus != BMC_HARDFAIL) &&
> + (mIpmiInstance->BmcStatus != BMC_UPDATE_IN_PROGRESS)) {
>
> +                    BMC_INTERFACE_STATUS  BmcStatus;
> [Isaac] I would prefer this at the beginning of the function with other local
> variables.
>
> +
> + mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState =
> + IpmiInterfaceInitialized;
>
> +                    Status = CheckSelfTestByInterfaceType(
> [Isaac] Please insert space before (.  Please add newline between the
> interesting code and the variable init.
>
> +                                        &mIpmiInstance->IpmiTransport2,
>
> +                                        &BmcStatus,
>
> +                                        SysInterfaceKcs);
> [Isaac] Put ); on its own line as per other code conventions.
>
> +                    if (!EFI_ERROR (Status) && (BmcStatus !=
> + BmcStatusHardFail)) {
>
> +                        InterfaceState = IpmiInterfaceInitialized;
>
> +                    } else {
>
> +
> + mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState =
> + IpmiInterfaceInitError;
>
> +                    }
>
> +                }
>
> +                break;
>
> +#endif
>
> +
>
> +#if BtInterfaceSupport
>
> +            case SysInterfaceBt:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState ==
> + IpmiInterfaceInitialized){
> [Isaac] Space before {
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                }
>
> +                break;
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +            case SysInterfaceSsif:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized) {
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                } else if
> + (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitError) {
>
> +                    // Register protocol notify for SMBUS Protocol.
>
> +                    Status = DxeRegisterProtocolCallback (
>
> +
> + &mIpmiInstance->IpmiTransport2.Interface.Ssif.SsifInterfaceApiGuid);
> [Isaac] Indent this line 2 characters from the beginning of the function name
> on the prior line per coding style.  Also put ); on its own line.
>
> +                }
>
> +                break;
>
> +#endif
>
> +#if IpmbInterfaceSupport
>
> +            case SysInterfaceIpmb:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized) {
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                } else if
> + (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitError) {
>
> +                    // Register Protocol notify for I2C Protocol.
>
> +                    Status = DxeRegisterProtocolCallback (
>
> +
> + &mIpmiInstance->IpmiTransport2.Interface.Ipmb.IpmbInterfaceApiGuid);
> [Isaac] Put ); on its own line as per other code conventions.
>
> +                }
>
> +                break;
>
> +#endif
>
> +            default:
>
> +                break;
>
> +        }
>
> +    }
>
> +
>
> +    // Any one of the Interface data should be initialized to install IPMI
> Transport2 Protocol.
>
> +    if (InterfaceState != IpmiInterfaceInitialized) {
>
> +        return EFI_SUCCESS;
>
> +    }
>
>
>
> +    Handle = NULL;
>
> +    Status = gBS->InstallProtocolInterface (
>
> +                    &Handle,
>
> +                    &gIpmiTransport2ProtocolGuid,
>
> +                    EFI_NATIVE_INTERFACE,
>
> +                    &mIpmiInstance->IpmiTransport2
>
> +                    );
>
> +    ASSERT_EFI_ERROR (Status);
>
>      return EFI_SUCCESS;
>
>    }
>
>  } // InitializeIpmiKcsPhysicalLayer()
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.c
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.c
> index 1af2d18f79..adf59374b3 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.c
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/
> +++ SmmGenericIpmi.c
> @@ -3,6 +3,7 @@
>
>
>    @copyright
>
>    Copyright 1999 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
> @@ -24,6 +25,7 @@
>  #include "IpmiBmcCommon.h"
>
>  #include "IpmiBmc.h"
>
>  #include <Library/TimerLib.h>
>
> +#include <Library/BmcCommonInterfaceLib.h>
>
>
>
>  IPMI_BMC_INSTANCE_DATA             *mIpmiInstance;
>
>  EFI_HANDLE                         mImageHandle;
>
> @@ -115,6 +117,123 @@ Returns:
>    return Status;
>
>  }
>
>
>
> +/**
>
> +    Initialize the API and parameters for IPMI Transport2 Instance
> [Isaac] There are a lot of four space indents.
>
> +
>
> +    @param[in] IpmiInstance         Pointer to IPMI Instance
>
> +
>
> +    @return VOID    Nothing.
>
> +
>
> +**/
>
> +VOID
>
> +InitIpmiTransport2 (
>
> +  IN  IPMI_BMC_INSTANCE_DATA    *IpmiInstance
>
> +  )
>
> +{
>
> +
>
> +  IpmiInstance->IpmiTransport2.InterfaceType           = FixedPcdGet8
> (PcdDefaultSystemInterface);
>
> +  IpmiInstance->IpmiTransport2.IpmiTransport2BmcStatus = BmcStatusOk;
>
> +  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2      =
> IpmiSendCommand2;
>
> +  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2Ex    =
> IpmiSendCommand2Ex;
>
> +
>
> +#if BtInterfaceSupport
>
> +  InitBtInterfaceData (&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +  InitSsifInterfaceData (&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  InitIpmbInterfaceData (&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +}
>
> +
>
> +/**
>
> +    Notify call back to initialize the interfaces and install Smm Ipmi
>
> +    protocol.
>
> +
>
> +    @param[in] Protocol     Pointer to the protocol guid.
>
> +    @param[in] Interface    Pointer to the protocol instance.
>
> +    @param[in] Handle       Handle on which the protocol is installed.
>
> +
>
> +    @return EFI_STATUS  Status of Notify call back.
>
> +
>
> +**/
>
> +EFI_STATUS
>
> +EFIAPI
>
> +SmmNotifyCallback (
>
> +  IN CONST  EFI_GUID    *Protocol,
>
> +  IN        VOID        *Interface,
>
> +  IN        EFI_HANDLE  Handle
>
> +  )
>
> +{
>
> +
>
> +  EFI_STATUS             Status;
>
> +  IPMI_INTERFACE_STATE   InterfaceState;
>
> +
>
> +  InterfaceState = IpmiInterfaceNotReady;
>
> +
>
> +#if SsifInterfaceSupport
>
> +  InitSsifInterfaceData(&mIpmiInstance->IpmiTransport2);
>
> +
>
> +  if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized){
> [Isaac] Space before {
>
> +      InterfaceState = IpmiInterfaceInitialized;
>
> +  }
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  InitIpmbInterfaceData(&mIpmiInstance->IpmiTransport2);
>
> +#endif
>
> +
>
> +  if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized){
> [Isaac] Space before {
>
> +      InterfaceState = IpmiInterfaceInitialized;
>
> +  }
>
> +  if (InterfaceState != IpmiInterfaceInitialized) {
>
> +      return EFI_SUCCESS;
>
> +  }
>
> +
>
> +  // Default Interface data should be initialized to install Ipmi Transport2
> Protocol.
>
> +  if (InterfaceState == IpmiInterfaceInitialized) {
>
> +      Handle = NULL;
>
> +      Status = gSmst->SmmInstallProtocolInterface (
>
> +                          &Handle,
>
> +                          &gSmmIpmiTransport2ProtocolGuid,
>
> +                          EFI_NATIVE_INTERFACE,
>
> +                          &mIpmiInstance->IpmiTransport2
>
> +                          );
>
> +  }
>
> +  return EFI_SUCCESS;
>
> +}
>
> +
>
> +/**
>
> +    Registers Protocol call back
>
> +
>
> +    @param ProtocolGuid       Pointer to Protocol GUID to register call back
>
> +
>
> +    @retval EFI_INVALID_PARAMETER   If the ProtocolGuid is 0 or NULL.
>
> +    @retval Others                  Status of Notify registration.
>
> +**/
>
> +EFI_STATUS
>
> +SmmRegisterProtocolCallback (
>
> +  IN  EFI_GUID  *ProtocolGuid
>
> +)
> [Isaac] Not Indented the same way other functions are.
>
> +{
>
> +    EFI_STATUS  Status;
>
> +    VOID        *Registration;
>
> +
>
> +    if ((ProtocolGuid == NULL) ||
>
> +        ((ProtocolGuid != NULL) && IsZeroBuffer (ProtocolGuid, sizeof
> + (EFI_GUID)))) {
>
> +        return EFI_INVALID_PARAMETER;
>
> +    }
>
> +
>
> +    Status = gSmst->SmmRegisterProtocolNotify (
>
> +                        ProtocolGuid,
>
> +                        SmmNotifyCallback,
>
> +                        &Registration );
>
> +    return Status;
>
> +}
>
> +
>
>  EFI_STATUS
>
>  SmmInitializeIpmiKcsPhysicalLayer (
>
>    IN EFI_HANDLE             ImageHandle,
>
> @@ -142,10 +261,13 @@ Returns:
>    UINT8                            ErrorCount;
>
>    EFI_HANDLE                       Handle;
>
>    EFI_STATUS_CODE_VALUE            StatusCodeValue[MAX_SOFT_COUNT];
>
> +  IPMI_INTERFACE_STATE             InterfaceState;
>
> +  UINT8                            Index;
>
>
>
>    DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer entry \n"));
>
> -  ErrorCount = 0;
>
> -  mImageHandle = ImageHandle;
>
> +  ErrorCount     = 0;
>
> +  mImageHandle   = ImageHandle;
>
> +  InterfaceState = IpmiInterfaceNotReady;
>
>
>
>    mIpmiInstance = AllocateZeroPool (sizeof (IPMI_BMC_INSTANCE_DATA));
>
>    ASSERT (mIpmiInstance != NULL);
>
> @@ -170,6 +292,7 @@ Returns:
>      mIpmiInstance->IpmiTransport.IpmiSubmitCommand  =
> IpmiSendCommand;
>
>      mIpmiInstance->IpmiTransport.GetBmcStatus       = IpmiGetBmcStatus;
>
>
>
> +#if KcsInterfaceSupport
>
>      DEBUG ((DEBUG_INFO,"IPMI: Waiting for Getting BMC DID in SMM \n"));
>
>      //
>
>      // Get the Device ID and check if the system is in Force Update mode.
>
> @@ -191,6 +314,79 @@ Returns:
>                        &mIpmiInstance->IpmiTransport
>
>                        );
>
>      ASSERT_EFI_ERROR (Status);
>
> +#endif
>
> +
>
> +    InitIpmiTransport2(mIpmiInstance);
> [Isaac] Space before (
>
> +
>
> +    // Check interface data initialized successfully else register notify protocol.
>
> +    for (Index = SysInterfaceKcs; Index < SysInterfaceMax; Index++) {
>
> +
>
> +        switch (Index) {
>
> +#if KcsInterfaceSupport
>
> +            case SysInterfaceKcs:
>
> +                if ((mIpmiInstance->BmcStatus != BMC_HARDFAIL) &&
> + (mIpmiInstance->BmcStatus != BMC_UPDATE_IN_PROGRESS)) {
>
> +                    BMC_INTERFACE_STATUS  BmcStatus;
> [Isaac] Better to be with other local variables at beginning of function.
>
> +
> + mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState =
> + IpmiInterfaceInitialized;
>
> +                    Status = CheckSelfTestByInterfaceType(
> [Isaac] Insert space before (
>
> +
> + &mIpmiInstance->IpmiTransport2,
>
> +                                               &BmcStatus,
>
> +                                               SysInterfaceKcs);
> [Isaac] Place ); on own line.
>
> +                    if (!EFI_ERROR (Status) && (BmcStatus !=
> + BmcStatusHardFail)) {
>
> +                        InterfaceState = IpmiInterfaceInitialized;
>
> +                    } else {
>
> +
> + mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState =
> + IpmiInterfaceInitError;
>
> +                    }
>
> +                }
>
> +                break;
>
> +#endif
>
> +
>
> +#if BtInterfaceSupport
>
> +            case SysInterfaceBt:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState ==
> + IpmiInterfaceInitialized){
> [Isaac] Insert space before {
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                }
>
> +                break;
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +            case SysInterfaceSsif:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized){
> [Isaac] Insert space before {
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                } else if
> + (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitError) {
>
> +                    // Register protocol notify for SMBUS Protocol.
>
> +                    Status = SmmRegisterProtocolCallback
> + (&mIpmiInstance->IpmiTransport2.Interface.Ssif.SsifInterfaceApiGuid);
>
> +                }
>
> +                break;
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +            case SysInterfaceIpmb:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized){
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                } else if
> + (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitError) {
>
> +                    // Register protocol notify for SMBUS Protocol.
>
> +                    Status = SmmRegisterProtocolCallback
> + (&mIpmiInstance->IpmiTransport2.Interface.Ipmb.IpmbInterfaceApiGuid);
>
> +                }
>
> +                break;
>
> +#endif
>
> +            default:
>
> +                break;
>
> +        }
>
> +    }
>
> +
>
> +    // Default Interface data should be initialized to install Ipmi Transport2
> Protocol.
>
> +    if (InterfaceState == IpmiInterfaceInitialized) {
>
> +        Handle = NULL;
>
> +        Status = gSmst->SmmInstallProtocolInterface (
>
> +                          &Handle,
>
> +                          &gSmmIpmiTransport2ProtocolGuid,
>
> +                          EFI_NATIVE_INTERFACE,
>
> +                          &mIpmiInstance->IpmiTransport2
>
> +                          );
>
> +        if (EFI_ERROR(Status)) {
>
> +            DEBUG ((DEBUG_ERROR,"IPMI Transport2 protocol install
> + Status = %r \n",Status));
>
> +        }
>
> +    }
>
>
>
>      DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer exit \n"));
>
>
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.inf
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.inf
> index f430195d1e..12dc17ae84 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.inf
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/
> +++ SmmGenericIpmi.inf
> @@ -3,6 +3,7 @@
>  #
>
>  # @copyright
>
>  # Copyright 2010 - 2021 Intel Corporation. <BR>
>
> +# Copyright (c) 1985 - 2023, American Megatrends International LLC.
> +<BR>
>
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  ##
>
>
>
> @@ -39,15 +40,26 @@
>    IoLib
>
>    ReportStatusCodeLib
>
>    TimerLib
>
> +  BmcCommonInterfaceLib
>
> +  BtInterfaceLib
>
> +  SsifInterfaceLib
>
> +  IpmbInterfaceLib
>
>
>
>  [Protocols]
>
>    gSmmIpmiTransportProtocolGuid                     # PROTOCOL
> ALWAYS_PRODUCED
>
> +  gSmmIpmiTransport2ProtocolGuid                    # PROTOCOL
> ALWAYS_PRODUCED
>
>
>
>  [Guids]
>
>
>
>  [Pcd]
>
>    gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiSmmIoBaseAddress
>
>    gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiBmcReadyDelayTimer
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport
>
>
>
>  [Depex]
>
>   gIpmiTransportProtocolGuid
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> ec
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> ec
> index 8c1b902446..2131ec475b 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> ec
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> +++ ec
> @@ -9,6 +9,7 @@
>  #
>
>  # Copyright (c) 2019-2021, Intel Corporation. All rights reserved.<BR>
>
>  # Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.<BR>
>
> +# Copyright (c) 1985 - 2023, American Megatrends International LLC.
> +<BR>
>
>  #
>
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  #
>
> @@ -43,16 +44,26 @@
>    #
>
>    IpmiBaseLib|Include/Library/IpmiBaseLib.h
>
>
>
> +  ##  @libraryclass  Provides generic functions among all interfaces.
>
> +  BmcCommonInterfaceLib|Include/Library/BmcCommonInterfaceLib.h
>
> +  BtInterfaceLib|Include/Library/BtInterfaceLib.h
>
> +  SsifInterfaceLib|Include/Library/SsifInterfaceLib.h
>
> +  IpmbInterfaceLib|Include/Library/IpmbInterfaceLib.h
>
> +
>
>  [Guids]
>
>    gIpmiFeaturePkgTokenSpaceGuid  =  {0xc05283f6, 0xd6a8, 0x48f3, {0x9b,
> 0x59, 0xfb, 0xca, 0x71, 0x32, 0x0f, 0x12}}
>
> +  gPeiIpmiHobGuid                = {0xcb4d3e13, 0x1e34, 0x4373, {0x8a, 0x81,
> 0xe9, 0x0, 0x10, 0xf1, 0xdb, 0xa4}}
>
>
>
>  [Ppis]
>
>    gPeiIpmiTransportPpiGuid = {0x7bf5fecc, 0xc5b5, 0x4b25, {0x81, 0x1b,
> 0xb4, 0xb5, 0xb, 0x28, 0x79, 0xf7}}
>
> +  gPeiIpmiTransport2PpiGuid = {0x8122CEBD, 0xF4FD, 0x4EA8, { 0x97,
> + 0x6C, 0xF0, 0x30, 0xAD, 0xDC, 0x4C, 0xB4 }}
>
>
>
>  [Protocols]
>
>    gIpmiTransportProtocolGuid  = {0x6bb945e8, 0x3743, 0x433e, {0xb9, 0x0e,
> 0x29, 0xb3, 0x0d, 0x5d, 0xc6, 0x30}}
>
>    gSmmIpmiTransportProtocolGuid  = {0x8bb070f1, 0xa8f3, 0x471d, {0x86,
> 0x16, 0x77, 0x4b, 0xa3, 0xf4, 0x30, 0xa0}}
>
>    gEfiVideoPrintProtocolGuid     = {0x3dbf3e06, 0x9d0c, 0x40d3, {0xb2, 0x17,
> 0x45, 0x5f, 0x33, 0x9e, 0x29, 0x09}}
>
> +  gIpmiTransport2ProtocolGuid = { 0x4A1D0E66, 0x5271, 0x4E22, { 0x83,
> + 0xFE, 0x90, 0x92, 0x1B, 0x74, 0x82, 0x13 }}
>
> +  gSmmIpmiTransport2ProtocolGuid = { 0x1DBD1503, 0x0A60, 0x4230, {
> + 0xAA, 0xA3, 0x80, 0x16, 0xD8, 0xC3, 0xDE, 0x2F }}
>
>
>
>  [PcdsFeatureFlag]
>
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiFeatureEnable|FALSE|BOOLEAN|0x
> A0000001
>
> @@ -61,6 +72,40 @@
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdMaxSOLChannels|3|UINT8|0xF000000
> 1
>
>    #When True, BIOS will send a Pre-Boot signal to BMC
>
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdSignalPreBootToBmc|FALSE|BOOLEAN|
> 0xF0000002
>
> +  #  typedef enum {
>
> +  #      SysInterfaceUnknown, // Unknown interface type.
>
> +  #      SysInterfaceKcs,     // Kcs interface = 1.
>
> +  #      SysInterfaceSmic,    // Smic interface = 2.
>
> +  #      SysInterfaceBt,      // Bt interface = 3.
>
> +  #      SysInterfaceSsif,    // Ssif interface = 4.
>
> +  #      SysInterfaceMax   // Maximum interface type.
>
> +  #  } SYSTEM_INTERFACE_TYPE;
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface|1|UINT8|0xF00
> 0
> + 0003
>
> +
>
> +  #BT Base address, retry counter and delay parameters
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtCommandRetryCounter|0x0004E400
> |UINT
> + 32|0xF0000004
>
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort|0xE4|UINT16|0xF00000
> 05
>
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtBufferPort|0xE5|UINT16|0xF000000
> 6
>
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtDelayPerRetry|15|UINT32|0xF00000
> 07
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterruptMaskPort|0xE6|UINT16|0xF
> 00
> + 00008
>
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtBufferSize|0x40|UINT8|0xF0000009
>
> +
>
> +  #SSIF slave address, retry counter and delay parameters
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdSsifSlaveAddress|0x10|UINT16|0xF000
> 00
> + 0A
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdSsifRequestRetriesDelay|0xCB20|UINT3
> 2
> + |0xF000000B
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdSsifCommandtRetryCounter|0x5|UINT1
> 6|0
> + xF000000C
>
> +
>
> +   #Interface access type for BMC communication. 0-MMIO, 1-IO
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiDefaultAccessType|1|UINT8|0xF00
> 00
> + 00D
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdMmioBaseAddress|0x0|UINT32|0xF00
> 0000E
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBaseAddressRange|0x0|UINT32|0xF00
> 0000
> + F
>
> +
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBmcSlaveAddress|0x20|UINT32|0xF00
> 0001
> + 0
>
> +
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport|1|UINT8|0xF0000
> 01
> + 1
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport|1|UINT8|0xF00000
> 12
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport|1|UINT8|0xF0000
> 0
> + 13
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport|1|UINT8|0xF00
> 000
> + 14
>
>
>
>  [PcdsDynamic, PcdsDynamicEx]
>
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdFRB2EnabledFlag|TRUE|BOOLEAN|0xD
> 0000001
>
> --
> 2.38.1.windows.1
> -The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI). This communication is intended
> to be read only by the individual or entity to whom it is addressed or by their
> designee. If the reader of this message is not the intended recipient, you are
> on notice that any distribution of this message, in any form, is strictly
> prohibited. Please promptly notify the sender by reply e-mail or by telephone
> at 770-246-8600, and then delete or destroy all copies of the transmission.
>
>
> 
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel][edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM
  2023-06-13  4:35   ` Chang, Abner
@ 2023-06-13 17:12     ` Isaac Oram
  2023-06-21  6:38     ` [edk2-devel] [edk2-platforms][PATCH " Arun K
  1 sibling, 0 replies; 6+ messages in thread
From: Isaac Oram @ 2023-06-13 17:12 UTC (permalink / raw)
  To: Chang, Abner, devel@edk2.groups.io, Arun K
  Cc: Desimone, Nathaniel L, Ramkumar Krishnamoorthi, Gao, Liming

I agree that less work and duplication is generally preferable.  I don't have an opinion in this case.  My experience is that a transition to a new solution takes years.  If there is a benefit to having both implementations, I am fine with that.

Regards,
Isaac

-----Original Message-----
From: Chang, Abner <Abner.Chang@amd.com> 
Sent: Monday, June 12, 2023 9:36 PM
To: devel@edk2.groups.io; Oram, Isaac W <isaac.w.oram@intel.com>; Arun K <arunk@ami.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Ramkumar Krishnamoorthi <ramkumark@ami.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: RE: [edk2-devel][edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM

[AMD Official Use Only - General]

Hi Isaac,
I had replied to AMI and had them to work on the ManageabilityPkg directly (I think it was few months ago) as we migrated all IPMI stuff from Intel IpmiFeaturePkg to ManageabilityPkg.
Do you think we still have to spend effort on IpmiFeaturePkg? AMD also has one IPMI feature waiting for upstream to edk2-platform, work on both packages will lead to divergency.
I suggest we just have AMI to revise their code to compliant with ManageabilityPkg.
Regards,
Abner

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Isaac
> Oram via groups.io
> Sent: Tuesday, June 13, 2023 8:27 AM
> To: Arun K <arunk@ami.com>; devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Ramkumar
> Krishnamoorthi <ramkumark@ami.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>
> Subject: Re: [edk2-devel][edk2-platforms][PATCH V3-1]
> IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Comments inline, prefaced with "[Isaac]".  Mostly style things that you could
> probably fix with uncrustify also.
>
> The main concern is the controls seem confused with #defines and PCD.
> Simplifying to just use the PCD directly seems clear to me and eliminating the
> #if logic in favor of regular C logic is also preferred.
>
> Thanks,
> Isaac
>
> -----Original Message-----
> From: Arun K <arunk@ami.com>
> Sent: Monday, June 12, 2023 5:52 AM
> To: devel@edk2.groups.io; Arun K <arunk@ami.com>
> Cc: Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Ramkumar Krishnamoorthi
> <ramkumark@ami.com>; Gao, Liming <gaoliming@byosoft.com.cn>
> Subject: [edk2-devel][edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided
> multiple IPMI interface support in DXE and SMM
>
> Created IpmiTransport2 PPI/Protocol to support multiple IPMI BMC Interface
> support such as KCS/BT/SSIF with 2 API's
> IpmiSubmitCommand2 & IpmiSubmitCommand2Ex.
> IpmiSubmitCommand2 - This API use the default interface
> (PcdDefaultSystemInterface) to send IPMI command.
> IpmiSubmitCommand2Ex - This API use the specific interface type to send
> IPMI command which is passed as an argument.
>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>
> Signed-off-by: Arun K <arunk@ami.com>
> ---
>  .../GenericIpmi/Common/IpmiBmc.h              |  10 +
>  .../GenericIpmi/Common/IpmiBmcCommon.h        |   2 +
>  .../GenericIpmi/Common/IpmiHooks.c            | 256 ++++++++++++++++++
>  .../GenericIpmi/Common/IpmiHooks.h            |  93 ++++++-
>  .../GenericIpmi/Dxe/GenericIpmi.inf           |  14 +-
>  .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 205 ++++++++++++++
>  .../GenericIpmi/Smm/SmmGenericIpmi.c          | 200 +++++++++++++-
>  .../GenericIpmi/Smm/SmmGenericIpmi.inf        |  12 +
>  .../IpmiFeaturePkg/IpmiFeaturePkg.dec         |  45 +++
>  9 files changed, 832 insertions(+), 5 deletions(-)
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmc.h
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmc.h
> index d306a085e5..19fb2a26a3 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmc.h
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> m
> +++ on/IpmiBmc.h
> @@ -3,6 +3,7 @@
>
>
>    @copyright
>
>    Copyright 1999 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
> @@ -21,6 +22,7 @@
>  #include <Library/ReportStatusCodeLib.h>
>
>  #include <Library/IpmiBaseLib.h>
>
>  #include <Protocol/IpmiTransportProtocol.h>
>
> +#include <Protocol/IpmiTransport2Protocol.h>
>
>
>
>  #include "IpmiBmcCommon.h"
>
>  #include "KcsBmc.h"
>
> @@ -41,4 +43,12 @@
>    SM_IPMI_BMC_SIGNATURE \
>
>    )
>
>
>
> +#define INSTANCE_FROM_IPMI_TRANSPORT2_THIS(a) \
>
> +  CR ( \
>
> +  a, \
>
> +  IPMI_BMC_INSTANCE_DATA, \
>
> +  IpmiTransport2, \
>
> +  SM_IPMI_BMC_SIGNATURE \
>
> +  )
>
> +
>
>  #endif // _IPMI_BMC_H_
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmcCommon.h
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmcCommon.h
> index 06eab62aae..3b252f5f1c 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiBmcCommon.h
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> m
> +++ on/IpmiBmcCommon.h
> @@ -3,6 +3,7 @@
>
>
>    @copyright
>
>    Copyright 1999 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
> @@ -55,6 +56,7 @@ typedef struct {
>    UINT8               SoftErrorCount;
>
>    UINT16              IpmiIoBase;
>
>    IPMI_TRANSPORT      IpmiTransport;
>
> +  IPMI_TRANSPORT2     IpmiTransport2;
>
>    EFI_HANDLE          IpmiSmmHandle;
>
>  } IPMI_BMC_INSTANCE_DATA;
>
>
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.c
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.c
> index b2788e5a4c..19e5c1c04b 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.c
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> m
> +++ on/IpmiHooks.c
> @@ -3,6 +3,7 @@
>
>
>    @copyright
>
>    Copyright 2014 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
> @@ -48,6 +49,11 @@ Returns:
>
>
>  --*/
>
>  {
>
> +
>
> +  if (This == NULL) {
>
> +      return EFI_INVALID_PARAMETER;
> [Isaac] The preceding line is improperly indented.
>
> +  }
>
> +
>
>    //
>
>    // This Will be unchanged ( BMC/KCS style )
>
>    //
>
> @@ -64,6 +70,251 @@ Returns:
>             );
>
>  } // IpmiSendCommand()
>
>
>
> +EFI_STATUS
>
> +EFIAPI
>
> +IpmiSendCommand2 (
>
> +  IN      IPMI_TRANSPORT2              *This,
>
> +  IN      UINT8                        NetFunction,
>
> +  IN      UINT8                        Lun,
>
> +  IN      UINT8                        Command,
>
> +  IN      UINT8                        *CommandData,
>
> +  IN      UINT32                       CommandDataSize,
>
> +  IN OUT  UINT8                        *ResponseData,
>
> +  IN OUT  UINT32                       *ResponseDataSize
>
> +  )
>
> +/*++
>
> +
>
> +Routine Description:
>
> +
>
> +  This API use the default interface (PcdDefaultSystemInterface) to
> + send IPMI command
>
> +  in the right mode to the appropiate device, ME or BMC.
>
> +
>
> +Arguments:
>
> +
>
> +  This              - Pointer to IPMI protocol instance
>
> +  NetFunction       - Net Function of command to send
>
> +  Lun               - LUN of command to send
>
> +  Command           - IPMI command to send
>
> +  CommandData       - Pointer to command data buffer, if needed
>
> +  CommandDataSize   - Size of command data buffer
>
> +  ResponseData      - Pointer to response data buffer
>
> +  ResponseDataSize  - Pointer to response data buffer size
>
> +
>
> +Returns:
>
> +
>
> +  EFI_INVALID_PARAMETER - One of the input values is bad
>
> +  EFI_DEVICE_ERROR      - IPMI command failed
>
> +  EFI_BUFFER_TOO_SMALL  - Response buffer is too small
>
> +  EFI_UNSUPPORTED       - Command is not supported by BMC
>
> +  EFI_SUCCESS           - Command completed successfully
>
> +
>
> +--*/
>
> +{
>
> +
>
> +  IPMI_BMC_INSTANCE_DATA  *IpmiInstance;
>
> +
>
> +  if (This == NULL) {
>
> +      return EFI_INVALID_PARAMETER;
> [Isaac] The preceding line is improperly indented.
>
> +  }
>
> +
>
> +  IpmiInstance = INSTANCE_FROM_IPMI_TRANSPORT2_THIS(This);
>
> +
>
> +#if KcsInterfaceSupport
> [Isaac] I don't think that using #defines helps the code.  I think obscuring the
> PCD makes the code harder to understand.  Also not following the
> MACRO_NAMING_CONVENTION.
> If you really prefer the macro, change it to something like
> IS_KCS_INTERFACE_PCD_ENABLED.
> I also prefer using regular C code instead of c preprocessors.  All major
> toolchains support optimization that should remove any benefit to using #if.
> If we use regular if statements, then code is compiled and thus tested for build
> more regularly.
>
> +  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceKcs)
> + &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.KcsInterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiSendCommand (
>
> +               &IpmiInstance->IpmiTransport,
>
> +               NetFunction,
>
> +               Lun,
>
> +               Command,
>
> +               CommandData,
>
> +               CommandDataSize,
>
> +               ResponseData,
>
> +               ResponseDataSize
>
> +               );
>
> +  }
>
> +#endif
>
> +
>
> +#if BtInterfaceSupport
>
> +  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceBt) &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiBtSendCommandToBmc (
>
> +               &IpmiInstance->IpmiTransport2,
>
> +               NetFunction,
>
> +               Lun,
>
> +               Command,
>
> +               CommandData,
>
> +               (UINT8) CommandDataSize,
>
> +               ResponseData,
>
> +               (UINT8*) ResponseDataSize,
>
> +               NULL
>
> +               );
>
> +  }
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceSsif)
> + &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiSsifSendCommandToBmc (
>
> +               &IpmiInstance->IpmiTransport2,
>
> +               NetFunction,
>
> +               Lun,
>
> +               Command,
>
> +               CommandData,
>
> +               (UINT8) CommandDataSize,
>
> +               ResponseData,
>
> +               (UINT8*) ResponseDataSize,
>
> +               NULL
>
> +               );
>
> +  }
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  if ((IpmiInstance->IpmiTransport2.InterfaceType == SysInterfaceIpmb)
> + &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiIpmbSendCommandToBmc (
>
> +               &IpmiInstance->IpmiTransport2,
>
> +               NetFunction,
>
> +               Lun,
>
> +               Command,
>
> +               CommandData,
>
> +               (UINT8) CommandDataSize,
>
> +               ResponseData,
>
> +               (UINT8*) ResponseDataSize,
>
> +               NULL
>
> +               );
>
> +  }
>
> +#endif
>
> +  return EFI_UNSUPPORTED;
> [Isaac] Should we comment this a bit more?  It isn't really obvious if hitting this
> is a valid configuration.  Should there be an assert here to indicate that one of
> the interfaces must be enabled?  Are the function comments about default
> interface correct?  Same for next function.
>
> +} // IpmiSendCommand2()
>
> +
>
> +EFI_STATUS
>
> +EFIAPI
>
> +IpmiSendCommand2Ex (
>
> +  IN      IPMI_TRANSPORT2              *This,
>
> +  IN      UINT8                        NetFunction,
>
> +  IN      UINT8                        Lun,
>
> +  IN      UINT8                        Command,
>
> +  IN      UINT8                        *CommandData,
>
> +  IN      UINT32                       CommandDataSize,
>
> +  IN OUT  UINT8                        *ResponseData,
>
> +  IN OUT  UINT32                       *ResponseDataSize,
>
> +  IN      SYSTEM_INTERFACE_TYPE        InterfaceType
>
> +  )
>
> +{
>
> +/*++
>
> +Routine Description:
>
> +
>
> +  This API use the specific interface type to send IPMI command
>
> +  in the right mode to the appropiate device, ME or BMC.
>
> +
>
> +Arguments:
>
> +
>
> +  This              - Pointer to IPMI protocol instance
>
> +  NetFunction       - Net Function of command to send
>
> +  Lun               - LUN of command to send
>
> +  Command           - IPMI command to send
>
> +  CommandData       - Pointer to command data buffer, if needed
>
> +  CommandDataSize   - Size of command data buffer
>
> +  ResponseData      - Pointer to response data buffer
>
> +  ResponseDataSize  - Pointer to response data buffer size
>
> +  InterfaceType     - BMC Interface type.
>
> +
>
> +Returns:
>
> +
>
> +  EFI_INVALID_PARAMETER - One of the input values is bad
>
> +  EFI_DEVICE_ERROR      - IPMI command failed
>
> +  EFI_BUFFER_TOO_SMALL  - Response buffer is too small
>
> +  EFI_UNSUPPORTED       - Command is not supported by BMC
>
> +  EFI_SUCCESS           - Command completed successfully
>
> +
>
> +--*/
>
> +
>
> +  IPMI_BMC_INSTANCE_DATA  *IpmiInstance;
>
> +
>
> +  if (This == NULL) {
>
> +      return EFI_INVALID_PARAMETER;
>
> +  }
>
> +
>
> +  IpmiInstance = INSTANCE_FROM_IPMI_TRANSPORT2_THIS(This);
>
> +
>
> +#if KcsInterfaceSupport
>
> +  if ((InterfaceType == SysInterfaceKcs) &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.KcsInterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiSendCommand (
>
> +                &IpmiInstance->IpmiTransport,
>
> +                NetFunction,
>
> +                Lun,
>
> +                Command,
>
> +                CommandData,
>
> +                CommandDataSize,
>
> +                ResponseData,
>
> +                ResponseDataSize
>
> +                );
>
> +     }
>
> +#endif
>
> +
>
> +#if BtInterfaceSupport
>
> +  if ((InterfaceType == SysInterfaceBt) &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiBtSendCommandToBmc (
>
> +                &IpmiInstance->IpmiTransport2,
>
> +                NetFunction,
>
> +                Lun,
>
> +                Command,
>
> +                CommandData,
>
> +                (UINT8) CommandDataSize,
>
> +                ResponseData,
>
> +                (UINT8*) ResponseDataSize,
>
> +                NULL
>
> +                );
>
> +  }
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +  if ((InterfaceType == SysInterfaceSsif) &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiSsifSendCommandToBmc (
>
> +                &IpmiInstance->IpmiTransport2,
>
> +                NetFunction,
>
> +                Lun,
>
> +                Command,
>
> +                CommandData,
>
> +                (UINT8) CommandDataSize,
>
> +                ResponseData,
>
> +                (UINT8*) ResponseDataSize,
>
> +                NULL
>
> +                );
>
> +  }
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  if ((InterfaceType == SysInterfaceIpmb) &&
>
> +      (IpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized)) {
>
> +
>
> +      return IpmiIpmbSendCommandToBmc (
>
> +                &IpmiInstance->IpmiTransport2,
>
> +                NetFunction,
>
> +                Lun,
>
> +                Command,
>
> +                CommandData,
>
> +                (UINT8) CommandDataSize,
>
> +                ResponseData,
>
> +                (UINT8*) ResponseDataSize,
>
> +                NULL
>
> +                );
>
> +  }
>
> +#endif
>
> +  return EFI_UNSUPPORTED;
>
> +}
>
> +
>
>  EFI_STATUS
>
>  EFIAPI
>
>  IpmiGetBmcStatus (
>
> @@ -89,6 +340,11 @@ Returns:
>
>
>  --*/
>
>  {
>
> +
>
> +  if ((This == NULL) || (BmcStatus == NULL) || (ComAddress == NULL)) {
>
> +      return EFI_INVALID_PARAMETER;
>
> +  }
>
> +
>
>    return IpmiBmcStatus (
>
>             This,
>
>             BmcStatus,
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.h
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.h
> index 823cc08c61..3933e07443 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> mon/IpmiHooks.h
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Com
> m
> +++ on/IpmiHooks.h
> @@ -3,13 +3,21 @@
>
>
>    @copyright
>
>    Copyright 2014 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
>  #ifndef _IPMI_HOOKS_H
>
>  #define _IPMI_HOOKS_H
>
>
>
> -#include "IpmiBmc.h"
>
> +#include <Protocol/IpmiTransportProtocol.h>
>
> +#include <Protocol/IpmiTransport2Protocol.h>
>
> +#include <Library/BtInterfaceLib.h>
>
> +#include <Library/SsifInterfaceLib.h>
>
> +#include <Library/IpmbInterfaceLib.h>
>
> +#include <Library/DebugLib.h>
>
> +#include<IpmiBmcCommon.h>
>
> +#include<IpmiBmc.h>
> [Isaac] Add a space before < in the two preceding lines.
>
>
>
>  //
>
>  // Internal(hook) function list
>
> @@ -54,6 +62,89 @@ Returns:
>  --*/
>
>  ;
>
>
>
> +EFI_STATUS
>
> +EFIAPI
>
> +IpmiSendCommand2 (
>
> +  IN      IPMI_TRANSPORT2              *This,
>
> +  IN      UINT8                        NetFunction,
>
> +  IN      UINT8                        Lun,
>
> +  IN      UINT8                        Command,
>
> +  IN      UINT8                        *CommandData,
>
> +  IN      UINT32                       CommandDataSize,
>
> +  IN OUT  UINT8                        *ResponseData,
>
> +  IN OUT  UINT32                       *ResponseDataSize
>
> +  )
>
> +/*++
>
> +
>
> +Routine Description:
>
> +
>
> +  This API use the default interface (PcdDefaultSystemInterface) to
> + send IPMI command
>
> +  in the right mode to the appropiate device, ME or BMC.
>
> +
>
> +Arguments:
>
> +
>
> +  This              - Pointer to IPMI protocol instance
>
> +  NetFunction       - Net Function of command to send
>
> +  Lun               - LUN of command to send
>
> +  Command           - IPMI command to send
>
> +  CommandData       - Pointer to command data buffer, if needed
>
> +  CommandDataSize   - Size of command data buffer
>
> +  ResponseData      - Pointer to response data buffer
>
> +  ResponseDataSize  - Pointer to response data buffer size
>
> +
>
> +Returns:
>
> +
>
> +  EFI_INVALID_PARAMETER - One of the input values is bad
>
> +  EFI_DEVICE_ERROR      - IPMI command failed
>
> +  EFI_BUFFER_TOO_SMALL  - Response buffer is too small
>
> +  EFI_UNSUPPORTED       - Command is not supported by BMC
>
> +  EFI_SUCCESS           - Command completed successfully
>
> +
>
> +--*/
>
> +;
>
> +
>
> +EFI_STATUS
>
> +EFIAPI
>
> +IpmiSendCommand2Ex (
>
> +  IN      IPMI_TRANSPORT2              *This,
>
> +  IN      UINT8                        NetFunction,
>
> +  IN      UINT8                        Lun,
>
> +  IN      UINT8                        Command,
>
> +  IN      UINT8                        *CommandData,
>
> +  IN      UINT32                       CommandDataSize,
>
> +  IN OUT  UINT8                        *ResponseData,
>
> +  IN OUT  UINT32                       *ResponseDataSize,
>
> +  IN      SYSTEM_INTERFACE_TYPE        InterfaceType
>
> +  )
>
> +/*++
>
> +Routine Description:
>
> +
>
> +  This API use the specific interface type to send IPMI command
>
> +  in the right mode to the appropiate device, ME or BMC.
>
> +
>
> +Arguments:
>
> +
>
> +  This              - Pointer to IPMI protocol instance
>
> +  NetFunction       - Net Function of command to send
>
> +  Lun               - LUN of command to send
>
> +  Command           - IPMI command to send
>
> +  CommandData       - Pointer to command data buffer, if needed
>
> +  CommandDataSize   - Size of command data buffer
>
> +  ResponseData      - Pointer to response data buffer
>
> +  ResponseDataSize  - Pointer to response data buffer size
>
> +  InterfaceType     - BMC Interface type.
>
> +
>
> +Returns:
>
> +
>
> +  EFI_INVALID_PARAMETER - One of the input values is bad
>
> +  EFI_DEVICE_ERROR      - IPMI command failed
>
> +  EFI_BUFFER_TOO_SMALL  - Response buffer is too small
>
> +  EFI_UNSUPPORTED       - Command is not supported by BMC
>
> +  EFI_SUCCESS           - Command completed successfully
>
> +
>
> +--*/
>
> +;
>
> +
>
>  EFI_STATUS
>
>  EFIAPI
>
>  IpmiGetBmcStatus (
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> GenericIpmi.inf
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> GenericIpmi.inf
> index 8d80aeb6b5..1564ceb08a 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> GenericIpmi.inf
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> +++ GenericIpmi.inf
> @@ -3,6 +3,7 @@
>  #
>
>  # @copyright
>
>  # Copyright 2010 - 2021 Intel Corporation. <BR>
>
> +# Copyright (c) 1985 - 2023, American Megatrends International LLC.
> +<BR>
>
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  ##
>
>
>
> @@ -49,16 +50,25 @@
>    IoLib
>
>    ReportStatusCodeLib
>
>    TimerLib
>
> +  BmcCommonInterfaceLib
>
> +  BtInterfaceLib
>
> +  SsifInterfaceLib
>
> +  IpmbInterfaceLib
>
>
>
>  [Protocols]
>
>    gIpmiTransportProtocolGuid               # PROTOCOL ALWAYS_PRODUCED
>
>    gEfiVideoPrintProtocolGuid
>
> -
>
> -[Guids]
>
> +  gIpmiTransport2ProtocolGuid
>
>
>
>  [Pcd]
>
>    gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiIoBaseAddress
>
>    gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiBmcReadyDelayTimer
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort
>
>
>
>  [Depex]
>
>    gEfiRuntimeArchProtocolGuid AND
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> IpmiInit.c
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> IpmiInit.c
> index c333ca2e06..74d96f8684 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> IpmiInit.c
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/
> +++ IpmiInit.c
> @@ -3,6 +3,7 @@
>
>
>    @copyright
>
>    Copyright 1999 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
> @@ -13,6 +14,7 @@
>  #include "IpmiBmc.h"
>
>  #include "IpmiPhysicalLayer.h"
>
>  #include <Library/TimerLib.h>
>
> +#include <Library/BmcCommonInterfaceLib.h>
>
>  #ifdef FAST_VIDEO_SUPPORT
>
>    #include <Protocol/VideoPrint.h>
>
>  #endif
>
> @@ -351,6 +353,130 @@ Returns:
>    return Status;
>
>  } // GetDeviceId()
>
>
>
> +/**
>
> +    Initialize the API and parameters for IPMI Transport2 Instance
>
> +
>
> +    @param[in] IpmiInstance         Pointer to IPMI Instance
>
> +
>
> +    @return VOID    Nothing.
>
> +
>
> +**/
>
> +VOID
>
> +InitIpmiTransport2 (
>
> +  IN  IPMI_BMC_INSTANCE_DATA *IpmiInstance
>
> +  )
>
> +{
>
> +
>
> +  IpmiInstance->IpmiTransport2.InterfaceType            = FixedPcdGet8
> (PcdDefaultSystemInterface);
>
> +  IpmiInstance->IpmiTransport2.IpmiTransport2BmcStatus  = BmcStatusOk;
>
> +  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2       =
> IpmiSendCommand2;
>
> +  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2Ex     =
> IpmiSendCommand2Ex;
>
> +
>
> +#if BtInterfaceSupport
>
> +  InitBtInterfaceData(&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +  InitSsifInterfaceData(&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  InitIpmbInterfaceData(&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +}
>
> +
>
> +/**
>
> +    Notify call back function.
>
> +
>
> +    @param[in] Event    Event which caused this handler.
>
> +    @param[in] Context  Context passed during Event Handler registration.
>
> +
>
> +    @return VOID    Nothing.
>
> +
>
> +**/
>
> +VOID
>
> +EFIAPI
>
> +DxeNotifyCallback (
>
> +  IN EFI_EVENT  Event,
>
> +  IN VOID       *Context
>
> +  )
>
> +{
>
> +  EFI_STATUS             Status;
>
> +  IPMI_INTERFACE_STATE   InterfaceState;
>
> +  EFI_HANDLE             Handle;
>
> +
>
> +  InterfaceState = IpmiInterfaceNotReady;
>
> +
>
> +#if SsifInterfaceSupport
>
> +  InitSsifInterfaceData(&mIpmiInstance->IpmiTransport2);
>
> +
>
> +  if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized) {
>
> +      InterfaceState = IpmiInterfaceInitialized;
>
> +  }
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  InitIpmbInterfaceData(&mIpmiInstance->IpmiTransport2);
>
> +
>
> +  if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized) {
>
> +      InterfaceState = IpmiInterfaceInitialized;
>
> +  }
>
> +#endif
>
> +  // Default Interface data should be initialized to install Ipmi Transport2
> Protocol.
>
> +  if (InterfaceState != IpmiInterfaceInitialized) {
>
> +      return;
>
> +  }
>
> +
>
> +  Handle = NULL;
>
> +  Status = gBS->InstallProtocolInterface (
>
> +                  &Handle,
>
> +                  &gIpmiTransport2ProtocolGuid,
>
> +                  EFI_NATIVE_INTERFACE,
>
> +                  &mIpmiInstance->IpmiTransport2
>
> +                  );
>
> +  ASSERT_EFI_ERROR (Status);
>
> +}
>
> +
>
> +/**
>
> +    Registers protocol notify call back.
>
> +
>
> +    @param[in] ProtocolGuid     Pointer to Protocol Guid to register
>
> +                                call back.
>
> +
>
> +    @retval EFI_INVALID_PARAMETER   If the ProtocolGuid is 0 or NULL.
>
> +    @retval Others                  Status of call back registration.
>
> +
>
> +**/
>
> +EFI_STATUS
>
> +DxeRegisterProtocolCallback (
>
> +  IN EFI_GUID   *ProtocolGuid
>
> +  )
>
> +{
>
> +  EFI_STATUS  Status;
>
> +  EFI_EVENT   NotifyEvent;
>
> +  VOID        *Registration;
>
> +
>
> +  if ((ProtocolGuid == NULL) ||
>
> +      ((ProtocolGuid != NULL) && IsZeroBuffer (ProtocolGuid, sizeof
> + (EFI_GUID)))) {
>
> +      return EFI_INVALID_PARAMETER;
>
> +  }
>
> +
>
> +  Status = gBS->CreateEvent (
>
> +                  EVT_NOTIFY_SIGNAL,
>
> +                  TPL_NOTIFY,
>
> +                  DxeNotifyCallback,
>
> +                  NULL,
>
> +                  &NotifyEvent);
>
> +
>
> +  if (!EFI_ERROR (Status)) {
>
> +      Status = gBS->RegisterProtocolNotify (
>
> +                      ProtocolGuid,
>
> +                      NotifyEvent,
>
> +                      &Registration);
>
> +  }
>
> +
>
> +  return Status;
>
> +}
>
>
>
>  /**
>
>    This function initializes KCS interface to BMC.
>
> @@ -376,7 +502,10 @@ InitializeIpmiKcsPhysicalLayer (
>    UINT8                  ErrorCount;
>
>    EFI_HANDLE             Handle;
>
>    UINT8                  Index;
>
> +  IPMI_INTERFACE_STATE   InterfaceState = IpmiInterfaceNotReady;
>
> +#if KcsInterfaceSupport
>
>    EFI_STATUS_CODE_VALUE  StatusCodeValue[MAX_SOFT_COUNT];
>
> +#endif
>
>
>
>    ErrorCount = 0;
>
>    mImageHandle = ImageHandle;
>
> @@ -405,6 +534,8 @@ InitializeIpmiKcsPhysicalLayer (
>      mIpmiInstance->Signature                        = SM_IPMI_BMC_SIGNATURE;
>
>      mIpmiInstance->SlaveAddress                     = BMC_SLAVE_ADDRESS;
>
>      mIpmiInstance->BmcStatus                        = BMC_NOTREADY;
>
> +
>
> +#if KcsInterfaceSupport
>
>      mIpmiInstance->IpmiTransport.IpmiSubmitCommand  =
> IpmiSendCommand;
>
>      mIpmiInstance->IpmiTransport.GetBmcStatus       = IpmiGetBmcStatus;
>
>
>
> @@ -454,7 +585,81 @@ InitializeIpmiKcsPhysicalLayer (
>                        );
>
>        ASSERT_EFI_ERROR (Status);
>
>      }
>
> +#endif
>
> +
>
> +    // Initialise the IPMI transport2
> [Isaac] I find the mismatch in spelling with some use of s and more use of z to
> be confusing.
>
> +    InitIpmiTransport2(mIpmiInstance);
> [Isaac] Space before (
>
> +
>
> +    // Check interface data initialized successfully else register notify protocol.
>
> +    for (Index = SysInterfaceKcs; Index < SysInterfaceMax; Index++) {
>
> +
>
> +        switch (Index) {
>
> +#if KcsInterfaceSupport
> [Isaac] A lot of the following code is not indented properly.
>
> +            case SysInterfaceKcs:
>
> +                if ((mIpmiInstance->BmcStatus != BMC_HARDFAIL) &&
> + (mIpmiInstance->BmcStatus != BMC_UPDATE_IN_PROGRESS)) {
>
> +                    BMC_INTERFACE_STATUS  BmcStatus;
> [Isaac] I would prefer this at the beginning of the function with other local
> variables.
>
> +
> + mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState =
> + IpmiInterfaceInitialized;
>
> +                    Status = CheckSelfTestByInterfaceType(
> [Isaac] Please insert space before (.  Please add newline between the
> interesting code and the variable init.
>
> +                                        &mIpmiInstance->IpmiTransport2,
>
> +                                        &BmcStatus,
>
> +                                        SysInterfaceKcs);
> [Isaac] Put ); on its own line as per other code conventions.
>
> +                    if (!EFI_ERROR (Status) && (BmcStatus !=
> + BmcStatusHardFail)) {
>
> +                        InterfaceState = IpmiInterfaceInitialized;
>
> +                    } else {
>
> +
> + mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState =
> + IpmiInterfaceInitError;
>
> +                    }
>
> +                }
>
> +                break;
>
> +#endif
>
> +
>
> +#if BtInterfaceSupport
>
> +            case SysInterfaceBt:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState ==
> + IpmiInterfaceInitialized){
> [Isaac] Space before {
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                }
>
> +                break;
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +            case SysInterfaceSsif:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized) {
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                } else if
> + (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitError) {
>
> +                    // Register protocol notify for SMBUS Protocol.
>
> +                    Status = DxeRegisterProtocolCallback (
>
> +
> + &mIpmiInstance->IpmiTransport2.Interface.Ssif.SsifInterfaceApiGuid);
> [Isaac] Indent this line 2 characters from the beginning of the function name
> on the prior line per coding style.  Also put ); on its own line.
>
> +                }
>
> +                break;
>
> +#endif
>
> +#if IpmbInterfaceSupport
>
> +            case SysInterfaceIpmb:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized) {
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                } else if
> + (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitError) {
>
> +                    // Register Protocol notify for I2C Protocol.
>
> +                    Status = DxeRegisterProtocolCallback (
>
> +
> + &mIpmiInstance->IpmiTransport2.Interface.Ipmb.IpmbInterfaceApiGuid);
> [Isaac] Put ); on its own line as per other code conventions.
>
> +                }
>
> +                break;
>
> +#endif
>
> +            default:
>
> +                break;
>
> +        }
>
> +    }
>
> +
>
> +    // Any one of the Interface data should be initialized to install IPMI
> Transport2 Protocol.
>
> +    if (InterfaceState != IpmiInterfaceInitialized) {
>
> +        return EFI_SUCCESS;
>
> +    }
>
>
>
> +    Handle = NULL;
>
> +    Status = gBS->InstallProtocolInterface (
>
> +                    &Handle,
>
> +                    &gIpmiTransport2ProtocolGuid,
>
> +                    EFI_NATIVE_INTERFACE,
>
> +                    &mIpmiInstance->IpmiTransport2
>
> +                    );
>
> +    ASSERT_EFI_ERROR (Status);
>
>      return EFI_SUCCESS;
>
>    }
>
>  } // InitializeIpmiKcsPhysicalLayer()
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.c
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.c
> index 1af2d18f79..adf59374b3 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.c
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/
> +++ SmmGenericIpmi.c
> @@ -3,6 +3,7 @@
>
>
>    @copyright
>
>    Copyright 1999 - 2021 Intel Corporation. <BR>
>
> +  Copyright (c) 1985 - 2023, American Megatrends International LLC.
> + <BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
>
>
>
> @@ -24,6 +25,7 @@
>  #include "IpmiBmcCommon.h"
>
>  #include "IpmiBmc.h"
>
>  #include <Library/TimerLib.h>
>
> +#include <Library/BmcCommonInterfaceLib.h>
>
>
>
>  IPMI_BMC_INSTANCE_DATA             *mIpmiInstance;
>
>  EFI_HANDLE                         mImageHandle;
>
> @@ -115,6 +117,123 @@ Returns:
>    return Status;
>
>  }
>
>
>
> +/**
>
> +    Initialize the API and parameters for IPMI Transport2 Instance
> [Isaac] There are a lot of four space indents.
>
> +
>
> +    @param[in] IpmiInstance         Pointer to IPMI Instance
>
> +
>
> +    @return VOID    Nothing.
>
> +
>
> +**/
>
> +VOID
>
> +InitIpmiTransport2 (
>
> +  IN  IPMI_BMC_INSTANCE_DATA    *IpmiInstance
>
> +  )
>
> +{
>
> +
>
> +  IpmiInstance->IpmiTransport2.InterfaceType           = FixedPcdGet8
> (PcdDefaultSystemInterface);
>
> +  IpmiInstance->IpmiTransport2.IpmiTransport2BmcStatus = BmcStatusOk;
>
> +  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2      =
> IpmiSendCommand2;
>
> +  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2Ex    =
> IpmiSendCommand2Ex;
>
> +
>
> +#if BtInterfaceSupport
>
> +  InitBtInterfaceData (&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +  InitSsifInterfaceData (&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  InitIpmbInterfaceData (&IpmiInstance->IpmiTransport2);
>
> +#endif
>
> +}
>
> +
>
> +/**
>
> +    Notify call back to initialize the interfaces and install Smm Ipmi
>
> +    protocol.
>
> +
>
> +    @param[in] Protocol     Pointer to the protocol guid.
>
> +    @param[in] Interface    Pointer to the protocol instance.
>
> +    @param[in] Handle       Handle on which the protocol is installed.
>
> +
>
> +    @return EFI_STATUS  Status of Notify call back.
>
> +
>
> +**/
>
> +EFI_STATUS
>
> +EFIAPI
>
> +SmmNotifyCallback (
>
> +  IN CONST  EFI_GUID    *Protocol,
>
> +  IN        VOID        *Interface,
>
> +  IN        EFI_HANDLE  Handle
>
> +  )
>
> +{
>
> +
>
> +  EFI_STATUS             Status;
>
> +  IPMI_INTERFACE_STATE   InterfaceState;
>
> +
>
> +  InterfaceState = IpmiInterfaceNotReady;
>
> +
>
> +#if SsifInterfaceSupport
>
> +  InitSsifInterfaceData(&mIpmiInstance->IpmiTransport2);
>
> +
>
> +  if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized){
> [Isaac] Space before {
>
> +      InterfaceState = IpmiInterfaceInitialized;
>
> +  }
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +  InitIpmbInterfaceData(&mIpmiInstance->IpmiTransport2);
>
> +#endif
>
> +
>
> +  if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized){
> [Isaac] Space before {
>
> +      InterfaceState = IpmiInterfaceInitialized;
>
> +  }
>
> +  if (InterfaceState != IpmiInterfaceInitialized) {
>
> +      return EFI_SUCCESS;
>
> +  }
>
> +
>
> +  // Default Interface data should be initialized to install Ipmi Transport2
> Protocol.
>
> +  if (InterfaceState == IpmiInterfaceInitialized) {
>
> +      Handle = NULL;
>
> +      Status = gSmst->SmmInstallProtocolInterface (
>
> +                          &Handle,
>
> +                          &gSmmIpmiTransport2ProtocolGuid,
>
> +                          EFI_NATIVE_INTERFACE,
>
> +                          &mIpmiInstance->IpmiTransport2
>
> +                          );
>
> +  }
>
> +  return EFI_SUCCESS;
>
> +}
>
> +
>
> +/**
>
> +    Registers Protocol call back
>
> +
>
> +    @param ProtocolGuid       Pointer to Protocol GUID to register call back
>
> +
>
> +    @retval EFI_INVALID_PARAMETER   If the ProtocolGuid is 0 or NULL.
>
> +    @retval Others                  Status of Notify registration.
>
> +**/
>
> +EFI_STATUS
>
> +SmmRegisterProtocolCallback (
>
> +  IN  EFI_GUID  *ProtocolGuid
>
> +)
> [Isaac] Not Indented the same way other functions are.
>
> +{
>
> +    EFI_STATUS  Status;
>
> +    VOID        *Registration;
>
> +
>
> +    if ((ProtocolGuid == NULL) ||
>
> +        ((ProtocolGuid != NULL) && IsZeroBuffer (ProtocolGuid, sizeof
> + (EFI_GUID)))) {
>
> +        return EFI_INVALID_PARAMETER;
>
> +    }
>
> +
>
> +    Status = gSmst->SmmRegisterProtocolNotify (
>
> +                        ProtocolGuid,
>
> +                        SmmNotifyCallback,
>
> +                        &Registration );
>
> +    return Status;
>
> +}
>
> +
>
>  EFI_STATUS
>
>  SmmInitializeIpmiKcsPhysicalLayer (
>
>    IN EFI_HANDLE             ImageHandle,
>
> @@ -142,10 +261,13 @@ Returns:
>    UINT8                            ErrorCount;
>
>    EFI_HANDLE                       Handle;
>
>    EFI_STATUS_CODE_VALUE            StatusCodeValue[MAX_SOFT_COUNT];
>
> +  IPMI_INTERFACE_STATE             InterfaceState;
>
> +  UINT8                            Index;
>
>
>
>    DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer entry \n"));
>
> -  ErrorCount = 0;
>
> -  mImageHandle = ImageHandle;
>
> +  ErrorCount     = 0;
>
> +  mImageHandle   = ImageHandle;
>
> +  InterfaceState = IpmiInterfaceNotReady;
>
>
>
>    mIpmiInstance = AllocateZeroPool (sizeof (IPMI_BMC_INSTANCE_DATA));
>
>    ASSERT (mIpmiInstance != NULL);
>
> @@ -170,6 +292,7 @@ Returns:
>      mIpmiInstance->IpmiTransport.IpmiSubmitCommand  =
> IpmiSendCommand;
>
>      mIpmiInstance->IpmiTransport.GetBmcStatus       = IpmiGetBmcStatus;
>
>
>
> +#if KcsInterfaceSupport
>
>      DEBUG ((DEBUG_INFO,"IPMI: Waiting for Getting BMC DID in SMM \n"));
>
>      //
>
>      // Get the Device ID and check if the system is in Force Update mode.
>
> @@ -191,6 +314,79 @@ Returns:
>                        &mIpmiInstance->IpmiTransport
>
>                        );
>
>      ASSERT_EFI_ERROR (Status);
>
> +#endif
>
> +
>
> +    InitIpmiTransport2(mIpmiInstance);
> [Isaac] Space before (
>
> +
>
> +    // Check interface data initialized successfully else register notify protocol.
>
> +    for (Index = SysInterfaceKcs; Index < SysInterfaceMax; Index++) {
>
> +
>
> +        switch (Index) {
>
> +#if KcsInterfaceSupport
>
> +            case SysInterfaceKcs:
>
> +                if ((mIpmiInstance->BmcStatus != BMC_HARDFAIL) &&
> + (mIpmiInstance->BmcStatus != BMC_UPDATE_IN_PROGRESS)) {
>
> +                    BMC_INTERFACE_STATUS  BmcStatus;
> [Isaac] Better to be with other local variables at beginning of function.
>
> +
> + mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState =
> + IpmiInterfaceInitialized;
>
> +                    Status = CheckSelfTestByInterfaceType(
> [Isaac] Insert space before (
>
> +
> + &mIpmiInstance->IpmiTransport2,
>
> +                                               &BmcStatus,
>
> +                                               SysInterfaceKcs);
> [Isaac] Place ); on own line.
>
> +                    if (!EFI_ERROR (Status) && (BmcStatus !=
> + BmcStatusHardFail)) {
>
> +                        InterfaceState = IpmiInterfaceInitialized;
>
> +                    } else {
>
> +
> + mIpmiInstance->IpmiTransport2.Interface.KcsInterfaceState =
> + IpmiInterfaceInitError;
>
> +                    }
>
> +                }
>
> +                break;
>
> +#endif
>
> +
>
> +#if BtInterfaceSupport
>
> +            case SysInterfaceBt:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Bt.InterfaceState ==
> + IpmiInterfaceInitialized){
> [Isaac] Insert space before {
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                }
>
> +                break;
>
> +#endif
>
> +
>
> +#if SsifInterfaceSupport
>
> +            case SysInterfaceSsif:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitialized){
> [Isaac] Insert space before {
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                } else if
> + (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState ==
> + IpmiInterfaceInitError) {
>
> +                    // Register protocol notify for SMBUS Protocol.
>
> +                    Status = SmmRegisterProtocolCallback
> + (&mIpmiInstance->IpmiTransport2.Interface.Ssif.SsifInterfaceApiGuid);
>
> +                }
>
> +                break;
>
> +#endif
>
> +
>
> +#if IpmbInterfaceSupport
>
> +            case SysInterfaceIpmb:
>
> +                if
> + (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitialized){
>
> +                    InterfaceState = IpmiInterfaceInitialized;
>
> +                } else if
> + (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState ==
> + IpmiInterfaceInitError) {
>
> +                    // Register protocol notify for SMBUS Protocol.
>
> +                    Status = SmmRegisterProtocolCallback
> + (&mIpmiInstance->IpmiTransport2.Interface.Ipmb.IpmbInterfaceApiGuid);
>
> +                }
>
> +                break;
>
> +#endif
>
> +            default:
>
> +                break;
>
> +        }
>
> +    }
>
> +
>
> +    // Default Interface data should be initialized to install Ipmi Transport2
> Protocol.
>
> +    if (InterfaceState == IpmiInterfaceInitialized) {
>
> +        Handle = NULL;
>
> +        Status = gSmst->SmmInstallProtocolInterface (
>
> +                          &Handle,
>
> +                          &gSmmIpmiTransport2ProtocolGuid,
>
> +                          EFI_NATIVE_INTERFACE,
>
> +                          &mIpmiInstance->IpmiTransport2
>
> +                          );
>
> +        if (EFI_ERROR(Status)) {
>
> +            DEBUG ((DEBUG_ERROR,"IPMI Transport2 protocol install
> + Status = %r \n",Status));
>
> +        }
>
> +    }
>
>
>
>      DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer exit \n"));
>
>
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.inf
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.inf
> index f430195d1e..12dc17ae84 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/SmmGenericIpmi.inf
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Sm
> m/
> +++ SmmGenericIpmi.inf
> @@ -3,6 +3,7 @@
>  #
>
>  # @copyright
>
>  # Copyright 2010 - 2021 Intel Corporation. <BR>
>
> +# Copyright (c) 1985 - 2023, American Megatrends International LLC.
> +<BR>
>
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  ##
>
>
>
> @@ -39,15 +40,26 @@
>    IoLib
>
>    ReportStatusCodeLib
>
>    TimerLib
>
> +  BmcCommonInterfaceLib
>
> +  BtInterfaceLib
>
> +  SsifInterfaceLib
>
> +  IpmbInterfaceLib
>
>
>
>  [Protocols]
>
>    gSmmIpmiTransportProtocolGuid                     # PROTOCOL
> ALWAYS_PRODUCED
>
> +  gSmmIpmiTransport2ProtocolGuid                    # PROTOCOL
> ALWAYS_PRODUCED
>
>
>
>  [Guids]
>
>
>
>  [Pcd]
>
>    gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiSmmIoBaseAddress
>
>    gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiBmcReadyDelayTimer
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport
>
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport
>
>
>
>  [Depex]
>
>   gIpmiTransportProtocolGuid
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> ec
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> ec
> index 8c1b902446..2131ec475b 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> ec
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> +++ ec
> @@ -9,6 +9,7 @@
>  #
>
>  # Copyright (c) 2019-2021, Intel Corporation. All rights reserved.<BR>
>
>  # Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.<BR>
>
> +# Copyright (c) 1985 - 2023, American Megatrends International LLC.
> +<BR>
>
>  #
>
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  #
>
> @@ -43,16 +44,26 @@
>    #
>
>    IpmiBaseLib|Include/Library/IpmiBaseLib.h
>
>
>
> +  ##  @libraryclass  Provides generic functions among all interfaces.
>
> +  BmcCommonInterfaceLib|Include/Library/BmcCommonInterfaceLib.h
>
> +  BtInterfaceLib|Include/Library/BtInterfaceLib.h
>
> +  SsifInterfaceLib|Include/Library/SsifInterfaceLib.h
>
> +  IpmbInterfaceLib|Include/Library/IpmbInterfaceLib.h
>
> +
>
>  [Guids]
>
>    gIpmiFeaturePkgTokenSpaceGuid  =  {0xc05283f6, 0xd6a8, 0x48f3, {0x9b,
> 0x59, 0xfb, 0xca, 0x71, 0x32, 0x0f, 0x12}}
>
> +  gPeiIpmiHobGuid                = {0xcb4d3e13, 0x1e34, 0x4373, {0x8a, 0x81,
> 0xe9, 0x0, 0x10, 0xf1, 0xdb, 0xa4}}
>
>
>
>  [Ppis]
>
>    gPeiIpmiTransportPpiGuid = {0x7bf5fecc, 0xc5b5, 0x4b25, {0x81, 0x1b,
> 0xb4, 0xb5, 0xb, 0x28, 0x79, 0xf7}}
>
> +  gPeiIpmiTransport2PpiGuid = {0x8122CEBD, 0xF4FD, 0x4EA8, { 0x97,
> + 0x6C, 0xF0, 0x30, 0xAD, 0xDC, 0x4C, 0xB4 }}
>
>
>
>  [Protocols]
>
>    gIpmiTransportProtocolGuid  = {0x6bb945e8, 0x3743, 0x433e, {0xb9, 0x0e,
> 0x29, 0xb3, 0x0d, 0x5d, 0xc6, 0x30}}
>
>    gSmmIpmiTransportProtocolGuid  = {0x8bb070f1, 0xa8f3, 0x471d, {0x86,
> 0x16, 0x77, 0x4b, 0xa3, 0xf4, 0x30, 0xa0}}
>
>    gEfiVideoPrintProtocolGuid     = {0x3dbf3e06, 0x9d0c, 0x40d3, {0xb2, 0x17,
> 0x45, 0x5f, 0x33, 0x9e, 0x29, 0x09}}
>
> +  gIpmiTransport2ProtocolGuid = { 0x4A1D0E66, 0x5271, 0x4E22, { 0x83,
> + 0xFE, 0x90, 0x92, 0x1B, 0x74, 0x82, 0x13 }}
>
> +  gSmmIpmiTransport2ProtocolGuid = { 0x1DBD1503, 0x0A60, 0x4230, {
> + 0xAA, 0xA3, 0x80, 0x16, 0xD8, 0xC3, 0xDE, 0x2F }}
>
>
>
>  [PcdsFeatureFlag]
>
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiFeatureEnable|FALSE|BOOLEAN|0x
> A0000001
>
> @@ -61,6 +72,40 @@
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdMaxSOLChannels|3|UINT8|0xF000000
> 1
>
>    #When True, BIOS will send a Pre-Boot signal to BMC
>
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdSignalPreBootToBmc|FALSE|BOOLEAN|
> 0xF0000002
>
> +  #  typedef enum {
>
> +  #      SysInterfaceUnknown, // Unknown interface type.
>
> +  #      SysInterfaceKcs,     // Kcs interface = 1.
>
> +  #      SysInterfaceSmic,    // Smic interface = 2.
>
> +  #      SysInterfaceBt,      // Bt interface = 3.
>
> +  #      SysInterfaceSsif,    // Ssif interface = 4.
>
> +  #      SysInterfaceMax   // Maximum interface type.
>
> +  #  } SYSTEM_INTERFACE_TYPE;
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdDefaultSystemInterface|1|UINT8|0xF00
> 0
> + 0003
>
> +
>
> +  #BT Base address, retry counter and delay parameters
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtCommandRetryCounter|0x0004E400
> |UINT
> + 32|0xF0000004
>
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtControlPort|0xE4|UINT16|0xF00000
> 05
>
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtBufferPort|0xE5|UINT16|0xF000000
> 6
>
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtDelayPerRetry|15|UINT32|0xF00000
> 07
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterruptMaskPort|0xE6|UINT16|0xF
> 00
> + 00008
>
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtBufferSize|0x40|UINT8|0xF0000009
>
> +
>
> +  #SSIF slave address, retry counter and delay parameters
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdSsifSlaveAddress|0x10|UINT16|0xF000
> 00
> + 0A
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdSsifRequestRetriesDelay|0xCB20|UINT3
> 2
> + |0xF000000B
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdSsifCommandtRetryCounter|0x5|UINT1
> 6|0
> + xF000000C
>
> +
>
> +   #Interface access type for BMC communication. 0-MMIO, 1-IO
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiDefaultAccessType|1|UINT8|0xF00
> 00
> + 00D
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdMmioBaseAddress|0x0|UINT32|0xF00
> 0000E
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBaseAddressRange|0x0|UINT32|0xF00
> 0000
> + F
>
> +
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBmcSlaveAddress|0x20|UINT32|0xF00
> 0001
> + 0
>
> +
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdKcsInterfaceSupport|1|UINT8|0xF0000
> 01
> + 1
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdBtInterfaceSupport|1|UINT8|0xF00000
> 12
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdSsifInterfaceSupport|1|UINT8|0xF0000
> 0
> + 13
>
> +
> +
> gIpmiFeaturePkgTokenSpaceGuid.PcdIpmbInterfaceSupport|1|UINT8|0xF00
> 000
> + 14
>
>
>
>  [PcdsDynamic, PcdsDynamicEx]
>
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdFRB2EnabledFlag|TRUE|BOOLEAN|0xD
> 0000001
>
> --
> 2.38.1.windows.1
> -The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI). This communication is intended
> to be read only by the individual or entity to whom it is addressed or by their
> designee. If the reader of this message is not the intended recipient, you are
> on notice that any distribution of this message, in any form, is strictly
> prohibited. Please promptly notify the sender by reply e-mail or by telephone
> at 770-246-8600, and then delete or destroy all copies of the transmission.
>
>
> 
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM
  2023-06-13  4:35   ` Chang, Abner
  2023-06-13 17:12     ` Isaac Oram
@ 2023-06-21  6:38     ` Arun K
  2023-08-14 12:55       ` Huang, Yanbo
  1 sibling, 1 reply; 6+ messages in thread
From: Arun K @ 2023-06-21  6:38 UTC (permalink / raw)
  To: Chang, Abner, devel

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

Hi Isaac,

Seems Uncrustify has not done this for IpmiFeaturePkg. Do I need to provide the changes for the complete IpmiFeaturePkg or for my changes alone fine?

Sure, I will use the PCD directly instead of macros. the main concern of #if is to avoid compiler warnings when the library is not mapped for the disabled interfaces. so, can we proceed with #if logic?
Please let me know your comments on this.

Thanks,
Arun

[-- Attachment #2: Type: text/html, Size: 499 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM
  2023-06-21  6:38     ` [edk2-devel] [edk2-platforms][PATCH " Arun K
@ 2023-08-14 12:55       ` Huang, Yanbo
  0 siblings, 0 replies; 6+ messages in thread
From: Huang, Yanbo @ 2023-08-14 12:55 UTC (permalink / raw)
  To: Arun K, devel

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

Hi Arun,

When I apply these changes in intel platform, I will hit some "warning treated as error" issues, so I file a bug in tianocore: https://bugzilla.tianocore.org/show_bug.cgi?id=4522
Could you please help to check it?
Thanks!

Best Regards,
Yanbo Huang


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



[-- Attachment #2: Type: text/html, Size: 1174 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-08-14 12:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-12 12:52 [edk2-devel][edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM Arun K
2023-06-13  0:27 ` Isaac Oram
2023-06-13  4:35   ` Chang, Abner
2023-06-13 17:12     ` Isaac Oram
2023-06-21  6:38     ` [edk2-devel] [edk2-platforms][PATCH " Arun K
2023-08-14 12:55       ` Huang, Yanbo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox