Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 26/29] RiscVPkg/SmbiosDxe: Generic SMBIOS DXE driver for RISC-V platforms.

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Mon, Sep 23, 2019 at 08:31:52AM +0800, Abner Chang wrote:
> RISC-V generic SMBIOS DXE driver for building up SMBIOS type 4, type 7
> and type 44 records.
> 
> Signed-off-by: Abner Chang <abner.chang@hpe.com>
> ---
>  RiscVPkg/Include/ProcessorSpecificDataHob.h        |  95 ++++++
>  RiscVPkg/Include/SmbiosProcessorSpecificData.h     |  58 ++++
>  RiscVPkg/RiscVPkg.dec                              |   6 +
>  RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.c      | 339 +++++++++++++++++++++
>  RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.h      |  32 ++
>  RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.inf    |  58 ++++
>  RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.uni    |  12 +
>  .../Universal/SmbiosDxe/RiscVSmbiosDxeExtra.uni    |  13 +
>  8 files changed, 613 insertions(+)
>  create mode 100644 RiscVPkg/Include/ProcessorSpecificDataHob.h
>  create mode 100644 RiscVPkg/Include/SmbiosProcessorSpecificData.h
>  create mode 100644 RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.c
>  create mode 100644 RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.h
>  create mode 100644 RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.inf
>  create mode 100644 RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.uni
>  create mode 100644 RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxeExtra.uni
> 
> diff --git a/RiscVPkg/Include/ProcessorSpecificDataHob.h b/RiscVPkg/Include/ProcessorSpecificDataHob.h
> new file mode 100644
> index 0000000..6798a9d
> --- /dev/null
> +++ b/RiscVPkg/Include/ProcessorSpecificDataHob.h

None of the things defined in here are HOBs, they are structures to
hold information that will be put into HOBs.
Can we merge all of these definitions into
SmbiosProcessorSpecificData.h and delete this file?

> @@ -0,0 +1,95 @@
> +/** @file
> +  Definition of Processor Specific Data HOB.
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef _RISC_V_PROCESSOR_SPECIFIC_DATA_HOB_H_
> +#define _RISC_V_PROCESSOR_SPECIFIC_DATA_HOB_H_

Please drop leading _.

> +
> +#include <IndustryStandard/SmBios.h>

This file also uses Uefi.h, please include it, so we don't depend on
other files pulling it in for us.

> +
> +#define TO_BE_FILLED 0
> +#define TO_BE_FILLED_BY_VENDOR 0
> +#define TO_BE_FILLED_BY_RISC_V_SMBIOS_DXE_DRIVER 0
> +#define TO_BE_FILLED_BY_CODE 0

These defines are never used, please drop,

> +
> +#pragma pack(1)
> +
> +///
> +/// RISC-V processor specific data HOB
> +///
> +typedef struct {
> +  EFI_GUID ParentPrcessorGuid;
> +  UINTN    ParentProcessorUid;
> +  EFI_GUID CoreGuid;
> +  VOID     *Context;        // The additional information of this core which
> +                            // built in PEI phase and carried to DXE phase.
> +                            // The content is pocessor or platform specific.
> +  SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA ProcessorSpecificData;
> +} RISC_V_PROCESSOR_SPECIFIC_DATA_HOB;
> +
> +///
> +/// RISC-V SMBIOS type 4 (Processor) GUID data HOB
> +///
> +typedef struct {
> +  EFI_GUID PrcessorGuid;
> +  UINTN    ProcessorUid;
> +  SMBIOS_TABLE_TYPE4 SmbiosType4Processor;
> +  UINT16             EndingZero;

Please align indentation of struct members.

> +} RISC_V_PROCESSOR_TYPE4_DATA_HOB;
> +
> +#define RISC_V_CACHE_INFO_NOT_PROVIDED 0xFFFF
> +
> +#define RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_MASK 0x7
> +   #define RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_1 0x01
> +   #define RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_2 0x02
> +   #define RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_3 0x03
> +
> +#define RISC_V_CACHE_CONFIGURATION_SOCKET_BIT_POSITION 3
> +#define RISC_V_CACHE_CONFIGURATION_SOCKET_MASK (0x1 << RISC_V_CACHE_CONFIGURATION_SOCKET_BIT_POSITION)
> +  #define RISC_V_CACHE_CONFIGURATION_SOCKET_SOCKETED (0x1 << RISC_V_CACHE_CONFIGURATION_SOCKET_BIT_POSITION)
> +
> +#define RISC_V_CACHE_CONFIGURATION_LOCATION_BIT_POSITION 5
> +#define RISC_V_CACHE_CONFIGURATION_LOCATION_MASK (0x3 << RISC_V_CACHE_CONFIGURATION_LOCATION_BIT_POSITION)
> +  #define RISC_V_CACHE_CONFIGURATION_LOCATION_INTERNAL (0x0 << RISC_V_CACHE_CONFIGURATION_LOCATION_BIT_POSITION)
> +  #define RISC_V_CACHE_CONFIGURATION_LOCATION_EXTERNAL (0x1 << RISC_V_CACHE_CONFIGURATION_LOCATION_BIT_POSITION)
> +  #define RISC_V_CACHE_CONFIGURATION_LOCATION_RESERVED (0x2 << RISC_V_CACHE_CONFIGURATION_LOCATION_BIT_POSITION)
> +  #define RISC_V_CACHE_CONFIGURATION_LOCATION_UNKNOWN  (0x3 << RISC_V_CACHE_CONFIGURATION_LOCATION_BIT_POSITION)
> +
> +#define RISC_V_CACHE_CONFIGURATION_ENABLE_BIT_POSITION 7
> +#define RISC_V_CACHE_CONFIGURATION_ENABLE_MASK       (0x1 << RISC_V_CACHE_CONFIGURATION_ENABLE_BIT_POSITION)
> +  #define RISC_V_CACHE_CONFIGURATION_ENABLED           (0x1 << RISC_V_CACHE_CONFIGURATION_ENABLE_BIT_POSITION)
> +
> +#define RISC_V_CACHE_CONFIGURATION_MODE_BIT_POSITION 8
> +#define RISC_V_CACHE_CONFIGURATION_MODE_MASK       (0x3 << RISC_V_CACHE_CONFIGURATION_MODE_BIT_POSITION)
> +  #define RISC_V_CACHE_CONFIGURATION_MODE_WT       (0x0 << RISC_V_CACHE_CONFIGURATION_MODE_BIT_POSITION)
> +  #define RISC_V_CACHE_CONFIGURATION_MODE_WB       (0x1 << RISC_V_CACHE_CONFIGURATION_MODE_BIT_POSITION)
> +  #define RISC_V_CACHE_CONFIGURATION_MODE_VARIES   (0x2 << RISC_V_CACHE_CONFIGURATION_MODE_BIT_POSITION)
> +  #define RISC_V_CACHE_CONFIGURATION_MODE_UNKNOWN  (0x3 << RISC_V_CACHE_CONFIGURATION_MODE_BIT_POSITION)
> +///
> +/// RISC-V SMBIOS type 7 (Cache) GUID data HOB
> +///
> +typedef struct {
> +  EFI_GUID           PrcessorGuid;
> +  UINTN              ProcessorUid;
> +  SMBIOS_TABLE_TYPE7 SmbiosType7Cache;
> +  UINT16             EndingZero;
> +} RISC_V_PROCESSOR_TYPE7_DATA_HOB;
> +
> +///
> +/// RISC-V SMBIOS type 7 (Cache) GUID data HOB
> +///
> +typedef struct {
> +  RISC_V_PROCESSOR_TYPE4_DATA_HOB *Processor;
> +  RISC_V_PROCESSOR_TYPE7_DATA_HOB *L1InstCache;
> +  RISC_V_PROCESSOR_TYPE7_DATA_HOB *L1DataCache;
> +  RISC_V_PROCESSOR_TYPE7_DATA_HOB *L2Cache;
> +  RISC_V_PROCESSOR_TYPE7_DATA_HOB *L3Cache;
> +} RISC_V_PROCESSOR_SMBIOS_DATA_HOB;

I don't see this structure being used anywhere - can it be deleted?

> +
> +#pragma pack()
> +
> +#endif
> diff --git a/RiscVPkg/Include/SmbiosProcessorSpecificData.h b/RiscVPkg/Include/SmbiosProcessorSpecificData.h
> new file mode 100644
> index 0000000..36aa4ab
> --- /dev/null
> +++ b/RiscVPkg/Include/SmbiosProcessorSpecificData.h
> @@ -0,0 +1,58 @@
> +/** @file
> +  Industry Standard Definitions of RISC-V Processor Specific data defined in
> +  below link for complaiant with SMBIOS Table Specification v3.3.0.
> +  https://github.com/riscv/riscv-smbios
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef _SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA_H_
> +#define _SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA_H_

Drop leading _.

> +
> +#include <IndustryStandard/SmBios.h>
> +
> +#include <RiscV.h>
> +
> +#pragma pack(1)
> +
> +typedef enum{
> +  RegisterUnsupported = 0x00,
> +  RegisterLen32       = 0x01,
> +  RegisterLen64       = 0x02,
> +  RegisterLen128      = 0x03
> +} RISC_V_REGISTER_LENGTH;
> +
> +#define SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA_REVISION 0x100
> +
> +#define SMBIOS_RISC_V_PSD_MACHINE_MODE_SUPPORTED    (0x01 << 0)
> +#define SMBIOS_RISC_V_PSD_SUPERVISOR_MODE_SUPPORTED (0x01 << 2)
> +#define SMBIOS_RISC_V_PSD_USER_MODE_SUPPORTED       (0x01 << 3)
> +#define SMBIOS_RISC_V_PSD_DEBUG_MODE_SUPPORTED      (0x01 << 7)
> +
> +///
> +/// RISC-V processor specific data for SMBIOS type 44
> +///
> +typedef struct {
> +  UINT16            Revision;
> +  UINT8             Length;
> +  RISCV_UINT128     HartId;
> +  UINT8             BootHartId;
> +  RISCV_UINT128     MachineVendorId;
> +  RISCV_UINT128     MachineArchId;
> +  RISCV_UINT128     MachineImplId;
> +  UINT32            InstSetSupported;
> +  UINT8             PrivilegeModeSupported;
> +  RISCV_UINT128     MModeExcepDelegation;
> +  RISCV_UINT128     MModeInterruptDelegation;
> +  UINT8             HartXlen;
> +  UINT8             MachineModeXlen;
> +  UINT8             Reserved;
> +  UINT8             SupervisorModeXlen;
> +  UINT8             UserModeXlen;
> +} SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA;
> +
> +#pragma pack()
> +#endif
> +
> diff --git a/RiscVPkg/RiscVPkg.dec b/RiscVPkg/RiscVPkg.dec
> index a91392f..b316223 100644
> --- a/RiscVPkg/RiscVPkg.dec
> +++ b/RiscVPkg/RiscVPkg.dec
> @@ -24,6 +24,12 @@
>    gUefiRiscVPkgTokenSpaceGuid  = { 0x4261e9c8, 0x52c0, 0x4b34, { 0x85, 0x3d, 0x48, 0x46, 0xea, 0xd3, 0xb7, 0x2c}}
>  
>  [PcdsFixedAtBuild]
> +  # Processor Specific Data GUID HOB GUID
> +  gUefiRiscVPkgTokenSpaceGuid.PcdProcessorSpecificDataGuidHobGuid|{0x20, 0x72, 0xD5, 0x2F, 0xCF, 0x3C, 0x4C, 0xBC, 0xB1, 0x65, 0x94, 0x90, 0xDC, 0xF2, 0xFA, 0x93}|VOID*|0x00001000
> +  gUefiRiscVPkgTokenSpaceGuid.PcdProcessorSmbiosGuidHobGuid|{0x0F, 0x34, 0x00, 0x92, 0x04, 0x12, 0x45, 0x4A, 0x9C, 0x11, 0xB8, 0x8B, 0xDF, 0xC6, 0xFA, 0x6F}|VOID*|0x00001001
> +  gUefiRiscVPkgTokenSpaceGuid.PcdProcessorSmbiosType4GuidHobGuid|{0x5B, 0x36, 0xEA, 0x23, 0x79, 0x6D, 0x4F, 0xCF, 0x9C, 0x22, 0x25, 0xC0, 0x89, 0x8C, 0x25, 0xB9}|VOID*|0x00001002
> +  gUefiRiscVPkgTokenSpaceGuid.PcdProcessorSmbiosType7GuidHobGuid|{0xBF, 0xB4, 0x6D, 0x1B, 0x7E, 0x10, 0x47, 0x44, 0xB8, 0xBD, 0xFF, 0x1E, 0xDD, 0xDF, 0x71, 0x65}|VOID*|0x00001003
> +
>    #
>    #                                                   1000000000
>    # PcdRiscVMachineTimerTickInNanoSecond = ---------------------------------------
> diff --git a/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.c b/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.c
> new file mode 100644
> index 0000000..032f559
> --- /dev/null
> +++ b/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.c
> @@ -0,0 +1,339 @@
> +/** @file
> +  RISC-V generic SMBIOS DXE driver to build up SMBIOS type 4, type 7 and type 44 records.
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "RiscVSmbiosDxe.h"
> +
> +#define RISCV_SMBIOS_DEBUG_INFO 1

I would greatly prefer if we could drop this and
If there's still active development of the tables, I can go along with
keeping it while on the -staging branch, but it needs to go before the
port is merged to edk2 master.
You can use DEBUG_VERBOSE instead.

> +
> +EFI_SMBIOS_PROTOCOL   *Smbios;

mSmBios. And STATIC.

> +
> +/**
> +  This function builds SMBIOS type 7 record according to
> +  the given  RISC_V_PROCESSOR_TYPE7_DATA_HOB.
> +
> +  @param Type4DataHob       Pointer to RISC_V_PROCESSOR_TYPE4_DATA_HOB
> +  @param Type7DataHob       Pointer to RISC_V_PROCESSOR_TYPE7_DATA_HOB
> +  @param SmbiosHandle       Pointer to SMBIOS_HANDLE
> +
> +  @retval EFI_STATUS
> +
> +**/
> +static

STATIC

> +EFI_STATUS
> +BuildSmbiosType7 (
> + IN RISC_V_PROCESSOR_TYPE4_DATA_HOB *Type4DataHob,
> + IN RISC_V_PROCESSOR_TYPE7_DATA_HOB *Type7DataHob,
> + OUT SMBIOS_HANDLE *SmbiosHandle
> +)
> +{
> +  EFI_STATUS Status;
> +  SMBIOS_HANDLE Handle;
> +
> +  if (!CompareGuid (&Type4DataHob->PrcessorGuid, &Type7DataHob->PrcessorGuid) ||
> +    Type4DataHob->ProcessorUid != Type7DataHob->ProcessorUid) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  Handle = SMBIOS_HANDLE_PI_RESERVED;
> +  Type7DataHob->SmbiosType7Cache.Hdr.Type = SMBIOS_TYPE_CACHE_INFORMATION;
> +  Type7DataHob->SmbiosType7Cache.Hdr.Length = sizeof(SMBIOS_TABLE_TYPE7);
> +  Type7DataHob->SmbiosType7Cache.Hdr.Handle = 0;
> +  Type7DataHob->EndingZero = 0;
> +  Status = Smbios->Add (Smbios, NULL, &Handle, &Type7DataHob->SmbiosType7Cache.Hdr);
> +  if (EFI_ERROR(Status)) {
> +    DEBUG ((DEBUG_ERROR, "[RISC-V SMBIOS Builder]: Fail to add SMBIOS Type 7\n"));
> +    return Status;
> +  }
> +  DEBUG ((DEBUG_INFO, "[RISC-V SMBIOS Builder]: SMBIOS Type 7 was added. SMBIOS Handle: 0x%x\n", Handle));
> +#if RISCV_SMBIOS_DEBUG_INFO
> +  DEBUG ((DEBUG_INFO, "                         Cache belone to processor GUID: %g\n", &Type7DataHob->PrcessorGuid));
> +  DEBUG ((DEBUG_INFO, "                         Cache belone processor  UID: %d\n", Type7DataHob->ProcessorUid));
> +  DEBUG ((DEBUG_INFO, "                         ==============================\n"));
> +  DEBUG ((DEBUG_INFO, "                         Socket Designation: %d\n", Type7DataHob->SmbiosType7Cache.SocketDesignation));
> +  DEBUG ((DEBUG_INFO, "                         Cache Configuration: 0x%x\n", Type7DataHob->SmbiosType7Cache.CacheConfiguration));
> +  DEBUG ((DEBUG_INFO, "                         Maximum Cache Size: 0x%x\n", Type7DataHob->SmbiosType7Cache.MaximumCacheSize));
> +  DEBUG ((DEBUG_INFO, "                         Installed Size: 0x%x\n", Type7DataHob->SmbiosType7Cache.InstalledSize));
> +  DEBUG ((DEBUG_INFO, "                         Supported SRAM Type: 0x%x\n", Type7DataHob->SmbiosType7Cache.SupportedSRAMType));
> +  DEBUG ((DEBUG_INFO, "                         Current SRAMT ype: 0x%x\n", Type7DataHob->SmbiosType7Cache.CurrentSRAMType));
> +  DEBUG ((DEBUG_INFO, "                         Cache Speed: 0x%x\n", Type7DataHob->SmbiosType7Cache.CacheSpeed));
> +  DEBUG ((DEBUG_INFO, "                         Error Correction Type: 0x%x\n", Type7DataHob->SmbiosType7Cache.ErrorCorrectionType));
> +  DEBUG ((DEBUG_INFO, "                         System Cache Type: 0x%x\n", Type7DataHob->SmbiosType7Cache.SystemCacheType));
> +  DEBUG ((DEBUG_INFO, "                         Associativity: 0x%x\n", Type7DataHob->SmbiosType7Cache.Associativity));
> +#endif
> +
> +  *SmbiosHandle = Handle;
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  This function builds SMBIOS type 4 record according to
> +  the given  RISC_V_PROCESSOR_TYPE4_DATA_HOB.
> +
> +  @param Type4DataHob       Pointer to RISC_V_PROCESSOR_TYPE4_DATA_HOB
> +  @param SmbiosHandle       Pointer to SMBIOS_HANDLE
> +
> +  @retval EFI_STATUS
> +
> +**/
> +static
> +EFI_STATUS
> +BuildSmbiosType4 (
> +  IN RISC_V_PROCESSOR_TYPE4_DATA_HOB *Type4DataHob,
> +  OUT SMBIOS_HANDLE *SmbiosHandle
> +  )
> +{
> +  EFI_HOB_GUID_TYPE *GuidHob;

The code would be more readable if this was called something like Type7Hob.

> +  RISC_V_PROCESSOR_TYPE7_DATA_HOB *Type7HobData;
> +  SMBIOS_HANDLE Cache;
> +  SMBIOS_HANDLE Processor;
> +  EFI_STATUS Status;
> +
> +  DEBUG ((DEBUG_INFO, "[RISC-V SMBIOS Builder]: Building Type 4.\n"));

Again, please avoid the custom [] message tags - add %a __FUNCTION__
where helpul. Throughout.

> +  DEBUG ((DEBUG_INFO, "                         Processor GUID: %g\n", &Type4DataHob->PrcessorGuid));
> +  DEBUG ((DEBUG_INFO, "                         Processor UUID: %d\n", Type4DataHob->ProcessorUid));
> +
> +  Type4DataHob->SmbiosType4Processor.L1CacheHandle = RISC_V_CACHE_INFO_NOT_PROVIDED;
> +  Type4DataHob->SmbiosType4Processor.L2CacheHandle = RISC_V_CACHE_INFO_NOT_PROVIDED;
> +  Type4DataHob->SmbiosType4Processor.L3CacheHandle = RISC_V_CACHE_INFO_NOT_PROVIDED;
> +  GuidHob = (EFI_HOB_GUID_TYPE *)GetFirstGuidHob ((EFI_GUID *)PcdGetPtr(PcdProcessorSmbiosType7GuidHobGuid));
> +  if (GuidHob == NULL) {
> +    DEBUG ((DEBUG_ERROR, "[RISC-V SMBIOS Builder]: No RISC-V SMBIOS Type7 data HOB found.\n"));
> +    return EFI_NOT_FOUND;
> +  }
> +  //
> +  // Go through each RISC_V_PROCESSOR_TYPE4_DATA_HOB for multiple processors.
> +  //
> +  do {
> +    Type7HobData = (RISC_V_PROCESSOR_TYPE7_DATA_HOB *)GET_GUID_HOB_DATA (GuidHob);
> +    Status = BuildSmbiosType7 (Type4DataHob, Type7HobData, &Cache);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    if ((Type7HobData->SmbiosType7Cache.SystemCacheType & RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_MASK) ==
> +        RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_1) {
> +      Type4DataHob->SmbiosType4Processor.L1CacheHandle = Cache;
> +    } else if ((Type7HobData->SmbiosType7Cache.SystemCacheType & RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_MASK) ==
> +        RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_2) {
> +      Type4DataHob->SmbiosType4Processor.L2CacheHandle = Cache;
> +    } else if ((Type7HobData->SmbiosType7Cache.SystemCacheType & RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_MASK) ==
> +        RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_3) {
> +      Type4DataHob->SmbiosType4Processor.L3CacheHandle = Cache;
> +    } else {
> +      DEBUG ((DEBUG_ERROR, "[RISC-V SMBIOS Builder]: Improper cache level of SMBIOS handle %d\n", Cache));
> +    }
> +    GuidHob = GetNextGuidHob((EFI_GUID *)PcdGetPtr(PcdProcessorSmbiosType7GuidHobGuid), GET_NEXT_HOB(GuidHob));
> +  } while (GuidHob != NULL);
> +
> +  //
> +  // Build SMBIOS Type 4 record
> +  //
> +  Processor = SMBIOS_HANDLE_PI_RESERVED;
> +  Type4DataHob->SmbiosType4Processor.Hdr.Type = SMBIOS_TYPE_PROCESSOR_INFORMATION;
> +  Type4DataHob->SmbiosType4Processor.Hdr.Length = sizeof(SMBIOS_TABLE_TYPE4);
> +  Type4DataHob->SmbiosType4Processor.Hdr.Handle = 0;
> +  Type4DataHob->EndingZero = 0;
> +  Status = Smbios->Add (Smbios, NULL, &Processor, &Type4DataHob->SmbiosType4Processor.Hdr);
> +  if (EFI_ERROR(Status)) {
> +    DEBUG ((DEBUG_ERROR, "[RISC-V SMBIOS Builder]: Fail to add SMBIOS Type 4\n"));
> +    return Status;
> +  }
> +  DEBUG ((DEBUG_INFO, "[RISC-V SMBIOS Builder]: SMBIOS Type 4 was added. SMBIOS Handle: 0x%x\n", Processor));
> +#if RISCV_SMBIOS_DEBUG_INFO
> +  DEBUG ((DEBUG_INFO, "                         Socket StringID: %d\n", Type4DataHob->SmbiosType4Processor.Socket));
> +  DEBUG ((DEBUG_INFO, "                         Processor Type: 0x%x\n", Type4DataHob->SmbiosType4Processor.ProcessorType));
> +  DEBUG ((DEBUG_INFO, "                         Processor Family: 0x%x\n", Type4DataHob->SmbiosType4Processor.ProcessorFamily));
> +  DEBUG ((DEBUG_INFO, "                         Processor Manufacture StringID: %d\n", Type4DataHob->SmbiosType4Processor.ProcessorManufacture));
> +  DEBUG ((DEBUG_INFO, "                         Processor Id: 0x%x:0x%x\n", \
> +          Type4DataHob->SmbiosType4Processor.ProcessorId.Signature, Type4DataHob->SmbiosType4Processor.ProcessorId.FeatureFlags));
> +  DEBUG ((DEBUG_INFO, "                         Processor Version StringID: %d\n", Type4DataHob->SmbiosType4Processor.ProcessorVersion));
> +  DEBUG ((DEBUG_INFO, "                         Voltage: 0x%x\n", Type4DataHob->SmbiosType4Processor.Voltage));
> +  DEBUG ((DEBUG_INFO, "                         External Clock: 0x%x\n", Type4DataHob->SmbiosType4Processor.ExternalClock));
> +  DEBUG ((DEBUG_INFO, "                         Max Speed: 0x%x\n", Type4DataHob->SmbiosType4Processor.MaxSpeed));
> +  DEBUG ((DEBUG_INFO, "                         Current Speed: 0x%x\n", Type4DataHob->SmbiosType4Processor.CurrentSpeed));
> +  DEBUG ((DEBUG_INFO, "                         Status: 0x%x\n", Type4DataHob->SmbiosType4Processor.Status));
> +  DEBUG ((DEBUG_INFO, "                         ProcessorUpgrade: 0x%x\n", Type4DataHob->SmbiosType4Processor.ProcessorUpgrade));
> +  DEBUG ((DEBUG_INFO, "                         L1 Cache Handle: 0x%x\n", Type4DataHob->SmbiosType4Processor.L1CacheHandle));
> +  DEBUG ((DEBUG_INFO, "                         L2 Cache Handle: 0x%x\n",Type4DataHob->SmbiosType4Processor.L2CacheHandle));
> +  DEBUG ((DEBUG_INFO, "                         L3 Cache Handle: 0x%x\n", Type4DataHob->SmbiosType4Processor.L3CacheHandle));
> +  DEBUG ((DEBUG_INFO, "                         Serial Number StringID: %d\n", Type4DataHob->SmbiosType4Processor.SerialNumber));
> +  DEBUG ((DEBUG_INFO, "                         Asset Tag StringID: %d\n", Type4DataHob->SmbiosType4Processor.AssetTag));
> +  DEBUG ((DEBUG_INFO, "                         Part Number StringID: %d\n", Type4DataHob->SmbiosType4Processor.PartNumber));
> +  DEBUG ((DEBUG_INFO, "                         Core Count: %d\n", Type4DataHob->SmbiosType4Processor.CoreCount));
> +  DEBUG ((DEBUG_INFO, "                         Enabled CoreCount: %d\n", Type4DataHob->SmbiosType4Processor.EnabledCoreCount));
> +  DEBUG ((DEBUG_INFO, "                         Thread Count: %d\n", Type4DataHob->SmbiosType4Processor.ThreadCount));
> +  DEBUG ((DEBUG_INFO, "                         Processor Characteristics: 0x%x\n", Type4DataHob->SmbiosType4Processor.ProcessorCharacteristics));
> +  DEBUG ((DEBUG_INFO, "                         Processor Family2: 0x%x\n", Type4DataHob->SmbiosType4Processor.ProcessorFamily2));
> +  DEBUG ((DEBUG_INFO, "                         Core Count 2: %d\n", Type4DataHob->SmbiosType4Processor.CoreCount2));
> +  DEBUG ((DEBUG_INFO, "                         Enabled CoreCount : %d\n", Type4DataHob->SmbiosType4Processor.EnabledCoreCount2));
> +  DEBUG ((DEBUG_INFO, "                         Thread Count 2: %d\n", Type4DataHob->SmbiosType4Processor.ThreadCount2));
> +#endif
> +
> +  *SmbiosHandle = Processor;
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  This function builds SMBIOS type 44 record according..
> +
> +  @param Type4DataHob      Pointer to RISC_V_PROCESSOR_TYPE4_DATA_HOB
> +  @param Type4Handle       SMBIOS handle of type 4
> +
> +  @retval EFI_STATUS
> +
> +**/
> +EFI_STATUS
> +BuildSmbiosType44 (
> +  IN RISC_V_PROCESSOR_TYPE4_DATA_HOB *Type4DataHob,
> +  IN SMBIOS_HANDLE Type4Handle
> +  )
> +{
> +  EFI_HOB_GUID_TYPE *GuidHob;

ProcessorSpecificDataHob? Or Type44Hob?

> +  RISC_V_PROCESSOR_SPECIFIC_DATA_HOB *ProcessorSpecificData;
> +  SMBIOS_HANDLE RiscVType44;
> +  SMBIOS_TABLE_TYPE44 *Type44Ptr;
> +  EFI_STATUS Status;
> +
> +  DEBUG ((DEBUG_INFO, "[RISC-V SMBIOS Builder]: Building Type 44 for...\n"));
> +#if RISCV_SMBIOS_DEBUG_INFO
> +  DEBUG ((DEBUG_INFO, "                         Processor GUID: %g\n", &Type4DataHob->PrcessorGuid));
> +  DEBUG ((DEBUG_INFO, "                         Processor UUID: %d\n", Type4DataHob->ProcessorUid));
> +#endif
> +
> +  GuidHob = (EFI_HOB_GUID_TYPE *)GetFirstGuidHob ((EFI_GUID *)PcdGetPtr(PcdProcessorSpecificDataGuidHobGuid));
> +  if (GuidHob == NULL) {
> +    DEBUG ((DEBUG_ERROR, "[RISC-V SMBIOS Builder]: No RISC_V_PROCESSOR_SPECIFIC_DATA_HOB found.\n"));
> +    return EFI_NOT_FOUND;
> +  }
> +  //
> +  // Go through each RISC_V_PROCESSOR_SPECIFIC_DATA_HOB for multiple cores.
> +  //
> +  do {
> +    ProcessorSpecificData = (RISC_V_PROCESSOR_SPECIFIC_DATA_HOB *)GET_GUID_HOB_DATA (GuidHob);
> +    if (!CompareGuid (&ProcessorSpecificData->ParentPrcessorGuid, &Type4DataHob->PrcessorGuid) ||
> +      ProcessorSpecificData->ParentProcessorUid != Type4DataHob->ProcessorUid) {
> +      GuidHob = GetNextGuidHob((EFI_GUID *)PcdGetPtr(PcdProcessorSpecificDataGuidHobGuid), GET_NEXT_HOB(GuidHob));
> +      if (GuidHob == NULL) {
> +        break;
> +      }
> +      continue;
> +    }
> +
> +#if RISCV_SMBIOS_DEBUG_INFO
> +    DEBUG ((DEBUG_INFO, "[                        ================================\n"));
> +    DEBUG ((DEBUG_INFO, "[                        Core GUID: %g\n", &ProcessorSpecificData->CoreGuid));
> +#endif
> +
> +    Type44Ptr = AllocateZeroPool(sizeof(SMBIOS_TABLE_TYPE44) + sizeof(SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA) + 2); // Two ending zero.
> +    if (Type44Ptr == NULL) {
> +      return EFI_NOT_FOUND;
> +    }
> +    Type44Ptr->Hdr.Type = SMBIOS_TYPE_PROCESSOR_ADDITIONAL_INFORMATION;
> +    Type44Ptr->Hdr.Handle = 0;
> +    Type44Ptr->Hdr.Length = sizeof(SMBIOS_TABLE_TYPE44) + sizeof(SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA);
> +    Type44Ptr->RefHandle = Type4Handle;
> +    Type44Ptr->ProcessorSpecificBlock.Length = sizeof(SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA);
> +    Type44Ptr->ProcessorSpecificBlock.ProcessorArchType = Type4DataHob->SmbiosType4Processor.ProcessorFamily2 -
> +                                                          ProcessorFamilyRiscvRV32 + \
> +                                                          ProcessorSpecificBlockArchTypeRiscVRV32;
> +    CopyMem ((VOID *)(Type44Ptr + 1), (VOID *)&ProcessorSpecificData->ProcessorSpecificData, sizeof (SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA));
> +
> +#if RISCV_SMBIOS_DEBUG_INFO
> +    DEBUG ((DEBUG_INFO, "[                        Core type: %d\n", Type44Ptr->ProcessorSpecificBlock.ProcessorArchType));
> +    DEBUG ((DEBUG_INFO, "                           HartId = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->HartId.Value64_L));
> +    DEBUG ((DEBUG_INFO, "                           Is Boot Hart? = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->BootHartId));
> +    DEBUG ((DEBUG_INFO, "                           PrivilegeModeSupported = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->PrivilegeModeSupported));
> +    DEBUG ((DEBUG_INFO, "                           MModeExcepDelegation = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->MModeExcepDelegation.Value64_L));
> +    DEBUG ((DEBUG_INFO, "                           MModeInterruptDelegation = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->MModeInterruptDelegation.Value64_L));
> +    DEBUG ((DEBUG_INFO, "                           HartXlen = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->HartXlen));
> +    DEBUG ((DEBUG_INFO, "                           MachineModeXlen = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->MachineModeXlen));
> +    DEBUG ((DEBUG_INFO, "                           SupervisorModeXlen = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->SupervisorModeXlen));
> +    DEBUG ((DEBUG_INFO, "                           UserModeXlen = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->UserModeXlen));
> +    DEBUG ((DEBUG_INFO, "                           InstSetSupported = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->InstSetSupported));
> +    DEBUG ((DEBUG_INFO, "                           MachineVendorId = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->MachineVendorId.Value64_L));
> +    DEBUG ((DEBUG_INFO, "                           MachineArchId = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->MachineArchId.Value64_L));
> +    DEBUG ((DEBUG_INFO, "                           MachineImplId = 0x%x\n", ((SMBIOS_RISC_V_PROCESSOR_SPECIFIC_DATA *)(Type44Ptr + 1))->MachineImplId.Value64_L));
> +#endif
> +
> +    //
> +    // Add to SMBIOS table.
> +    //
> +    RiscVType44 = SMBIOS_HANDLE_PI_RESERVED;
> +    Status = Smbios->Add (Smbios, NULL, &RiscVType44, &Type44Ptr->Hdr);
> +    if (EFI_ERROR(Status)) {
> +      DEBUG ((DEBUG_ERROR, "[RISC-V SMBIOS Builder]: Fail to add SMBIOS Type 44\n"));
> +      return Status;
> +    }
> +    DEBUG ((DEBUG_INFO, "[RISC-V SMBIOS Builder]: SMBIOS Type 44 was added. SMBIOS Handle: 0x%x\n", RiscVType44));
> +
> +    GuidHob = GetNextGuidHob((EFI_GUID *)PcdGetPtr(PcdProcessorSpecificDataGuidHobGuid), GET_NEXT_HOB(GuidHob));
> +  } while (GuidHob != NULL);
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Entry point of RISC-V SMBIOS builder.
> +
> +  @param ImageHandle     Image handle this driver.
> +  @param SystemTable     Pointer to the System Table.
> +
> +  @retval EFI_SUCCESS           Thread can be successfully created
> +  @retval EFI_OUT_OF_RESOURCES  Cannot allocate protocol data structure
> +  @retval EFI_DEVICE_ERROR      Cannot create the thread
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +RiscVSmbiosBuilderEntry (
> +  IN EFI_HANDLE                            ImageHandle,
> +  IN EFI_SYSTEM_TABLE                      *SystemTable
> +  )
> +{
> +  EFI_STATUS Status;
> +  EFI_HOB_GUID_TYPE *GuidHob;

Type4Hob?

> +  RISC_V_PROCESSOR_TYPE4_DATA_HOB *Type4HobData;
> +  SMBIOS_HANDLE Processor;
> +
> +  DEBUG ((DEBUG_INFO, "[RISC-V SMBIOS Builder]: %a entry\n", __FUNCTION__));
> +
> +  Status = gBS->LocateProtocol (
> +                  &gEfiSmbiosProtocolGuid,
> +                  NULL,
> +                  (VOID **)&Smbios
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "[RISC-V SMBIOS Builder]: Locate SMBIOS Protocol fail\n"));
> +    return Status;
> +  }
> +  GuidHob = (EFI_HOB_GUID_TYPE *)GetFirstGuidHob ((EFI_GUID *)PcdGetPtr(PcdProcessorSmbiosType4GuidHobGuid));
> +  if (GuidHob == NULL) {
> +    DEBUG ((DEBUG_ERROR, "[RISC-V SMBIOS Builder]: No RISC-V SMBIOS information found.\n"));
> +    return EFI_NOT_FOUND;
> +  }
> +  Type4HobData = (RISC_V_PROCESSOR_TYPE4_DATA_HOB *)GET_GUID_HOB_DATA (GuidHob);
> +  Status = EFI_NOT_FOUND;
> +  //
> +  // Go through each RISC_V_PROCESSOR_TYPE4_DATA_HOB for multiple processors.
> +  //
> +  do {
> +    Status = BuildSmbiosType4 (Type4HobData, &Processor);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "[RISC-V SMBIOS Builder]: No RISC-V SMBIOS type 4 created.\n"));
> +      ASSERT (FALSE);
> +    }
> +    Status = BuildSmbiosType44 (Type4HobData, Processor);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "[RISC-V SMBIOS Builder]: No RISC-V SMBIOS type 44 found.\n"));
> +      ASSERT (FALSE);
> +    }
> +
> +    GuidHob = GetNextGuidHob((EFI_GUID *)PcdGetPtr(PcdProcessorSmbiosType4GuidHobGuid), GET_NEXT_HOB(GuidHob));
> +  } while (GuidHob != NULL);
> +  DEBUG ((DEBUG_INFO, "[RISC-V SMBIOS Builder]: %a exit\n", __FUNCTION__));
> +  return Status;
> +}
> +
> diff --git a/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.h b/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.h
> new file mode 100644
> index 0000000..dfa1fc6
> --- /dev/null
> +++ b/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.h
> @@ -0,0 +1,32 @@
> +/** @file
> +  RISC-V SMBIOS Builder DXE module header file.
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef _RISC_V_SMBIOS_DXE_H_
> +#define _RISC_V_SMBIOS_DXE_H_
> +
> +#include <PiDxe.h>
> +
> +#include <Protocol/Cpu.h>
> +#include <Protocol/Smbios.h>
> +
> +#include <Library/UefiDriverEntryPoint.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/DxeServicesTableLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/HobLib.h>
> +
> +#include <SmbiosProcessorSpecificData.h>
> +#include <ProcessorSpecificDataHob.h>

Please move all of these include statements to the files that actually
use them, then delete this file.

> +
> +#endif
> +
> diff --git a/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.inf b/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.inf
> new file mode 100644
> index 0000000..59b814a
> --- /dev/null
> +++ b/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.inf
> @@ -0,0 +1,58 @@
> +## @file
> +#  RISC-V SMBIOS DXE module.
> +#
> +#  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001b
> +  BASE_NAME                      = RiscVSmbiosDxe
> +  MODULE_UNI_FILE                = RiscVSmbiosDxe.uni
> +  FILE_GUID                      = 5FC01647-AADD-42E1-AD99-DF4CB89F5A92
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = RiscVSmbiosBuilderEntry
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec

Please sort alphabetically.

/
    Leif

> +  RiscVPkg/RiscVPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  BaseMemoryLib
> +  CpuLib
> +  DebugLib
> +  DxeServicesTableLib
> +  HobLib
> +  MemoryAllocationLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +  UefiLib
> +
> +[Sources]
> +  RiscVSmbiosDxe.c
> +  RiscVSmbiosDxe.h
> +
> +[Protocols]
> +  gEfiSmbiosProtocolGuid        # Consumed
> +
> +[Guids]
> +
> +
> +[Pcd]
> +
> +[FixedPcd]
> +  gUefiRiscVPkgTokenSpaceGuid.PcdProcessorSmbiosGuidHobGuid
> +  gUefiRiscVPkgTokenSpaceGuid.PcdProcessorSmbiosType4GuidHobGuid
> +  gUefiRiscVPkgTokenSpaceGuid.PcdProcessorSmbiosType7GuidHobGuid
> +  gUefiRiscVPkgTokenSpaceGuid.PcdProcessorSpecificDataGuidHobGuid
> +
> +[Depex]
> +  gEfiSmbiosProtocolGuid
> +
> +[UserExtensions.TianoCore."ExtraFiles"]
> +  RiscVSmbiosDxeExtra.uni
> diff --git a/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.uni b/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.uni
> new file mode 100644
> index 0000000..1bffe09
> --- /dev/null
> +++ b/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.uni
> @@ -0,0 +1,12 @@
> +// /** @file
> +//
> +// Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +//
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +// **/
> +
> +#string STR_MODULE_ABSTRACT             #language en-US "RISC-V Processor SMBIOS Builder"
> +
> +#string STR_MODULE_DESCRIPTION          #language en-US "Build RISC-V Processor SMBIOS Type 4, 7, 44 records."
> +
> diff --git a/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxeExtra.uni b/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxeExtra.uni
> new file mode 100644
> index 0000000..4b37ca2
> --- /dev/null
> +++ b/RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxeExtra.uni
> @@ -0,0 +1,13 @@
> +// /** @file
> +// RISC-V SMBIOS Builder Localized Strings and Content
> +//
> +// Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +//
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +// **/
> +
> +#string STR_PROPERTIES_MODULE_NAME
> +#language en-US
> +"RISC-V SMBIOS Record Builder DXE Driver"
> +
> -- 
> 2.7.4
> 
> 
> 
>