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

["Girish Mahadevan via groups.io" <gmahadevan=nvidia.com@groups.io>]

Hi Sami

My responses inline [GM]

Best Regards
Girish

On 7/24/2023 11:48 AM, Sami Mujawar wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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.
>>

[GM]
As a first pass we're setting up the generators to just blindly use 
SMBIOS specific CM objects to setup these tables.

Second pass could be to have the generators try to rely on ACPI objects 
(PPTT) or fallback to Architecture specific libraries to figure out how 
to populate these tables. Honestly we need to think about this a little 
more, we'd love to swap notes.
(We are in the prelim stages of these so haven't gotten very far)


>> 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.
>>

[GM]
Will do.

>> 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.
> 
[GM]
ok, I will send out a patch train for the core generator code and 2 of 
the Generators.

Best Regards
Girish

>> 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 (#107253): https://edk2.groups.io/g/devel/message/107253
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]
-=-=-=-=-=-=-=-=-=-=-=-