[edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator

Chang, Abner via groups.io abner.chang=amd.com at groups.io
Tue Sep 13 03:00:10 UTC 2022


[AMD Official Use Only - General]

One question in below with tag [Abner],

> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Sami
> Mujawar via groups.io
> Sent: Monday, September 12, 2022 10:57 PM
> To: Girish Mahadevan <gmahadevan at nvidia.com>; devel at edk2.groups.io;
> Alexei Fedorov <Alexei.Fedorov at arm.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at arm.com>; Jeff
> Brasen <jbrasen at nvidia.com>; Ashish Singhal <ashishsingha at nvidia.com>;
> Akanksha Jain <Akanksha.Jain2 at arm.com>; Matteo Carlini
> <Matteo.Carlini at arm.com>; Hemendra Dassanayake
> <Hemendra.Dassanayake at arm.com>; Nick Ramirez <nramirez at nvidia.com>;
> William Watson <wwatson at nvidia.com>; Akanksha Jain
> <Akanksha.Jain2 at arm.com>; nd at arm.com
> Subject: Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios
> Type17 Table generator
> 
> [CAUTION: External Email]
> 
> Hi Girish,
> 
> Thank you for this patch and for the effort for bringing forward dynamic
> SMBIOS generation.
> 
> Please find my feedback inline marked [SAMI].
> 
> Regards,
> 
> Sami Mujawar
> 
> On 26/08/2022 06:37 pm, Girish Mahadevan wrote:
> > Add a new CM object to describe memory devices and setup a new
> > Generator Library for SMBIOS Type17 table.
> >
> > Signed-off-by: Girish Mahadevan<gmahadevan at nvidia.com>
> > ---
> >   .../Include/ArmNameSpaceObjects.h             |  59 +++
> >   .../SmbiosType17Lib/SmbiosType17Generator.c   | 338
> ++++++++++++++++++
> >   .../SmbiosType17Lib/SmbiosType17LibArm.inf    |  32 ++
> >   3 files changed, 429 insertions(+)
> >   create mode 100644
> DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Ge
> nerator.c
> >   create mode 100644
> >
> DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Lib
> Arm
> > .inf
> >
> > diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> > b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> > index 102e0f96be..199a19e997 100644
> > --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> > +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> > @@ -63,6 +63,7 @@ typedef enum ArmObjectID {
> >     EArmObjPciInterruptMapInfo,          ///< 39 - Pci Interrupt Map Info
> >     EArmObjRmr,                          ///< 40 - Reserved Memory Range Node
> >     EArmObjMemoryRangeDescriptor,        ///< 41 - Memory Range
> Descriptor
> > +  EArmObjMemoryDeviceInfo,             ///< 42 - Memory Device Information
> >     EArmObjMax
> >   } EARM_OBJECT_ID;
> >
> > @@ -1070,6 +1071,64 @@ typedef struct CmArmRmrDescriptor {
> >     UINT64    Length;
> >   } CM_ARM_MEMORY_RANGE_DESCRIPTOR;
> >
> > +/** A structure that describes the physical memory device.
> > +
> > +  The physical memory devices on the system are described by this object.
> > +
> > +  SMBIOS Specification v3.5.0 Type17
> > +
> > +  ID: EArmObjMemoryDeviceInfo,
> > +*/
> > +typedef struct CmArmMemoryDeviceInfo {
> [SAMI] I think we may need a Token pointing to the Type 16 object so that
> the Physical Memory Array Handle can be setup, see my comment below
> about the HandleManager.
> > +  /** Size of the device.
> > +    Size of the device in bytes.
> > +  */
> > +  UINT64  Size;
> > +
> > +  /** Device Set */
> > +  UINT8   DeviceSet;
> > +
> > +  /** Speed of the device
> > +    Speed of the device in MegaTransfers/second.
> > +  */
> > +  UINT32  Speed;
> > +
> > +  /** Serial Number of device  */
> > +  CHAR8   *SerialNum;
> > +
> > +  /** AssetTag identifying the device */
> > +  CHAR8   *AssetTag;
> > +
> > +  /** Device Locator String for the device.
> > +   String that describes the slot or position of the device on the board.
> > +   */
> > +  CHAR8   *DeviceLocator;
> > +
> > +  /** Bank locator string for the device.
> > +   String that describes the bank where the device is located.
> > +   */
> > +  CHAR8   *BankLocator;
> > +
> > +  /** Firmware version of the memory device */
> > +  CHAR8   *FirmwareVersion;
> > +
> > +  /** Manufacturer Id.
> > +   2 byte Manufacturer Id as per JEDEC Standard JEP106AV  */
> > +  UINT16  ModuleManufacturerId;
> > +
> > +  /** Manufacturer Product Id
> > +   2 byte Manufacturer Id as designated by Manufacturer.
> > +  */
> > +  UINT16  ModuleProductId;
> > +
> > +  /** Device Attributes */
> > +  UINT8   Attributes;
> > +
> > +  /** Device Configured Voltage in millivolts */
> > +  UINT16  ConfiguredVoltage;
> [SAMI] This field does not appear to be used in the generator. If the
> intention is to use this in the future, then it may be better to add this at a
> later stage.
> > +} CM_ARM_MEMORY_DEVICE_INFO;
> > +
> >   #pragma pack()
> >
> >   #endif // ARM_NAMESPACE_OBJECTS_H_
> > diff --git
> >
> a/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17G
> ene
> > rator.c
> >
> b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17
> Gene
> > rator.c
> > new file mode 100644
> > index 0000000000..5683ca570f
> > --- /dev/null
> > +++
> b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17
> > +++ Generator.c
> > @@ -0,0 +1,338 @@
> > +/** @file
> > +  SMBIOS Type17 Table Generator.
> > +
> > +  Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights
> reserved.
> > +  Copyright (c) 2020 - 2021, Arm Limited. All rights reserved.<BR>
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > +
> > +#include <Library/BaseLib.h>
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/PrintLib.h>
> > +#include <Library/MemoryAllocationLib.h> #include
> > +<Library/SmbiosType17FixupLib.h>
> [SAMI] I could not find SmbiosType17FixupLib.h in this patch series. Can you
> check, please?
> > +
> > +// Module specific include files.
> > +#include <ConfigurationManagerObject.h> #include
> > +<ConfigurationManagerHelper.h> #include
> > +<Protocol/ConfigurationManagerProtocol.h>
> > +#include <Protocol/Smbios.h>
> [SAMI] I think Protocol/Smbios.h may not be required in this file. Can you
> check, please?
> > +#include <IndustryStandard/SmBios.h>
> > +
> > +/** This macro expands to a function that retrieves the Memory Device
> > +    information from the Configuration Manager.
> > +*/
> > +GET_OBJECT_LIST (
> > +  EObjNameSpaceArm,
> > +  EArmObjMemoryDeviceInfo,
> > +  CM_ARM_MEMORY_DEVICE_INFO
> > +  )
> > +
> > +// Default Values for Memory Device
> > +STATIC SMBIOS_TABLE_TYPE17 MemDeviceInfoTemplate = {
> > +  {                                      // Hdr
> > +    EFI_SMBIOS_TYPE_MEMORY_DEVICE,       // Type
> > +    sizeof (SMBIOS_TABLE_TYPE17),        // Length
> > +    0                                    // Handle
> > +  },
> > +  0,                                     // MemoryArrayHandle
> 
> [SAMI] Do you have any thoughts on how the MemoryArrayHandle can be
> setup?
> 
> The same applies for the following MemoryErrorInformationHandle field.
> 
> I think we need some sort of a HandleManager in DynamicTablesFramework
> that can keep track of the mappings between SMBIOS Objects and Table
> Handles.
> 
> e.g. Smbios - HandleManager
> 
> +-------------------------------+-------------------------------+
> 
>        |    Object Token               | Table Handle                  |
> 
> +-------------------------------+-------------------------------+
> 
>        | Type16Obj_token         | Type 16 Table handle    |
> 
> +-------------------------------+-------------------------------+
> 
>        ...
> 
> - The Type17Object i.e. CM_ARM_MEMORY_DEVICE_INFO can then hold a
> token for the Type16Object.
> 
>   - If Type 17 table is to be installed, DynamicTablemanager shall search the
> SMBIOS table list to see if a Type16 table is requested to be installed.
> 
> - If a Type16 table is present in the list of SMBIOS table to install, the Type16
> table shall be installed first and an entry is made in the Smbios
> HandleManager to create a mapping of Type16Obj_token  <==> Type16
> Table Handle.
> 
> - The Type17 table can now be built and if a the Type16Object token is
> provided in CM_ARM_MEMORY_DEVICE_INFO, the Smbios HandleManager
> shall be searched (using Type16Obj_token) to retrieve the Type16 Table
> handle and populate the Type 17 Physical Memory Array Handle field.
> 
> I think we may have to experiment a bit before we arrive at the correct
> design. However, please do let me know your thoughts on the above.
> 
> [SAMI]
> > +  0,                                     // MemoryErrorInformationHandle
> > +  0xFFFF,                                // TotalWidth
> > +  0xFFFF,                                // DataWIdth
> 
> [SAMI] I need to find out how these fields should be populated, but the
> Annex A, SMBIOS specification version 3.6.0 says the following:
> 
> 4.8.4 Total Width is not 0FFFFh (Unknown) if the memory device is installed.
> (Size is not 0.)
> 
> 4.8.5 Data Width is not 0FFFFh (Unknown).
> 
> Can you explain how this field is used, please?
> 
> [/SAMI]
> 
> > +  0x7FFF,                                // Size (always use Extended Size)
> 
> [SAMI] I think this field should be set based on the Size.
> 
> The spec says "If the size is 32 GB-1 MB or greater, the field value is 7FFFh
> and the actual size is stored in the Extended Size field."
> 
> I think it would be good to have a static function  that encodes the size in
> wither the Size field or the Extended Size field.
> 
> e.g. UpdateSmbiosTable17Size (SMBIOS_TABLE_TYPE17 *SmbiosRecord,
> UINTN
> Size) {
> 
>           if (Size > 32GB-1MB) {
> 
>              SmbiosRecord->Size = EncodeSize (xxx);
> 
>            } else {
> 
>               SmbiosRecord->ExtendedSize = EncodeExtendedSize (xxx);
> 
>           }
> 
>      }
> 
> [/SAMI]
> 
> > +  MemoryTypeUnknown,                     // FormFactor
> > +  0xFF,                                  // DeviceSet
> > +  1,                                     // Device Locator
> > +  2,                                     // Bank Locator
> > +  MemoryTypeSdram,                       // MemoryType
> > +  {                                      // TypeDetail
> > +    0
> > +  },
> > +  0xFFFF,                                // Speed (Use Extended Speed)
> [SAMI] Maybe we need a helper function (similar to
> UpdateSmbiosTable17Size()) for this field as well.
> > +  0,                                     // Manufacturer
> > +                                         // (Unused Use ModuleManufactuerId)
> > +  3,                                     // SerialNumber
> > +  4,                                     // AssetTag
> > +  0,                                     // PartNumber
> > +                                         // (Unused Use ModuleProductId)
> > +  0,                                     // Attributes
> > +  0,                                     // ExtendedSize
> > +  2000,                                  // ConfiguredMemoryClockSpeed
> [SAMI] Should this be provided through CM_ARM_MEMORY_DEVICE_INFO?
> > +  0,                                     // MinimumVoltage
> > +  0,                                     // MaximumVoltage
> > +  0,                                     // ConfiguredVoltage
> > +  MemoryTechnologyDram,                  // MemoryTechnology
> [SAMI] Should this be provided through CM_ARM_MEMORY_DEVICE_INFO?
> > +  {                                      // MemoryOperatingModeCapability
> > +    .Uint16 = 0x000
> > +  },
> [SAMI] I think the above initialisation may not be portable.
> > +  5,                                    // FirmwareVersion
> > +  0,                                    // ModuleManufacturerId
> > +  0,                                    // ModuleProductId
> > +  0,                                    // MemorySubsystemContollerManufacturerId
> > +  0,                                    // MemorySybsystemControllerProductId
> > +  0,                                    // NonVolatileSize
> > +  0,                                    // VolatileSize
> > +  0,                                    // CacheSize
> > +  0,                                    // LogicalSize
> > +  0,                                    // ExtendedSpeed
> > +  0,                                    // ExtendedConfiguredMemorySpeed
> > +};
> > +
> > +STATIC CHAR8  UnknownStr[] = "Unknown\0";
> [SAMI] Would it be possible to add the CONST qualifier, please?
> > +
> > +STATIC
> > +EFI_STATUS
> > +EFIAPI
> > +FreeSmbiosType17TableEx (
> > +  IN      CONST SMBIOS_TABLE_GENERATOR                   *CONST    This,
> > +  IN      CONST CM_STD_OBJ_SMBIOS_TABLE_INFO             *CONST
> SmbiosTableInfo,
> > +  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL     *CONST
> CfgMgrProtocol,
> > +  IN OUT        SMBIOS_STRUCTURE                         ***CONST  Table,
> > +  IN      CONST UINTN                                              TableCount
> > +  )
> > +{
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/** Construct SMBIOS Type17 Table describing memory devices.
> > +
> > +  If this function allocates any resources then they must be freed
> > + in the FreeXXXXTableResources function.
> > +
> > +  @param [in]  This            Pointer to the SMBIOS table generator.
> > +  @param [in]  SmbiosTableInfo Pointer to the SMBIOS table information.
> > +  @param [in]  CfgMgrProtocol  Pointer to the Configuration Manager
> > +                               Protocol interface.
> > +  @param [out] Table           Pointer to the SMBIOS table.
> > +
> > +  @retval EFI_SUCCESS            Table generated successfully.
> > +  @retval EFI_BAD_BUFFER_SIZE    The size returned by the Configuration
> > +                                 Manager is less than the Object size for
> > +                                 the requested object.
> > +  @retval EFI_INVALID_PARAMETER  A parameter is invalid.
> > +  @retval EFI_NOT_FOUND          Could not find information.
> > +  @retval EFI_OUT_OF_RESOURCES   Could not allocate memory.
> > +  @retval EFI_UNSUPPORTED        Unsupported configuration.
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +EFIAPI
> > +BuildSmbiosType17TableEx (
> > +  IN  CONST SMBIOS_TABLE_GENERATOR                         *This,
> > +  IN        CM_STD_OBJ_SMBIOS_TABLE_INFO           *CONST
> SmbiosTableInfo,
> > +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL   *CONST
> CfgMgrProtocol,
> > +  OUT       SMBIOS_STRUCTURE                               ***Table,
> > +  OUT       UINTN                                  *CONST  TableCount
> > +  )
> > +{
> > +  EFI_STATUS                 Status;
> > +  UINT32                     NumMemDevices;
> > +  SMBIOS_STRUCTURE           **TableList;
> > +  CM_ARM_MEMORY_DEVICE_INFO  *MemoryDevicesInfo;
> > +  UINTN                      Index;
> > +  UINTN                      SerialNumLen;
> > +  CHAR8                      *SerialNum;
> > +  UINTN                      AssetTagLen;
> > +  CHAR8                      *AssetTag;
> > +  UINTN                      DeviceLocatorLen;
> > +  CHAR8                      *DeviceLocator;
> > +  UINTN                      BankLocatorLen;
> > +  CHAR8                      *BankLocator;
> > +  UINTN                      FirmwareVersionLen;
> > +  CHAR8                      *FirmwareVersion;
> > +  CHAR8                      *OptionalStrings;
> > +  SMBIOS_TABLE_TYPE17        *SmbiosRecord;
> > +
> > +  ASSERT (This != NULL);
> > +  ASSERT (SmbiosTableInfo != NULL);
> > +  ASSERT (CfgMgrProtocol != NULL);
> > +  ASSERT (Table != NULL);
> > +  ASSERT (TableCount != NULL);
> > +  ASSERT (SmbiosTableInfo->TableGeneratorId == This->GeneratorID);
> > +
> > +  DEBUG ((DEBUG_ERROR, "%a : Start \n", __FUNCTION__));  *Table =
> > + NULL;  Status = GetEArmObjMemoryDeviceInfo (
> > +             CfgMgrProtocol,
> > +             CM_NULL_TOKEN,
> > +             &MemoryDevicesInfo,
> > +             &NumMemDevices
> > +             );
[Abner] 
SMBIOS type 17 record is generic to all platform architectures, however here we have the dependency with ARM namespace object. So my question is what should we do if non-ARM platforms would like to leverage this library? 
Thanks
Abner
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((
> > +      DEBUG_ERROR,
> > +      "Failed to get Memory Devices CM Object %r\n",
> > +      Status
> > +      ));
> > +    return Status;
> > +  }
> > +
> > +  TableList = (SMBIOS_STRUCTURE **)AllocateZeroPool (sizeof
> > + (SMBIOS_STRUCTURE *) * NumMemDevices);
> [SAMI] The memory allocated here should be freed in
> FreeSmbiosType17TableEx.
> > +  if (TableList == NULL) {
> > +    DEBUG ((DEBUG_ERROR, "Failed to alloc memory for %u devices
> table\n"));
> > +    Status = EFI_OUT_OF_RESOURCES;
> > +    goto exit;
> > +  }
> > +
> > +  for (Index = 0; Index < NumMemDevices; Index++) {
> > +    if (MemoryDevicesInfo[Index].SerialNum != NULL) {
> > +      SerialNumLen = AsciiStrLen (MemoryDevicesInfo[Index].SerialNum);
> > +      SerialNum    = MemoryDevicesInfo[Index].SerialNum;
> > +    } else {
> > +      SerialNumLen = AsciiStrLen (UnknownStr);
> > +      SerialNum    = UnknownStr;
> 
> [SAMI] If the serial number is not provided, then should the string field be
> set to 0?
> 
> See Section 6.1.3, SMBIOS Spec Version 3.6.0 which states
> 
> "If a string field references no string, a null (0) is placed in that string field."
> 
> [/SAMI]
> 
> > +    }
> > +
> > +    if (MemoryDevicesInfo[Index].AssetTag != NULL) {
> > +      AssetTagLen = AsciiStrLen (MemoryDevicesInfo[Index].AssetTag);
> > +      AssetTag    = MemoryDevicesInfo[Index].AssetTag;
> > +    } else {
> > +      AssetTagLen = AsciiStrLen (UnknownStr);
> > +      AssetTag    = UnknownStr;
> > +    }
> > +
> > +    if (MemoryDevicesInfo[Index].DeviceLocator != NULL) {
> > +      DeviceLocatorLen = AsciiStrLen
> (MemoryDevicesInfo[Index].DeviceLocator);
> > +      DeviceLocator    = MemoryDevicesInfo[Index].DeviceLocator;
> > +    } else {
> > +      DeviceLocatorLen = AsciiStrLen (UnknownStr);
> > +      DeviceLocator    = UnknownStr;
> > +    }
> > +
> > +    if (MemoryDevicesInfo[Index].BankLocator != NULL) {
> > +      BankLocatorLen = AsciiStrLen
> (MemoryDevicesInfo[Index].BankLocator);
> > +      BankLocator    = MemoryDevicesInfo[Index].BankLocator;
> > +    } else {
> > +      BankLocatorLen = AsciiStrLen (UnknownStr);
> > +      BankLocator    = UnknownStr;
> > +    }
> > +
> > +    if (MemoryDevicesInfo[Index].FirmwareVersion != NULL) {
> > +      FirmwareVersionLen = AsciiStrLen
> (MemoryDevicesInfo[Index].FirmwareVersion);
> > +      FirmwareVersion    = MemoryDevicesInfo[Index].FirmwareVersion;
> > +    } else {
> > +      FirmwareVersionLen = AsciiStrLen (UnknownStr);
> > +      FirmwareVersion    = UnknownStr;
> > +    }
> > +
> > +    SmbiosRecord = (SMBIOS_TABLE_TYPE17 *)AllocateZeroPool (
> > +                                            sizeof (SMBIOS_TABLE_TYPE17) +
> > +                                            SerialNumLen + 1 +
> > +                                            AssetTagLen + 1 + DeviceLocatorLen + 1 +
> > +                                            BankLocatorLen + 1 + FirmwareVersionLen + 1 + 1
> > +                                            );
> [SAMI] The memory allocated here needs to be freed in
> FreeSmbiosType17TableEx as  SmbiosProtocol->Add () makes a copy of the
> SmbiosTable, see
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FMdeModulePkg%2FU
> niversal%2FSmbiosDxe%2FSmbiosDxe.c%23L476&data=05%7C01%7Cab
> ner.chang%40amd.com%7C0f7ab9327b14444577e608da94cf2b73%7C3dd8961
> fe4884e608e11a82d994e183d%7C0%7C0%7C637985914789485474%7CUnkno
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=NSB9I00z4gS0N5
> 97Bs1IMWIJoPPgzdnHsVrgpcPOuHw%3D&reserved=0
> and
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FMdeModulePkg%2FU
> niversal%2FSmbiosDxe%2FSmbiosDxe.c%23L516&data=05%7C01%7Cab
> ner.chang%40amd.com%7C0f7ab9327b14444577e608da94cf2b73%7C3dd8961
> fe4884e608e11a82d994e183d%7C0%7C0%7C637985914789485474%7CUnkno
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=HgtuAt2lf%2F9L1
> jNAPpShJCrDKSb7V0t6kE8UuTUHS7c%3D&reserved=0.
> 
> > +    if (SmbiosRecord == NULL) {
> > +      Status = EFI_OUT_OF_RESOURCES;
> > +      goto exit;
> > +    }
> > +
> > +    CopyMem (SmbiosRecord, &MemDeviceInfoTemplate, sizeof
> (SMBIOS_TABLE_TYPE17));
> > +    SmbiosRecord->ExtendedSize         = MemoryDevicesInfo[Index].Size;
> 
> [SAMI] CM_ARM_MEMORY_DEVICE_INFO.Size is documented as size in
> bytes, while looking at the SMBIOS specification, section 7.18.5 for the
> Extended Size Bits 30:0 represent the size of the memory device in
> megabytes.
> 
> I think it will be useful to create UpdateSmbiosTable17Size() which does the
> appropriate encoding and updation of the SMBIOS table fieds.
> 
> [/SAMI]
> 
> > +    SmbiosRecord->DeviceSet            =
> MemoryDevicesInfo[Index].DeviceSet;
> > +    SmbiosRecord->ModuleManufacturerID =
> > +      MemoryDevicesInfo[Index].ModuleManufacturerId;
> > +    SmbiosRecord->ModuleProductID =
> > +      MemoryDevicesInfo[Index].ModuleProductId;
> > +    SmbiosRecord->Attributes =
> > +      MemoryDevicesInfo[Index].Attributes;
> > +    SmbiosRecord->ExtendedSpeed = MemoryDevicesInfo[Index].Speed;
> > +    OptionalStrings             = (CHAR8 *)(SmbiosRecord + 1);
> > +    AsciiSPrint (OptionalStrings, DeviceLocatorLen + 1,
> > + DeviceLocator);
> [SAMI] I think we can simplify the publishing of the SMBIOS strings using
> SmbiosStringTableLib. Please see the patch at
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
> 2.groups.io%2Fg%2Fdevel%2Fmessage%2F93651&data=05%7C01%7Cab
> ner.chang%40amd.com%7C0f7ab9327b14444577e608da94cf2b73%7C3dd8961
> fe4884e608e11a82d994e183d%7C0%7C0%7C637985914789485474%7CUnkno
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=B8WFDH%2FYQy
> vWiGZaXSM5GFKMKcLSeZYIsCYSJaGBiOM%3D&reserved=0
> > +    OptionalStrings = OptionalStrings + DeviceLocatorLen + 1;
> > +    AsciiSPrint (OptionalStrings, BankLocatorLen + 1, BankLocator);
> > +    OptionalStrings = OptionalStrings + BankLocatorLen + 1;
> > +    AsciiSPrint (OptionalStrings, SerialNumLen + 1, SerialNum);
> > +    OptionalStrings = OptionalStrings + SerialNumLen + 1;
> > +    AsciiSPrint (OptionalStrings, AssetTagLen + 1, AssetTag);
> > +    OptionalStrings = OptionalStrings + AssetTagLen + 1;
> > +    AsciiSPrint (OptionalStrings, FirmwareVersionLen + 1, FirmwareVersion);
> > +    OptionalStrings  = OptionalStrings + FirmwareVersionLen + 1;
> > +    TableList[Index] = (SMBIOS_STRUCTURE *)SmbiosRecord;  }
> > +
> > +  *Table      = TableList;
> > +  *TableCount = NumMemDevices;
> > +
> > +exit:
> > +  DEBUG ((DEBUG_ERROR, "%a : Done \n", __FUNCTION__));
> > +  return Status;
> > +}
> > +
> > +/** The interface for the SMBIOS Type17 Table Generator.
> > +*/
> > +STATIC
> > +CONST
> > +SMBIOS_TABLE_GENERATOR  SmbiosType17Generator = {
> > +  // Generator ID
> > +  CREATE_STD_SMBIOS_TABLE_GEN_ID (EStdSmbiosTableIdType17),
> > +  // Generator Description
> > +  L"SMBIOS.TYPE17.GENERATOR",
> > +  // SMBIOS Table Type
> > +  EFI_SMBIOS_TYPE_MEMORY_DEVICE,
> > +  NULL,
> > +  NULL,
> > +  // Build table function Extended.
> > +  BuildSmbiosType17TableEx,
> > +  // Free function Extended.
> > +  FreeSmbiosType17TableEx
> > +};
> > +
> > +/** Register the Generator with the SMBIOS Table Factory.
> > +
> > +  @param [in]  ImageHandle  The handle to the image.
> > +  @param [in]  SystemTable  Pointer to the System Table.
> > +
> > +  @retval EFI_SUCCESS           The Generator is registered.
> > +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> > +  @retval EFI_ALREADY_STARTED   The Generator for the Table ID
> > +                                is already registered.
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +SmbiosType17LibConstructor (
> > +  IN  EFI_HANDLE        ImageHandle,
> > +  IN  EFI_SYSTEM_TABLE  *SystemTable
> > +  )
> > +{
> > +  EFI_STATUS  Status;
> > +
> > +  Status = RegisterSmbiosTableGenerator (&SmbiosType17Generator);
> > + DEBUG ((
> > +    DEBUG_INFO,
> > +    "SMBIOS Type 17: Register Generator. Status = %r\n",
> > +    Status
> > +    ));
> > +  ASSERT_EFI_ERROR (Status);
> > +
> > +  return Status;
> > +}
> > +
> > +/** Deregister the Generator from the SMBIOS Table Factory.
> > +
> > +  @param [in]  ImageHandle  The handle to the image.
> > +  @param [in]  SystemTable  Pointer to the System Table.
> > +
> > +  @retval EFI_SUCCESS           The Generator is deregistered.
> > +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> > +  @retval EFI_NOT_FOUND         The Generator is not registered.
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +SmbiosType17LibDestructor (
> > +  IN  EFI_HANDLE        ImageHandle,
> > +  IN  EFI_SYSTEM_TABLE  *SystemTable
> > +  )
> > +{
> > +  EFI_STATUS  Status;
> > +
> > +  Status = DeregisterSmbiosTableGenerator (&SmbiosType17Generator);
> > +  DEBUG ((
> > +    DEBUG_INFO,
> > +    "SMBIOS Type17: Deregister Generator. Status = %r\n",
> > +    Status
> > +    ));
> > +  ASSERT_EFI_ERROR (Status);
> > +  return Status;
> > +}
> > diff --git
> >
> a/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Li
> bA
> > rm.inf
> >
> b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Li
> bA
> > rm.inf
> > new file mode 100644
> > index 0000000000..78a80b75f0
> > --- /dev/null
> > +++
> b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17
> > +++ LibArm.inf
> > @@ -0,0 +1,32 @@
> > +## @file
> > +# SMBIOS Type17 Table Generator
> > +#
> > +#  Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights
> reserved.
> > +#  Copyright (c) 2019 - 2021, Arm Limited. All rights reserved.<BR> #
> > +#  SPDX-License-Identifier: BSD-2-Clause-Patent ##
> > +
> > +[Defines]
> > +  INF_VERSION    = 0x0001001B
> > +  BASE_NAME      = SmbiosType17LibArm
> > +  FILE_GUID      = 1f063bac-f8f1-4e08-8ffd-9aae52c75497
> > +  VERSION_STRING = 1.0
> > +  MODULE_TYPE    = DXE_DRIVER
> > +  LIBRARY_CLASS  = NULL|DXE_DRIVER
> > +  CONSTRUCTOR    = SmbiosType17LibConstructor
> > +  DESTRUCTOR     = SmbiosType17LibDestructor
> > +
> > +[Sources]
> > +  SmbiosType17Generator.c
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  ArmPlatformPkg/ArmPlatformPkg.dec
> > +  DynamicTablesPkg/DynamicTablesPkg.dec
> > +
> > +[LibraryClasses]
> > +  BaseLib
> > +  DebugLib
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#93687): https://edk2.groups.io/g/devel/message/93687
Mute This Topic: https://groups.io/mt/93275299/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-




More information about the edk2-devel-archive mailing list