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

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

Hi Girish,

Apologies for the delay in reply.

Please find my response marked inline as [SAMI].

Regards,

Sami Mujawar

On 28/01/2023 12:09 am, Girish Mahadevan wrote:
> Hi Sami
>
> Sorry for the long silence. I've sent you v2 of the patch (only 
> framework not the Type17) which includes a link to a github branch.
>
> Few more comments inline , prefixed by [GM].
>
> Best Regards
> Girish
>
> On 10/18/2022 9:36 AM, Sami Mujawar via groups.io wrote:
>> *External email: Use caution opening links or attachments*
>>
>>
>> Hi Girish,
>>
>> Please find my response inline marked [SAMI].
>>
>> Regards,
>>
>> Sami Mujawar
>>
>> On 04/10/2022 11:43 pm, Girish Mahadevan via groups.io wrote:
>>> Hello Sami
>>>
>>> Thank you so much for your review, I apologize for the late response.
>>>
>>> My comment in line about the handle manager [GM].
>>>
>>> Best Regards
>>> Girish
>>>
>>> On 9/12/2022 8:57 AM, Sami Mujawar wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 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@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/SmbiosType17Generator.c
>>>>>   create mode 100644 
>>>>> DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17LibArm.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/SmbiosType17Generator.c 
>>>>> b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Generator.c 
>>>>>
>>>>> new file mode 100644
>>>>> index 0000000000..5683ca570f
>>>>> --- /dev/null
>>>>> +++ 
>>>>> b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Generator.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.
>>>>
>>>
>>> [GM] I agree with the idea of having a map of CM_OBJECT_TOKENs to 
>>> SMBIOS handles. I've added this to the DynamicTableFactoryProtocol.
>>>
>>> However when it comes to actually figuring out and adding the 
>>> reference SMBIOS handle We've come up with a slightly different 
>>> approach.
>>>
>>> If I understood what you were saying above is:
>>>  If we happen to parse a Type17 table
>>>    if that Type17 table has a token to a Physical memory array CM_OBJ
>>>      if there is an existing Smbios Handle (look up this handle using
>>>                                              the CM_OBJECT_TOKEN)
>>>          then use this handle.
>>>      else if there is a generator for Type16 registered
>>>          call the Type16 generator code first
>>>
>>> IMO this gets a bit difficult to manage. Right now the 
>>> DynamicTableManagerDxe walks the array of EStdObjSmbiosTableList CM 
>>> objects, finds the generator for each Table, invokes it, gets an 
>>> SMBIOS record that it then installs via the SmbiosDxe driver.
>>> It seemed a bit convoluted to call the generator and install an 
>>> SMBIOS table while within the generator of another Smbios table.
>>> And if there are layers of dependencies (or multiple dependencies) 
>>> it could add to the complexities.
>>>
>>> Instead what we came up with is:
>>>
>>> If we happen to parse a Type17 table
>>>   if that Type17 table has a token to a Physical memory array CM_OBJ
>>>      if there is an existing Smbios Handle (look up this handle using
>>>                                              the CM_OBJECT_TOKEN )
>>>          then use this handle.
>>>      else if there is a generator for Type16 Registered
>>>            Generate an SMBIOS handle [since SmbiosDxe maintains the
>>>                                       handle DB privately I had to
>>>                                       pick a handle and check if it
>>>                                       clashes with existing records]
>>>            Add this SMBIOS handle to the map.
>>>      else // No generator present
>>>           Proceed without adding any reference
>>>
>>>
>>> Before Adding any SMBIOS table, we check if there is an existing 
>>> SMBIOS handle for the CM_OBJECT_TOKEN (the generator will return a 
>>> CM_OBJECT_TOKEN for each SMBIOS record created).
>>> If there is an existing SMBIOS handle, pass that in, else pass in 
>>> SMBIOS_HANDLE_PI_RESERVED and let SmbiosDxe assign the handle.
>>>
>>> Here is a sample implementation of this approach (it is a WIP, but I 
>>> wanted to get your thoughts on it)
>>>
>>> https://github.com/tianocore/edk2/compare/master...gmahadevan:edk2-upstream:RFC/smbios-dyntables 
>>>
>>
>> [SAMI] I had a look at the above branch and have the following 
>> suggestions:
>>
>> a. To resolve the dependency order, please see my patches for SMBIOS 
>> table dispatcher at: https://edk2.groups.io/g/devel/message/95340
>
> [GM]
> Thanks !
> I've setup a new patch/github branch that includes the dispatcher code 
> and the SMBIOS string helper patch.
>
> The SMBIOS string helper and dispatcher code look good to me.
>
>>
>> You should be able to apply these patches on your 
>> 'edk2-upstream:RFC/smbios-dyntables' branch and enable the dispatcher 
>> in ProcessSmbiosTables().
>>
>> e.g. In file 
>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c, 
>> the SMBIOS table dispatcher can be invoked once it is initialised as 
>> shown below.
>>
>> @@ -1007,19 +1007,24 @@ ProcessSmbiosTables (
>>       SmbiosTableCount
>>       ));
>>
>> +  // Initialise the SMBIOS Table Dispatcher state machine.
>> +  InitSmbiosTableDispatcher (SmbiosTableInfo, SmbiosTableCount);
>> +
>>     for (Idx = 0; Idx < SmbiosTableCount; Idx++) {
>> -    Status = BuildAndInstallSmbiosTable (
>> +    Status = DispatchSmbiosTable (
>> +               SmbiosTableInfo[Idx].TableType,
>>                  TableFactoryProtocol,
>>                  CfgMgrProtocol,
>>                  SmbiosProtocol,
>> -               &SmbiosTableInfo[Idx]
>> +               SmbiosTableInfo,
>> +               SmbiosTableCount
>>                  );
>>       if (EFI_ERROR (Status)) {
>>         DEBUG ((
>>           DEBUG_ERROR,
>> -        "ERROR: Failed to install SMBIOS Table." \
>> -        " Id = %u Status = %r\n",
>> -        SmbiosTableInfo[Idx].TableGeneratorId,
>> +        "ERROR: Failed to dispatch SMBIOS Table for installation." \
>> +        " Table Type = %u Status = %r\n",
>> +        SmbiosTableInfo[Idx].TableType,
>>           Status
>>           ));
>>       }
>>
>> b. With the SMBIOS dispatcher patch and the handle manager, it should 
>> be possible to update the handles for dependent tables. This should 
>> remove the need for the handle generation and management.
>>
>> c. With regards to the SMBIOS handle manager, I think it would be 
>> better to add CM_OBJECT_ID for the SmbiosCmTokenin SMBIOS_HANDLE_MAP.
>>
>> d. A new SMBIOS object namespace should be defined.
>>
>>      e.g Namespace ID 1010b or 1100b - SMBIOS Objects, see 
>> https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Include/ConfigurationManagerObject.h#L30-L34
>>
>>     With this change, CM_ARM_MEMORY_DEVICE_INFO becomes 
>> CM_SMBIOS_MEMORY_DEVICE_INFO
>>
>>      Similarly, EArmObjMemoryDeviceInfo becomes 
>> ESmbiosObjMemoryDeviceInfo
>>
>> e. With regards to DynamicTableManager.c, I think we should split the 
>> ACPI & SMBIOS processing in individual files (e.g. AcpiBuilder.c & 
>> SmbiosBuilder.c) under DynamicTableManagerDxe.
>>
>> f. Status appears to be returned uninitialised in 
>> DynamicTableManagerDxeInitialize().
>>
>> g. DynamicTablesPkg\Library\Smbios\Arm\SmbiosType17Lib can be moved 
>> to DynamicTablesPkg\Library\Smbios\SmbiosType17Lib.
>>
>> I prefer a github branch with the patch as it is very helpful to view 
>> the code being reviewed. However, it is difficult to provide 
>> feedback. Is it possible to submit a patch for the changes with the 
>> link for the guthub branch in the future, please?
>
> [GM]
> I've sent a new version of the patch minus the Type17 code as I wanted 
> to just focus on the framework code first.
> I've incorporated the comments from above except for the suggestion on 
> using CM_OBJECT_ID instead of the CM_TOKEN for the SmbiosHandleMap.
[SAMI] Sorry for not being clear in my earlier response. I meant we need 
CM_OBJECT_ID in addition to the CM_OBJECT_TOKEN. See example below:
typedef struct SmbiosHandleCmObjMap {
   ESTD_SMBIOS_TABLE_ID  SmbiosTableId;
   SMBIOS_HANDLE         SmbiosTblHandle;
   +CM_OBJECT_ID          ObjectId;
   CM_OBJECT_TOKEN       SmbiosCmToken;
} SMBIOS_HANDLE_MAP;

Otherwise it would not be possible to identify the type of object 
pointed by SmbiosCmToken, right?

[/SAMI]


> IMO the CM_TOKEN is better since we can have unique tokens for cases 
> where a single SMBIOS CM_OBJECT_ID can have multiple entries 
> (type17/type9 etc), let me know your thoughts.

[SAMI] I think I am missing something, can you explain the scenario, please?

>
> Best Regards
> Girish
>
>>
>> I am not sure if we need an edk2-staging branch for this feature. But 
>> if you think it would be helpful then please let me know and I will 
>> send out a request to create a new feature branch.
>>
>> [/SAMI]
>>
>>> Sorry for the long message, please let me know your thoughts.
>>>
>>> [/GM]
>>>
>>>> [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
>>>>> +             );
>>>>> +  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%2Fgithub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FMdeModulePkg%2FUniversal%2FSmbiosDxe%2FSmbiosDxe.c%23L476&amp;data=05%7C01%7Cgmahadevan%40nvidia.com%7C1169a52a2ad140a8d79308da94cf2762%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637985914717380634%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=EnTxn07ekjA7bGUeCg2c0BaUMVW0yU5JFjXNOcZuhQA%3D&amp;reserved=0 
>>>>
>>>> and
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FMdeModulePkg%2FUniversal%2FSmbiosDxe%2FSmbiosDxe.c%23L516&amp;data=05%7C01%7Cgmahadevan%40nvidia.com%7C1169a52a2ad140a8d79308da94cf2762%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637985914717380634%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=d6kU6CdywK4fnOdxE8CyTTM9eQHDE38FzZB7SA2FTqc%3D&amp;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%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F93651&amp;data=05%7C01%7Cgmahadevan%40nvidia.com%7C1169a52a2ad140a8d79308da94cf2762%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637985914717380634%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=4xHMP2KfNlcdeBrtH%2FIT1F249uWIoz0XZreF3FSugq8%3D&amp;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/SmbiosType17LibArm.inf 
>>>>> b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17LibArm.inf 
>>>>>
>>>>> new file mode 100644
>>>>> index 0000000000..78a80b75f0
>>>>> --- /dev/null
>>>>> +++ 
>>>>> b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17LibArm.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
>>>
>>>
>>>
>>>
>>>
>>