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]

Yes, I know. Just letting you know there are some non industry standard common code in this package and you probably can give some clarifications. 😊
Sure, I am looking at the patch now.

Abner

> -----Original Message-----
> From: Huang, Li-Xia <lisa.huang@intel.com>
> Sent: Thursday, October 26, 2023 1:50 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,
>
> FAST_VIDEO_SUPPORT exists in GenericIPMI Dxe driver, my patch focuses on
> StandaloneMm support, and will not touch other drivers.
> BTW, the patch format is also updated and pass all check.
> Could you help to review again? Thanks.
>
> Regards,
> Lisa
>
> -----Original Message-----
> From: Chang, Abner <Abner.Chang@amd.com>
> Sent: Tuesday, October 24, 2023 6:43 PM
> To: Huang, Li-Xia <lisa.huang@intel.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]
>
> BTW Lisa,
> As Issac told me the implementation which Intel upstream to IpmiFeaturePkg
> was from a pretty old intel code base. Also, something like
> FAST_VIDEO_SUPPORT, BmcSlaveAddress and GetBmcStatus seems not used.
> I think those code should be removed as well.
>
> 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 (#110077): https://edk2.groups.io/g/devel/message/110077
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]
-=-=-=-=-=-=-=-=-=-=-=-