[edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator
Girish Mahadevan via groups.io
gmahadevan=nvidia.com at groups.io
Sat Jan 28 00:09:09 UTC 2023
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 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/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&data=05%7C01%7Cgmahadevan%40nvidia.com%7C1169a52a2ad140a8d79308da94cf2762%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637985914717380634%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=EnTxn07ekjA7bGUeCg2c0BaUMVW0yU5JFjXNOcZuhQA%3D&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&data=05%7C01%7Cgmahadevan%40nvidia.com%7C1169a52a2ad140a8d79308da94cf2762%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637985914717380634%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=d6kU6CdywK4fnOdxE8CyTTM9eQHDE38FzZB7SA2FTqc%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%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F93651&data=05%7C01%7Cgmahadevan%40nvidia.com%7C1169a52a2ad140a8d79308da94cf2762%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637985914717380634%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=4xHMP2KfNlcdeBrtH%2FIT1F249uWIoz0XZreF3FSugq8%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/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
>>
>>
>>
>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99205): https://edk2.groups.io/g/devel/message/99205
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