Re: [PATCH v2 2/4] DynamicTablesPkg: Add SMBIOS table dispatcher

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Girish,

On 10/03/2023 04:23 pm, Girish Mahadevan wrote:
> Hi Sami
>
> Response inline.[GM]
>
> Best Regards
> Girish
>
> On 3/9/2023 3:41 AM, Sami Mujawar wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> Hi Girish,
>>
>> Thank you for your feedback.
>>
>> Please find my response inline marked [SAMI].
>>
>> Regards,
>>
>> Sami Mujawar
>>
>> On 08/03/2023, 17:41, "Girish Mahadevan" <gmahadevan@nvidia.com 
>> <mailto:gmahadevan@nvidia.com>> wrote:
>>
>>
>> Hi Sami
>>
>>
>> Thanks for v2, I will apply these to my tree and test it out.
>> One small comment before I review/test the patch train inline (prefixed
>> by [GM])
>>
>>
>> Best Regards
>> Girish
>>
>>
>> On 3/8/2023 1:16 AM, Sami Mujawar wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> Some SMBIOS structure/table fields have dependency on other SMBIOS
>>> structures/tables. These dependencies are established using handles
>>> pointing to the dependent tables.
>>>
>>> A SMBIOS table handle can be obtained by either installing a SMBIOS
>>> table or by allocating a handle, which requires complex management
>>> to avoid any clashes.
>>>
>>> Obtaining a SMBIOS handle by installation requires that the dependent
>>> table is installed before the parent SMBIOS table can be installed.
>>> Therefore, introduce a SMBIOS table dispatcher that walks the SMBIOS
>>> dependency list and schedules the dependent tables to be installed
>>> before the parent table is installed.
>>>
>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com 
>>> <mailto:sami.mujawar@arm.com>>
>>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com 
>>> <mailto:Alexei.Fedorov@arm.com>>
>>> Cc: Pierre Gondois <pierre.gondois@arm.com 
>>> <mailto:pierre.gondois@arm.com>>
>>> Cc: Girish Mahadevan <gmahadevan@nvidia.com 
>>> <mailto:gmahadevan@nvidia.com>>
>>> Cc: Jeff Brasen <jbrasen@nvidia.com <mailto:jbrasen@nvidia.com>>
>>> Cc: Ashish Singhal <ashishsingha@nvidia.com 
>>> <mailto:ashishsingha@nvidia.com>>
>>> Cc: Nick Ramirez <nramirez@nvidia.com <mailto:nramirez@nvidia.com>>
>>> Cc: William Watson <wwatson@nvidia.com <mailto:wwatson@nvidia.com>>
>>> Cc: Abner Chang <abner.chang@amd.com <mailto:abner.chang@amd.com>>
>>> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com 
>>> <mailto:Samer.El-Haj-Mahmoud@arm.com>>
>>> Cc: Jose Marinho <Jose.Marinho@arm.com <mailto:Jose.Marinho@arm.com>>
>>> ---
>>>
>>> Notes:
>>> v2:
>>> - Update dispatcher state machine to remove StUnknown. [SAMI]
>>> - Move extern function to header file and move debug [ABNER]
>>> code together.
>>> - Updated code based on review feedback to move extern [SAMI]
>>> function to header file and also moved the debug code
>>> together.
>>> Ref: 
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F95341&data=05%7C01%7Cgmahadevan%40nvidia.com%7C57205f56a9f84d2470b408db208ade8b%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638139553079754853%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Jab1Rf4UjvIJybcAXFAVXRZBxGaOQki%2BC1KxnPjaxGE%3D&reserved=0 
>>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F95341&data=05%7C01%7Cgmahadevan%40nvidia.com%7C57205f56a9f84d2470b408db208ade8b%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638139553079754853%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Jab1Rf4UjvIJybcAXFAVXRZBxGaOQki%2BC1KxnPjaxGE%3D&reserved=0> 
>>>
>>>
>>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
>>> | 4 +-
>>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/SmbiosTableDispatcher.c 
>>> | 282 ++++++++++++++++++++
>>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/SmbiosTableDispatcher.h 
>>> | 159 +++++++++++
>>> 3 files changed, 444 insertions(+), 1 deletion(-)
>>>
>>> diff --git 
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
>>>
>>> index 
>>> ad8b3d037c169ca848b5ea84fd1086b83f37876f..b09272d883c6f4f05eb03dcdab3efeda92b2fbb6 
>>> 100644
>>> --- 
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>>> +++ 
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>>> @@ -1,7 +1,7 @@
>>> ## @file
>>> # Module that drives the table generation and installation process.
>>> #
>>> -# Copyright (c) 2017 - 2019, ARM Limited. All rights reserved.
>>> +# Copyright (c) 2017 - 2022, Arm Limited. All rights reserved.
>>> #
>>> # SPDX-License-Identifier: BSD-2-Clause-Patent
>>> ##
>>> @@ -22,6 +22,8 @@ [Defines]
>>>
>>> [Sources]
>>> DynamicTableManagerDxe.c
>>> + SmbiosTableDispatcher.c
>>> + SmbiosTableDispatcher.h
>>>
>>> [Packages]
>>> MdePkg/MdePkg.dec
>>> diff --git 
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/SmbiosTableDispatcher.c 
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/SmbiosTableDispatcher.c 
>>>
>>> new file mode 100644
>>> index 
>>> 0000000000000000000000000000000000000000..0e728538d9f6eb0b164fea3a160d3233db833f8d
>>> --- /dev/null
>>> +++ 
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/SmbiosTableDispatcher.c
>>> @@ -0,0 +1,282 @@
>>> +/** @file
>>> + Dynamic Smbios Table Dispatcher
>>> +
>>> + Copyright (c) 2022 - 2023, Arm Limited. All rights reserved.
>>> +
>>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> +**/
>>> +#include <Library/DebugLib.h>
>>> +#include <Protocol/Smbios.h>
>>> +
>>> +#include <Include/StandardNameSpaceObjects.h>
>>> +#include <SmbiosTableDispatcher.h>
>>> +
>>> +/**
>>> + The SMBIOS dispatcher state table.
>>> +
>>> + The SMBIOS dispatcher state table is used to establish the dependency
>>> + order in which the SMBIOS tables are installed. This allows the 
>>> SMBIOS
>>> + dispatcher to dispatch the dependent tables for installation 
>>> before the
>>> + parent table is installed.
>>> + The SMBIOS_TABLE_DISPATCHER.Dependency[] field is used to 
>>> establish the
>>> + dependency list.
>>> + Elements in the Dependency list are resolved by increasing index. 
>>> However,
>>> + all orders are equivalent as:
>>> + - the Parent SMBIOS table will only be installed once all 
>>> dependencies
>>> + have been satisfied.
>>> + - no cyclic dependency is allowed.
>>> + The dependency list is terminated by SMTT_NULL.
>>> +*/
>>> +STATIC
>>> +SMBIOS_TABLE_DISPATCHER mSmBiosDispatcher[MAX_SMBIOS_TABLES] = {
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_BIOS_INFORMATION, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_SYSTEM_INFORMATION, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_BASEBOARD_INFORMATION, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_SYSTEM_ENCLOSURE, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_PROCESSOR_INFORMATION, 
>>> SMBIOS_TYPE_CACHE_INFORMATION, SMTT_NULL, SMTT_NULL, SMTT_NULL, 
>>> SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_MEMORY_CONTROLLER_INFORMATION, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_MEMORY_MODULE_INFORMATON, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_CACHE_INFORMATION, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_PORT_CONNECTOR_INFORMATION, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_SYSTEM_SLOTS, SMTT_NULL, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_ONBOARD_DEVICE_INFORMATION, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_OEM_STRINGS, SMTT_NULL, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_SYSTEM_CONFIGURATION_OPTIONS, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_BIOS_LANGUAGE_INFORMATION, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_GROUP_ASSOCIATIONS, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_SYSTEM_EVENT_LOG, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_PHYSICAL_MEMORY_ARRAY, 
>>> SMBIOS_TYPE_32BIT_MEMORY_ERROR_INFORMATION, 
>>> SMBIOS_TYPE_64BIT_MEMORY_ERROR_INFORMATION, SMTT_NULL, SMTT_NULL, 
>>> SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_MEMORY_DEVICE, 
>>> SMBIOS_TYPE_PHYSICAL_MEMORY_ARRAY, 
>>> SMBIOS_TYPE_32BIT_MEMORY_ERROR_INFORMATION, 
>>> SMBIOS_TYPE_64BIT_MEMORY_ERROR_INFORMATION, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_32BIT_MEMORY_ERROR_INFORMATION, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_MEMORY_ARRAY_MAPPED_ADDRESS, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_MEMORY_DEVICE_MAPPED_ADDRESS, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_BUILT_IN_POINTING_DEVICE, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_PORTABLE_BATTERY, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_SYSTEM_RESET, SMTT_NULL, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_HARDWARE_SECURITY, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_SYSTEM_POWER_CONTROLS, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_VOLTAGE_PROBE, SMTT_NULL, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_COOLING_DEVICE, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_TEMPERATURE_PROBE, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_ELECTRICAL_CURRENT_PROBE, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_OUT_OF_BAND_REMOTE_ACCESS, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_BOOT_INTEGRITY_SERVICE, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_SYSTEM_BOOT_INFORMATION, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_64BIT_MEMORY_ERROR_INFORMATION, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_MANAGEMENT_DEVICE, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_MANAGEMENT_DEVICE_COMPONENT, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_MANAGEMENT_DEVICE_THRESHOLD_DATA, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_MEMORY_CHANNEL, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_IPMI_DEVICE_INFORMATION, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_SYSTEM_POWER_SUPPLY, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_ADDITIONAL_INFORMATION, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP 
>>> (SMBIOS_TYPE_ONBOARD_DEVICES_EXTENDED_INFORMATION, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP 
>>> (SMBIOS_TYPE_MANAGEMENT_CONTROLLER_HOST_INTERFACE, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_TPM_DEVICE, SMTT_NULL, SMTT_NULL, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_PROCESSOR_ADDITIONAL_INFORMATION, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_FIRMWARE_INVENTORY_INFORMATION, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL),
>>> + SMBIOS_TABLE_DEP (SMBIOS_TYPE_STRING_PROPERTY_INFORMATION, 
>>> SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL, SMTT_NULL)
>>> +};
>>> +
>>> +#if !defined (MDEPKG_NDEBUG)
>>> +
>>> +/**
>>> + A string table describing the SMBIOS dispatcher states.
>>> +*/
>>> +STATIC
>>> +CONST CHAR8 *SmbiosTableStateTxt[] = {
>>> + "StNotPresent",
>>> + "StPresent",
>>> + "StDispatched"
>>> +};
>>> +
>>> +/**
>>> + Print the SMBIOS Table Dispatcher state information.
>>> +
>>> + @param Verbose Print detailed report
>>> +**/
>>> +STATIC
>>> +VOID
>>> +EFIAPI
>>> +PrintDispatcherStatus (
>>> + IN BOOLEAN Verbose
>>> + )
>>> +{
>>> + UINTN Index;
>>> +
>>> + DEBUG ((DEBUG_VERBOSE, "Dispatcher Status:\n"));
>>> + for (Index = 0; Index < ARRAY_SIZE (mSmBiosDispatcher); Index++) {
>>> + if ((!Verbose) && (mSmBiosDispatcher[Index].State == StNotPresent)) {
>>> + continue;
>>> + }
>>> +
>>> + DEBUG ((
>>> + DEBUG_VERBOSE,
>>> + "%02d: %10a [%02d, %02d, %02d, %02d, %02d]\n",
>>> + mSmBiosDispatcher[Index].TableType,
>>> + SmbiosTableStateTxt[mSmBiosDispatcher[Index].State],
>>> + mSmBiosDispatcher[Index].Dependency[0],
>>> + mSmBiosDispatcher[Index].Dependency[1],
>>> + mSmBiosDispatcher[Index].Dependency[2],
>>> + mSmBiosDispatcher[Index].Dependency[3],
>>> + mSmBiosDispatcher[Index].Dependency[4]
>>> + ));
>>> + } // for
>>> +
>>> + DEBUG ((DEBUG_VERBOSE, "\n"));
>>> +}
>>> +
>>> +#define DEBUG_PRINT_DISPATCHER_STATUS(Verbose) 
>>> PrintDispatcherStatus (Verbose)
>>> +#else
>>> +#define DEBUG_PRINT_DISPATCHER_STATUS(x)
>>> +#endif
>>> +
>>> +/**
>>> + Initialise the SMBIOS table dispatcher.
>>> +
>>> + @param SmbiosTableInfo Pointer to the list of SMBIOS tables to be 
>>> installed.
>>> + @param SmbiosTableCount Count of SMBIOS tables to be installed.
>>> +**/
>>> +VOID
>>> +EFIAPI
>>> +InitSmbiosTableDispatcher (
>>> + IN CM_STD_OBJ_SMBIOS_TABLE_INFO *SmbiosTableInfo,
>>> + IN UINT32 SmbiosTableCount
>>> + )
>>> +{
>>> + UINTN Index;
>>> + SMBIOS_TABLE_TYPE TableType;
>>> +
>>> + // Search for the list of SMBIOS tables presented
>>> + // for installation and update the dispatcher status.
>>> + for (Index = 0; Index < SmbiosTableCount; Index++) {
>>> + TableType = SmbiosTableInfo[Index].TableType;
>>> + ASSERT (mSmBiosDispatcher[TableType].State != StPresent);
>>
>>
>> [GM]
>> This ASSERT will be hit even if we have multiple SMBIOS CM objects of
>> the same type which is legal (for e.g there are multiple tables for
>> type17/19 )
>>
>> [SAMI] The understanding is that SmbiosTableInfo[] will only contain 
>> one entry for a given TableType as it is used to indicate which 
>> generators are to be instantiated.
>>
>> Following is the sequence of operations:
>> - The dispatcher is initialised in InitSmbiosTableDispatcher(), which 
>> will set the mSmBiosDispatcher[<Table_Type>].State to StPresent if 
>> SmbiosTableInfo[] contains a table of Table_Type.
>> - SmbiosTableInfo[] will only contain one entry for each table type, 
>> e.g. even if there are multiple Type17/Type19 tables to be installed. 
>> This entry indicates that table(s) of Type17/Type19 is/are requested 
>> to be installed and that the corresponding generator must be 
>> instantiated.
>> - Once the SMBIOS table generator is instantiated, it will then 
>> request for the object(s) of the required type from the Configuration 
>> Manager that are required to construct the table.
>> - The generator can then decide if multiple SMBIOS tables are to be 
>> constructed based on the objects returned by the Configuration 
>> Manager. e.g. if multiple Cm_Type17_objects are returned then the 
>> corresponding number of Type17 tables will be built by the generator.
>>
>> Therefore, I think we should not hit the above assert.
>>
>
> [GM]
> That sounds right, I'd made a mistake when installing the CM objects.
>
> I should be installing one of each SMBIOS table so that 
> GetEStdObjSmbiosTableList gets the list of the available tables and 
> each generator should install multiple tables of a given type based on 
> the number of CM objects available.
>
> I did expand the existing SMBIOS factory code to install multiple tables.
>
> Thanks for the clarification.
>
> Best Regards
> Girish
>
[SAMI] No worries. Please let me know if you face any issues with the 
dispatcher or if you identify any problem during integration.

Regards,

Sami Mujawar

>
>> However, the code that may need to change/extend is the call to 
>> BuildAndInstallSmbiosTable(). I am not sure if we need to indicate 
>> that there could be multiple SMBIOS tables that may be installed. For 
>> now, BuildAndInstallSmbiosTable() implies that a generator of the 
>> specified type is to be instantiated for building the SMBIOS table of 
>> the specified type.
>>
>> Hope this clarifies the scenario and please do let me know if I am 
>> missing something.
>>
>> Also, I have only tested this patch with simulated data and there may 
>> be some issues that we may find when the full stack is integrated.
>> So, thank you for going through the patch in detail.
>>
>> [SAMI]
>>
>> Best Regards
>> Girish
>>
>>
>>> + mSmBiosDispatcher[TableType].State = StPresent;
>>> + }
>>> +
>>> + DEBUG_PRINT_DISPATCHER_STATUS (FALSE);
>>> +}
>>> +
>>> +/** Schedule the dispatch of a SMBIOS table.
>>> +
>>> + The SMBIOS dispatcher state table is used to establish the dependency
>>> + order in which the SMBIOS tables are installed. This allows the 
>>> SMBIOS
>>> + dispatcher to dispatch the dependent tables for installation 
>>> before the
>>> + parent table is installed.
>>> + The SMBIOS_TABLE_DISPATCHER.Dependency[] field is used to 
>>> establish the
>>> + dependency list.
>>> + Elements in the Dependency list are resolved by increasing index. 
>>> However,
>>> + all orders are equivalent as:
>>> + - the Parent SMBIOS table will only be installed once all 
>>> dependencies
>>> + have been satisfied.
>>> + - no cyclic dependency is allowed.
>>> + The dependency list is terminated by SMTT_NULL.
>>> +
>>> + @param [in] TableType The SMBIOS table type to schedule for
>>> + dispatch.
>>> + @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.
>>> + @param [in] SmbiosTableCount Count of SMBIOS table info objects.
>>> +
>>> + @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.
>>> +**/
>>> +EFI_STATUS
>>> +EFIAPI
>>> +DispatchSmbiosTable (
>>> + IN CONST SMBIOS_TABLE_TYPE TableType,
>>> + 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,
>>> + IN CONST UINT32 SmbiosTableCount
>>> + )
>>> +{
>>> + EFI_STATUS Status;
>>> + UINTN Index;
>>> + SMBIOS_TABLE_DISPATCHER *Disp;
>>> +
>>> + DEBUG ((DEBUG_VERBOSE, "->DP %02d\n", TableType));
>>> + Disp = &mSmBiosDispatcher[TableType];
>>> + if (Disp->State == StNotPresent) {
>>> + DEBUG ((DEBUG_VERBOSE, "<-DP %02d : EFI_NOT_FOUND\n", TableType));
>>> + return EFI_NOT_FOUND;
>>> + }
>>> +
>>> + if (Disp->State == StDispatched) {
>>> + DEBUG ((DEBUG_VERBOSE, "<-DP %02d : EFI_ALREADY_STARTED\n", 
>>> TableType));
>>> + return EFI_ALREADY_STARTED;
>>> + }
>>> +
>>> + // Table is present so check the dependency.
>>> + for (Index = 0; Index < MAX_SMBIOS_DEPENDENCY; Index++) {
>>> + // Check if the dependency list is terminated by SMTT_NULL.
>>> + if (Disp->Dependency[Index] == SMTT_NULL) {
>>> + break;
>>> + }
>>> +
>>> + Status = DispatchSmbiosTable (
>>> + Disp->Dependency[Index],
>>> + TableFactoryProtocol,
>>> + CfgMgrProtocol,
>>> + SmbiosProtocol,
>>> + SmbiosTableInfo,
>>> + SmbiosTableCount
>>> + );
>>> + if (EFI_ERROR (Status)) {
>>> + if ((Status == EFI_ALREADY_STARTED) || (Status == EFI_NOT_FOUND)) {
>>> + // Some dependencies may already be satisfied
>>> + // as other tables may also have similar
>>> + // dependencies i.e. EFI_ALREADY_STARTED
>>> + // Or
>>> + // the dependent table may be optional
>>> + // and not provided i.e. EFI_NOT_FOUND.
>>> + continue;
>>> + }
>>> +
>>> + DEBUG ((DEBUG_VERBOSE, "<-DP %02d : Status = %d\n", TableType, 
>>> Status));
>>> + return Status;
>>> + }
>>> + }
>>> +
>>> + DEBUG ((DEBUG_VERBOSE, "DP %02d : Status = %d\n", TableType, 
>>> Status));
>>> +
>>> + // All dependencies satisfied - Install SMBIOS table
>>> + Disp->State = StDispatched;
>>> + // Find the SMBIOS table info matching the TableType
>>> + for (Index = 0; Index < SmbiosTableCount; Index++) {
>>> + if (SmbiosTableInfo[Index].TableType == TableType) {
>>> + break;
>>> + }
>>> + }
>>> +
>>> + Status = BuildAndInstallSmbiosTable (
>>> + TableFactoryProtocol,
>>> + CfgMgrProtocol,
>>> + SmbiosProtocol,
>>> + &SmbiosTableInfo[Index]
>>> + );
>>> + if (EFI_ERROR (Status)) {
>>> + DEBUG ((
>>> + DEBUG_ERROR,
>>> + "ERROR: Failed to install SMBIOS Table." \
>>> + " Id = %u Status = %r\n",
>>> + SmbiosTableInfo[Index].TableGeneratorId,
>>> + Status
>>> + ));
>>> + }
>>> +
>>> + DEBUG_PRINT_DISPATCHER_STATUS (FALSE);
>>> + DEBUG ((DEBUG_VERBOSE, "<-DP %0d\n", TableType));
>>> + return Status;
>>> +}
>>> diff --git 
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/SmbiosTableDispatcher.h 
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/SmbiosTableDispatcher.h 
>>>
>>> new file mode 100644
>>> index 
>>> 0000000000000000000000000000000000000000..d59eab4223c142293bdaf4905588f08e7a4a467f
>>> --- /dev/null
>>> +++ 
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/SmbiosTableDispatcher.h
>>> @@ -0,0 +1,159 @@
>>> +/** @file
>>> +
>>> + Copyright (c) 2022 - 2023, Arm Limited. All rights reserved.
>>> +
>>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> +**/
>>> +
>>> +#ifndef SMBIOS_TABLE_DISPATCHER_H_
>>> +#define SMBIOS_TABLE_DISPATCHER_H_
>>> +
>>> +#include <Protocol/ConfigurationManagerProtocol.h>
>>> +#include <Protocol/DynamicTableFactoryProtocol.h>
>>> +
>>> +/**
>>> + A SMBIOS Table Type from the OEM range reserved for terminating
>>> + the SMBIOS table dispatch dependency.
>>> +
>>> + Note: According to the SMBIOS specification, Table Types 0
>>> + through 127 (7Fh) are reserved for and defined by the
>>> + SMBIOS specification.
>>> + Types 128 through 256 (80h to FFh) are available for system and
>>> + OEM-specific information.
>>> +
>>> + This Dynamic SMBIOS table generation implementation defines
>>> + TableType FFh as a NULL table which is used by the Dynamic
>>> + SMBIOS table dispatcher to terminate the dependency list.
>>> +*/
>>> +#define SMTT_NULL 0xFF
>>> +
>>> +/**
>>> + A macro defining the maximum number of dependendant SMBIOS tables
>>> + represented by the SMBIOS table dispatcher.
>>> +*/
>>> +#define MAX_SMBIOS_DEPENDENCY 5
>>> +
>>> +/**
>>> + A macro defining the maximum table types handled by the SMBIOS
>>> + table dispatcher.
>>> +*/
>>> +#define MAX_SMBIOS_TABLES (SMBIOS_TYPE_STRING_PROPERTY_INFORMATION 
>>> + 1)
>>> +
>>> +/**
>>> + A helper macro to populate the SMBIOS table dispatcher table
>>> +*/
>>> +#define SMBIOS_TABLE_DEP(TableId, Dep1, Dep2, Dep3, Dep4, Dep5) \
>>> + { \
>>> + TableId, \
>>> + StNotPresent, \
>>> + { Dep1, Dep2, Dep3, Dep4, Dep5 } \
>>> + }
>>> +
>>> +/**
>>> + An enum describing the states of the SMBIOS table dispatcher.
>>> +*/
>>> +typedef enum SmbiosTableState {
>>> + StNotPresent, ///< SMBIOS table is not present for installation.
>>> + StPresent, ///< SMBIOS table is present for installation.
>>> + StDispatched ///< SMBIOS table generators have been dispatched.
>>> +} SMBIOS_TABLE_STATE;
>>> +
>>> +/**
>>> + A structure describing the dependencies for a SMBIOS table and
>>> + the dispatcher state information.
>>> +*/
>>> +typedef struct SmBiosTableDispatcher {
>>> + /// SMBIOS Structure/Table Type
>>> + SMBIOS_TABLE_TYPE TableType;
>>> + /// SMBIOS dispatcher state
>>> + SMBIOS_TABLE_STATE State;
>>> + /// SMBIOS Structure/Table dependency list
>>> + /// The list is terminated using SMTT_NULL.
>>> + SMBIOS_TABLE_TYPE Dependency[MAX_SMBIOS_DEPENDENCY];
>>> +} SMBIOS_TABLE_DISPATCHER;
>>> +
>>> +/**
>>> + A helper function to build and install a SMBIOS table.
>>> +
>>> + @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.
>>> +**/
>>> +extern
>>> +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
>>> + );
>>> +
>>> +/**
>>> + Initialise the SMBIOS table dispatcher.
>>> +
>>> + @param SmbiosTableInfo Pointer to the list of SMBIOS tables to be 
>>> installed.
>>> + @param SmbiosTableCount Count of SMBIOS tables to be installed.
>>> +**/
>>> +VOID
>>> +EFIAPI
>>> +InitSmbiosTableDispatcher (
>>> + IN CM_STD_OBJ_SMBIOS_TABLE_INFO *SmbiosTableInfo,
>>> + IN UINT32 SmbiosTableCount
>>> + );
>>> +
>>> +/** Schedule the dispatch of a SMBIOS table.
>>> +
>>> + The SMBIOS dispatcher state table is used to establish the dependency
>>> + order in which the SMBIOS tables are installed. This allows the 
>>> SMBIOS
>>> + dispatcher to dispatch the dependent tables for installation 
>>> before the
>>> + parent table is installed.
>>> + The SMBIOS_TABLE_DISPATCHER.Dependency[] field is used to 
>>> establish the
>>> + dependency list.
>>> + Elements in the Dependency list are resolved by increasing index. 
>>> However,
>>> + all orders are equivalent as:
>>> + - the Parent SMBIOS table will only be installed once all 
>>> dependencies
>>> + have been satisfied.
>>> + - no cyclic dependency is allowed.
>>> + The dependency list is terminated by SMTT_NULL.
>>> +
>>> + @param [in] TableType The SMBIOS table type to schedule for
>>> + dispatch.
>>> + @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.
>>> + @param [in] SmbiosTableCount Count of SMBIOS table info objects.
>>> +
>>> + @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.
>>> +**/
>>> +EFI_STATUS
>>> +EFIAPI
>>> +DispatchSmbiosTable (
>>> + IN CONST SMBIOS_TABLE_TYPE TableType,
>>> + 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,
>>> + IN CONST UINT32 SmbiosTableCount
>>> + );
>>> +
>>> +#endif // SMBIOS_TABLE_DISPATCHER_H_
>>> -- 
>>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>>
>>
>>
>>