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

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

Hi Pranav,

Please find my comments inline marked [SAMI].

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 1 table (System Information) that includes
> information about manufacturer, product name, version, serial number and
> other information related to the system identification.
>
> Signed-off-by: Pranav Madhu <pranav.madhu@arm.com>
> ---
>   Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf    |   1 +
>   Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h      |   6 +
>   Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c      |   1 +
>   Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c | 132 ++++++++++++++++++++
>   4 files changed, 140 insertions(+)
>
> diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> index 3568380f8404..f7beb1c66c80 100644
> --- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> +++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> @@ -16,6 +16,7 @@
>   [Sources]
>     SmbiosPlatformDxe.c
>     Type0BiosInformation.c
> +  Type1SystemInformation.c
>
>   [Packages]
>     ArmPkg/ArmPkg.dec
> diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h
> index 091e0ec8314e..d7b3aadba948 100644
> --- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h
> +++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h
> @@ -17,4 +17,10 @@ InstallBiosInformation (
>     IN     EFI_SMBIOS_PROTOCOL    *Smbios
>     );
>
> +EFI_STATUS
> +EFIAPI
> +InstallSystemInformation (
> +  IN     EFI_SMBIOS_PROTOCOL    *Smbios
> +  );
> +
>   #endif // SMBIOS_PLATFORM_DXE_H_
> diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> index eb3ba45ca654..7b478063e223 100644
> --- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> +++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> @@ -27,6 +27,7 @@ typedef EFI_STATUS (*ARM_RD_SMBIOS_TABLE_INSTALL_FPTR)(EFI_SMBIOS_PROTOCOL *);
>   STATIC
>   ARM_RD_SMBIOS_TABLE_INSTALL_FPTR mSmbiosTableList[] = {
>     &InstallBiosInformation,
> +  &InstallSystemInformation,
>   };
>
>   /**
> diff --git a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c
> new file mode 100644
> index 000000000000..4559af81046b
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c
> @@ -0,0 +1,132 @@
> +/** @file
> +  SMBIOS Type 1 (System information) table for ARM RD platforms.
> +
> +  This file installs SMBIOS Type 1 (System information) table for Arm's
> +  Reference Design platforms. Type 1 table defines attributes of the
> +  overall system such as manufacturer, product name, UUID etc.
> +
> +  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.2
> +**/
> +
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <Protocol/Smbios.h>
> +
> +#include "SgiPlatform.h"
> +
> +#define PRODUCT_NAME_BASE  6    /* Product name base from TYPE1_STRINGS */
> +#define TYPE1_STRINGS                                   \
> +  "ARM LTD\0"                   /* Manufacturer */      \
> +  "Version not set\0"           /* Version */           \
> +  "Serial not set\0"            /* Serial number */     \
> +  "Not Applicable\0"            /* SKU */               \
> +  "Not Applicable\0"            /* Family */            \
> +  "SGI575\0"                    /* Product Names */     \
> +  "RdN1Edge\0"                                          \
> +  "RdN1EdgeX2\0"                                        \
> +  "RdE1Edge\0"                                          \
> +  "RdV1\0"                                              \
> +  "RdV1Mc\0"                                            \
> +  "RdN2\0"
> +
> +/* SMBIOS Type1 structure */
> +#pragma pack(1)
> +struct ArmRdSmbiosType1 {
> +  SMBIOS_TABLE_TYPE1  Base;
> +  UINT8               Strings[sizeof (TYPE1_STRINGS)];
[SAMI] CHAR8 ?
> +} ARM_TYPE1;
> +#pragma pack()
> +
> +STATIC GUID mSmbiosUid[] = {
> +  /* Sgi575        */
> +  {0xdd7cad0a, 0x227c, 0x4ed4, {0x9f, 0x42, 0xa9, 0x8b, 0xd6, 0xa2, 0x42, 0x6c}},
> +  /* Rd-N1-Edge    */
> +  {0x80984efe, 0x404a, 0x43e0, {0xad, 0xa4, 0x63, 0xa0, 0xe0, 0xc4, 0x5e, 0x60}},
> +  /* Rd-N1-Edge-X2 */
> +  {0x2cc4f916, 0x267a, 0x4251, {0x95, 0x6e, 0xf0, 0x49, 0x82, 0xbe, 0x94, 0x58}},
> +  /* Rd-E1-Edge    */
> +  {0x567f35c4, 0x104f, 0x447b, {0xa0, 0x94, 0x89, 0x2f, 0xbd, 0xb6, 0x5a, 0x55}},
> +  /* Rd-V1         */
> +  {0xc481f0b1, 0x237c, 0x42d7, {0x98, 0xb2, 0xb4, 0xb4, 0x8d, 0xb5, 0x4f, 0x50}},
> +  /* Rd-V1Mc       */
> +  {0x1f3a0806, 0x18b5, 0x4eca, {0xad, 0xcd, 0xba, 0x9b, 0x07, 0xb1, 0x0a, 0xcf}},
> +  /* Rd-N2         */
> +  {0xf2cded73, 0x37f9, 0x4ec9, {0xd9, 0xf9, 0x89, 0x9b, 0x74, 0x91, 0x20, 0x49}}
> +};
> +
> +/* System information */
> +static struct ArmRdSmbiosType1 mArmRdSmbiosType1 = {
[SAMI] STATIC in capital letters?
> +  {
> +    {
> +      // SMBIOS header
> +      EFI_SMBIOS_TYPE_SYSTEM_INFORMATION, // Type 1
> +      sizeof (SMBIOS_TABLE_TYPE1),        // Length
> +      SMBIOS_HANDLE_PI_RESERVED,          // Assign an unused handle number
> +    },
> +    1,                      // Manufacturer
> +    PRODUCT_NAME_BASE,      // Product Name, update dynamically
> +    2,                      // Version
> +    3,                      // Serial
> +    {0},                    // UUID, Update dymanically
> +    1,                      // Wakeup type other
> +    4,                      // SKU
> +    5,                      // Family
> +  },
> +  // Text strings (unformatted)
> +  TYPE1_STRINGS
> +};
> +
> +/**
> +  Install SMBIOS System information Table.
> +
> +  Install the SMBIOS system information (type 1) 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
> +InstallSystemInformation (
> +  IN     EFI_SMBIOS_PROTOCOL    *Smbios
> +  )
> +{
> +  EFI_STATUS Status;
> +  EFI_SMBIOS_HANDLE SmbiosHandle;
> +
> +  SmbiosHandle = ((EFI_SMBIOS_TABLE_HEADER *)&mArmRdSmbiosType1)->Handle;
> +
> +  /* Choose the product name from TYPE1_STRINGS based on the product ID */
> +  if (SgiGetProductId () != UnknownId) {
> +    mArmRdSmbiosType1.Base.ProductName =
> +      PRODUCT_NAME_BASE + (SgiGetProductId  () - 1);
> +    CopyGuid (&mArmRdSmbiosType1.Base.Uuid,
> +              &mSmbiosUid[SgiGetProductId  () - 1]);
[SAMI] Apart from the horizontal spacing issues, I think minor
optimisation can be achieved by calling SgiGetProductId () once and the
return value stored in a local variable for subsequent use.
[/SAMI]
> +  } else {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  /* Install type 1 table */
> +  Status = Smbios->Add (
> +                     Smbios,
> +                     NULL,
> +                     &SmbiosHandle,
> +                     (EFI_SMBIOS_TABLE_HEADER *)&mArmRdSmbiosType1
> +                     );
> +  if (Status != EFI_SUCCESS) {
[SAMI]  if (EFI_ERROR (Status)) ? Same comment for other patches in this
seires.
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "SMBIOS: Failed to install Type1 SMBIOS table.\n"
> +      ));
> +  }
> +
> +  return Status;
> +}

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.