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

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

Hi Girish,

Thank you for taking this effort forward

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 22/07/2023 12:14 am, Girish Mahadevan wrote:
> Hi Sami
>
> Your patches worked. There was just one thing I had to add to get 
> compile to work. (inline with [GM]), other than that I think you can 
> get this in.
[SAMI] Thanks for testing these patches.
>
> We've implemented about 18 table generators. (Types  0, 1, 2, 3, 8, 9, 
> 11, 13, 14, 16, 17, 19, 32, 38, 39, 41, 43, 45)
> Two more are under way (Type 4.7).
[SAMI] I would be keen to understand how the Type 4 and 7 generators 
would work as I would prefer to avoid duplication of data.
>
> I've setup a github repo here
> https://github.com/tianocore/edk2/compare/master...gmahadevan:edk2-upstream:RFC/smbios-dyntables-v2?expand=1 
>
[SAMI] Thanks for sharing the github branch. Please also share a github 
branch when you send out your patches for review.
>
> I could send you one massive patch train (22 patches or so) or split 
> it up into smaller chunks perhaps just the table generation stuff 
> first and then each generator library.
>
[SAMI] I think it may be good to split the patch series to include table 
generation and then 1 or 2 generators to understand the code flow. The 
remaining generators can follow later. I think Type 4 and 7 would be 
really interesting, but you can send what you already have.

> Let me know.
>
> Best Regards
> Girish
>
>
> On 3/10/2023 9:28 AM, Sami Mujawar wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 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://edk2.groups.io/g/devel/message/95341
>>>>> <https://edk2.groups.io/g/devel/message/95341>
>>>>>
>>>>>
>>>>> 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
>>>>> +
> [GM]
> Not sure if it was an oversight, but you missed adding the extern to 
> BuildAndInstallSmbiosTable
[SAMI] Yes, you are correct. I had tested my changes only using 
simulation so may have removed the stub function when posting the patches.
>
>>>>> + 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),
> [GM]:
> We've made changes to this and to the Firmware inventory
[SAMI] Did I get something wrong or missed something here? Can you let 
me know, please?
>
>
>>>>> + 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)'
>>>>>
>>>>
>>>>
>>>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107186): https://edk2.groups.io/g/devel/message/107186
Mute This Topic: https://groups.io/mt/97468481/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-