public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Girish Mahadevan" <gmahadevan@nvidia.com>
To: Sami Mujawar <Sami.Mujawar@arm.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>,
	Pierre Gondois <Pierre.Gondois@arm.com>,
	"abner.chang@amd.com" <abner.chang@amd.com>,
	"Jeff Brasen (jbrasen@nvidia.com)" <jbrasen@nvidia.com>,
	"ashishsingha@nvidia.com" <ashishsingha@nvidia.com>,
	"nramirez@nvidia.com" <nramirez@nvidia.com>,
	"wwatson@nvidia.com" <wwatson@nvidia.com>,
	Matteo Carlini <Matteo.Carlini@arm.com>,
	Akanksha Jain <Akanksha.Jain2@arm.com>,
	Ben Adderson <Ben.Adderson@arm.com>,
	Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>,
	Jose Marinho <Jose.Marinho@arm.com>, nd <nd@arm.com>
Subject: Re: [PATCH v2 2/4] DynamicTablesPkg: Add SMBIOS table dispatcher
Date: Fri, 10 Mar 2023 09:23:57 -0700	[thread overview]
Message-ID: <59d00c3a-0ed8-f73a-68e0-70eb304a7d25@nvidia.com> (raw)
In-Reply-To: <8FC92511-1E75-4C4B-91C7-9D252728C69E@arm.com>

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


> 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)'
>>
> 
> 
> 

  reply	other threads:[~2023-03-10 16:24 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-08  8:16 [PATCH v2 0/4] DynamicTablesPkg: Introduce SMBIOS dispatcher Sami Mujawar
2023-03-08  8:16 ` [PATCH v2 1/4] DynamicTablesPkg: Define a SMBIOS Structure/Table type Sami Mujawar
2023-03-08  8:16 ` [PATCH v2 2/4] DynamicTablesPkg: Add SMBIOS table dispatcher Sami Mujawar
2023-03-08 17:41   ` Girish Mahadevan
2023-03-09 10:41     ` Sami Mujawar
2023-03-10 16:23       ` Girish Mahadevan [this message]
2023-03-10 16:28         ` Sami Mujawar
2023-07-21 23:14           ` [edk2-devel] " Girish Mahadevan via groups.io
2023-07-24 17:48             ` Sami Mujawar
2023-07-25 23:29               ` Girish Mahadevan via groups.io
     [not found]               ` <1775402AB0EAA56A.16579@groups.io>
2023-08-09 16:13                 ` Girish Mahadevan via groups.io
2023-03-08  8:16 ` [PATCH v2 3/4] DynamicTablesPkg: Update SMBIOS dispatcher dependency table Sami Mujawar
2023-03-08  8:16 ` [PATCH v2 4/4] DynamicTablesPkg: Add Ordered dispatch support for SMBIOS tables Sami Mujawar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=59d00c3a-0ed8-f73a-68e0-70eb304a7d25@nvidia.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox