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