<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p>Hi Girish,</p>
<p>Please find my response inline marked [SAMI].</p>
<p>Regards,</p>
<p>Sami Mujawar<br>
</p>
<div class="moz-cite-prefix">On 04/10/2022 11:43 pm, Girish
Mahadevan via groups.io wrote:<br>
</div>
<blockquote type="cite" cite="mid:2204e785-d05a-a47d-2da6-a9b6b8e608a0@nvidia.com">Hello
Sami
<br>
<br>
Thank you so much for your review, I apologize for the late
response.
<br>
<br>
My comment in line about the handle manager [GM].
<br>
<br>
Best Regards
<br>
Girish
<br>
<br>
On 9/12/2022 8:57 AM, Sami Mujawar wrote:
<br>
<blockquote type="cite">External email: Use caution opening links
or attachments
<br>
<br>
<br>
Hi Girish,
<br>
<br>
Thank you for this patch and for the effort for bringing forward
dynamic
<br>
SMBIOS generation.
<br>
<br>
Please find my feedback inline marked [SAMI].
<br>
<br>
Regards,
<br>
<br>
Sami Mujawar
<br>
<br>
On 26/08/2022 06:37 pm, Girish Mahadevan wrote:
<br>
<blockquote type="cite">Add a new CM object to describe memory
devices and setup a new
<br>
Generator Library for SMBIOS Type17 table.
<br>
<br>
Signed-off-by: Girish Mahadevan<a class="moz-txt-link-rfc2396E" href="mailto:gmahadevan@nvidia.com"><gmahadevan@nvidia.com></a>
<br>
---
<br>
.../Include/ArmNameSpaceObjects.h | 59 +++
<br>
.../SmbiosType17Lib/SmbiosType17Generator.c | 338
++++++++++++++++++
<br>
.../SmbiosType17Lib/SmbiosType17LibArm.inf | 32 ++
<br>
3 files changed, 429 insertions(+)
<br>
create mode 100644
DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Generator.c<br>
create mode 100644
DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17LibArm.inf<br>
<br>
diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
<br>
index 102e0f96be..199a19e997 100644
<br>
--- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
<br>
+++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
<br>
@@ -63,6 +63,7 @@ typedef enum ArmObjectID {
<br>
EArmObjPciInterruptMapInfo, ///< 39 - Pci
Interrupt Map Info
<br>
EArmObjRmr, ///< 40 - Reserved
Memory Range Node
<br>
EArmObjMemoryRangeDescriptor, ///< 41 - Memory
Range Descriptor
<br>
+ EArmObjMemoryDeviceInfo, ///< 42 - Memory
Device Information
<br>
EArmObjMax
<br>
} EARM_OBJECT_ID;
<br>
<br>
@@ -1070,6 +1071,64 @@ typedef struct CmArmRmrDescriptor {
<br>
UINT64 Length;
<br>
} CM_ARM_MEMORY_RANGE_DESCRIPTOR;
<br>
<br>
+/** A structure that describes the physical memory device.
<br>
+
<br>
+ The physical memory devices on the system are described by
this object.
<br>
+
<br>
+ SMBIOS Specification v3.5.0 Type17
<br>
+
<br>
+ ID: EArmObjMemoryDeviceInfo,
<br>
+*/
<br>
+typedef struct CmArmMemoryDeviceInfo {
<br>
</blockquote>
[SAMI] I think we may need a Token pointing to the Type 16
object so
<br>
that the Physical Memory Array Handle can be setup, see my
comment below
<br>
about the HandleManager.
<br>
<blockquote type="cite">+ /** Size of the device.
<br>
+ Size of the device in bytes.
<br>
+ */
<br>
+ UINT64 Size;
<br>
+
<br>
+ /** Device Set */
<br>
+ UINT8 DeviceSet;
<br>
+
<br>
+ /** Speed of the device
<br>
+ Speed of the device in MegaTransfers/second.
<br>
+ */
<br>
+ UINT32 Speed;
<br>
+
<br>
+ /** Serial Number of device */
<br>
+ CHAR8 *SerialNum;
<br>
+
<br>
+ /** AssetTag identifying the device */
<br>
+ CHAR8 *AssetTag;
<br>
+
<br>
+ /** Device Locator String for the device.
<br>
+ String that describes the slot or position of the device
on the board.
<br>
+ */
<br>
+ CHAR8 *DeviceLocator;
<br>
+
<br>
+ /** Bank locator string for the device.
<br>
+ String that describes the bank where the device is
located.
<br>
+ */
<br>
+ CHAR8 *BankLocator;
<br>
+
<br>
+ /** Firmware version of the memory device */
<br>
+ CHAR8 *FirmwareVersion;
<br>
+
<br>
+ /** Manufacturer Id.
<br>
+ 2 byte Manufacturer Id as per JEDEC Standard JEP106AV
<br>
+ */
<br>
+ UINT16 ModuleManufacturerId;
<br>
+
<br>
+ /** Manufacturer Product Id
<br>
+ 2 byte Manufacturer Id as designated by Manufacturer.
<br>
+ */
<br>
+ UINT16 ModuleProductId;
<br>
+
<br>
+ /** Device Attributes */
<br>
+ UINT8 Attributes;
<br>
+
<br>
+ /** Device Configured Voltage in millivolts */
<br>
+ UINT16 ConfiguredVoltage;
<br>
</blockquote>
[SAMI] This field does not appear to be used in the generator.
If the
<br>
intention is to use this in the future, then it may be better to
add
<br>
this at a later stage.
<br>
<blockquote type="cite">+} CM_ARM_MEMORY_DEVICE_INFO;
<br>
+
<br>
#pragma pack()
<br>
<br>
#endif // ARM_NAMESPACE_OBJECTS_H_
<br>
diff --git
a/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Generator.c
b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Generator.c
<br>
new file mode 100644
<br>
index 0000000000..5683ca570f
<br>
--- /dev/null
<br>
+++
b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Generator.c<br>
@@ -0,0 +1,338 @@
<br>
+/** @file
<br>
+ SMBIOS Type17 Table Generator.
<br>
+
<br>
+ Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES.
All rights reserved.
<br>
+ Copyright (c) 2020 - 2021, Arm Limited. All rights
reserved.<BR>
<br>
+
<br>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
<br>
+**/
<br>
+
<br>
+#include <Library/BaseLib.h>
<br>
+#include <Library/BaseMemoryLib.h>
<br>
+#include <Library/DebugLib.h>
<br>
+#include <Library/PrintLib.h>
<br>
+#include <Library/MemoryAllocationLib.h>
<br>
+#include <Library/SmbiosType17FixupLib.h>
<br>
</blockquote>
[SAMI] I could not find SmbiosType17FixupLib.h in this patch
series. Can
<br>
you check, please?
<br>
<blockquote type="cite">+
<br>
+// Module specific include files.
<br>
+#include <ConfigurationManagerObject.h>
<br>
+#include <ConfigurationManagerHelper.h>
<br>
+#include <Protocol/ConfigurationManagerProtocol.h>
<br>
+#include <Protocol/Smbios.h>
<br>
</blockquote>
[SAMI] I think Protocol/Smbios.h may not be required in this
file. Can
<br>
you check, please?
<br>
<blockquote type="cite">+#include
<IndustryStandard/SmBios.h>
<br>
+
<br>
+/** This macro expands to a function that retrieves the
Memory Device
<br>
+ information from the Configuration Manager.
<br>
+*/
<br>
+GET_OBJECT_LIST (
<br>
+ EObjNameSpaceArm,
<br>
+ EArmObjMemoryDeviceInfo,
<br>
+ CM_ARM_MEMORY_DEVICE_INFO
<br>
+ )
<br>
+
<br>
+// Default Values for Memory Device
<br>
+STATIC SMBIOS_TABLE_TYPE17 MemDeviceInfoTemplate = {
<br>
+ { // Hdr
<br>
+ EFI_SMBIOS_TYPE_MEMORY_DEVICE, // Type
<br>
+ sizeof (SMBIOS_TABLE_TYPE17), // Length
<br>
+ 0 // Handle
<br>
+ },
<br>
+ 0, // MemoryArrayHandle
<br>
</blockquote>
<br>
[SAMI] Do you have any thoughts on how the MemoryArrayHandle can
be setup?
<br>
<br>
The same applies for the following MemoryErrorInformationHandle
field.
<br>
<br>
I think we need some sort of a HandleManager in
DynamicTablesFramework
<br>
that can keep track of the mappings between SMBIOS Objects and
Table
<br>
Handles.
<br>
<br>
e.g. Smbios - HandleManager
<br>
<br>
+-------------------------------+-------------------------------+
<br>
<br>
| Object Token | Table
Handle |
<br>
<br>
+-------------------------------+-------------------------------+
<br>
<br>
| Type16Obj_token | Type 16 Table handle |
<br>
<br>
+-------------------------------+-------------------------------+
<br>
<br>
...
<br>
<br>
- The Type17Object i.e. CM_ARM_MEMORY_DEVICE_INFO can then hold
a token
<br>
for the Type16Object.
<br>
<br>
- If Type 17 table is to be installed, DynamicTablemanager
shall
<br>
search the SMBIOS table list to see if a Type16 table is
requested to be
<br>
installed.
<br>
<br>
- If a Type16 table is present in the list of SMBIOS table to
install,
<br>
the Type16 table shall be installed first and an entry is made
in the
<br>
Smbios HandleManager to create a mapping of Type16Obj_token
<==> Type16
<br>
Table Handle.
<br>
<br>
- The Type17 table can now be built and if a the Type16Object
token is
<br>
provided in CM_ARM_MEMORY_DEVICE_INFO, the Smbios HandleManager
shall be
<br>
searched (using Type16Obj_token) to retrieve the Type16 Table
handle and
<br>
populate the Type 17 Physical Memory Array Handle field.
<br>
<br>
I think we may have to experiment a bit before we arrive at the
correct
<br>
design. However, please do let me know your thoughts on the
above.
<br>
<br>
</blockquote>
<br>
[GM] I agree with the idea of having a map of CM_OBJECT_TOKENs to
SMBIOS handles. I've added this to the
DynamicTableFactoryProtocol.
<br>
<br>
However when it comes to actually figuring out and adding the
reference SMBIOS handle We've come up with a slightly different
approach.
<br>
<br>
If I understood what you were saying above is:
<br>
If we happen to parse a Type17 table
<br>
if that Type17 table has a token to a Physical memory array
CM_OBJ
<br>
if there is an existing Smbios Handle (look up this handle
using
<br>
the CM_OBJECT_TOKEN)
<br>
then use this handle.
<br>
else if there is a generator for Type16 registered
<br>
call the Type16 generator code first
<br>
<br>
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.
<br>
It seemed a bit convoluted to call the generator and install an
SMBIOS table while within the generator of another Smbios table.
<br>
And if there are layers of dependencies (or multiple dependencies)
it could add to the complexities.
<br>
<br>
Instead what we came up with is:
<br>
<br>
If we happen to parse a Type17 table
<br>
if that Type17 table has a token to a Physical memory array
CM_OBJ
<br>
if there is an existing Smbios Handle (look up this handle
using
<br>
the CM_OBJECT_TOKEN )
<br>
then use this handle.
<br>
else if there is a generator for Type16 Registered
<br>
Generate an SMBIOS handle [since SmbiosDxe maintains
the
<br>
handle DB privately I had to
<br>
pick a handle and check if
it
<br>
clashes with existing
records]
<br>
Add this SMBIOS handle to the map.
<br>
else // No generator present
<br>
Proceed without adding any reference
<br>
<br>
<br>
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).
<br>
If there is an existing SMBIOS handle, pass that in, else pass in
SMBIOS_HANDLE_PI_RESERVED and let SmbiosDxe assign the handle.
<br>
<br>
Here is a sample implementation of this approach (it is a WIP, but
I wanted to get your thoughts on it)
<br>
<br>
<a class="moz-txt-link-freetext" href="https://github.com/tianocore/edk2/compare/master...gmahadevan:edk2-upstream:RFC/smbios-dyntables">https://github.com/tianocore/edk2/compare/master...gmahadevan:edk2-upstream:RFC/smbios-dyntables</a>
<br>
</blockquote>
<p>[SAMI] I had a look at the above branch and have the following
suggestions:<br>
</p>
<p>a. To resolve the dependency order, please see my patches for
SMBIOS table dispatcher at:
<a class="moz-txt-link-freetext" href="https://edk2.groups.io/g/devel/message/95340">https://edk2.groups.io/g/devel/message/95340</a><br>
</p>
<p>You should be able to apply these patches on your
'edk2-upstream:RFC/smbios-dyntables' branch and enable the
dispatcher in ProcessSmbiosTables().</p>
<p>e.g. In file
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c,
the SMBIOS table dispatcher can be invoked once it is initialised
as shown below.<br>
</p>
<p>@@ -1007,19 +1007,24 @@ ProcessSmbiosTables (<br>
SmbiosTableCount<br>
));<br>
<br>
+ // Initialise the SMBIOS Table Dispatcher state machine.<br>
+ InitSmbiosTableDispatcher (SmbiosTableInfo, SmbiosTableCount);<br>
+<br>
for (Idx = 0; Idx < SmbiosTableCount; Idx++) {<br>
- Status = BuildAndInstallSmbiosTable (<br>
+ Status = DispatchSmbiosTable (<br>
+ SmbiosTableInfo[Idx].TableType,<br>
TableFactoryProtocol,<br>
CfgMgrProtocol,<br>
SmbiosProtocol,<br>
- &SmbiosTableInfo[Idx]<br>
+ SmbiosTableInfo,<br>
+ SmbiosTableCount<br>
);<br>
if (EFI_ERROR (Status)) {<br>
DEBUG ((<br>
DEBUG_ERROR,<br>
- "ERROR: Failed to install SMBIOS Table." \<br>
- " Id = %u Status = %r\n",<br>
- SmbiosTableInfo[Idx].TableGeneratorId,<br>
+ "ERROR: Failed to dispatch SMBIOS Table for
installation." \<br>
+ " Table Type = %u Status = %r\n",<br>
+ SmbiosTableInfo[Idx].TableType,<br>
Status<br>
));<br>
}<br>
</p>
<p>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.<br>
</p>
<p>c. With regards to the SMBIOS handle manager, I think it would be
better to add <span style="color: #000000;">CM_OBJECT_ID for the
</span><span style="color: #000000;">SmbiosCmToken</span><span style="color: #000000;"> in SMBIOS_HANDLE_MAP.<br>
</span></p>
<p><span style="color: #000000;">d.</span> A new SMBIOS object
namespace should be defined.</p>
<p> e.g Namespace ID 1010b or 1100b - SMBIOS Objects, see <a class="moz-txt-link-freetext" href="https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Include/ConfigurationManagerObject.h#L30-L34">https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Include/ConfigurationManagerObject.h#L30-L34</a></p>
<p> With this change, <span style="color: #000000;">CM_ARM_MEMORY_DEVICE_INFO
becomes </span>CM_SMBIOS_MEMORY_DEVICE_INFO <br>
</p>
<p> Similarly, <span style="color: #098658;">EArmObjMemoryDeviceInfo</span>
becomes <span style="color: #098658;">ESmbiosObjMemoryDeviceInfo</span><br>
</p>
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. <br>
<p>f. Status appears to be returned uninitialised in <span style="color: #000000;">DynamicTableManagerDxeInitialize().</span></p>
<p><span style="color: #000000;">g.
DynamicTablesPkg\Library\Smbios\Arm\SmbiosType17Lib can be moved
to DynamicTablesPkg\Library\Smbios\SmbiosType17Lib.<br>
</span></p>
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? <br>
<p>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.<br>
</p>
<p>[/SAMI]</p>
<blockquote type="cite" cite="mid:2204e785-d05a-a47d-2da6-a9b6b8e608a0@nvidia.com">Sorry
for the long message, please let me know your thoughts.
<br>
<br>
[/GM]
<br>
<br>
<blockquote type="cite">[SAMI]
<br>
<blockquote type="cite">+
0, //
MemoryErrorInformationHandle
<br>
+ 0xFFFF, // TotalWidth
<br>
+ 0xFFFF, // DataWIdth
<br>
</blockquote>
<br>
[SAMI] I need to find out how these fields should be populated,
but the
<br>
Annex A, SMBIOS specification version 3.6.0 says the following:
<br>
<br>
4.8.4 Total Width is not 0FFFFh (Unknown) if the memory device
is
<br>
installed. (Size is not 0.)
<br>
<br>
4.8.5 Data Width is not 0FFFFh (Unknown).
<br>
<br>
Can you explain how this field is used, please?
<br>
<br>
[/SAMI]
<br>
<br>
<blockquote type="cite">+
0x7FFF, // Size (always use
Extended Size)
<br>
</blockquote>
<br>
[SAMI] I think this field should be set based on the Size.
<br>
<br>
The spec says "If the size is 32 GB-1 MB or greater, the field
value is
<br>
7FFFh and the actual size is stored in the Extended Size field."
<br>
<br>
I think it would be good to have a static function that encodes
the
<br>
size in wither the Size field or the Extended Size field.
<br>
<br>
e.g. UpdateSmbiosTable17Size (SMBIOS_TABLE_TYPE17 *SmbiosRecord,
UINTN
<br>
Size) {
<br>
<br>
if (Size > 32GB-1MB) {
<br>
<br>
SmbiosRecord->Size = EncodeSize (xxx);
<br>
<br>
} else {
<br>
<br>
SmbiosRecord->ExtendedSize = EncodeExtendedSize
(xxx);
<br>
<br>
}
<br>
<br>
}
<br>
<br>
[/SAMI]
<br>
<br>
<blockquote type="cite">+
MemoryTypeUnknown, // FormFactor
<br>
+ 0xFF, // DeviceSet
<br>
+ 1, // Device Locator
<br>
+ 2, // Bank Locator
<br>
+ MemoryTypeSdram, // MemoryType
<br>
+ { // TypeDetail
<br>
+ 0
<br>
+ },
<br>
+ 0xFFFF, // Speed (Use
Extended Speed)
<br>
</blockquote>
[SAMI] Maybe we need a helper function (similar to
<br>
UpdateSmbiosTable17Size()) for this field as well.
<br>
<blockquote type="cite">+
0, // Manufacturer
<br>
+ // (Unused Use
ModuleManufactuerId)
<br>
+ 3, // SerialNumber
<br>
+ 4, // AssetTag
<br>
+ 0, // PartNumber
<br>
+ // (Unused Use
ModuleProductId)
<br>
+ 0, // Attributes
<br>
+ 0, // ExtendedSize
<br>
+ 2000, //
ConfiguredMemoryClockSpeed
<br>
</blockquote>
[SAMI] Should this be provided through
CM_ARM_MEMORY_DEVICE_INFO?
<br>
<blockquote type="cite">+
0, // MinimumVoltage
<br>
+ 0, // MaximumVoltage
<br>
+ 0, // ConfiguredVoltage
<br>
+ MemoryTechnologyDram, // MemoryTechnology
<br>
</blockquote>
[SAMI] Should this be provided through
CM_ARM_MEMORY_DEVICE_INFO?
<br>
<blockquote type="cite">+
{ //
MemoryOperatingModeCapability
<br>
+ .Uint16 = 0x000
<br>
+ },
<br>
</blockquote>
[SAMI] I think the above initialisation may not be portable.
<br>
<blockquote type="cite">+ 5,
// FirmwareVersion
<br>
+ 0, //
ModuleManufacturerId
<br>
+ 0, // ModuleProductId
<br>
+ 0, //
MemorySubsystemContollerManufacturerId
<br>
+ 0, //
MemorySybsystemControllerProductId
<br>
+ 0, // NonVolatileSize
<br>
+ 0, // VolatileSize
<br>
+ 0, // CacheSize
<br>
+ 0, // LogicalSize
<br>
+ 0, // ExtendedSpeed
<br>
+ 0, //
ExtendedConfiguredMemorySpeed
<br>
+};
<br>
+
<br>
+STATIC CHAR8 UnknownStr[] = "Unknown\0";
<br>
</blockquote>
[SAMI] Would it be possible to add the CONST qualifier, please?
<br>
<blockquote type="cite">+
<br>
+STATIC
<br>
+EFI_STATUS
<br>
+EFIAPI
<br>
+FreeSmbiosType17TableEx (
<br>
+ IN CONST SMBIOS_TABLE_GENERATOR
*CONST This,
<br>
+ IN CONST CM_STD_OBJ_SMBIOS_TABLE_INFO
*CONST SmbiosTableInfo,
<br>
+ IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL
*CONST CfgMgrProtocol,
<br>
+ IN OUT SMBIOS_STRUCTURE
***CONST Table,
<br>
+ IN CONST
UINTN TableCount
<br>
+ )
<br>
+{
<br>
+ return EFI_SUCCESS;
<br>
+}
<br>
+
<br>
+/** Construct SMBIOS Type17 Table describing memory devices.
<br>
+
<br>
+ If this function allocates any resources then they must be
freed
<br>
+ in the FreeXXXXTableResources function.
<br>
+
<br>
+ @param [in] This Pointer to the SMBIOS table
generator.
<br>
+ @param [in] SmbiosTableInfo Pointer to the SMBIOS table
information.
<br>
+ @param [in] CfgMgrProtocol Pointer to the Configuration
Manager
<br>
+ Protocol interface.
<br>
+ @param [out] Table Pointer to the SMBIOS table.
<br>
+
<br>
+ @retval EFI_SUCCESS Table generated
successfully.
<br>
+ @retval EFI_BAD_BUFFER_SIZE The size returned by the
Configuration
<br>
+ Manager is less than the
Object size for
<br>
+ the requested object.
<br>
+ @retval EFI_INVALID_PARAMETER A parameter is invalid.
<br>
+ @retval EFI_NOT_FOUND Could not find information.
<br>
+ @retval EFI_OUT_OF_RESOURCES Could not allocate memory.
<br>
+ @retval EFI_UNSUPPORTED Unsupported configuration.
<br>
+**/
<br>
+STATIC
<br>
+EFI_STATUS
<br>
+EFIAPI
<br>
+BuildSmbiosType17TableEx (
<br>
+ IN CONST SMBIOS_TABLE_GENERATOR
*This,
<br>
+ IN CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST
SmbiosTableInfo,
<br>
+ IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST
CfgMgrProtocol,
<br>
+ OUT SMBIOS_STRUCTURE
***Table,
<br>
+ OUT UINTN *CONST
TableCount
<br>
+ )
<br>
+{
<br>
+ EFI_STATUS Status;
<br>
+ UINT32 NumMemDevices;
<br>
+ SMBIOS_STRUCTURE **TableList;
<br>
+ CM_ARM_MEMORY_DEVICE_INFO *MemoryDevicesInfo;
<br>
+ UINTN Index;
<br>
+ UINTN SerialNumLen;
<br>
+ CHAR8 *SerialNum;
<br>
+ UINTN AssetTagLen;
<br>
+ CHAR8 *AssetTag;
<br>
+ UINTN DeviceLocatorLen;
<br>
+ CHAR8 *DeviceLocator;
<br>
+ UINTN BankLocatorLen;
<br>
+ CHAR8 *BankLocator;
<br>
+ UINTN FirmwareVersionLen;
<br>
+ CHAR8 *FirmwareVersion;
<br>
+ CHAR8 *OptionalStrings;
<br>
+ SMBIOS_TABLE_TYPE17 *SmbiosRecord;
<br>
+
<br>
+ ASSERT (This != NULL);
<br>
+ ASSERT (SmbiosTableInfo != NULL);
<br>
+ ASSERT (CfgMgrProtocol != NULL);
<br>
+ ASSERT (Table != NULL);
<br>
+ ASSERT (TableCount != NULL);
<br>
+ ASSERT (SmbiosTableInfo->TableGeneratorId ==
This->GeneratorID);
<br>
+
<br>
+ DEBUG ((DEBUG_ERROR, "%a : Start \n", __FUNCTION__));
<br>
+ *Table = NULL;
<br>
+ Status = GetEArmObjMemoryDeviceInfo (
<br>
+ CfgMgrProtocol,
<br>
+ CM_NULL_TOKEN,
<br>
+ &MemoryDevicesInfo,
<br>
+ &NumMemDevices
<br>
+ );
<br>
+ if (EFI_ERROR (Status)) {
<br>
+ DEBUG ((
<br>
+ DEBUG_ERROR,
<br>
+ "Failed to get Memory Devices CM Object %r\n",
<br>
+ Status
<br>
+ ));
<br>
+ return Status;
<br>
+ }
<br>
+
<br>
+ TableList = (SMBIOS_STRUCTURE **)AllocateZeroPool (sizeof
(SMBIOS_STRUCTURE *) * NumMemDevices);
<br>
</blockquote>
[SAMI] The memory allocated here should be freed in
<br>
FreeSmbiosType17TableEx.
<br>
<blockquote type="cite">+ if (TableList == NULL) {
<br>
+ DEBUG ((DEBUG_ERROR, "Failed to alloc memory for %u
devices table\n"));
<br>
+ Status = EFI_OUT_OF_RESOURCES;
<br>
+ goto exit;
<br>
+ }
<br>
+
<br>
+ for (Index = 0; Index < NumMemDevices; Index++) {
<br>
+ if (MemoryDevicesInfo[Index].SerialNum != NULL) {
<br>
+ SerialNumLen = AsciiStrLen
(MemoryDevicesInfo[Index].SerialNum);
<br>
+ SerialNum = MemoryDevicesInfo[Index].SerialNum;
<br>
+ } else {
<br>
+ SerialNumLen = AsciiStrLen (UnknownStr);
<br>
+ SerialNum = UnknownStr;
<br>
</blockquote>
<br>
[SAMI] If the serial number is not provided, then should the
string
<br>
field be set to 0?
<br>
<br>
See Section 6.1.3, SMBIOS Spec Version 3.6.0 which states
<br>
<br>
"If a string field references no string, a null (0) is placed in
that
<br>
string field."
<br>
<br>
[/SAMI]
<br>
<br>
<blockquote type="cite">+ }
<br>
+
<br>
+ if (MemoryDevicesInfo[Index].AssetTag != NULL) {
<br>
+ AssetTagLen = AsciiStrLen
(MemoryDevicesInfo[Index].AssetTag);
<br>
+ AssetTag = MemoryDevicesInfo[Index].AssetTag;
<br>
+ } else {
<br>
+ AssetTagLen = AsciiStrLen (UnknownStr);
<br>
+ AssetTag = UnknownStr;
<br>
+ }
<br>
+
<br>
+ if (MemoryDevicesInfo[Index].DeviceLocator != NULL) {
<br>
+ DeviceLocatorLen = AsciiStrLen
(MemoryDevicesInfo[Index].DeviceLocator);
<br>
+ DeviceLocator =
MemoryDevicesInfo[Index].DeviceLocator;
<br>
+ } else {
<br>
+ DeviceLocatorLen = AsciiStrLen (UnknownStr);
<br>
+ DeviceLocator = UnknownStr;
<br>
+ }
<br>
+
<br>
+ if (MemoryDevicesInfo[Index].BankLocator != NULL) {
<br>
+ BankLocatorLen = AsciiStrLen
(MemoryDevicesInfo[Index].BankLocator);
<br>
+ BankLocator = MemoryDevicesInfo[Index].BankLocator;
<br>
+ } else {
<br>
+ BankLocatorLen = AsciiStrLen (UnknownStr);
<br>
+ BankLocator = UnknownStr;
<br>
+ }
<br>
+
<br>
+ if (MemoryDevicesInfo[Index].FirmwareVersion != NULL) {
<br>
+ FirmwareVersionLen = AsciiStrLen
(MemoryDevicesInfo[Index].FirmwareVersion);
<br>
+ FirmwareVersion =
MemoryDevicesInfo[Index].FirmwareVersion;
<br>
+ } else {
<br>
+ FirmwareVersionLen = AsciiStrLen (UnknownStr);
<br>
+ FirmwareVersion = UnknownStr;
<br>
+ }
<br>
+
<br>
+ SmbiosRecord = (SMBIOS_TABLE_TYPE17 *)AllocateZeroPool (
<br>
+ sizeof
(SMBIOS_TABLE_TYPE17) +
<br>
+ SerialNumLen + 1
+
<br>
+ AssetTagLen + 1 +
DeviceLocatorLen + 1 +
<br>
+ BankLocatorLen +
1 + FirmwareVersionLen + 1 + 1
<br>
+ );
<br>
</blockquote>
[SAMI] The memory allocated here needs to be freed in
<br>
FreeSmbiosType17TableEx as SmbiosProtocol->Add () makes a
copy of the
<br>
SmbiosTable, see
<br>
<a class="moz-txt-link-freetext" href="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">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</a>
<br>
and
<br>
<a class="moz-txt-link-freetext" href="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">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</a>.
<br>
<br>
<blockquote type="cite">+ if (SmbiosRecord == NULL) {
<br>
+ Status = EFI_OUT_OF_RESOURCES;
<br>
+ goto exit;
<br>
+ }
<br>
+
<br>
+ CopyMem (SmbiosRecord, &MemDeviceInfoTemplate, sizeof
(SMBIOS_TABLE_TYPE17));
<br>
+ SmbiosRecord->ExtendedSize =
MemoryDevicesInfo[Index].Size;
<br>
</blockquote>
<br>
[SAMI] CM_ARM_MEMORY_DEVICE_INFO.Size is documented as size in
bytes,
<br>
while looking at the SMBIOS specification, section 7.18.5 for
the
<br>
Extended Size Bits 30:0 represent the size of the memory device
in
<br>
megabytes.
<br>
<br>
I think it will be useful to create UpdateSmbiosTable17Size()
which does
<br>
the appropriate encoding and updation of the SMBIOS table fieds.
<br>
<br>
[/SAMI]
<br>
<br>
<blockquote type="cite">+
SmbiosRecord->DeviceSet =
MemoryDevicesInfo[Index].DeviceSet;
<br>
+ SmbiosRecord->ModuleManufacturerID =
<br>
+ MemoryDevicesInfo[Index].ModuleManufacturerId;
<br>
+ SmbiosRecord->ModuleProductID =
<br>
+ MemoryDevicesInfo[Index].ModuleProductId;
<br>
+ SmbiosRecord->Attributes =
<br>
+ MemoryDevicesInfo[Index].Attributes;
<br>
+ SmbiosRecord->ExtendedSpeed =
MemoryDevicesInfo[Index].Speed;
<br>
+ OptionalStrings = (CHAR8 *)(SmbiosRecord +
1);
<br>
+ AsciiSPrint (OptionalStrings, DeviceLocatorLen + 1,
DeviceLocator);
<br>
</blockquote>
[SAMI] I think we can simplify the publishing of the SMBIOS
strings
<br>
using SmbiosStringTableLib. Please see the patch at
<br>
<a class="moz-txt-link-freetext" href="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">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</a>
<br>
<blockquote type="cite">+ OptionalStrings = OptionalStrings +
DeviceLocatorLen + 1;
<br>
+ AsciiSPrint (OptionalStrings, BankLocatorLen + 1,
BankLocator);
<br>
+ OptionalStrings = OptionalStrings + BankLocatorLen + 1;
<br>
+ AsciiSPrint (OptionalStrings, SerialNumLen + 1,
SerialNum);
<br>
+ OptionalStrings = OptionalStrings + SerialNumLen + 1;
<br>
+ AsciiSPrint (OptionalStrings, AssetTagLen + 1, AssetTag);
<br>
+ OptionalStrings = OptionalStrings + AssetTagLen + 1;
<br>
+ AsciiSPrint (OptionalStrings, FirmwareVersionLen + 1,
FirmwareVersion);
<br>
+ OptionalStrings = OptionalStrings + FirmwareVersionLen +
1;
<br>
+ TableList[Index] = (SMBIOS_STRUCTURE *)SmbiosRecord;
<br>
+ }
<br>
+
<br>
+ *Table = TableList;
<br>
+ *TableCount = NumMemDevices;
<br>
+
<br>
+exit:
<br>
+ DEBUG ((DEBUG_ERROR, "%a : Done \n", __FUNCTION__));
<br>
+ return Status;
<br>
+}
<br>
+
<br>
+/** The interface for the SMBIOS Type17 Table Generator.
<br>
+*/
<br>
+STATIC
<br>
+CONST
<br>
+SMBIOS_TABLE_GENERATOR SmbiosType17Generator = {
<br>
+ // Generator ID
<br>
+ CREATE_STD_SMBIOS_TABLE_GEN_ID (EStdSmbiosTableIdType17),
<br>
+ // Generator Description
<br>
+ L"SMBIOS.TYPE17.GENERATOR",
<br>
+ // SMBIOS Table Type
<br>
+ EFI_SMBIOS_TYPE_MEMORY_DEVICE,
<br>
+ NULL,
<br>
+ NULL,
<br>
+ // Build table function Extended.
<br>
+ BuildSmbiosType17TableEx,
<br>
+ // Free function Extended.
<br>
+ FreeSmbiosType17TableEx
<br>
+};
<br>
+
<br>
+/** Register the Generator with the SMBIOS Table Factory.
<br>
+
<br>
+ @param [in] ImageHandle The handle to the image.
<br>
+ @param [in] SystemTable Pointer to the System Table.
<br>
+
<br>
+ @retval EFI_SUCCESS The Generator is registered.
<br>
+ @retval EFI_INVALID_PARAMETER A parameter is invalid.
<br>
+ @retval EFI_ALREADY_STARTED The Generator for the Table
ID
<br>
+ is already registered.
<br>
+**/
<br>
+EFI_STATUS
<br>
+EFIAPI
<br>
+SmbiosType17LibConstructor (
<br>
+ IN EFI_HANDLE ImageHandle,
<br>
+ IN EFI_SYSTEM_TABLE *SystemTable
<br>
+ )
<br>
+{
<br>
+ EFI_STATUS Status;
<br>
+
<br>
+ Status = RegisterSmbiosTableGenerator
(&SmbiosType17Generator);
<br>
+ DEBUG ((
<br>
+ DEBUG_INFO,
<br>
+ "SMBIOS Type 17: Register Generator. Status = %r\n",
<br>
+ Status
<br>
+ ));
<br>
+ ASSERT_EFI_ERROR (Status);
<br>
+
<br>
+ return Status;
<br>
+}
<br>
+
<br>
+/** Deregister the Generator from the SMBIOS Table Factory.
<br>
+
<br>
+ @param [in] ImageHandle The handle to the image.
<br>
+ @param [in] SystemTable Pointer to the System Table.
<br>
+
<br>
+ @retval EFI_SUCCESS The Generator is
deregistered.
<br>
+ @retval EFI_INVALID_PARAMETER A parameter is invalid.
<br>
+ @retval EFI_NOT_FOUND The Generator is not
registered.
<br>
+**/
<br>
+EFI_STATUS
<br>
+EFIAPI
<br>
+SmbiosType17LibDestructor (
<br>
+ IN EFI_HANDLE ImageHandle,
<br>
+ IN EFI_SYSTEM_TABLE *SystemTable
<br>
+ )
<br>
+{
<br>
+ EFI_STATUS Status;
<br>
+
<br>
+ Status = DeregisterSmbiosTableGenerator
(&SmbiosType17Generator);
<br>
+ DEBUG ((
<br>
+ DEBUG_INFO,
<br>
+ "SMBIOS Type17: Deregister Generator. Status = %r\n",
<br>
+ Status
<br>
+ ));
<br>
+ ASSERT_EFI_ERROR (Status);
<br>
+ return Status;
<br>
+}
<br>
diff --git
a/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17LibArm.inf
b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17LibArm.inf
<br>
new file mode 100644
<br>
index 0000000000..78a80b75f0
<br>
--- /dev/null
<br>
+++
b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17LibArm.inf<br>
@@ -0,0 +1,32 @@
<br>
+## @file
<br>
+# SMBIOS Type17 Table Generator
<br>
+#
<br>
+# Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES.
All rights reserved.
<br>
+# Copyright (c) 2019 - 2021, Arm Limited. All rights
reserved.<BR>
<br>
+#
<br>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
<br>
+##
<br>
+
<br>
+[Defines]
<br>
+ INF_VERSION = 0x0001001B
<br>
+ BASE_NAME = SmbiosType17LibArm
<br>
+ FILE_GUID = 1f063bac-f8f1-4e08-8ffd-9aae52c75497
<br>
+ VERSION_STRING = 1.0
<br>
+ MODULE_TYPE = DXE_DRIVER
<br>
+ LIBRARY_CLASS = NULL|DXE_DRIVER
<br>
+ CONSTRUCTOR = SmbiosType17LibConstructor
<br>
+ DESTRUCTOR = SmbiosType17LibDestructor
<br>
+
<br>
+[Sources]
<br>
+ SmbiosType17Generator.c
<br>
+
<br>
+[Packages]
<br>
+ MdePkg/MdePkg.dec
<br>
+ MdeModulePkg/MdeModulePkg.dec
<br>
+ EmbeddedPkg/EmbeddedPkg.dec
<br>
+ ArmPlatformPkg/ArmPlatformPkg.dec
<br>
+ DynamicTablesPkg/DynamicTablesPkg.dec
<br>
+
<br>
+[LibraryClasses]
<br>
+ BaseLib
<br>
+ DebugLib
<br>
</blockquote>
</blockquote>
<br>
<br>
<br>
<br>
<br>
</blockquote>
</body>
</html>
<div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links:<p> You receive all messages sent to this group. <p> <a target="_blank" href="https://edk2.groups.io/g/devel/message/95342">View/Reply Online (#95342)</a> | | <a target="_blank" href="https://groups.io/mt/93275299/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a><br> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com]<br> <div width="1" style="color:white;clear:both">_._,_._,_</div>