* [PATCH 1/2] DynamicTablesPkg: Add SMBIOS table generation
@ 2022-08-26 17:37 gmahadevan
2022-08-26 17:37 ` [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator Girish Mahadevan
2022-09-12 14:55 ` [PATCH 1/2] DynamicTablesPkg: Add SMBIOS table generation Sami Mujawar
0 siblings, 2 replies; 11+ messages in thread
From: gmahadevan @ 2022-08-26 17:37 UTC (permalink / raw)
To: devel, Sami Mujawar, Alexei Fedorov
Cc: Samer El-Haj-Mahmoud, Jeff Brasen, Ashish Singhal, Akanksha Jain,
Matteo Carlini, Hemendra Dassanayake, Nick Ramirez,
William Watson, Girish Mahadevan
Modify the DynamicTableManagerDxe driver to install SMBIOS tables in
addition to ACPI tables.
Instead of adding gEfiSmbiosProtocolGuid to the DEPEX list, setup
callback notifications for gEfiSmbiosProtocolGuid and
gEfiAcpiTableProtocolGuid and install the SMBIOS and ACPI tables
in the respective notification ready callback functions.
Add the ability to install multiple SMBIOS tables to the SMBIOS
factory generator similar to ACPI.
Signed-off-by: Girish Mahadevan <gmahadevan@nvidia.com>
---
.../DynamicTableManagerDxe.c | 585 +++++++++++++++++-
.../DynamicTableManagerDxe.inf | 2 +-
.../Include/SmbiosTableGenerator.h | 50 ++
3 files changed, 628 insertions(+), 9 deletions(-)
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
index ed62299f9b..1642d6c387 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
@@ -8,9 +8,11 @@
**/
#include <Library/DebugLib.h>
+#include <Library/UefiLib.h>
#include <Library/PcdLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Protocol/AcpiTable.h>
+#include <Protocol/Smbios.h>
// Module specific include files.
#include <AcpiTableGenerator.h>
@@ -31,6 +33,18 @@ GET_OBJECT_LIST (
CM_STD_OBJ_ACPI_TABLE_INFO
)
+/** This macro expands to a function that retrieves the SMBIOS Table
+ List from the Configuration Manager.
+*/
+GET_OBJECT_LIST (
+ EObjNameSpaceStandard,
+ EStdObjSmbiosTableList,
+ CM_STD_OBJ_SMBIOS_TABLE_INFO
+ )
+
+STATIC VOID *AcpiTableProtocolRegistration;
+STATIC VOID *SmbiosProtocolRegistration;
+
/** A helper function to build and install a single ACPI table.
This is a helper function that invokes the Table generator interface
@@ -478,6 +492,449 @@ VerifyMandatoryTablesArePresent (
return Status;
}
+/** A helper function to build and install an SMBIOS table.
+
+ This is a helper function that invokes the Table generator interface
+ for building an SMBIOS table. It uses the SmbiosProtocol to install the
+ table, then frees the resources allocated for generating it.
+
+ @param [in] TableFactoryProtocol Pointer to the Table Factory Protocol
+ interface.
+ @param [in] CfgMgrProtocol Pointer to the Configuration Manager
+ Protocol Interface.
+ @param [in] SmbiosProtocol Pointer to the SMBIOS protocol.
+ @param [in] SmbiosTableInfo Pointer to the SMBIOS table Info.
+
+ @retval EFI_SUCCESS Success.
+ @retval EFI_INVALID_PARAMETER A parameter is invalid.
+ @retval EFI_NOT_FOUND Required object is not found.
+ @retval EFI_BAD_BUFFER_SIZE Size returned by the Configuration Manager
+ is less than the Object size for the
+ requested object.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+BuildAndInstallSingleSmbiosTable (
+ IN CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *CONST TableFactoryProtocol,
+ IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
+ IN CONST SMBIOS_TABLE_GENERATOR *CONST Generator,
+ IN EFI_SMBIOS_PROTOCOL *SmbiosProtocol,
+ IN CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST SmbiosTableInfo
+ )
+{
+ EFI_STATUS Status;
+ EFI_STATUS Status1;
+ SMBIOS_STRUCTURE *SmbiosTable;
+ EFI_SMBIOS_HANDLE TableHandle;
+
+ SmbiosTable = NULL;
+ Status = Generator->BuildSmbiosTable (
+ Generator,
+ SmbiosTableInfo,
+ CfgMgrProtocol,
+ &SmbiosTable
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to Build Table." \
+ " TableGeneratorId = 0x%x. Status = %r\n",
+ SmbiosTableInfo->TableGeneratorId,
+ Status
+ ));
+ // Free any allocated resources.
+ goto exit_handler;
+ }
+
+ if (SmbiosTable == NULL) {
+ Status = EFI_NOT_FOUND;
+ goto exit_handler;
+ }
+
+ TableHandle = SMBIOS_HANDLE_PI_RESERVED;
+ // Install SMBIOS table
+ Status = SmbiosProtocol->Add (
+ SmbiosProtocol,
+ NULL,
+ &TableHandle,
+ SmbiosTable
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to Install SMBIOS Table. Status = %r\n",
+ Status
+ ));
+ // Free any allocated resources.
+ goto exit_handler;
+ }
+
+ DEBUG ((
+ DEBUG_INFO,
+ "INFO: SMBIOS Table installed. Status = %r\n",
+ Status
+ ));
+
+exit_handler:
+ // Free any resources allocated for generating the tables.
+ if (Generator->FreeTableResources != NULL) {
+ Status1 = Generator->FreeTableResources (
+ Generator,
+ SmbiosTableInfo,
+ CfgMgrProtocol,
+ &SmbiosTable
+ );
+ if (EFI_ERROR (Status1)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to Free Table Resources." \
+ "TableGeneratorId = 0x%x. Status = %r\n",
+ SmbiosTableInfo->TableGeneratorId,
+ Status1
+ ));
+ }
+
+ // Return the first error status in case of failure
+ if (!EFI_ERROR (Status)) {
+ Status = Status1;
+ }
+ }
+
+ return Status;
+}
+
+/** A helper function to build and install multiple SMBIOS tables.
+
+ This is a helper function that invokes the Table generator interface
+ for building SMBIOS tables. It uses the SmbiosProtocol to install the
+ tables, then frees the resources allocated for generating it.
+
+ @param [in] TableFactoryProtocol Pointer to the Table Factory Protocol
+ interface.
+ @param [in] CfgMgrProtocol Pointer to the Configuration Manager
+ Protocol Interface.
+ @param [in] Generator Pointer to the SmbiosTable generator.
+ @param [in] SmbiosProtocol Pointer to the Smbios protocol.
+ @param [in] AcpiTableInfo Pointer to the SMBIOS table Info.
+
+ @retval EFI_SUCCESS Success.
+ @retval EFI_INVALID_PARAMETER A parameter is invalid.
+ @retval EFI_NOT_FOUND Required object is not found.
+ @retval EFI_BAD_BUFFER_SIZE Size returned by the Configuration Manager
+ is less than the Object size for the
+ requested object.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+BuildAndInstallMultipleSmbiosTables (
+ IN CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *CONST TableFactoryProtocol,
+ IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
+ IN CONST SMBIOS_TABLE_GENERATOR *CONST Generator,
+ IN EFI_SMBIOS_PROTOCOL *SmbiosProtocol,
+ IN CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST SmbiosTableInfo
+ )
+{
+ EFI_STATUS Status;
+ EFI_STATUS Status1;
+ SMBIOS_STRUCTURE **SmbiosTable;
+ EFI_SMBIOS_HANDLE TableHandle;
+ UINTN TableCount;
+ UINTN Index;
+
+ TableCount = 0;
+ Status = Generator->BuildSmbiosTableEx (
+ Generator,
+ SmbiosTableInfo,
+ CfgMgrProtocol,
+ &SmbiosTable,
+ &TableCount
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to Build Table." \
+ " TableGeneratorId = 0x%x. Status = %r\n",
+ SmbiosTableInfo->TableGeneratorId,
+ Status
+ ));
+ // Free any allocated resources.
+ goto exit_handler;
+ }
+
+ if ((SmbiosTable == NULL) || (TableCount == 0)) {
+ Status = EFI_NOT_FOUND;
+ DEBUG ((
+ DEBUG_ERROR,
+ "%a: TableCount %u SmbiosTable %p \n",
+ __FUNCTION__,
+ TableCount,
+ SmbiosTable
+ ));
+ goto exit_handler;
+ }
+
+ for (Index = 0; Index < TableCount; Index++) {
+ TableHandle = SMBIOS_HANDLE_PI_RESERVED;
+ // Install SMBIOS table
+ Status = SmbiosProtocol->Add (
+ SmbiosProtocol,
+ NULL,
+ &TableHandle,
+ SmbiosTable[Index]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to Install SMBIOS Table. Status = %r\n",
+ Status
+ ));
+ // Free any allocated resources.
+ goto exit_handler;
+ }
+
+ DEBUG ((
+ DEBUG_INFO,
+ "INFO: SMBIOS Table installed. Status = %r\n",
+ Status
+ ));
+ }
+
+exit_handler:
+ // Free any resources allocated for generating the tables.
+ if (Generator->FreeTableResourcesEx != NULL) {
+ Status1 = Generator->FreeTableResourcesEx (
+ Generator,
+ SmbiosTableInfo,
+ CfgMgrProtocol,
+ &SmbiosTable,
+ TableCount
+ );
+ if (EFI_ERROR (Status1)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to Free Table Resources." \
+ "TableGeneratorId = 0x%x. Status = %r\n",
+ SmbiosTableInfo->TableGeneratorId,
+ Status1
+ ));
+ }
+
+ // Return the first error status in case of failure
+ if (!EFI_ERROR (Status)) {
+ Status = Status1;
+ }
+ }
+
+ return Status;
+}
+
+/** A helper function to invoke a Table generator
+
+ This is a helper function that invokes the Table generator interface
+ for building an SMBIOS table. It uses the SmbiosProtocol to install the
+ table, then frees the resources allocated for generating it.
+
+ @param [in] TableFactoryProtocol Pointer to the Table Factory Protocol
+ interface.
+ @param [in] CfgMgrProtocol Pointer to the Configuration Manager
+ Protocol Interface.
+ @param [in] SmbiosProtocol Pointer to the SMBIOS protocol.
+ @param [in] SmbiosTableInfo Pointer to the SMBIOS table Info.
+
+ @retval EFI_SUCCESS Success.
+ @retval EFI_INVALID_PARAMETER A parameter is invalid.
+ @retval EFI_NOT_FOUND Required object is not found.
+ @retval EFI_BAD_BUFFER_SIZE Size returned by the Configuration Manager
+ is less than the Object size for the
+ requested object.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+BuildAndInstallSmbiosTable (
+ IN CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *CONST TableFactoryProtocol,
+ IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
+ IN EFI_SMBIOS_PROTOCOL *SmbiosProtocol,
+ IN CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST SmbiosTableInfo
+ )
+{
+ EFI_STATUS Status;
+ CONST SMBIOS_TABLE_GENERATOR *Generator;
+
+ ASSERT (TableFactoryProtocol != NULL);
+ ASSERT (CfgMgrProtocol != NULL);
+ ASSERT (SmbiosProtocol != NULL);
+ ASSERT (SmbiosTableInfo != NULL);
+
+ DEBUG ((
+ DEBUG_INFO,
+ "INFO: EStdObjSmbiosTableList: Address = 0x%p," \
+ " TableGeneratorId = 0x%x\n",
+ SmbiosTableInfo,
+ SmbiosTableInfo->TableGeneratorId
+ ));
+
+ Generator = NULL;
+ Status = TableFactoryProtocol->GetSmbiosTableGenerator (
+ TableFactoryProtocol,
+ SmbiosTableInfo->TableGeneratorId,
+ &Generator
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Table Generator not found." \
+ " TableGeneratorId = 0x%x. Status = %r\n",
+ SmbiosTableInfo->TableGeneratorId,
+ Status
+ ));
+ return Status;
+ }
+
+ if (Generator == NULL) {
+ return EFI_NOT_FOUND;
+ }
+
+ DEBUG ((
+ DEBUG_INFO,
+ "INFO: Generator found : %s\n",
+ Generator->Description
+ ));
+
+ if (Generator->BuildSmbiosTableEx != NULL) {
+ Status = BuildAndInstallMultipleSmbiosTables (
+ TableFactoryProtocol,
+ CfgMgrProtocol,
+ Generator,
+ SmbiosProtocol,
+ SmbiosTableInfo
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to find build and install SMBIOS Tables." \
+ " Status = %r\n",
+ Status
+ ));
+ }
+ } else if (Generator->BuildSmbiosTable != NULL) {
+ Status = BuildAndInstallSingleSmbiosTable (
+ TableFactoryProtocol,
+ CfgMgrProtocol,
+ Generator,
+ SmbiosProtocol,
+ SmbiosTableInfo
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to find build and install SMBIOS Table." \
+ " Status = %r\n",
+ Status
+ ));
+ }
+ } else {
+ Status = EFI_INVALID_PARAMETER;
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Table Generator does not implement the" \
+ "SMBIOS_TABLE_GENERATOR_BUILD_TABLE interface." \
+ " TableGeneratorId = 0x%x. Status = %r\n",
+ SmbiosTableInfo->TableGeneratorId,
+ Status
+ ));
+ }
+
+ return Status;
+}
+
+/** Generate and install SMBIOS tables.
+
+ The function gathers the information necessary for installing the
+ SMBIOS tables from the Configuration Manager, invokes the generators
+ and installs them (via BuildAndInstallAcpiTable).
+
+ @param [in] TableFactoryProtocol Pointer to the Table Factory Protocol
+ interface.
+ @param [in] CfgMgrProtocol Pointer to the Configuration Manager
+ Protocol Interface.
+
+ @retval EFI_SUCCESS Success.
+ @retval EFI_NOT_FOUND If a mandatory table or a generator is not found.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+ProcessSmbiosTables (
+ IN CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *CONST TableFactoryProtocol,
+ IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol
+ )
+{
+ EFI_STATUS Status;
+ EFI_SMBIOS_PROTOCOL *SmbiosProtocol;
+ CM_STD_OBJ_SMBIOS_TABLE_INFO *SmbiosTableInfo;
+ UINT32 SmbiosTableCount;
+ UINT32 Idx;
+
+ Status = gBS->LocateProtocol (&gEfiSmbiosProtocolGuid, NULL, (VOID **)&SmbiosProtocol);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Could not locate SMBIOS protocol. %r\n", Status));
+ return Status;
+ }
+
+ Status = GetEStdObjSmbiosTableList (
+ CfgMgrProtocol,
+ CM_NULL_TOKEN,
+ &SmbiosTableInfo,
+ &SmbiosTableCount
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to get SMBIOS Table List. Status = %r\n",
+ Status
+ ));
+ return Status;
+ }
+
+ if (SmbiosTableCount == 0) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: EStdObjSmbiosTableList: SmbiosTableCount = %d\n",
+ SmbiosTableCount
+ ));
+ return EFI_NOT_FOUND;
+ }
+
+ DEBUG ((
+ DEBUG_INFO,
+ "INFO: EStdObjSmbiosTableList: SmbiosTableCount = %d\n",
+ SmbiosTableCount
+ ));
+
+ for (Idx = 0; Idx < SmbiosTableCount; Idx++) {
+ Status = BuildAndInstallSmbiosTable (
+ TableFactoryProtocol,
+ CfgMgrProtocol,
+ SmbiosProtocol,
+ &SmbiosTableInfo[Idx]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to install SMBIOS Table." \
+ " Id = %u Status = %r\n",
+ SmbiosTableInfo[Idx].TableGeneratorId,
+ Status
+ ));
+ }
+ }
+
+ return Status;
+}
+
/** Generate and install ACPI tables.
The function gathers the information necessary for installing the
@@ -664,11 +1121,11 @@ ProcessAcpiTables (
@retval EFI_NOT_FOUND Required interface/object was not found.
@retval EFI_INVALID_PARAMETER Some parameter is incorrect/invalid.
**/
-EFI_STATUS
-EFIAPI
-DynamicTableManagerDxeInitialize (
- IN EFI_HANDLE ImageHandle,
- IN EFI_SYSTEM_TABLE *SystemTable
+STATIC
+VOID
+AcpiTableProtocolReady (
+ IN EFI_EVENT Event,
+ IN VOID *Context
)
{
EFI_STATUS Status;
@@ -689,7 +1146,7 @@ DynamicTableManagerDxeInitialize (
" Status = %r\n",
Status
));
- return Status;
+ return;
}
// Locate the Configuration Manager for the Platform
@@ -704,7 +1161,7 @@ DynamicTableManagerDxeInitialize (
"ERROR: Failed to find Configuration Manager protocol. Status = %r\n",
Status
));
- return Status;
+ return;
}
Status = GetCgfMgrInfo (CfgMgrProtocol, &CfgMfrInfo);
@@ -714,7 +1171,7 @@ DynamicTableManagerDxeInitialize (
"ERROR: Failed to get Configuration Manager info. Status = %r\n",
Status
));
- return Status;
+ return;
}
DEBUG ((
@@ -737,6 +1194,118 @@ DynamicTableManagerDxeInitialize (
Status
));
}
+}
+
+STATIC
+VOID
+SmbiosProtocolReady (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ EFI_STATUS Status;
+ EDKII_CONFIGURATION_MANAGER_PROTOCOL *CfgMgrProtocol;
+ CM_STD_OBJ_CONFIGURATION_MANAGER_INFO *CfgMfrInfo;
+ EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *TableFactoryProtocol;
+
+ // Locate the Dynamic Table Factory
+ Status = gBS->LocateProtocol (
+ &gEdkiiDynamicTableFactoryProtocolGuid,
+ NULL,
+ (VOID **)&TableFactoryProtocol
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to find Dynamic Table Factory protocol." \
+ " Status = %r\n",
+ Status
+ ));
+ return;
+ }
+
+ // Locate the Configuration Manager for the Platform
+ Status = gBS->LocateProtocol (
+ &gEdkiiConfigurationManagerProtocolGuid,
+ NULL,
+ (VOID **)&CfgMgrProtocol
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to find Configuration Manager protocol. Status = %r\n",
+ Status
+ ));
+ return;
+ }
+
+ Status = GetCgfMgrInfo (CfgMgrProtocol, &CfgMfrInfo);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to get Configuration Manager info. Status = %r\n",
+ Status
+ ));
+ return;
+ }
+
+ DEBUG ((
+ DEBUG_INFO,
+ "INFO: Configuration Manager Version = 0x%x, OemID = %c%c%c%c%c%c\n",
+ CfgMfrInfo->Revision,
+ CfgMfrInfo->OemId[0],
+ CfgMfrInfo->OemId[1],
+ CfgMfrInfo->OemId[2],
+ CfgMfrInfo->OemId[3],
+ CfgMfrInfo->OemId[4],
+ CfgMfrInfo->OemId[5]
+ ));
+
+ Status = ProcessSmbiosTables (TableFactoryProtocol, CfgMgrProtocol);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: SMBIOS Table processing failure. Status = %r\n",
+ Status
+ ));
+ }
+}
+
+EFI_STATUS
+EFIAPI
+DynamicTableManagerDxeInitialize (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+ EFI_EVENT AcpiEvent;
+ EFI_EVENT SmbiosEvent;
+
+ AcpiEvent = EfiCreateProtocolNotifyEvent (
+ &gEfiAcpiTableProtocolGuid,
+ TPL_CALLBACK,
+ AcpiTableProtocolReady,
+ NULL,
+ &AcpiTableProtocolRegistration
+ );
+ if (AcpiEvent == NULL) {
+ DEBUG ((DEBUG_ERROR, "%a: Failed to ACPI create protocol event\r\n", __FUNCTION__));
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ SmbiosEvent = EfiCreateProtocolNotifyEvent (
+ &gEfiSmbiosProtocolGuid,
+ TPL_CALLBACK,
+ SmbiosProtocolReady,
+ NULL,
+ &SmbiosProtocolRegistration
+ );
+ if (SmbiosEvent == NULL) {
+ DEBUG ((DEBUG_ERROR, "%a: Failed to SMBIOS create protocol event\r\n", __FUNCTION__));
+ gBS->CloseEvent (AcpiEvent);
+ return EFI_OUT_OF_RESOURCES;
+ }
return Status;
}
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
index 028c3d413c..ccf58f6099 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
@@ -36,12 +36,12 @@
[Protocols]
gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
+ gEfiSmbiosProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEdkiiConfigurationManagerProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEdkiiDynamicTableFactoryProtocolGuid # PROTOCOL ALWAYS_CONSUMED
[Depex]
- gEfiAcpiTableProtocolGuid AND
gEdkiiConfigurationManagerProtocolGuid AND
gEdkiiDynamicTableFactoryProtocolGuid
diff --git a/DynamicTablesPkg/Include/SmbiosTableGenerator.h b/DynamicTablesPkg/Include/SmbiosTableGenerator.h
index 934d56c90d..7bf6300d90 100644
--- a/DynamicTablesPkg/Include/SmbiosTableGenerator.h
+++ b/DynamicTablesPkg/Include/SmbiosTableGenerator.h
@@ -168,6 +168,48 @@ typedef EFI_STATUS (*SMBIOS_TABLE_GENERATOR_FREE_TABLE) (
IN SMBIOS_STRUCTURE **Table
);
+/** This function pointer describes the interface to SMBIOS table build
+ functions provided by the SMBIOS table generator and called by the
+ Table Manager to build an SMBIOS table.
+
+ @param [in] Generator 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 generated SMBIOS table.
+
+ @return EFI_SUCCESS If the table is generated successfully or other
+ failure codes as returned by the generator.
+**/
+typedef EFI_STATUS (*SMBIOS_TABLE_GENERATOR_BUILD_TABLEEX) (
+ IN CONST SMBIOS_TABLE_GENERATOR *Generator,
+ 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
+ );
+
+/** This function pointer describes the interface to used by the
+ Table Manager to give the generator an opportunity to free
+ any resources allocated for building the SMBIOS table.
+
+ @param [in] Generator 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 [in] Table Pointer to the generated SMBIOS table.
+
+ @return EFI_SUCCESS If freed successfully or other failure codes
+ as returned by the generator.
+**/
+typedef EFI_STATUS (*SMBIOS_TABLE_GENERATOR_FREE_TABLEEX) (
+ IN CONST SMBIOS_TABLE_GENERATOR *Generator,
+ IN CONST CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST SmbiosTableInfo,
+ IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
+ IN SMBIOS_STRUCTURE ***Table,
+ IN CONST UINTN TableCount
+ );
+
/** The SMBIOS_TABLE_GENERATOR structure provides an interface that the
Table Manager can use to invoke the functions to build SMBIOS tables.
*/
@@ -189,6 +231,14 @@ typedef struct SmbiosTableGenerator {
allocated for building the SMBIOS table.
*/
SMBIOS_TABLE_GENERATOR_FREE_TABLE FreeTableResources;
+
+ /// SMBIOS table extended build function pointer.
+ SMBIOS_TABLE_GENERATOR_BUILD_TABLEEX BuildSmbiosTableEx;
+
+ /** The function to free any resources
+ allocated for building the SMBIOS table.
+ */
+ SMBIOS_TABLE_GENERATOR_FREE_TABLEEX FreeTableResourcesEx;
} SMBIOS_TABLE_GENERATOR;
/** Register SMBIOS table factory generator.
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator
2022-08-26 17:37 [PATCH 1/2] DynamicTablesPkg: Add SMBIOS table generation gmahadevan
@ 2022-08-26 17:37 ` Girish Mahadevan
2022-09-12 14:57 ` Sami Mujawar
2022-09-12 14:55 ` [PATCH 1/2] DynamicTablesPkg: Add SMBIOS table generation Sami Mujawar
1 sibling, 1 reply; 11+ messages in thread
From: Girish Mahadevan @ 2022-08-26 17:37 UTC (permalink / raw)
To: devel, Sami Mujawar, Alexei Fedorov
Cc: Samer El-Haj-Mahmoud, Jeff Brasen, Ashish Singhal, Akanksha Jain,
Matteo Carlini, Hemendra Dassanayake, Nick Ramirez,
William Watson, Girish Mahadevan
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 {
+ /** 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;
+} 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>
+
+// Module specific include files.
+#include <ConfigurationManagerObject.h>
+#include <ConfigurationManagerHelper.h>
+#include <Protocol/ConfigurationManagerProtocol.h>
+#include <Protocol/Smbios.h>
+#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
+ 0, // MemoryErrorInformationHandle
+ 0xFFFF, // TotalWidth
+ 0xFFFF, // DataWIdth
+ 0x7FFF, // Size (always use Extended Size)
+ MemoryTypeUnknown, // FormFactor
+ 0xFF, // DeviceSet
+ 1, // Device Locator
+ 2, // Bank Locator
+ MemoryTypeSdram, // MemoryType
+ { // TypeDetail
+ 0
+ },
+ 0xFFFF, // Speed (Use Extended Speed)
+ 0, // Manufacturer
+ // (Unused Use ModuleManufactuerId)
+ 3, // SerialNumber
+ 4, // AssetTag
+ 0, // PartNumber
+ // (Unused Use ModuleProductId)
+ 0, // Attributes
+ 0, // ExtendedSize
+ 2000, // ConfiguredMemoryClockSpeed
+ 0, // MinimumVoltage
+ 0, // MaximumVoltage
+ 0, // ConfiguredVoltage
+ MemoryTechnologyDram, // MemoryTechnology
+ { // MemoryOperatingModeCapability
+ .Uint16 = 0x000
+ },
+ 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";
+
+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);
+ 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;
+ }
+
+ 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
+ );
+ if (SmbiosRecord == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto exit;
+ }
+
+ CopyMem (SmbiosRecord, &MemDeviceInfoTemplate, sizeof (SMBIOS_TABLE_TYPE17));
+ SmbiosRecord->ExtendedSize = MemoryDevicesInfo[Index].Size;
+ 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);
+ 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
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator
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 ` [edk2-devel] " Chang, Abner
2022-10-04 22:43 ` Girish Mahadevan
0 siblings, 2 replies; 11+ messages in thread
From: Sami Mujawar @ 2022-09-12 14:57 UTC (permalink / raw)
To: Girish Mahadevan, devel, Alexei Fedorov
Cc: Samer El-Haj-Mahmoud, Jeff Brasen, Ashish Singhal, Akanksha Jain,
Matteo Carlini, Hemendra Dassanayake, Nick Ramirez,
William Watson, Akanksha Jain, nd@arm.com
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.
[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://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c#L476
and
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c#L516.
> + 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://edk2.groups.io/g/devel/message/93651
> + 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator
2022-09-12 14:57 ` Sami Mujawar
@ 2022-09-13 3:00 ` Chang, Abner
2022-09-14 15:34 ` Sami Mujawar
2022-10-04 22:43 ` Girish Mahadevan
1 sibling, 1 reply; 11+ messages in thread
From: Chang, Abner @ 2022-09-13 3:00 UTC (permalink / raw)
To: devel@edk2.groups.io, sami.mujawar@arm.com, Girish Mahadevan,
Alexei Fedorov
Cc: Samer El-Haj-Mahmoud, Jeff Brasen, Ashish Singhal, Akanksha Jain,
Matteo Carlini, Hemendra Dassanayake, Nick Ramirez,
William Watson, nd@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
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator
2022-09-13 3:00 ` [edk2-devel] " Chang, Abner
@ 2022-09-14 15:34 ` Sami Mujawar
2022-09-15 15:19 ` Chang, Abner
0 siblings, 1 reply; 11+ messages in thread
From: Sami Mujawar @ 2022-09-14 15:34 UTC (permalink / raw)
To: Chang, Abner, devel@edk2.groups.io, Girish Mahadevan,
Alexei Fedorov
Cc: Samer El-Haj-Mahmoud, Jeff Brasen (jbrasen@nvidia.com),
Ashish Singhal, Akanksha Jain, Matteo Carlini,
Hemendra Dassanayake, Nick Ramirez, William Watson, nd
Hi Abner,
Please see my response inline marked [SAMI].
Regards,
Sami Mujawar
On 13/09/2022, 04:00, "Chang, Abner" <Abner.Chang@amd.com> wrote:
[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
...
> > +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.
[SAMI] It would certainly be very good to have a common codebase across architectures. We welcome contribution from community members towards this effort.
So my question is what should we do if non-ARM platforms would like to leverage this library?
[SAMI] I think we could define the SMBIOS specific objects in a separate namespace ID e.g. 1010b - SMBIOS Objects , see https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Include/ConfigurationManagerObject.h#L30-L34
We can then define the SMBIOS objects as SMBIOS namespace objects. However, I would like to avoid duplicating any information between the ARM namespace objects and SMBIOS namespace objects (e.g. information about CPU, Cache, etc.).
I have some initial thoughts on how this could be done by introducing an object mapper. However, I would first like to understand if you intend to use the Dynamic SMBIOS support only or you will use the Dynamic ACPI support as well?
[/SAMI]
Thanks
Abner
> > + if (EFI_ERROR (Status)) {
> > + DEBUG ((
> > + DEBUG_ERROR,
> > + "Failed to get Memory Devices CM Object %r\n",
> > + Status
> > + ));
> > + return Status;
> > + }
...
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator
2022-09-14 15:34 ` Sami Mujawar
@ 2022-09-15 15:19 ` Chang, Abner
0 siblings, 0 replies; 11+ messages in thread
From: Chang, Abner @ 2022-09-15 15:19 UTC (permalink / raw)
To: Sami Mujawar, devel@edk2.groups.io, Girish Mahadevan,
Alexei Fedorov
Cc: Samer El-Haj-Mahmoud, Jeff Brasen (jbrasen@nvidia.com),
Ashish Singhal, Akanksha Jain, Matteo Carlini,
Hemendra Dassanayake, Nick Ramirez, William Watson, nd
[AMD Official Use Only - General]
> -----Original Message-----
> From: Sami Mujawar <Sami.Mujawar@arm.com>
> Sent: Wednesday, September 14, 2022 11:35 PM
> To: Chang, Abner <Abner.Chang@amd.com>; devel@edk2.groups.io; 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) <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 <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17
> Table generator
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Abner,
>
> Please see my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> On 13/09/2022, 04:00, "Chang, Abner" <Abner.Chang@amd.com> wrote:
>
> [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
> ...
>
> > > +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.
> [SAMI] It would certainly be very good to have a common codebase across
> architectures. We welcome contribution from community members towards this
> effort.
> So my question is what should we do if non-ARM platforms would like to
> leverage this library?
> [SAMI] I think we could define the SMBIOS specific objects in a separate
> namespace ID e.g. 1010b - SMBIOS Objects , see
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> om%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FDynamicTablesPkg%2FInclude
> %2FConfigurationManagerObject.h%23L30-
> L34&data=05%7C01%7CAbner.Chang%40amd.com%7C425be8c5f1464596
> 4ada08da9666aa5f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6
> 37987664940879922%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAi
> LCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&am
> p;sdata=trKkXJe7yDBkoKIlSk5kCI0tZ6nG573wXqORHfd5y2o%3D&reserved=
> 0
> We can then define the SMBIOS objects as SMBIOS namespace objects.
[Abner]
This sounds good. We can define SMBIOS name space and the corresponding object in SmbiosNameSpaceObjects.h under \Include. So platform has to install EDKII_CONFIGURATION_MANAGER_PROTOCOL and returns the SMBIOS object, right?
> However, I would like to avoid duplicating any information between the ARM
> namespace objects and SMBIOS namespace objects (e.g. information about CPU,
> Cache, etc.).
> I have some initial thoughts on how this could be done by introducing an object
> mapper.
[Abner]
I think you refer to have SMBIOS namespace object as the generic one, but introduce a wrapper on it for the processor arch specific information?
However, I would first like to understand if you intend to use the
> Dynamic SMBIOS support only or you will use the Dynamic ACPI support as well?
[ Abner]
I am not sure if we will also use Dynamic ACPI table at the moment but that would be possible. The current use case I can think of is to build up SMBIOS 42h using dynamic SMBIOS table generator.
This also brings another question, the current DynamicTableFactoryDxe pulls in ACPI/SMBIOS/DT table generator functions; what if the platform only uses SMBIOS functionality? Pull in ACPI/DT code into DynamicTableFactoryDxe seems redundant. I think we can consider to make AcpiTableFactory/DeviceTreeTableFactory/SmbiosTableFactor as libraries, use NULL instances (empty function for Get/Register/Deregister generator) as default for DynamicTableFactoryDxe. Platform can pull in the library that has the implementation into build on demand. Not for now but we can do it later if you think this makes sense.
BTW what is the 'E' prefix to "Arm" means? E.g. Get'E'ArmObjMemoryDeviceInfo
Thanks
Abner
> [/SAMI]
> Thanks
> Abner
>
> > > + if (EFI_ERROR (Status)) {
> > > + DEBUG ((
> > > + DEBUG_ERROR,
> > > + "Failed to get Memory Devices CM Object %r\n",
> > > + Status
> > > + ));
> > > + return Status;
> > > + }
> ...
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator
2022-09-12 14:57 ` Sami Mujawar
2022-09-13 3:00 ` [edk2-devel] " Chang, Abner
@ 2022-10-04 22:43 ` Girish Mahadevan
2022-10-18 15:36 ` [edk2-devel] " Sami Mujawar
1 sibling, 1 reply; 11+ messages in thread
From: Girish Mahadevan @ 2022-10-04 22:43 UTC (permalink / raw)
To: Sami Mujawar, devel, Alexei Fedorov
Cc: Samer El-Haj-Mahmoud, Jeff Brasen, Ashish Singhal, Akanksha Jain,
Matteo Carlini, Hemendra Dassanayake, Nick Ramirez,
William Watson, nd@arm.com
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
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator
2022-10-04 22:43 ` Girish Mahadevan
@ 2022-10-18 15:36 ` Sami Mujawar
2023-01-28 0:09 ` Girish Mahadevan
0 siblings, 1 reply; 11+ messages in thread
From: Sami Mujawar @ 2022-10-18 15:36 UTC (permalink / raw)
To: devel, gmahadevan, Alexei Fedorov, Pierre Gondois, Abner.Chang
Cc: Samer El-Haj-Mahmoud, Jeff Brasen, Ashish Singhal, Akanksha Jain,
Matteo Carlini, Hemendra Dassanayake, Nick Ramirez,
William Watson, nd@arm.com
[-- Attachment #1: Type: text/plain, Size: 35371 bytes --]
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
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?
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
>
>
>
>
>
[-- Attachment #2: Type: text/html, Size: 70864 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator
2022-10-18 15:36 ` [edk2-devel] " Sami Mujawar
@ 2023-01-28 0:09 ` Girish Mahadevan
2023-03-08 8:18 ` Sami Mujawar
0 siblings, 1 reply; 11+ messages in thread
From: Girish Mahadevan @ 2023-01-28 0:09 UTC (permalink / raw)
To: devel, sami.mujawar, Alexei Fedorov, Pierre Gondois, Abner.Chang
Cc: Samer El-Haj-Mahmoud, Jeff Brasen, Ashish Singhal, Akanksha Jain,
Matteo Carlini, Hemendra Dassanayake, Nick Ramirez,
William Watson, nd@arm.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&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
>>
>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg: Add Smbios Type17 Table generator
2023-01-28 0:09 ` Girish Mahadevan
@ 2023-03-08 8:18 ` Sami Mujawar
0 siblings, 0 replies; 11+ messages in thread
From: Sami Mujawar @ 2023-03-08 8:18 UTC (permalink / raw)
To: Girish Mahadevan, devel, Alexei Fedorov, Pierre Gondois,
Abner.Chang
Cc: Samer El-Haj-Mahmoud, Jeff Brasen, Ashish Singhal, Akanksha Jain,
Matteo Carlini, Hemendra Dassanayake, Nick Ramirez,
William Watson, nd@arm.com, Jose Marinho
Hi Girish,
Apologies for the delay in reply.
Please find my response marked inline as [SAMI].
Regards,
Sami Mujawar
On 28/01/2023 12:09 am, Girish Mahadevan wrote:
> Hi Sami
>
> Sorry for the long silence. I've sent you v2 of the patch (only
> framework not the Type17) which includes a link to a github branch.
>
> Few more comments inline , prefixed by [GM].
>
> Best Regards
> Girish
>
> On 10/18/2022 9:36 AM, Sami Mujawar via groups.io wrote:
>> *External email: Use caution opening links or attachments*
>>
>>
>> Hi Girish,
>>
>> Please find my response inline marked [SAMI].
>>
>> Regards,
>>
>> Sami Mujawar
>>
>> On 04/10/2022 11:43 pm, Girish Mahadevan via groups.io wrote:
>>> Hello Sami
>>>
>>> Thank you so much for your review, I apologize for the late response.
>>>
>>> My comment in line about the handle manager [GM].
>>>
>>> Best Regards
>>> Girish
>>>
>>> On 9/12/2022 8:57 AM, Sami Mujawar wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> Hi Girish,
>>>>
>>>> Thank you for this patch and for the effort for bringing forward
>>>> dynamic
>>>> SMBIOS generation.
>>>>
>>>> Please find my feedback inline marked [SAMI].
>>>>
>>>> Regards,
>>>>
>>>> Sami Mujawar
>>>>
>>>> On 26/08/2022 06:37 pm, Girish Mahadevan wrote:
>>>>> Add a new CM object to describe memory devices and setup a new
>>>>> Generator Library for SMBIOS Type17 table.
>>>>>
>>>>> Signed-off-by: Girish Mahadevan<gmahadevan@nvidia.com>
>>>>> ---
>>>>> .../Include/ArmNameSpaceObjects.h | 59 +++
>>>>> .../SmbiosType17Lib/SmbiosType17Generator.c | 338
>>>>> ++++++++++++++++++
>>>>> .../SmbiosType17Lib/SmbiosType17LibArm.inf | 32 ++
>>>>> 3 files changed, 429 insertions(+)
>>>>> create mode 100644
>>>>> DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Generator.c
>>>>> create mode 100644
>>>>> DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17LibArm.inf
>>>>>
>>>>> diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
>>>>> b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
>>>>> index 102e0f96be..199a19e997 100644
>>>>> --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
>>>>> +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
>>>>> @@ -63,6 +63,7 @@ typedef enum ArmObjectID {
>>>>> EArmObjPciInterruptMapInfo, ///< 39 - Pci Interrupt
>>>>> Map Info
>>>>> EArmObjRmr, ///< 40 - Reserved Memory
>>>>> Range Node
>>>>> EArmObjMemoryRangeDescriptor, ///< 41 - Memory Range
>>>>> Descriptor
>>>>> + EArmObjMemoryDeviceInfo, ///< 42 - Memory Device
>>>>> Information
>>>>> EArmObjMax
>>>>> } EARM_OBJECT_ID;
>>>>>
>>>>> @@ -1070,6 +1071,64 @@ typedef struct CmArmRmrDescriptor {
>>>>> UINT64 Length;
>>>>> } CM_ARM_MEMORY_RANGE_DESCRIPTOR;
>>>>>
>>>>> +/** A structure that describes the physical memory device.
>>>>> +
>>>>> + The physical memory devices on the system are described by this
>>>>> object.
>>>>> +
>>>>> + SMBIOS Specification v3.5.0 Type17
>>>>> +
>>>>> + ID: EArmObjMemoryDeviceInfo,
>>>>> +*/
>>>>> +typedef struct CmArmMemoryDeviceInfo {
>>>> [SAMI] I think we may need a Token pointing to the Type 16 object so
>>>> that the Physical Memory Array Handle can be setup, see my comment
>>>> below
>>>> about the HandleManager.
>>>>> + /** Size of the device.
>>>>> + Size of the device in bytes.
>>>>> + */
>>>>> + UINT64 Size;
>>>>> +
>>>>> + /** Device Set */
>>>>> + UINT8 DeviceSet;
>>>>> +
>>>>> + /** Speed of the device
>>>>> + Speed of the device in MegaTransfers/second.
>>>>> + */
>>>>> + UINT32 Speed;
>>>>> +
>>>>> + /** Serial Number of device */
>>>>> + CHAR8 *SerialNum;
>>>>> +
>>>>> + /** AssetTag identifying the device */
>>>>> + CHAR8 *AssetTag;
>>>>> +
>>>>> + /** Device Locator String for the device.
>>>>> + String that describes the slot or position of the device on
>>>>> the board.
>>>>> + */
>>>>> + CHAR8 *DeviceLocator;
>>>>> +
>>>>> + /** Bank locator string for the device.
>>>>> + String that describes the bank where the device is located.
>>>>> + */
>>>>> + CHAR8 *BankLocator;
>>>>> +
>>>>> + /** Firmware version of the memory device */
>>>>> + CHAR8 *FirmwareVersion;
>>>>> +
>>>>> + /** Manufacturer Id.
>>>>> + 2 byte Manufacturer Id as per JEDEC Standard JEP106AV
>>>>> + */
>>>>> + UINT16 ModuleManufacturerId;
>>>>> +
>>>>> + /** Manufacturer Product Id
>>>>> + 2 byte Manufacturer Id as designated by Manufacturer.
>>>>> + */
>>>>> + UINT16 ModuleProductId;
>>>>> +
>>>>> + /** Device Attributes */
>>>>> + UINT8 Attributes;
>>>>> +
>>>>> + /** Device Configured Voltage in millivolts */
>>>>> + UINT16 ConfiguredVoltage;
>>>> [SAMI] This field does not appear to be used in the generator. If the
>>>> intention is to use this in the future, then it may be better to add
>>>> this at a later stage.
>>>>> +} CM_ARM_MEMORY_DEVICE_INFO;
>>>>> +
>>>>> #pragma pack()
>>>>>
>>>>> #endif // ARM_NAMESPACE_OBJECTS_H_
>>>>> diff --git
>>>>> a/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Generator.c
>>>>> b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Generator.c
>>>>>
>>>>> new file mode 100644
>>>>> index 0000000000..5683ca570f
>>>>> --- /dev/null
>>>>> +++
>>>>> b/DynamicTablesPkg/Library/Smbios/Arm/SmbiosType17Lib/SmbiosType17Generator.c
>>>>> @@ -0,0 +1,338 @@
>>>>> +/** @file
>>>>> + SMBIOS Type17 Table Generator.
>>>>> +
>>>>> + Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights
>>>>> reserved.
>>>>> + Copyright (c) 2020 - 2021, Arm Limited. All rights reserved.<BR>
>>>>> +
>>>>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>> +**/
>>>>> +
>>>>> +#include <Library/BaseLib.h>
>>>>> +#include <Library/BaseMemoryLib.h>
>>>>> +#include <Library/DebugLib.h>
>>>>> +#include <Library/PrintLib.h>
>>>>> +#include <Library/MemoryAllocationLib.h>
>>>>> +#include <Library/SmbiosType17FixupLib.h>
>>>> [SAMI] I could not find SmbiosType17FixupLib.h in this patch
>>>> series. Can
>>>> you check, please?
>>>>> +
>>>>> +// Module specific include files.
>>>>> +#include <ConfigurationManagerObject.h>
>>>>> +#include <ConfigurationManagerHelper.h>
>>>>> +#include <Protocol/ConfigurationManagerProtocol.h>
>>>>> +#include <Protocol/Smbios.h>
>>>> [SAMI] I think Protocol/Smbios.h may not be required in this file. Can
>>>> you check, please?
>>>>> +#include <IndustryStandard/SmBios.h>
>>>>> +
>>>>> +/** This macro expands to a function that retrieves the Memory
>>>>> Device
>>>>> + information from the Configuration Manager.
>>>>> +*/
>>>>> +GET_OBJECT_LIST (
>>>>> + EObjNameSpaceArm,
>>>>> + EArmObjMemoryDeviceInfo,
>>>>> + CM_ARM_MEMORY_DEVICE_INFO
>>>>> + )
>>>>> +
>>>>> +// Default Values for Memory Device
>>>>> +STATIC SMBIOS_TABLE_TYPE17 MemDeviceInfoTemplate = {
>>>>> + { // Hdr
>>>>> + EFI_SMBIOS_TYPE_MEMORY_DEVICE, // Type
>>>>> + sizeof (SMBIOS_TABLE_TYPE17), // Length
>>>>> + 0 // Handle
>>>>> + },
>>>>> + 0, // MemoryArrayHandle
>>>>
>>>> [SAMI] Do you have any thoughts on how the MemoryArrayHandle can be
>>>> setup?
>>>>
>>>> The same applies for the following MemoryErrorInformationHandle field.
>>>>
>>>> I think we need some sort of a HandleManager in DynamicTablesFramework
>>>> that can keep track of the mappings between SMBIOS Objects and Table
>>>> Handles.
>>>>
>>>> e.g. Smbios - HandleManager
>>>>
>>>> +-------------------------------+-------------------------------+
>>>>
>>>> | Object Token | Table
>>>> Handle |
>>>>
>>>> +-------------------------------+-------------------------------+
>>>>
>>>> | Type16Obj_token | Type 16 Table handle |
>>>>
>>>> +-------------------------------+-------------------------------+
>>>>
>>>> ...
>>>>
>>>> - The Type17Object i.e. CM_ARM_MEMORY_DEVICE_INFO can then hold a
>>>> token
>>>> for the Type16Object.
>>>>
>>>> - If Type 17 table is to be installed, DynamicTablemanager shall
>>>> search the SMBIOS table list to see if a Type16 table is requested
>>>> to be
>>>> installed.
>>>>
>>>> - If a Type16 table is present in the list of SMBIOS table to install,
>>>> the Type16 table shall be installed first and an entry is made in the
>>>> Smbios HandleManager to create a mapping of Type16Obj_token <==>
>>>> Type16
>>>> Table Handle.
>>>>
>>>> - The Type17 table can now be built and if a the Type16Object token is
>>>> provided in CM_ARM_MEMORY_DEVICE_INFO, the Smbios HandleManager
>>>> shall be
>>>> searched (using Type16Obj_token) to retrieve the Type16 Table
>>>> handle and
>>>> populate the Type 17 Physical Memory Array Handle field.
>>>>
>>>> I think we may have to experiment a bit before we arrive at the
>>>> correct
>>>> design. However, please do let me know your thoughts on the above.
>>>>
>>>
>>> [GM] I agree with the idea of having a map of CM_OBJECT_TOKENs to
>>> SMBIOS handles. I've added this to the DynamicTableFactoryProtocol.
>>>
>>> However when it comes to actually figuring out and adding the
>>> reference SMBIOS handle We've come up with a slightly different
>>> approach.
>>>
>>> If I understood what you were saying above is:
>>> If we happen to parse a Type17 table
>>> if that Type17 table has a token to a Physical memory array CM_OBJ
>>> if there is an existing Smbios Handle (look up this handle using
>>> the CM_OBJECT_TOKEN)
>>> then use this handle.
>>> else if there is a generator for Type16 registered
>>> call the Type16 generator code first
>>>
>>> IMO this gets a bit difficult to manage. Right now the
>>> DynamicTableManagerDxe walks the array of EStdObjSmbiosTableList CM
>>> objects, finds the generator for each Table, invokes it, gets an
>>> SMBIOS record that it then installs via the SmbiosDxe driver.
>>> It seemed a bit convoluted to call the generator and install an
>>> SMBIOS table while within the generator of another Smbios table.
>>> And if there are layers of dependencies (or multiple dependencies)
>>> it could add to the complexities.
>>>
>>> Instead what we came up with is:
>>>
>>> If we happen to parse a Type17 table
>>> if that Type17 table has a token to a Physical memory array CM_OBJ
>>> if there is an existing Smbios Handle (look up this handle using
>>> the CM_OBJECT_TOKEN )
>>> then use this handle.
>>> else if there is a generator for Type16 Registered
>>> Generate an SMBIOS handle [since SmbiosDxe maintains the
>>> handle DB privately I had to
>>> pick a handle and check if it
>>> clashes with existing records]
>>> Add this SMBIOS handle to the map.
>>> else // No generator present
>>> Proceed without adding any reference
>>>
>>>
>>> Before Adding any SMBIOS table, we check if there is an existing
>>> SMBIOS handle for the CM_OBJECT_TOKEN (the generator will return a
>>> CM_OBJECT_TOKEN for each SMBIOS record created).
>>> If there is an existing SMBIOS handle, pass that in, else pass in
>>> SMBIOS_HANDLE_PI_RESERVED and let SmbiosDxe assign the handle.
>>>
>>> Here is a sample implementation of this approach (it is a WIP, but I
>>> wanted to get your thoughts on it)
>>>
>>> https://github.com/tianocore/edk2/compare/master...gmahadevan:edk2-upstream:RFC/smbios-dyntables
>>>
>>
>> [SAMI] I had a look at the above branch and have the following
>> suggestions:
>>
>> a. To resolve the dependency order, please see my patches for SMBIOS
>> table dispatcher at: https://edk2.groups.io/g/devel/message/95340
>
> [GM]
> Thanks !
> I've setup a new patch/github branch that includes the dispatcher code
> and the SMBIOS string helper patch.
>
> The SMBIOS string helper and dispatcher code look good to me.
>
>>
>> You should be able to apply these patches on your
>> 'edk2-upstream:RFC/smbios-dyntables' branch and enable the dispatcher
>> in ProcessSmbiosTables().
>>
>> e.g. In file
>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c,
>> the SMBIOS table dispatcher can be invoked once it is initialised as
>> shown below.
>>
>> @@ -1007,19 +1007,24 @@ ProcessSmbiosTables (
>> SmbiosTableCount
>> ));
>>
>> + // Initialise the SMBIOS Table Dispatcher state machine.
>> + InitSmbiosTableDispatcher (SmbiosTableInfo, SmbiosTableCount);
>> +
>> for (Idx = 0; Idx < SmbiosTableCount; Idx++) {
>> - Status = BuildAndInstallSmbiosTable (
>> + Status = DispatchSmbiosTable (
>> + SmbiosTableInfo[Idx].TableType,
>> TableFactoryProtocol,
>> CfgMgrProtocol,
>> SmbiosProtocol,
>> - &SmbiosTableInfo[Idx]
>> + SmbiosTableInfo,
>> + SmbiosTableCount
>> );
>> if (EFI_ERROR (Status)) {
>> DEBUG ((
>> DEBUG_ERROR,
>> - "ERROR: Failed to install SMBIOS Table." \
>> - " Id = %u Status = %r\n",
>> - SmbiosTableInfo[Idx].TableGeneratorId,
>> + "ERROR: Failed to dispatch SMBIOS Table for installation." \
>> + " Table Type = %u Status = %r\n",
>> + SmbiosTableInfo[Idx].TableType,
>> Status
>> ));
>> }
>>
>> b. With the SMBIOS dispatcher patch and the handle manager, it should
>> be possible to update the handles for dependent tables. This should
>> remove the need for the handle generation and management.
>>
>> c. With regards to the SMBIOS handle manager, I think it would be
>> better to add CM_OBJECT_ID for the SmbiosCmTokenin SMBIOS_HANDLE_MAP.
>>
>> d. A new SMBIOS object namespace should be defined.
>>
>> e.g Namespace ID 1010b or 1100b - SMBIOS Objects, see
>> https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Include/ConfigurationManagerObject.h#L30-L34
>>
>> With this change, CM_ARM_MEMORY_DEVICE_INFO becomes
>> CM_SMBIOS_MEMORY_DEVICE_INFO
>>
>> Similarly, EArmObjMemoryDeviceInfo becomes
>> ESmbiosObjMemoryDeviceInfo
>>
>> e. With regards to DynamicTableManager.c, I think we should split the
>> ACPI & SMBIOS processing in individual files (e.g. AcpiBuilder.c &
>> SmbiosBuilder.c) under DynamicTableManagerDxe.
>>
>> f. Status appears to be returned uninitialised in
>> DynamicTableManagerDxeInitialize().
>>
>> g. DynamicTablesPkg\Library\Smbios\Arm\SmbiosType17Lib can be moved
>> to DynamicTablesPkg\Library\Smbios\SmbiosType17Lib.
>>
>> I prefer a github branch with the patch as it is very helpful to view
>> the code being reviewed. However, it is difficult to provide
>> feedback. Is it possible to submit a patch for the changes with the
>> link for the guthub branch in the future, please?
>
> [GM]
> I've sent a new version of the patch minus the Type17 code as I wanted
> to just focus on the framework code first.
> I've incorporated the comments from above except for the suggestion on
> using CM_OBJECT_ID instead of the CM_TOKEN for the SmbiosHandleMap.
[SAMI] Sorry for not being clear in my earlier response. I meant we need
CM_OBJECT_ID in addition to the CM_OBJECT_TOKEN. See example below:
typedef struct SmbiosHandleCmObjMap {
ESTD_SMBIOS_TABLE_ID SmbiosTableId;
SMBIOS_HANDLE SmbiosTblHandle;
+CM_OBJECT_ID ObjectId;
CM_OBJECT_TOKEN SmbiosCmToken;
} SMBIOS_HANDLE_MAP;
Otherwise it would not be possible to identify the type of object
pointed by SmbiosCmToken, right?
[/SAMI]
> IMO the CM_TOKEN is better since we can have unique tokens for cases
> where a single SMBIOS CM_OBJECT_ID can have multiple entries
> (type17/type9 etc), let me know your thoughts.
[SAMI] I think I am missing something, can you explain the scenario, please?
>
> Best Regards
> Girish
>
>>
>> I am not sure if we need an edk2-staging branch for this feature. But
>> if you think it would be helpful then please let me know and I will
>> send out a request to create a new feature branch.
>>
>> [/SAMI]
>>
>>> Sorry for the long message, please let me know your thoughts.
>>>
>>> [/GM]
>>>
>>>> [SAMI]
>>>>> + 0, //
>>>>> MemoryErrorInformationHandle
>>>>> + 0xFFFF, // TotalWidth
>>>>> + 0xFFFF, // DataWIdth
>>>>
>>>> [SAMI] I need to find out how these fields should be populated, but
>>>> the
>>>> Annex A, SMBIOS specification version 3.6.0 says the following:
>>>>
>>>> 4.8.4 Total Width is not 0FFFFh (Unknown) if the memory device is
>>>> installed. (Size is not 0.)
>>>>
>>>> 4.8.5 Data Width is not 0FFFFh (Unknown).
>>>>
>>>> Can you explain how this field is used, please?
>>>>
>>>> [/SAMI]
>>>>
>>>>> + 0x7FFF, // Size (always use
>>>>> Extended Size)
>>>>
>>>> [SAMI] I think this field should be set based on the Size.
>>>>
>>>> The spec says "If the size is 32 GB-1 MB or greater, the field
>>>> value is
>>>> 7FFFh and the actual size is stored in the Extended Size field."
>>>>
>>>> I think it would be good to have a static function that encodes the
>>>> size in wither the Size field or the Extended Size field.
>>>>
>>>> e.g. UpdateSmbiosTable17Size (SMBIOS_TABLE_TYPE17 *SmbiosRecord, UINTN
>>>> Size) {
>>>>
>>>> if (Size > 32GB-1MB) {
>>>>
>>>> SmbiosRecord->Size = EncodeSize (xxx);
>>>>
>>>> } else {
>>>>
>>>> SmbiosRecord->ExtendedSize = EncodeExtendedSize (xxx);
>>>>
>>>> }
>>>>
>>>> }
>>>>
>>>> [/SAMI]
>>>>
>>>>> + MemoryTypeUnknown, // FormFactor
>>>>> + 0xFF, // DeviceSet
>>>>> + 1, // Device Locator
>>>>> + 2, // Bank Locator
>>>>> + MemoryTypeSdram, // MemoryType
>>>>> + { // TypeDetail
>>>>> + 0
>>>>> + },
>>>>> + 0xFFFF, // Speed (Use Extended
>>>>> Speed)
>>>> [SAMI] Maybe we need a helper function (similar to
>>>> UpdateSmbiosTable17Size()) for this field as well.
>>>>> + 0, // Manufacturer
>>>>> + // (Unused Use
>>>>> ModuleManufactuerId)
>>>>> + 3, // SerialNumber
>>>>> + 4, // AssetTag
>>>>> + 0, // PartNumber
>>>>> + // (Unused Use
>>>>> ModuleProductId)
>>>>> + 0, // Attributes
>>>>> + 0, // ExtendedSize
>>>>> + 2000, //
>>>>> ConfiguredMemoryClockSpeed
>>>> [SAMI] Should this be provided through CM_ARM_MEMORY_DEVICE_INFO?
>>>>> + 0, // MinimumVoltage
>>>>> + 0, // MaximumVoltage
>>>>> + 0, // ConfiguredVoltage
>>>>> + MemoryTechnologyDram, // MemoryTechnology
>>>> [SAMI] Should this be provided through CM_ARM_MEMORY_DEVICE_INFO?
>>>>> + { //
>>>>> MemoryOperatingModeCapability
>>>>> + .Uint16 = 0x000
>>>>> + },
>>>> [SAMI] I think the above initialisation may not be portable.
>>>>> + 5, // FirmwareVersion
>>>>> + 0, // ModuleManufacturerId
>>>>> + 0, // ModuleProductId
>>>>> + 0, //
>>>>> MemorySubsystemContollerManufacturerId
>>>>> + 0, //
>>>>> MemorySybsystemControllerProductId
>>>>> + 0, // NonVolatileSize
>>>>> + 0, // VolatileSize
>>>>> + 0, // CacheSize
>>>>> + 0, // LogicalSize
>>>>> + 0, // ExtendedSpeed
>>>>> + 0, //
>>>>> ExtendedConfiguredMemorySpeed
>>>>> +};
>>>>> +
>>>>> +STATIC CHAR8 UnknownStr[] = "Unknown\0";
>>>> [SAMI] Would it be possible to add the CONST qualifier, please?
>>>>> +
>>>>> +STATIC
>>>>> +EFI_STATUS
>>>>> +EFIAPI
>>>>> +FreeSmbiosType17TableEx (
>>>>> + IN CONST SMBIOS_TABLE_GENERATOR *CONST This,
>>>>> + IN CONST CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST
>>>>> SmbiosTableInfo,
>>>>> + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST
>>>>> CfgMgrProtocol,
>>>>> + IN OUT SMBIOS_STRUCTURE ***CONST Table,
>>>>> + IN CONST UINTN TableCount
>>>>> + )
>>>>> +{
>>>>> + return EFI_SUCCESS;
>>>>> +}
>>>>> +
>>>>> +/** Construct SMBIOS Type17 Table describing memory devices.
>>>>> +
>>>>> + If this function allocates any resources then they must be freed
>>>>> + in the FreeXXXXTableResources function.
>>>>> +
>>>>> + @param [in] This Pointer to the SMBIOS table
>>>>> generator.
>>>>> + @param [in] SmbiosTableInfo Pointer to the SMBIOS table
>>>>> information.
>>>>> + @param [in] CfgMgrProtocol Pointer to the Configuration Manager
>>>>> + Protocol interface.
>>>>> + @param [out] Table Pointer to the SMBIOS table.
>>>>> +
>>>>> + @retval EFI_SUCCESS Table generated successfully.
>>>>> + @retval EFI_BAD_BUFFER_SIZE The size returned by the
>>>>> Configuration
>>>>> + Manager is less than the Object
>>>>> size for
>>>>> + the requested object.
>>>>> + @retval EFI_INVALID_PARAMETER A parameter is invalid.
>>>>> + @retval EFI_NOT_FOUND Could not find information.
>>>>> + @retval EFI_OUT_OF_RESOURCES Could not allocate memory.
>>>>> + @retval EFI_UNSUPPORTED Unsupported configuration.
>>>>> +**/
>>>>> +STATIC
>>>>> +EFI_STATUS
>>>>> +EFIAPI
>>>>> +BuildSmbiosType17TableEx (
>>>>> + IN CONST SMBIOS_TABLE_GENERATOR *This,
>>>>> + IN CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST SmbiosTableInfo,
>>>>> + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST
>>>>> CfgMgrProtocol,
>>>>> + OUT SMBIOS_STRUCTURE ***Table,
>>>>> + OUT UINTN *CONST TableCount
>>>>> + )
>>>>> +{
>>>>> + EFI_STATUS Status;
>>>>> + UINT32 NumMemDevices;
>>>>> + SMBIOS_STRUCTURE **TableList;
>>>>> + CM_ARM_MEMORY_DEVICE_INFO *MemoryDevicesInfo;
>>>>> + UINTN Index;
>>>>> + UINTN SerialNumLen;
>>>>> + CHAR8 *SerialNum;
>>>>> + UINTN AssetTagLen;
>>>>> + CHAR8 *AssetTag;
>>>>> + UINTN DeviceLocatorLen;
>>>>> + CHAR8 *DeviceLocator;
>>>>> + UINTN BankLocatorLen;
>>>>> + CHAR8 *BankLocator;
>>>>> + UINTN FirmwareVersionLen;
>>>>> + CHAR8 *FirmwareVersion;
>>>>> + CHAR8 *OptionalStrings;
>>>>> + SMBIOS_TABLE_TYPE17 *SmbiosRecord;
>>>>> +
>>>>> + ASSERT (This != NULL);
>>>>> + ASSERT (SmbiosTableInfo != NULL);
>>>>> + ASSERT (CfgMgrProtocol != NULL);
>>>>> + ASSERT (Table != NULL);
>>>>> + ASSERT (TableCount != NULL);
>>>>> + ASSERT (SmbiosTableInfo->TableGeneratorId == This->GeneratorID);
>>>>> +
>>>>> + DEBUG ((DEBUG_ERROR, "%a : Start \n", __FUNCTION__));
>>>>> + *Table = NULL;
>>>>> + Status = GetEArmObjMemoryDeviceInfo (
>>>>> + CfgMgrProtocol,
>>>>> + CM_NULL_TOKEN,
>>>>> + &MemoryDevicesInfo,
>>>>> + &NumMemDevices
>>>>> + );
>>>>> + if (EFI_ERROR (Status)) {
>>>>> + DEBUG ((
>>>>> + DEBUG_ERROR,
>>>>> + "Failed to get Memory Devices CM Object %r\n",
>>>>> + Status
>>>>> + ));
>>>>> + return Status;
>>>>> + }
>>>>> +
>>>>> + TableList = (SMBIOS_STRUCTURE **)AllocateZeroPool (sizeof
>>>>> (SMBIOS_STRUCTURE *) * NumMemDevices);
>>>> [SAMI] The memory allocated here should be freed in
>>>> FreeSmbiosType17TableEx.
>>>>> + if (TableList == NULL) {
>>>>> + DEBUG ((DEBUG_ERROR, "Failed to alloc memory for %u devices
>>>>> table\n"));
>>>>> + Status = EFI_OUT_OF_RESOURCES;
>>>>> + goto exit;
>>>>> + }
>>>>> +
>>>>> + for (Index = 0; Index < NumMemDevices; Index++) {
>>>>> + if (MemoryDevicesInfo[Index].SerialNum != NULL) {
>>>>> + SerialNumLen = AsciiStrLen
>>>>> (MemoryDevicesInfo[Index].SerialNum);
>>>>> + SerialNum = MemoryDevicesInfo[Index].SerialNum;
>>>>> + } else {
>>>>> + SerialNumLen = AsciiStrLen (UnknownStr);
>>>>> + SerialNum = UnknownStr;
>>>>
>>>> [SAMI] If the serial number is not provided, then should the string
>>>> field be set to 0?
>>>>
>>>> See Section 6.1.3, SMBIOS Spec Version 3.6.0 which states
>>>>
>>>> "If a string field references no string, a null (0) is placed in that
>>>> string field."
>>>>
>>>> [/SAMI]
>>>>
>>>>> + }
>>>>> +
>>>>> + if (MemoryDevicesInfo[Index].AssetTag != NULL) {
>>>>> + AssetTagLen = AsciiStrLen (MemoryDevicesInfo[Index].AssetTag);
>>>>> + AssetTag = MemoryDevicesInfo[Index].AssetTag;
>>>>> + } else {
>>>>> + AssetTagLen = AsciiStrLen (UnknownStr);
>>>>> + AssetTag = UnknownStr;
>>>>> + }
>>>>> +
>>>>> + if (MemoryDevicesInfo[Index].DeviceLocator != NULL) {
>>>>> + DeviceLocatorLen = AsciiStrLen
>>>>> (MemoryDevicesInfo[Index].DeviceLocator);
>>>>> + DeviceLocator = MemoryDevicesInfo[Index].DeviceLocator;
>>>>> + } else {
>>>>> + DeviceLocatorLen = AsciiStrLen (UnknownStr);
>>>>> + DeviceLocator = UnknownStr;
>>>>> + }
>>>>> +
>>>>> + if (MemoryDevicesInfo[Index].BankLocator != NULL) {
>>>>> + BankLocatorLen = AsciiStrLen
>>>>> (MemoryDevicesInfo[Index].BankLocator);
>>>>> + BankLocator = MemoryDevicesInfo[Index].BankLocator;
>>>>> + } else {
>>>>> + BankLocatorLen = AsciiStrLen (UnknownStr);
>>>>> + BankLocator = UnknownStr;
>>>>> + }
>>>>> +
>>>>> + if (MemoryDevicesInfo[Index].FirmwareVersion != NULL) {
>>>>> + FirmwareVersionLen = AsciiStrLen
>>>>> (MemoryDevicesInfo[Index].FirmwareVersion);
>>>>> + FirmwareVersion = MemoryDevicesInfo[Index].FirmwareVersion;
>>>>> + } else {
>>>>> + FirmwareVersionLen = AsciiStrLen (UnknownStr);
>>>>> + FirmwareVersion = UnknownStr;
>>>>> + }
>>>>> +
>>>>> + SmbiosRecord = (SMBIOS_TABLE_TYPE17 *)AllocateZeroPool (
>>>>> + sizeof
>>>>> (SMBIOS_TABLE_TYPE17) +
>>>>> + SerialNumLen + 1 +
>>>>> + AssetTagLen + 1 +
>>>>> DeviceLocatorLen + 1 +
>>>>> + BankLocatorLen + 1 + FirmwareVersionLen + 1 + 1
>>>>> + );
>>>> [SAMI] The memory allocated here needs to be freed in
>>>> FreeSmbiosType17TableEx as SmbiosProtocol->Add () makes a copy of the
>>>> SmbiosTable, see
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FMdeModulePkg%2FUniversal%2FSmbiosDxe%2FSmbiosDxe.c%23L476&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
>>>
>>>
>>>
>>>
>>>
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] DynamicTablesPkg: Add SMBIOS table generation
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:55 ` Sami Mujawar
1 sibling, 0 replies; 11+ messages in thread
From: Sami Mujawar @ 2022-09-12 14:55 UTC (permalink / raw)
To: Girish Mahadevan, devel, Alexei Fedorov
Cc: Samer El-Haj-Mahmoud, Jeff Brasen, Ashish Singhal, Akanksha Jain,
Matteo Carlini, Hemendra Dassanayake, Nick Ramirez,
William Watson, nd@arm.com
Hi Girish,
Thank you for this patch.
I am having difficulty applying this patch locally. Is it possible to
share this patch series on a github branch as well, please? It will
greatly help me with the review. I normally include a link to my github
branch (after the --- line) when positng to the mailing list see
https://edk2.groups.io/g/devel/message/93651
Other than that, please see my response inline marked [SAMI].
Regards,
Sami Mujawar
On 26/08/2022 06:37 pm, Girish Mahadevan wrote:
> Modify the DynamicTableManagerDxe driver to install SMBIOS tables in
> addition to ACPI tables.
> Instead of adding gEfiSmbiosProtocolGuid to the DEPEX list, setup
> callback notifications for gEfiSmbiosProtocolGuid and
> gEfiAcpiTableProtocolGuid and install the SMBIOS and ACPI tables
> in the respective notification ready callback functions.
>
> Add the ability to install multiple SMBIOS tables to the SMBIOS
> factory generator similar to ACPI.
>
> Signed-off-by: Girish Mahadevan <gmahadevan@nvidia.com>
> ---
> .../DynamicTableManagerDxe.c | 585 +++++++++++++++++-
> .../DynamicTableManagerDxe.inf | 2 +-
> .../Include/SmbiosTableGenerator.h | 50 ++
> 3 files changed, 628 insertions(+), 9 deletions(-)
>
> diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
> index ed62299f9b..1642d6c387 100644
> --- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
> +++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
> @@ -8,9 +8,11 @@
> **/
>
> #include <Library/DebugLib.h>
> +#include <Library/UefiLib.h>
> #include <Library/PcdLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Protocol/AcpiTable.h>
> +#include <Protocol/Smbios.h>
>
> // Module specific include files.
> #include <AcpiTableGenerator.h>
> @@ -31,6 +33,18 @@ GET_OBJECT_LIST (
> CM_STD_OBJ_ACPI_TABLE_INFO
> )
>
> +/** This macro expands to a function that retrieves the SMBIOS Table
> + List from the Configuration Manager.
> +*/
> +GET_OBJECT_LIST (
> + EObjNameSpaceStandard,
> + EStdObjSmbiosTableList,
> + CM_STD_OBJ_SMBIOS_TABLE_INFO
> + )
> +
> +STATIC VOID *AcpiTableProtocolRegistration;
> +STATIC VOID *SmbiosProtocolRegistration;
> +
> /** A helper function to build and install a single ACPI table.
>
> This is a helper function that invokes the Table generator interface
> @@ -478,6 +492,449 @@ VerifyMandatoryTablesArePresent (
> return Status;
> }
>
> +/** A helper function to build and install an SMBIOS table.
> +
> + This is a helper function that invokes the Table generator interface
> + for building an SMBIOS table. It uses the SmbiosProtocol to install the
> + table, then frees the resources allocated for generating it.
> +
> + @param [in] TableFactoryProtocol Pointer to the Table Factory Protocol
> + interface.
> + @param [in] CfgMgrProtocol Pointer to the Configuration Manager
> + Protocol Interface.
> + @param [in] SmbiosProtocol Pointer to the SMBIOS protocol.
> + @param [in] SmbiosTableInfo Pointer to the SMBIOS table Info.
> +
> + @retval EFI_SUCCESS Success.
> + @retval EFI_INVALID_PARAMETER A parameter is invalid.
> + @retval EFI_NOT_FOUND Required object is not found.
> + @retval EFI_BAD_BUFFER_SIZE Size returned by the Configuration Manager
> + is less than the Object size for the
> + requested object.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +BuildAndInstallSingleSmbiosTable (
> + IN CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *CONST TableFactoryProtocol,
> + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
> + IN CONST SMBIOS_TABLE_GENERATOR *CONST Generator,
> + IN EFI_SMBIOS_PROTOCOL *SmbiosProtocol,
> + IN CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST SmbiosTableInfo
> + )
> +{
> + EFI_STATUS Status;
> + EFI_STATUS Status1;
> + SMBIOS_STRUCTURE *SmbiosTable;
> + EFI_SMBIOS_HANDLE TableHandle;
> +
> + SmbiosTable = NULL;
> + Status = Generator->BuildSmbiosTable (
> + Generator,
> + SmbiosTableInfo,
> + CfgMgrProtocol,
> + &SmbiosTable
> + );
[SAMI] I think we may have to add another out parameter to the
BuildSmbiosTable () and BuildSmbiosTableEx () interfaces. This parameter
shall be a Reference Token that shall be a key in the mapping of
Reference Token to the SMBIOS record handle.
Please see my feedback in patch 2/2 about the need for a SMBIOS
HandleManager.
[/SAMI]
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to Build Table." \
> + " TableGeneratorId = 0x%x. Status = %r\n",
> + SmbiosTableInfo->TableGeneratorId,
> + Status
> + ));
> + // Free any allocated resources.
> + goto exit_handler;
> + }
> +
> + if (SmbiosTable == NULL) {
> + Status = EFI_NOT_FOUND;
> + goto exit_handler;
> + }
> +
> + TableHandle = SMBIOS_HANDLE_PI_RESERVED;
> + // Install SMBIOS table
> + Status = SmbiosProtocol->Add (
> + SmbiosProtocol,
> + NULL,
> + &TableHandle,
> + SmbiosTable
> + );
[SAMI] I think we need to call an SMBIOS HandleManager interface to map
the ReferenceToken received from BuildSmbiosTable () and
BuildSmbiosTableEx () and associate it with the TableHandle.
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to Install SMBIOS Table. Status = %r\n",
> + Status
> + ));
> + // Free any allocated resources.
> + goto exit_handler;
> + }
> +
> + DEBUG ((
> + DEBUG_INFO,
> + "INFO: SMBIOS Table installed. Status = %r\n",
> + Status
> + ));
> +
> +exit_handler:
> + // Free any resources allocated for generating the tables.
> + if (Generator->FreeTableResources != NULL) {
> + Status1 = Generator->FreeTableResources (
> + Generator,
> + SmbiosTableInfo,
> + CfgMgrProtocol,
> + &SmbiosTable
> + );
> + if (EFI_ERROR (Status1)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to Free Table Resources." \
> + "TableGeneratorId = 0x%x. Status = %r\n",
> + SmbiosTableInfo->TableGeneratorId,
> + Status1
> + ));
> + }
> +
> + // Return the first error status in case of failure
> + if (!EFI_ERROR (Status)) {
> + Status = Status1;
> + }
> + }
> +
> + return Status;
> +}
> +
> +/** A helper function to build and install multiple SMBIOS tables.
> +
> + This is a helper function that invokes the Table generator interface
> + for building SMBIOS tables. It uses the SmbiosProtocol to install the
> + tables, then frees the resources allocated for generating it.
> +
> + @param [in] TableFactoryProtocol Pointer to the Table Factory Protocol
> + interface.
> + @param [in] CfgMgrProtocol Pointer to the Configuration Manager
> + Protocol Interface.
> + @param [in] Generator Pointer to the SmbiosTable generator.
> + @param [in] SmbiosProtocol Pointer to the Smbios protocol.
> + @param [in] AcpiTableInfo Pointer to the SMBIOS table Info.
> +
> + @retval EFI_SUCCESS Success.
> + @retval EFI_INVALID_PARAMETER A parameter is invalid.
> + @retval EFI_NOT_FOUND Required object is not found.
> + @retval EFI_BAD_BUFFER_SIZE Size returned by the Configuration Manager
> + is less than the Object size for the
> + requested object.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +BuildAndInstallMultipleSmbiosTables (
> + IN CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *CONST TableFactoryProtocol,
> + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
> + IN CONST SMBIOS_TABLE_GENERATOR *CONST Generator,
> + IN EFI_SMBIOS_PROTOCOL *SmbiosProtocol,
> + IN CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST SmbiosTableInfo
> + )
> +{
> + EFI_STATUS Status;
> + EFI_STATUS Status1;
> + SMBIOS_STRUCTURE **SmbiosTable;
> + EFI_SMBIOS_HANDLE TableHandle;
> + UINTN TableCount;
> + UINTN Index;
> +
> + TableCount = 0;
> + Status = Generator->BuildSmbiosTableEx (
> + Generator,
> + SmbiosTableInfo,
> + CfgMgrProtocol,
> + &SmbiosTable,
> + &TableCount
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to Build Table." \
> + " TableGeneratorId = 0x%x. Status = %r\n",
> + SmbiosTableInfo->TableGeneratorId,
> + Status
> + ));
> + // Free any allocated resources.
> + goto exit_handler;
> + }
> +
> + if ((SmbiosTable == NULL) || (TableCount == 0)) {
> + Status = EFI_NOT_FOUND;
> + DEBUG ((
> + DEBUG_ERROR,
> + "%a: TableCount %u SmbiosTable %p \n",
> + __FUNCTION__,
> + TableCount,
> + SmbiosTable
> + ));
> + goto exit_handler;
> + }
> +
> + for (Index = 0; Index < TableCount; Index++) {
> + TableHandle = SMBIOS_HANDLE_PI_RESERVED;
> + // Install SMBIOS table
> + Status = SmbiosProtocol->Add (
> + SmbiosProtocol,
> + NULL,
> + &TableHandle,
> + SmbiosTable[Index]
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to Install SMBIOS Table. Status = %r\n",
> + Status
> + ));
> + // Free any allocated resources.
> + goto exit_handler;
> + }
> +
> + DEBUG ((
> + DEBUG_INFO,
> + "INFO: SMBIOS Table installed. Status = %r\n",
> + Status
> + ));
> + }
> +
> +exit_handler:
> + // Free any resources allocated for generating the tables.
> + if (Generator->FreeTableResourcesEx != NULL) {
> + Status1 = Generator->FreeTableResourcesEx (
> + Generator,
> + SmbiosTableInfo,
> + CfgMgrProtocol,
> + &SmbiosTable,
> + TableCount
> + );
> + if (EFI_ERROR (Status1)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to Free Table Resources." \
> + "TableGeneratorId = 0x%x. Status = %r\n",
> + SmbiosTableInfo->TableGeneratorId,
> + Status1
> + ));
> + }
> +
> + // Return the first error status in case of failure
> + if (!EFI_ERROR (Status)) {
> + Status = Status1;
> + }
> + }
> +
> + return Status;
> +}
> +
> +/** A helper function to invoke a Table generator
> +
> + This is a helper function that invokes the Table generator interface
> + for building an SMBIOS table. It uses the SmbiosProtocol to install the
> + table, then frees the resources allocated for generating it.
> +
> + @param [in] TableFactoryProtocol Pointer to the Table Factory Protocol
> + interface.
> + @param [in] CfgMgrProtocol Pointer to the Configuration Manager
> + Protocol Interface.
> + @param [in] SmbiosProtocol Pointer to the SMBIOS protocol.
> + @param [in] SmbiosTableInfo Pointer to the SMBIOS table Info.
> +
> + @retval EFI_SUCCESS Success.
> + @retval EFI_INVALID_PARAMETER A parameter is invalid.
> + @retval EFI_NOT_FOUND Required object is not found.
> + @retval EFI_BAD_BUFFER_SIZE Size returned by the Configuration Manager
> + is less than the Object size for the
> + requested object.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +BuildAndInstallSmbiosTable (
> + IN CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *CONST TableFactoryProtocol,
> + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
> + IN EFI_SMBIOS_PROTOCOL *SmbiosProtocol,
> + IN CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST SmbiosTableInfo
> + )
> +{
> + EFI_STATUS Status;
> + CONST SMBIOS_TABLE_GENERATOR *Generator;
> +
> + ASSERT (TableFactoryProtocol != NULL);
> + ASSERT (CfgMgrProtocol != NULL);
> + ASSERT (SmbiosProtocol != NULL);
> + ASSERT (SmbiosTableInfo != NULL);
> +
> + DEBUG ((
> + DEBUG_INFO,
> + "INFO: EStdObjSmbiosTableList: Address = 0x%p," \
> + " TableGeneratorId = 0x%x\n",
> + SmbiosTableInfo,
> + SmbiosTableInfo->TableGeneratorId
> + ));
> +
> + Generator = NULL;
> + Status = TableFactoryProtocol->GetSmbiosTableGenerator (
> + TableFactoryProtocol,
> + SmbiosTableInfo->TableGeneratorId,
> + &Generator
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Table Generator not found." \
> + " TableGeneratorId = 0x%x. Status = %r\n",
> + SmbiosTableInfo->TableGeneratorId,
> + Status
> + ));
> + return Status;
> + }
> +
> + if (Generator == NULL) {
> + return EFI_NOT_FOUND;
> + }
> +
> + DEBUG ((
> + DEBUG_INFO,
> + "INFO: Generator found : %s\n",
> + Generator->Description
> + ));
> +
> + if (Generator->BuildSmbiosTableEx != NULL) {
> + Status = BuildAndInstallMultipleSmbiosTables (
> + TableFactoryProtocol,
> + CfgMgrProtocol,
> + Generator,
> + SmbiosProtocol,
> + SmbiosTableInfo
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to find build and install SMBIOS Tables." \
> + " Status = %r\n",
> + Status
> + ));
> + }
> + } else if (Generator->BuildSmbiosTable != NULL) {
> + Status = BuildAndInstallSingleSmbiosTable (
> + TableFactoryProtocol,
> + CfgMgrProtocol,
> + Generator,
> + SmbiosProtocol,
> + SmbiosTableInfo
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to find build and install SMBIOS Table." \
> + " Status = %r\n",
> + Status
> + ));
> + }
> + } else {
> + Status = EFI_INVALID_PARAMETER;
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Table Generator does not implement the" \
> + "SMBIOS_TABLE_GENERATOR_BUILD_TABLE interface." \
> + " TableGeneratorId = 0x%x. Status = %r\n",
> + SmbiosTableInfo->TableGeneratorId,
> + Status
> + ));
> + }
> +
> + return Status;
> +}
> +
> +/** Generate and install SMBIOS tables.
> +
> + The function gathers the information necessary for installing the
> + SMBIOS tables from the Configuration Manager, invokes the generators
> + and installs them (via BuildAndInstallAcpiTable).
> +
> + @param [in] TableFactoryProtocol Pointer to the Table Factory Protocol
> + interface.
> + @param [in] CfgMgrProtocol Pointer to the Configuration Manager
> + Protocol Interface.
> +
> + @retval EFI_SUCCESS Success.
> + @retval EFI_NOT_FOUND If a mandatory table or a generator is not found.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +ProcessSmbiosTables (
> + IN CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *CONST TableFactoryProtocol,
> + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol
> + )
> +{
> + EFI_STATUS Status;
> + EFI_SMBIOS_PROTOCOL *SmbiosProtocol;
> + CM_STD_OBJ_SMBIOS_TABLE_INFO *SmbiosTableInfo;
> + UINT32 SmbiosTableCount;
> + UINT32 Idx;
> +
> + Status = gBS->LocateProtocol (&gEfiSmbiosProtocolGuid, NULL, (VOID **)&SmbiosProtocol);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Could not locate SMBIOS protocol. %r\n", Status));
> + return Status;
> + }
> +
> + Status = GetEStdObjSmbiosTableList (
> + CfgMgrProtocol,
> + CM_NULL_TOKEN,
> + &SmbiosTableInfo,
> + &SmbiosTableCount
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to get SMBIOS Table List. Status = %r\n",
> + Status
> + ));
> + return Status;
> + }
> +
> + if (SmbiosTableCount == 0) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: EStdObjSmbiosTableList: SmbiosTableCount = %d\n",
> + SmbiosTableCount
> + ));
> + return EFI_NOT_FOUND;
> + }
> +
[SAMI] I think we may need some additional code to sequence the
dependency of SMBIOS table generation. Please see my feedback about
SMBIOS Handle Manager in patch 2/2.
> + DEBUG ((
> + DEBUG_INFO,
> + "INFO: EStdObjSmbiosTableList: SmbiosTableCount = %d\n",
> + SmbiosTableCount
> + ));
> +
> + for (Idx = 0; Idx < SmbiosTableCount; Idx++) {
> + Status = BuildAndInstallSmbiosTable (
> + TableFactoryProtocol,
> + CfgMgrProtocol,
> + SmbiosProtocol,
> + &SmbiosTableInfo[Idx]
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to install SMBIOS Table." \
> + " Id = %u Status = %r\n",
> + SmbiosTableInfo[Idx].TableGeneratorId,
> + Status
> + ));
> + }
> + }
> +
> + return Status;
> +}
> +
> /** Generate and install ACPI tables.
>
> The function gathers the information necessary for installing the
> @@ -664,11 +1121,11 @@ ProcessAcpiTables (
> @retval EFI_NOT_FOUND Required interface/object was not found.
> @retval EFI_INVALID_PARAMETER Some parameter is incorrect/invalid.
> **/
> -EFI_STATUS
> -EFIAPI
> -DynamicTableManagerDxeInitialize (
> - IN EFI_HANDLE ImageHandle,
> - IN EFI_SYSTEM_TABLE *SystemTable
> +STATIC
> +VOID
> +AcpiTableProtocolReady (
> + IN EFI_EVENT Event,
> + IN VOID *Context
> )
> {
> EFI_STATUS Status;
> @@ -689,7 +1146,7 @@ DynamicTableManagerDxeInitialize (
> " Status = %r\n",
> Status
> ));
> - return Status;
> + return;
> }
>
> // Locate the Configuration Manager for the Platform
> @@ -704,7 +1161,7 @@ DynamicTableManagerDxeInitialize (
> "ERROR: Failed to find Configuration Manager protocol. Status = %r\n",
> Status
> ));
> - return Status;
> + return;
> }
>
> Status = GetCgfMgrInfo (CfgMgrProtocol, &CfgMfrInfo);
> @@ -714,7 +1171,7 @@ DynamicTableManagerDxeInitialize (
> "ERROR: Failed to get Configuration Manager info. Status = %r\n",
> Status
> ));
> - return Status;
> + return;
> }
>
> DEBUG ((
> @@ -737,6 +1194,118 @@ DynamicTableManagerDxeInitialize (
> Status
> ));
> }
> +}
> +
> +STATIC
> +VOID
> +SmbiosProtocolReady (
> + IN EFI_EVENT Event,
> + IN VOID *Context
> + )
> +{
> + EFI_STATUS Status;
> + EDKII_CONFIGURATION_MANAGER_PROTOCOL *CfgMgrProtocol;
> + CM_STD_OBJ_CONFIGURATION_MANAGER_INFO *CfgMfrInfo;
> + EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *TableFactoryProtocol;
> +
> + // Locate the Dynamic Table Factory
> + Status = gBS->LocateProtocol (
> + &gEdkiiDynamicTableFactoryProtocolGuid,
> + NULL,
> + (VOID **)&TableFactoryProtocol
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to find Dynamic Table Factory protocol." \
> + " Status = %r\n",
> + Status
> + ));
> + return;
> + }
> +
> + // Locate the Configuration Manager for the Platform
> + Status = gBS->LocateProtocol (
> + &gEdkiiConfigurationManagerProtocolGuid,
> + NULL,
> + (VOID **)&CfgMgrProtocol
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to find Configuration Manager protocol. Status = %r\n",
> + Status
> + ));
> + return;
> + }
> +
> + Status = GetCgfMgrInfo (CfgMgrProtocol, &CfgMfrInfo);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: Failed to get Configuration Manager info. Status = %r\n",
> + Status
> + ));
> + return;
> + }
> +
> + DEBUG ((
> + DEBUG_INFO,
> + "INFO: Configuration Manager Version = 0x%x, OemID = %c%c%c%c%c%c\n",
> + CfgMfrInfo->Revision,
> + CfgMfrInfo->OemId[0],
> + CfgMfrInfo->OemId[1],
> + CfgMfrInfo->OemId[2],
> + CfgMfrInfo->OemId[3],
> + CfgMfrInfo->OemId[4],
> + CfgMfrInfo->OemId[5]
> + ));
> +
> + Status = ProcessSmbiosTables (TableFactoryProtocol, CfgMgrProtocol);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: SMBIOS Table processing failure. Status = %r\n",
> + Status
> + ));
> + }
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +DynamicTableManagerDxeInitialize (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + EFI_STATUS Status;
> + EFI_EVENT AcpiEvent;
> + EFI_EVENT SmbiosEvent;
> +
> + AcpiEvent = EfiCreateProtocolNotifyEvent (
> + &gEfiAcpiTableProtocolGuid,
> + TPL_CALLBACK,
> + AcpiTableProtocolReady,
> + NULL,
> + &AcpiTableProtocolRegistration
> + );
> + if (AcpiEvent == NULL) {
> + DEBUG ((DEBUG_ERROR, "%a: Failed to ACPI create protocol event\r\n", __FUNCTION__));
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + SmbiosEvent = EfiCreateProtocolNotifyEvent (
> + &gEfiSmbiosProtocolGuid,
> + TPL_CALLBACK,
> + SmbiosProtocolReady,
> + NULL,
> + &SmbiosProtocolRegistration
> + );
> + if (SmbiosEvent == NULL) {
> + DEBUG ((DEBUG_ERROR, "%a: Failed to SMBIOS create protocol event\r\n", __FUNCTION__));
> + gBS->CloseEvent (AcpiEvent);
> + return EFI_OUT_OF_RESOURCES;
> + }
>
> return Status;
> }
> diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
> index 028c3d413c..ccf58f6099 100644
> --- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
> +++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
> @@ -36,12 +36,12 @@
>
> [Protocols]
> gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
> + gEfiSmbiosProtocolGuid # PROTOCOL ALWAYS_CONSUMED
>
> gEdkiiConfigurationManagerProtocolGuid # PROTOCOL ALWAYS_CONSUMED
> gEdkiiDynamicTableFactoryProtocolGuid # PROTOCOL ALWAYS_CONSUMED
>
> [Depex]
> - gEfiAcpiTableProtocolGuid AND
> gEdkiiConfigurationManagerProtocolGuid AND
> gEdkiiDynamicTableFactoryProtocolGuid
>
> diff --git a/DynamicTablesPkg/Include/SmbiosTableGenerator.h b/DynamicTablesPkg/Include/SmbiosTableGenerator.h
> index 934d56c90d..7bf6300d90 100644
> --- a/DynamicTablesPkg/Include/SmbiosTableGenerator.h
> +++ b/DynamicTablesPkg/Include/SmbiosTableGenerator.h
> @@ -168,6 +168,48 @@ typedef EFI_STATUS (*SMBIOS_TABLE_GENERATOR_FREE_TABLE) (
> IN SMBIOS_STRUCTURE **Table
> );
>
> +/** This function pointer describes the interface to SMBIOS table build
> + functions provided by the SMBIOS table generator and called by the
> + Table Manager to build an SMBIOS table.
> +
> + @param [in] Generator 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 generated SMBIOS table.
> +
> + @return EFI_SUCCESS If the table is generated successfully or other
> + failure codes as returned by the generator.
> +**/
> +typedef EFI_STATUS (*SMBIOS_TABLE_GENERATOR_BUILD_TABLEEX) (
> + IN CONST SMBIOS_TABLE_GENERATOR *Generator,
> + 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
> + );
> +
> +/** This function pointer describes the interface to used by the
> + Table Manager to give the generator an opportunity to free
> + any resources allocated for building the SMBIOS table.
> +
> + @param [in] Generator 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 [in] Table Pointer to the generated SMBIOS table.
> +
> + @return EFI_SUCCESS If freed successfully or other failure codes
> + as returned by the generator.
> +**/
> +typedef EFI_STATUS (*SMBIOS_TABLE_GENERATOR_FREE_TABLEEX) (
> + IN CONST SMBIOS_TABLE_GENERATOR *Generator,
> + IN CONST CM_STD_OBJ_SMBIOS_TABLE_INFO *CONST SmbiosTableInfo,
> + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
> + IN SMBIOS_STRUCTURE ***Table,
> + IN CONST UINTN TableCount
> + );
> +
> /** The SMBIOS_TABLE_GENERATOR structure provides an interface that the
> Table Manager can use to invoke the functions to build SMBIOS tables.
> */
> @@ -189,6 +231,14 @@ typedef struct SmbiosTableGenerator {
> allocated for building the SMBIOS table.
> */
> SMBIOS_TABLE_GENERATOR_FREE_TABLE FreeTableResources;
> +
> + /// SMBIOS table extended build function pointer.
> + SMBIOS_TABLE_GENERATOR_BUILD_TABLEEX BuildSmbiosTableEx;
> +
> + /** The function to free any resources
> + allocated for building the SMBIOS table.
> + */
> + SMBIOS_TABLE_GENERATOR_FREE_TABLEEX FreeTableResourcesEx;
> } SMBIOS_TABLE_GENERATOR;
>
> /** Register SMBIOS table factory generator.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-03-08 8:19 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 ` [edk2-devel] " Chang, Abner
2022-09-14 15:34 ` 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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox