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

["Girish Mahadevan" <gmahadevan@nvidia.com>]

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.
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.

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
>>
>>
>>
>>
>>
>