Re: [edk2-devel] [PATCH v2 1/1] IpmiFeaturePkg/GenericIpmi: Support Standalone MM

["Chang, Abner via groups.io" <abner.chang=amd.com@groups.io>]

[AMD Official Use Only - General]

Hi Lisa, please check the comments in line.

> -----Original Message-----
> From: Lixia Huang <lisa.huang@intel.com>
> Sent: Monday, October 30, 2023 9:20 AM
> To: devel@edk2.groups.io
> Cc: Chang, Abner <Abner.Chang@amd.com>; Nate DeSimone
> <nathaniel.l.desimone@intel.com>
> Subject: [PATCH v2 1/1] IpmiFeaturePkg/GenericIpmi: Support Standalone
> MM
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Add Standalone Mm Generic Impi driver. And add type 'PcdsFixedAtBuild'
> for PcdIpmiSmmIoBaseAddress to access in StandaloneMm driver
>
> Cc: Abner Chang <Abner.Chang@amd.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Signed-off-by: Lixia Huang <lisa.huang@intel.com>
> ---
>
> Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Standa
> loneMm/StandaloneMmGenericIpmi.c   | 148 ++++++++++++++++++++
>
> Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Standa
> loneMm/StandaloneMmGenericIpmi.inf |  52 +++++++
>
> Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.dec
> |   2 +
>  3 files changed, 202 insertions(+)
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Stan
> daloneMm/StandaloneMmGenericIpmi.c
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Stan
> daloneMm/StandaloneMmGenericIpmi.c
> new file mode 100644
> index 000000000000..a8751f5b1a1d
> --- /dev/null
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Stan
> daloneMm/StandaloneMmGenericIpmi.c
> @@ -0,0 +1,148 @@
> +/** @file
> +  Generic StandaloneMm IPMI stack driver
> +
> +  @copyright
We don't usually have @copyright tag in file header. Is this a new requirement for the patch?

> +  Copyright 2023 Intel Corporation. <BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +//
> +// Statements that include other files
> +//
> +#include <IndustryStandard/Ipmi.h>
> +#include <Uefi.h>
> +#include <Library/BaseLib.h>
> +#include <Library/SmmLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/MmServicesTableLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/IpmiBaseLib.h>
> +#include <SmStatusCodes.h>
> +#include "IpmiHooks.h"
> +#include "IpmiBmcCommon.h"
> +#include "IpmiBmc.h"
Could you please move above three include statement below #include <Library/TimerLib.h>?



> +#include <Library/TimerLib.h>
> +
> +IPMI_BMC_INSTANCE_DATA             *mIpmiInstance;
> +EFI_HANDLE                         mHandle;
> +
> +EFI_STATUS
> +SmmInitializeIpmiKcsPhysicalLayer (
> +  VOID
> +  )
> +/*++
> +
> +Routine Description:
> +  Setup and initialize the BMC for the SMM phase.  In order to verify the BMC
> is functioning
> +  as expected, the BMC Selftest is performed.  The results are then checked
> and any errors are
> +  reported to the error manager.  Errors are collected throughout this routine
> and reported
> +  just prior to installing the driver.  If there are more errors than
> MAX_SOFT_COUNT, then they
> +  will be ignored.
> +
> +Arguments:
> +  ImageHandle - Handle of this driver image
> +  SystemTable - Table containing standard EFI services
> +
> +Returns:
> +  EFI_SUCCESS - Successful driver initialization
> +
> +--*/
You still follow the old Intel coding style? I think we should have function header above the function. Could we relocate the function header?


> +{
> +  EFI_STATUS                       Status;
> +
> +  DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer entry \n"));
> +
> +  Status = gMmst->MmAllocatePool (
> +                    EfiRuntimeServicesData,
> +                    sizeof (IPMI_BMC_INSTANCE_DATA),
> +                    (VOID **)&mIpmiInstance);
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "mIpmiInstance mem alloc failed - 0x%x\n",
> Status));
> +    return Status;
> +  } else {
> +
> +    //
> +    // Initialize the KCS transaction timeout. Assume delay unit is 1000 us.
> +    //
> +    mIpmiInstance->KcsTimeoutPeriod = (BMC_KCS_TIMEOUT * 1000*1000) /
> KCS_DELAY_UNIT;
> +
> +    //
> +    // Initialize IPMI IO Base, we still use SMS IO base to get device ID and
> Seltest result since SMM IF may have different cmds supported
> +    //
> +    mIpmiInstance->IpmiIoBase                       = FixedPcdGet16
> (PcdIpmiSmmIoBaseAddress);
> +    mIpmiInstance->Signature                        = SM_IPMI_BMC_SIGNATURE;
> +    mIpmiInstance->SlaveAddress                     = BMC_SLAVE_ADDRESS;
> +    mIpmiInstance->BmcStatus                        = BMC_NOTREADY;
> +    mIpmiInstance->IpmiTransport.IpmiSubmitCommand  =
> IpmiSendCommand;
> +    mIpmiInstance->IpmiTransport.GetBmcStatus       = IpmiGetBmcStatus;
> +
> +    mHandle = NULL;
> +    Status = gMmst->MmInstallProtocolInterface (
> +                      &mHandle,
> +                      &gSmmIpmiTransportProtocolGuid,
> +                      EFI_NATIVE_INTERFACE,
> +                      &mIpmiInstance->IpmiTransport
> +                      );
> +    ASSERT_EFI_ERROR (Status);
> +
> +    DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer exit \n"));
> +
> +    return EFI_SUCCESS;
> +  }
> +}
> +
> +/**
> +  The module Entry Point of driver.
> +
> +  @param  ImageHandle    The firmware allocated handle for the EFI image.
> +  @param  SystemTable    A pointer to the MM System Table.
[In] [out] tag is missed in above two @param.

Abner

> +
> +  @retval EFI_SUCCESS    The entry point is executed successfully.
> +  @retval Other          Some error occurs when executing this entry point.
> +
> +**/
> +EFI_STATUS
> +InitializeGenericIpmiStandaloneMm (
> +  IN EFI_HANDLE             ImageHandle,
> +  IN EFI_MM_SYSTEM_TABLE    *SystemTable
> +  )
> +{
> +  SmmInitializeIpmiKcsPhysicalLayer ();
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Unloads an image.
> +
> +  @param[in] ImageHandle        Handle that identifies the image to be
> unloaded.
> +
> +  @retval EFI_SUCCESS           The image has been unloaded.
> +  @retval EFI_INVALID_PARAMETER ImageHandle is not a valid image handle.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +GenericIpmiStandaloneMmUnload (
> +  IN EFI_HANDLE             ImageHandle
> +  )
> +{
> +  EFI_STATUS                Status;
> +
> +  Status = EFI_SUCCESS;
> +  if (mIpmiInstance != NULL) {
> +    if (mHandle != NULL) {
> +      Status = gMmst->MmUninstallProtocolInterface (
> +                        mHandle,
> +                        &gSmmIpmiTransportProtocolGuid,
> +                        &mIpmiInstance->IpmiTransport
> +                        );
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +    gMmst->MmFreePool (mIpmiInstance);
> +  }
> +
> +  return Status;
> +}
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Stan
> daloneMm/StandaloneMmGenericIpmi.inf
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Stan
> daloneMm/StandaloneMmGenericIpmi.inf
> new file mode 100644
> index 000000000000..336e28368ed6
> --- /dev/null
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Stan
> daloneMm/StandaloneMmGenericIpmi.inf
> @@ -0,0 +1,52 @@
> +## @file
> +# StandaloneMm Generic IPMI SMM Driver.
> +#
> +# @copyright
> +# Copyright 2023 Intel Corporation. <BR>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +[Defines]
> +  INF_VERSION                         = 0x00010017
> +  BASE_NAME                           = StandaloneMmGenericIpmi
> +  FILE_GUID                           = 6899c3ea-0920-414f-9555-ad2f00f81060
> +  MODULE_TYPE                         = MM_STANDALONE
> +  VERSION_STRING                      = 1.0
> +  PI_SPECIFICATION_VERSION            = 0x00010032
> +  ENTRY_POINT                         = InitializeGenericIpmiStandaloneMm
> +  UNLOAD_IMAGE                        = GenericIpmiStandaloneMmUnload
> +
> +[Sources]
> +  ../Common/IpmiHooks.h
> +  ../Common/IpmiHooks.c
> +  ../Common/IpmiBmcCommon.h
> +  ../Common/KcsBmc.c
> +  ../Common/KcsBmc.h
> +  ../Common/IpmiBmc.c
> +  ../Common/IpmiBmc.h
> +  StandaloneMmGenericIpmi.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  IpmiFeaturePkg/IpmiFeaturePkg.dec
> +
> +[LibraryClasses]
> +  MemoryAllocationLib
> +  BaseLib
> +  MmServicesTableLib
> +  DebugLib
> +  StandaloneMmDriverEntryPoint
> +  IoLib
> +  ReportStatusCodeLib
> +  TimerLib
> +
> +[Protocols]
> +  gSmmIpmiTransportProtocolGuid                     # PROTOCOL
> ALWAYS_PRODUCED
> +
> +[Guids]
> +
> +[Pcd]
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiSmmIoBaseAddress
> +
> +[Depex]
> +  gIpmiTransportProtocolGuid
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> ec
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> ec
> index 5df71300cbd1..73dca30caae9 100644
> ---
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> ec
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.d
> ec
> @@ -129,4 +129,6 @@
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiIoBaseAddress|0xCA2|UINT16|0x
> D0000003
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdSioMailboxBaseAddress|0x600|UINT32
> |0xD0000004
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiBmcReadyDelayTimer|120|UINT8|
> 0xD0000005
> +
> +[PcdsFixedAtBuild, PcdsDynamic, PcdsDynamicEx]
>
> gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiSmmIoBaseAddress|0xCA2|UINT1
> 6|0xD0000006
> --
> 2.26.2.windows.1



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