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