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

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

[AMD Official Use Only - General]

Issac is removed.

Hi Lisa,
These two packages are diverging in IPMI transport (GenricIpmi) and some other high level drivers such as IpmiCommandLib. I plan to take below actions.
1. Remove IpmiCommandLib from IpmiFeaturePkg, as the IPMI command support in ManageabilityPkg has the same functionalities with few newly added IPMI commands. This library is independent of IPMI transport interface.
2. Remove Frb, IpmiFru, OsWdt and SolStatus from IpmiFeaturePkg as those were migrated to  ManageabilityPkg already.
3. I will get back to GenericIpmi and turn it into ManageabilityPkg framework then remove GenericIpmi from IpmiFeaturePkg.

So I would like to take back my suggestion having you to send the patch against ManageabilityPkg. As the transport implementation has some gap between these two packages.
Thank you
Abner

> -----Original Message-----
> From: Huang, Li-Xia <lisa.huang@intel.com>
> Sent: Monday, October 23, 2023 1:48 PM
> To: Chang, Abner <Abner.Chang@amd.com>; devel@edk2.groups.io
> Cc: Isaac Oram <isaac.w.oram@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Wu, Yidong <yidong.wu@intel.com>; Xu,
> Wei6 <wei6.xu@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1 1/1] IpmiFeaturePkg/GenericIpmi:
> Support Standalone MM
>
> [AMD Official Use Only - General]
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Abner,
>
> Thanks for your feedback.
> Since the IPMI implementation in these two packages are diverging now, and
> my patch has some dependency with common code in IpmiFeaturePkg , it will
> be better to resend this patch after the diverging is done.
>
> Our target schedule is  '23 WW51', could you let me know what's the schedule
> for these two packages'(IpmiFeaturePkg and ManageabilityPkg) diverging?
> Thanks.
>
> Regards,
> Lisa
>
> -----Original Message-----
> From: Chang, Abner <Abner.Chang@amd.com>
> Sent: Saturday, October 21, 2023 10:15 PM
> To: devel@edk2.groups.io; Huang, Li-Xia <lisa.huang@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1 1/1] IpmiFeaturePkg/GenericIpmi:
> Support Standalone MM
>
> [AMD Official Use Only - General]
>
> Hi Lisa,
> Issac was no longer the maintainer of IpmiFeaturePkg as he was retired. Nate
> had sent the maintainers update for review.
> As the conversation I had with Issac, we all agreed IpmiFeaturePkg should be
> deprecated as the IPMI related drivers are now located under
> ManageabilityPkg in edk2-platforms. The IPMI implementation in these two
> packages are diverging now, which is not good.
> Could you please send this patch against ManageabilityPkg? I can help to
> review it, so does Nate as he also proposed himself as the maintainers of
> ManageabilityPkg.
>
> Thanks
> Abner
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Huang,
> > Li-Xia via groups.io
> > Sent: Friday, October 20, 2023 2:46 PM
> > To: devel@edk2.groups.io
> > Cc: Isaac Oram <isaac.w.oram@intel.com>; Nate DeSimone
> > <nathaniel.l.desimone@intel.com>
> > Subject: [edk2-devel] [PATCH v1 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: Isaac Oram <isaac.w.oram@intel.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..52d8d2abdd0d
> > --- /dev/null
> > +++
> >
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Stan
> > daloneMm/StandaloneMmGenericIpmi.c
> > @@ -0,0 +1,148 @@
> > +/** @file
> >
> > +  Generic StandaloneMm IPMI stack driver
> >
> > +
> >
> > +  @copyright
> >
> > +  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"
> >
> > +#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
> >
> > +
> >
> > +--*/
> >
> > +{
> >
> > +  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.
> >
> > +
> >
> > +  @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;
> >
> > +}
> > \ No newline at end of file
> > 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 (#109823):
> > https://edk2.groups.io/g/devel/message/109823
> > Mute This Topic: https://groups.io/mt/102076516/7039027
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [abner.chang@amd.com]
> > -=-=-=-=-=-=
> >



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