Re: [edk2-devel] [edk2-platforms][PATCH 1/2] ManageabilityPkg/IpmiFrb: IPMI BMC ACPI Driver

["Chang, Abner" <abner.chang@amd.com>]

[AMD Official Use Only - General]

Hi Isaac,
For "INTEL", we can change it to all whitespace as the string would be patched in runtime using PcdAcpiDefaultOemId (which is also Intel 😊)
For "MSFT", I prefer to keep it as that value is hardcoded in the source code. We can create a PCD for this as well but this would be not in the migration scope that claims the same functionality with the one under Intel folder.

Thanks
Abner

> -----Original Message-----
> From: Oram, Isaac W <isaac.w.oram@intel.com>
> Sent: Friday, May 19, 2023 11:28 AM
> To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>
> Cc: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; Nickle
> Wang <nicklew@nvidia.com>; Tinh Nguyen
> <tinhnguyen@os.amperecomputing.com>
> Subject: RE: [edk2-devel] [edk2-platforms][PATCH 1/2]
> ManageabilityPkg/IpmiFrb: IPMI BMC ACPI Driver
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>
>
> You could consider changing "INTEL" and ('M', 'S', 'F', 'T') to more generic
> placeholders.  But that is also something that could be looked at separately
> and more widely.
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang,
> Abner via groups.io
> Sent: Saturday, May 13, 2023 8:49 AM
> To: devel@edk2.groups.io
> Cc: Oram, Isaac W <isaac.w.oram@intel.com>; Abdul Lateef Attar
> <abdattar@amd.com>; Nickle Wang <nicklew@nvidia.com>; Tinh Nguyen
> <tinhnguyen@os.amperecomputing.com>
> Subject: [edk2-devel] [edk2-platforms][PATCH 1/2]
> ManageabilityPkg/IpmiFrb: IPMI BMC ACPI Driver
>
> From: Abner Chang <abner.chang@amd.com>
>
> IpmiBmcAcpi is cloned from
> edk2-platforms/Features/Intel/OutOfBandManagement/
> IpmiFeaturePkg/BmcAcpi in order to consolidate
> edk2 system manageability support in one place.
> Uncustify is applied to C files and no functionalities are changed in this patch.
>
> We will still keep the one under IpmiFeaturePkg/BmcAcpi until the reference to
> this instance are removed from platforms.
>
> Signed-off-by: Abner Chang <abner.chang@amd.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Abdul Lateef Attar <abdattar@amd.com>
> Cc: Nickle Wang <nicklew@nvidia.com>
> Cc: Tinh Nguyen <tinhnguyen@os.amperecomputing.com>
> ---
>  .../Universal/IpmiBmcAcpi/BmcAcpi.inf         |  47 ++++
>  .../Universal/IpmiBmcAcpi/BmcAcpi.c           | 264 ++++++++++++++++++
>  .../Universal/IpmiBmcAcpi/BmcSsdt/BmcSsdt.asl |  28 ++
>  .../IpmiBmcAcpi/BmcSsdt/IpmiOprRegions.asi    |  58 ++++
>  4 files changed, 397 insertions(+)
>  create mode 100644
> Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.inf
>  create mode 100644
> Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
>  create mode 100644
> Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcSsdt/BmcSsdt.asl
>  create mode 100644
> Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcSsdt/IpmiOprRegion
> s.asi
>
> diff --git a/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.inf
> b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.inf
> new file mode 100644
> index 0000000000..a21c5b9b35
> --- /dev/null
> +++ b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.inf
> @@ -0,0 +1,47 @@
> +### @file
> +# Component description file for BMC ACPI.
> +#
> +# Copyright (c) 2018 - 2019, Intel Corporation. All rights
> +reserved.<BR> # # SPDX-License-Identifier: BSD-2-Clause-Patent # ###
> +
> +[Defines]
> +  INF_VERSION                   = 0x00010005
> +  BASE_NAME                     = BmcAcpi
> +  FILE_GUID                     = 09E3B4BE-F731-4903-AAE6-BD5D573B6140
> +  MODULE_TYPE                   = DXE_DRIVER
> +  VERSION_STRING                = 1.0
> +  ENTRY_POINT                   = BmcAcpiEntryPoint
> +
> +[Sources]
> +  BmcAcpi.c
> +  BmcSsdt/BmcSsdt.asl
> +
> +[Packages]
> +  ManageabilityPkg/ManageabilityPkg.dec
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +
> +[LibraryClasses]
> +  BaseMemoryLib
> +  DebugLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +  UefiLib
> +
> +[Protocols]
> +  gEfiFirmwareVolume2ProtocolGuid
> +  gEfiAcpiTableProtocolGuid
> +
> +[Pcd]
> +  gEfiMdePkgTokenSpaceGuid.PcdIpmiKcsIoBaseAddress
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId
> +
> +[Depex]
> +  gEfiAcpiTableProtocolGuid
> +
> +[BuildOptions]
> +  *_*_*_ASL_FLAGS = -oi
> diff --git a/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
> b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
> new file mode 100644
> index 0000000000..cf066dd095
> --- /dev/null
> +++ b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcAcpi.c
> @@ -0,0 +1,264 @@
> +/** @file
> +  IPMI BMC ACPI.
> +
> +  Copyright (c) 2018 - 2019, Intel Corporation. All rights
> + reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +//
> +// Statements that include other header files // #include <PiDxe.h>
> +
> +#include <IndustryStandard/Acpi.h>
> +#include <Protocol/AcpiSystemDescriptionTable.h>
> +#include <Protocol/FirmwareVolume2.h>
> +#include <Protocol/AcpiTable.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h> #include
> +<Library/BaseMemoryLib.h> #include <Library/UefiLib.h>
> +
> +#ifndef EFI_ACPI_CREATOR_ID
> +#define EFI_ACPI_CREATOR_ID  SIGNATURE_32 ('M', 'S', 'F', 'T') #endif
> +#ifndef EFI_ACPI_CREATOR_REVISION #define EFI_ACPI_CREATOR_REVISION
> +0x0100000D #endif
> +
> +/**
> +
> +  Locate the first instance of a protocol.  If the protocol requested
> + is an  FV protocol, then it will return the first FV that contains the
> + ACPI table  storage file.
> +
> +  @param [in] Protocol  The protocol to find.
> +  @param [in] Instance  Return pointer to the first instance of the protocol.
> +  @param [in] Type      The type of protocol to locate.
> +
> +  @retval EFI_SUCCESS           The function completed successfully.
> +  @retval EFI_NOT_FOUND         The protocol could not be located.
> +  @retval EFI_OUT_OF_RESOURCES  There are not enough resources to find
> the protocol.
> +
> +**/
> +EFI_STATUS
> +LocateSupportProtocol (
> +  IN   EFI_GUID  *Protocol,
> +  OUT  VOID      **Instance,
> +  IN   UINT32    Type
> +  )
> +{
> +  EFI_STATUS              Status;
> +  EFI_HANDLE              *HandleBuffer;
> +  UINTN                   NumberOfHandles;
> +  EFI_FV_FILETYPE         FileType;
> +  UINT32                  FvStatus = 0;
> +  EFI_FV_FILE_ATTRIBUTES  Attributes;
> +  UINTN                   Size;
> +  UINTN                   Index;
> +
> +  Status = gBS->LocateHandleBuffer (ByProtocol, Protocol, NULL,
> + &NumberOfHandles, &HandleBuffer);  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  //
> +  // Looking for FV with ACPI storage file  //  for (Index = 0; Index <
> + NumberOfHandles; Index++) {
> +    Status = gBS->HandleProtocol (HandleBuffer[Index], Protocol, Instance);
> +    ASSERT (!EFI_ERROR (Status));
> +
> +    if (!Type) {
> +      //
> +      // Not looking for the FV protocol, so find the first instance of the
> +      // protocol.  There should not be any errors because our handle buffer
> +      // should always contain at least one or LocateHandleBuffer would have
> +      // returned not found.
> +      //
> +      break;
> +    }
> +
> +    //
> +    // See if it has the ACPI storage file
> +    //
> +    Status = ((EFI_FIRMWARE_VOLUME2_PROTOCOL *)(*Instance))->ReadFile
> (
> +                                                               *Instance,
> +                                                               &gEfiCallerIdGuid,
> +                                                               NULL,
> +                                                               &Size,
> +                                                               &FileType,
> +                                                               &Attributes,
> +                                                               &FvStatus
> +                                                               );
> +
> +    //
> +    // If we found it, then we are done
> +    //
> +    if (!EFI_ERROR (Status)) {
> +      break;
> +    }
> +  }
> +
> +  gBS->FreePool (HandleBuffer);
> +  return Status;
> +}
> +
> +/**
> +  Update ACPI SSDT for BMC IPMI KCS device
> +
> +  @param [in] Table  Pointer to ACPI SSDT
> +
> +  @retval EFI_SUCCESS  SSDT is updated according to PCD settings **/
> +EFI_STATUS UpdateDeviceSsdtTable (
> +  IN OUT EFI_ACPI_COMMON_HEADER  *Table
> +  )
> +{
> +  EFI_ACPI_DESCRIPTION_HEADER  *TableHeader = NULL;
> +  UINT64                       TempOemTableId;
> +  UINT8                        *DataPtr;
> +  EFI_ACPI_IO_PORT_DESCRIPTOR  *IoRsc;
> +
> +  TableHeader = (EFI_ACPI_DESCRIPTION_HEADER *)Table;
> +
> +  //
> +  // Update the OEMID and OEM Table ID.
> +  //
> +  CopyMem (&TableHeader->OemId, PcdGetPtr (PcdAcpiDefaultOemId),
> sizeof
> + (TableHeader->OemId));  TempOemTableId = PcdGet64
> + (PcdAcpiDefaultOemTableId);  CopyMem (&TableHeader->OemTableId,
> &TempOemTableId, sizeof (UINT64));
> +  TableHeader->CreatorId       = EFI_ACPI_CREATOR_ID;
> +  TableHeader->CreatorRevision = EFI_ACPI_CREATOR_REVISION;
> +
> +  //
> +  // Update IO(Decode16, 0xCA2, 0xCA2, 0, 2)  //  DEBUG ((DEBUG_INFO,
> + "UpdateDeviceSsdtTable - IPMI\n"));  for (DataPtr = (UINT8 *)(Table +
> + 1);
> +       DataPtr < (UINT8 *)((UINT8 *)Table + Table->Length - 4);
> +       DataPtr++)
> +  {
> +    if (CompareMem (DataPtr, "_CRS", 4) == 0) {
> +      DataPtr += 4; // Skip _CRS
> +      ASSERT (*DataPtr == AML_BUFFER_OP);
> +      DataPtr++;  // Skip AML_BUFFER_OP
> +      ASSERT ((*DataPtr & (BIT7|BIT6)) == 0);
> +      DataPtr++;  // Skip PkgLength - 0xD
> +      ASSERT ((*DataPtr) == AML_BYTE_PREFIX);
> +      DataPtr++;  // Skip BufferSize OpCode
> +      DataPtr++;  // Skip BufferSize - 0xA
> +      IoRsc = (VOID *)DataPtr;
> +      ASSERT (IoRsc->Header.Bits.Type == ACPI_SMALL_ITEM_FLAG);
> +      ASSERT (IoRsc->Header.Bits.Name ==
> ACPI_SMALL_IO_PORT_DESCRIPTOR_NAME);
> +      ASSERT (IoRsc->Header.Bits.Length == sizeof
> (EFI_ACPI_IO_PORT_DESCRIPTOR) - sizeof
> (ACPI_SMALL_RESOURCE_HEADER));
> +      DEBUG ((DEBUG_INFO, "IPMI IO Base in ASL update - 0x%04x <=
> 0x%04x\n", IoRsc->BaseAddressMin, PcdGet16 (PcdIpmiKcsIoBaseAddress)));
> +      IoRsc->BaseAddressMin = PcdGet16 (PcdIpmiKcsIoBaseAddress);
> +      IoRsc->BaseAddressMax = PcdGet16 (PcdIpmiKcsIoBaseAddress);
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +
> +  Entry point for Acpi platform driver.
> +
> +  @param [in] ImageHandle  A handle for the image that is initializing this
> driver.
> +  @param [in] SystemTable  A pointer to the EFI system table.
> +
> +  @retval EFI_SUCCESS           Driver initialized successfully.
> +  @retval EFI_LOAD_ERROR        Failed to Initialize or has been loaded.
> +  @retval EFI_OUT_OF_RESOURCES  Could not allocate needed resources.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +BmcAcpiEntryPoint (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +  EFI_STATUS  AcpiStatus;
> +
> +  EFI_FIRMWARE_VOLUME2_PROTOCOL  *FwVol;
> +  INTN                           Instance      = 0;
> +  EFI_ACPI_COMMON_HEADER         *CurrentTable = NULL;
> +  UINTN                          TableHandle   = 0;
> +  UINT32                         FvStatus;
> +  UINT32                         Size;
> +
> +  EFI_ACPI_TABLE_PROTOCOL  *AcpiTable;
> +  UINTN                    TableSize;
> +
> +  //
> +  // Find the AcpiTable protocol
> +  //
> +  Status = gBS->LocateProtocol (&gEfiAcpiTableProtocolGuid, NULL, (VOID
> + **)&AcpiTable);  if (EFI_ERROR (Status)) {
> +    return EFI_ABORTED;
> +  }
> +
> +  //
> +  // Locate the firmware volume protocol  //  Status =
> + LocateSupportProtocol (&gEfiFirmwareVolume2ProtocolGuid, (VOID
> + **)&FwVol, 1);  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status   = EFI_SUCCESS;
> +  Instance = 0;
> +
> +  //
> +  // Read tables from the storage file.
> +  //
> +  while (!EFI_ERROR (Status)) {
> +    CurrentTable = NULL;
> +
> +    Status = FwVol->ReadSection (
> +                      FwVol,
> +                      &gEfiCallerIdGuid,
> +                      EFI_SECTION_RAW,
> +                      Instance,
> +                      (VOID **)&CurrentTable,
> +                      (UINTN *)&Size,
> +                      &FvStatus
> +                      );
> +    if (!EFI_ERROR (Status)) {
> +      //
> +      // Perform any table specific updates.
> +      //
> +      AcpiStatus = UpdateDeviceSsdtTable (CurrentTable);
> +      if (!EFI_ERROR (AcpiStatus)) {
> +        TableHandle = 0;
> +        TableSize   = ((EFI_ACPI_DESCRIPTION_HEADER *)CurrentTable)->Length;
> +        ASSERT (Size >= TableSize);
> +
> +        Status = AcpiTable->InstallAcpiTable (
> +                              AcpiTable,
> +                              CurrentTable,
> +                              TableSize,
> +                              &TableHandle
> +                              );
> +
> +        ASSERT_EFI_ERROR (Status);
> +      }
> +
> +      //
> +      // Increment the instance
> +      //
> +      Instance++;
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> diff --git
> a/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcSsdt/BmcSsdt.asl
> b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcSsdt/BmcSsdt.asl
> new file mode 100644
> index 0000000000..8157d744c8
> --- /dev/null
> +++
> b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcSsdt/BmcSsdt.as
> +++ l
> @@ -0,0 +1,28 @@
> +/** @file
> +  BMC ACPI SSDT.
> +
> +Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +DefinitionBlock (
> +    "BmcSsdt.aml",
> +    "SSDT",
> +    0x02,         // SSDT revision.
> +                  // A Revision field value greater than or equal to 2 signifies that
> integers
> +                  // declared within the Definition Block are to be evaluated as 64-bit
> values
> +    "INTEL",      // OEM ID (6 byte string), same as PcdAcpiDefaultOemId
> defined in MdeModulePkg.dec
> +    "BMCACPI",    // OEM table ID  (8 byte string)
> +    0x0           // OEM version of DSDT table (4 byte Integer)
> +    )
> +{
> +
> +  External(\_SB.PC00.LPC0, DeviceObj)
> +
> +  Scope (\_SB.PC00.LPC0)
> +  {
> +    #include "IpmiOprRegions.asi"
> +  }
> +
> +}
> diff --git
> a/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcSsdt/IpmiOprRegi
> ons.asi
> b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcSsdt/IpmiOprRegi
> ons.asi
> new file mode 100644
> index 0000000000..ffd6af3944
> --- /dev/null
> +++
> b/Features/ManageabilityPkg/Universal/IpmiBmcAcpi/BmcSsdt/IpmiOprReg
> +++ ions.asi
> @@ -0,0 +1,58 @@
> +/** @file
> +  IPMI ACPI SSDT.
> +
> +Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +Device(IPMC)
> +{
> +    // Config DWord, modified during POST
> +    // Bit definitions are the same as PPMFlags:
> +    // [00]    = Plug and Play BMC Detection enabled in setup
> +    // [31:01] = Reserved    = 0
> +
> +    Name(ECFL, 0x80000000)
> +
> +    // Return the interface specification revision
> +    Method(_SRV)
> +    {
> +        // IPMI Specification Revision v2.0
> +        Return(0x0200)
> +    }
> +
> +    Method(_STA, 0)
> +    {
> +        //
> +        // Assume OK
> +        //
> +          Store (0xF, Local0)
> +
> +        Return(Local0)
> +
> +        // Bit 0 - Set if the device is present.
> +        // Bit 1 - Set if the device is enabled and decoding its resources.
> +        // Bit 2 - Set if the device should be shown in the UI.
> +        // Bit 3 - Set if the device is functioning properly (cleared if the device
> failed its diagnostics).
> +        // Bit 4 - Set if the battery is present.
> +        // Bit 5 - Reserved (must be cleared).
> +    }   // end of _STA
> +
> +    // Return the x86 resources consumed by BMC
> +    Name(_CRS, ResourceTemplate()
> +    {
> +        // Uses 8-bit ports 0xCA2-0xCA5
> +        IO(Decode16, 0xCA2, 0xCA2, 0, 2)
> +    })
> +
> +    Name(_HID, "IPI0001")           // IPMI device
> +    Name(_IFT, 0x1)                 // KCS system interface type
> +    Name(_STR, Unicode("IPMI_KCS"))
> +
> +    Name(_UID, 0)  // First interface.
> +
> +
> +} // end of  Device(IPMC)
> +
> +
> --
> 2.37.1.windows.1
>
>
>
> 
>