Re: [edk2-platforms][PATCH V2 06/11] Platform/Sgi: Add SMBIOS Type4 Table

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Pranav,

Please find my comments inline marked [SAMI].

Some comments in previous patches apply here as well and are not mentioned.

With those addressed.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar


On 16/05/2021 10:29 AM, Pranav Madhu wrote:
> Add the SMBIOS type 4 table (Processor Information) that includes
> information about manufacture, family, processor id, maximum operating
> frequency, and other information related to the processor.
>
> Signed-off-by: Pranav Madhu <pranav.madhu@arm.com>
> ---
>   Platform/ARM/SgiPkg/SgiPlatform.dsc.inc                                   |   1 +
>   Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf       |   6 +
>   Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h         |  12 ++
>   Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c         |   1 +
>   Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c | 209 ++++++++++++++++++++
>   5 files changed, 229 insertions(+)
>
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
> index a0f217f5107c..091de0c99c74 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
> @@ -56,6 +56,7 @@
>     HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
>     MemoryAllocationLib|MdeModulePkg/Library/DxeCoreMemoryAllocationLib/DxeCoreMemoryAllocationLib.inf
>     PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> +  PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
>   
>   [LibraryClasses.common.DXE_DRIVER]
>     FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
> diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> index b3c1619ddc66..4652a9c62b88 100644
> --- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> +++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> @@ -18,6 +18,7 @@
>     Type0BiosInformation.c
>     Type1SystemInformation.c
>     Type3SystemEnclosure.c
> +  Type4ProcessorInformation.c
>   
>   [Packages]
>     ArmPkg/ArmPkg.dec
> @@ -27,9 +28,11 @@
>     Platform/ARM/SgiPkg/SgiPlatform.dec
>   
>   [LibraryClasses]
> +  ArmLib
>     ArmPlatformLib
>     DebugLib
>     HobLib
> +  PrintLib
>     UefiDriverEntryPoint
>   
>   [Guids]
> @@ -37,6 +40,9 @@
>     gArmSgiPlatformIdDescriptorGuid
>   
>   [FixedPcd]
> +  gArmPlatformTokenSpaceGuid.PcdClusterCount
> +  gArmPlatformTokenSpaceGuid.PcdCoreCount
> +  gArmSgiTokenSpaceGuid.PcdChipCount
>     gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareRevision
>   
>   [Protocols]
> diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h
> index 4a6f8be2a2c2..8a9be0cfc4c8 100644
> --- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h
> +++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h
> @@ -29,8 +29,20 @@ InstallSystemEnclosure (
>     IN     EFI_SMBIOS_PROTOCOL    *Smbios
>     );
>   
> +EFI_STATUS
> +EFIAPI
> +InstallProcessorInformation (
> +  IN     EFI_SMBIOS_PROTOCOL    *Smbios
> +  );
> +
>   enum SMBIOS_REFRENCE_HANDLES {
>     SMBIOS_HANDLE_ENCLOSURE = 0x1000,
> +  SMBIOS_HANDLE_CLUSTER1,
> +  SMBIOS_HANDLE_L1I_CACHE,
> +  SMBIOS_HANDLE_L1D_CACHE,
> +  SMBIOS_HANDLE_L2_CACHE,
> +  SMBIOS_HANDLE_L3_CACHE,
> +  SMBIOS_HANDLE_L4_CACHE,
>   };
>   
>   #endif // SMBIOS_PLATFORM_DXE_H_
> diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> index 5f4b833dc9fe..269bd0f9d843 100644
> --- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> +++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> @@ -29,6 +29,7 @@ ARM_RD_SMBIOS_TABLE_INSTALL_FPTR mSmbiosTableList[] = {
>     &InstallBiosInformation,
>     &InstallSystemInformation,
>     &InstallSystemEnclosure,
> +  &InstallProcessorInformation,
>   };
>   
>   /**
> diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c
> new file mode 100644
> index 000000000000..fda532b558af
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c
> @@ -0,0 +1,209 @@
> +/** @file
> +  SMBIOS Type 4 (Processor information) table for ARM RD platforms.
> +
> +  This file installs SMBIOS Type 4 (Processor information) Table for Arm's
> +  Reference Design platforms. It includes information about manufacture,
> +  family, processor id, maximum operating frequency, and other information
> +  related to the processor.
> +
> +  Copyright (c) 2021, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Specification Reference:
> +    - SMBIOS Reference Specification 3.4.0, Chapter 7.5
> +**/
> +
> +#include <Library/ArmLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PrintLib.h>
> +#include <Protocol/Smbios.h>
> +
> +#include "SgiPlatform.h"
> +#include "SmbiosPlatformDxe.h"
> +
> +#define NEOVERSE_E1_THREADS_PER_CORE    2
> +
> +#define SOCKET_TYPE_BASE        3
> +#define SOCKET_TYPE_NUM         1
> +#define PROCESSOR_VERSION_BASE  (SOCKET_TYPE_BASE + SOCKET_TYPE_NUM)
> +#define PROCESSOR_VERSION_NUM   8
> +#define SERIAL_NUMBER_BASE      (PROCESSOR_VERSION_BASE + PROCESSOR_VERSION_NUM)
> +#define TYPE4_STRINGS                                   \
> +  "0x000\0"                     /* Part Number */       \
> +  "ARM LTD\0"                   /* manufactuer */       \
> +  "Other\0"                     /* socket type */       \
> +  "Unknown\0"                   /* Processor Version */ \
> +  "Cortex-A75\0"                                        \
> +  "Neoverse-N1\0"                                       \
> +  "Neoverse-N1\0"                                       \
> +  "Neoverse-E1\0"                                       \
> +  "Neoverse-V1\0"                                       \
> +  "Neoverse-V1\0"                                       \
> +  "Neoverse-N2\0"                                       \
> +  "000-0\0"                     /* Serial number */     \
> +  "783-3\0"                                             \
> +  "786-1\0"                                             \
> +  "786-1\0"                                             \
> +  "786-2\0"                                             \
> +  "78A-1\0"                                             \
> +  "78A-2\0"                                             \
> +  "7B7-1\0"
> +
> +/* SMBIOS Type4 structure */
> +#pragma pack(1)
> +struct ArmRdSmbiosType4 {
> +  SMBIOS_TABLE_TYPE4  Base;
> +  UINT8               Strings[sizeof (TYPE4_STRINGS)];
> +} ARM_TYPE4;
> +#pragma pack()
> +
> +/* Processor information */
> +static struct ArmRdSmbiosType4 mArmRdSmbiosType4 = {
> +  {
> +    {
> +      // SMBIOS header
> +      EFI_SMBIOS_TYPE_PROCESSOR_INFORMATION, // Type 4
> +      sizeof (SMBIOS_TABLE_TYPE4),           // Length
> +      SMBIOS_HANDLE_CLUSTER1,                // handle number
> +    },
> +    SOCKET_TYPE_BASE,        // Socket type
> +    CentralProcessor,        // Processor type
> +    ProcessorFamilyIndicatorFamily2,
> +                             // Use Processor Family 2 field
> +    2,                       // Manufacturer string number
> +    {{0}, {0}},              // Processor id, update dynamically
> +    PROCESSOR_VERSION_BASE,  // Processor version, update dynamically
> +    {0, 0, 0, 0, 0, 1},      // Non legacy mode for processor voltage
> +    0,                       // External clock frequency unknown
> +    2600,                    // Max speed in MHz
> +    2600,                    // Current speed in MHz
> +    (                        // Status
> +     (1 << 6) |              // CPU Socket Populated
> +     (1 << 0)                // CPU Enabled
> +    ),
> +    ProcessorUpgradeOther,   // Processor Upgrade
> +    SMBIOS_HANDLE_L1I_CACHE, // L1 Cache handle
> +    SMBIOS_HANDLE_L2_CACHE,  // L2 Cache handle
> +    SMBIOS_HANDLE_L3_CACHE,  // L3 Cache handle
> +    0,                       // Processor serial number not set
> +    0,                       // Processor asset tag not set
> +    1,                       // Part number, update dynamically
> +    0,                       // Core count, update dynamically
> +    0,                       // Enabled core count, update dynamically
> +    0,                       // Thread per socket count
> +    (                        // Processor characteristics
> +      (1 << 2) |             // 64-bit Capable
> +      (1 << 3) |             // Multi-Core
> +      (1 << 5) |             // Execute Protection
> +      (1 << 6) |             // Enhanced Virtualization
> +      (1 << 7)               // Power/Performance Control
> +    ),
> +    ProcessorFamilyARM       // Processor Family 2
> +    },
> +  TYPE4_STRINGS
> +};
> +
> +/**
> +  Update the part-number string.
> +
> +  Get the part number from ProcessorId and update TYPE4_STRINGS
> +
> +  @param  ProcessorId    The processor Id read from MIDR register
> +**/
> +STATIC
> +VOID
> +UpdatePartNumber (
> +  IN     UINT64 ProcessorId
> +  )
> +{
> +  CHAR8  PartNumber[4] = {0};
> +  UINT16 PartNum;
> +
> +  PartNum = (UINT16)((ProcessorId >> 4) & 0xFFF);
> +
> +  /* Convert 3 digit hexadecimal partnumber to ASCII and update TYPE4_STRINGS */
> +  AsciiSPrint(PartNumber, sizeof (PartNumber), "%03x", PartNum);
[SAMI] Space needed between AsciiSPrintand (.
> +  CopyMem (&mArmRdSmbiosType4.Strings[2], PartNumber, sizeof (PartNumber));
[SAMI] Is the index for the string correct? Should it be 0?
Also can you check if the output works as expected? Can you send the 
smbiosview dump for this table, please?
[/SAMI]
> +}
> +
> +/**
> +  Install SMBIOS Processor information Table
> +
> +  Install the SMBIOS Processor information (type 4) table for Arm's Reference
> +  Design platforms.
> +
> +  @param[in]  Smbios   SMBIOS protocol.
> +
> +  @retval EFI_SUCCESS           Record was added.
> +  @retval EFI_NOT_FOUND         Unknown product id.
> +  @retval EFI_OUT_OF_RESOURCES  Record was not added.
> +  @retval EFI_ALREADY_STARTED   The SmbiosHandle passed in is already in use.
> +**/
> +EFI_STATUS
> +InstallProcessorInformation (
> +  IN     EFI_SMBIOS_PROTOCOL    *Smbios
> +  )
> +{
> +  EFI_STATUS Status;
> +  EFI_SMBIOS_HANDLE SmbiosHandle;
> +  UINT32 CoreCount;
> +  UINT64 *ProcessorId = (UINT64 *)&(mArmRdSmbiosType4.Base.ProcessorId);
> +
> +  SmbiosHandle = ((EFI_SMBIOS_TABLE_HEADER *)&mArmRdSmbiosType4)->Handle;
> +  CoreCount = (FixedPcdGet32 (PcdCoreCount) * FixedPcdGet32 (PcdClusterCount));
> +
> +  /* Set the core count and processor speed for all platforms */
> +  switch (SgiGetProductId ()) {
> +  case Sgi575:
> +  case RdN1Edge:
> +  case RdV1:
> +    mArmRdSmbiosType4.Base.CoreCount = CoreCount;
> +    mArmRdSmbiosType4.Base.EnabledCoreCount = CoreCount;
> +    mArmRdSmbiosType4.Base.ThreadCount = CoreCount;
> +    break;
> +  case RdN2:
> +    mArmRdSmbiosType4.Base.CoreCount = CoreCount;
> +    mArmRdSmbiosType4.Base.EnabledCoreCount = CoreCount;
> +    mArmRdSmbiosType4.Base.ThreadCount = CoreCount;
> +    mArmRdSmbiosType4.Base.MaxSpeed = 3200;      // Frequency in MHz
> +    mArmRdSmbiosType4.Base.CurrentSpeed = 3200;  // Frequency in MHz
> +    break;
> +  case RdN1EdgeX2:
> +  case RdV1Mc:
> +    mArmRdSmbiosType4.Base.CoreCount = CoreCount * FixedPcdGet32 (PcdChipCount);
> +    mArmRdSmbiosType4.Base.EnabledCoreCount = CoreCount * FixedPcdGet32 (PcdChipCount);
> +    mArmRdSmbiosType4.Base.ThreadCount = CoreCount * FixedPcdGet32 (PcdChipCount);
> +    break;
> +  case RdE1Edge:
> +    mArmRdSmbiosType4.Base.CoreCount = CoreCount / NEOVERSE_E1_THREADS_PER_CORE;
> +    mArmRdSmbiosType4.Base.EnabledCoreCount = CoreCount / NEOVERSE_E1_THREADS_PER_CORE;
> +    mArmRdSmbiosType4.Base.ThreadCount = CoreCount;
> +    mArmRdSmbiosType4.Base.MaxSpeed = 2300;      // Frequency in MHz
> +    mArmRdSmbiosType4.Base.CurrentSpeed = 2300;  // Frequency in MHz
> +    break;
> +  }
> +
> +  mArmRdSmbiosType4.Base.ProcessorVersion = PROCESSOR_VERSION_BASE + SgiGetProductId ();
> +  mArmRdSmbiosType4.Base.SerialNumber = SERIAL_NUMBER_BASE + SgiGetProductId ();
[SAMI] Minor optimisation can be achieved by storing the value returned 
by SgiGetProductId ()in a local variable.
> +
> +  /* Update processor-id and part number */
> +  *ProcessorId = ArmReadMidr();
[SAMI] Space needed between ArmReadMidr and (.
> +  UpdatePartNumber (*ProcessorId);
> +
> +  /* Install type 4 table */
> +  Status = Smbios->Add (
> +                     Smbios,
> +                     NULL,
> +                     &SmbiosHandle,
> +                     (EFI_SMBIOS_TABLE_HEADER *)&mArmRdSmbiosType4
> +                     );
> +  if (Status != EFI_SUCCESS) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "SMBIOS: Failed to install Type4 SMBIOS table.\n"
> +      ));
> +  }
> +
> +  return Status;
> +}