-
Notifications
You must be signed in to change notification settings - Fork 152
Dynamictables: Add SMBIOS dispatcher and generators #510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dynamictables: Add SMBIOS dispatcher and generators #510
Conversation
EObjNameSpaceArm, ///< ARM Objects Namespace | ||
EObjNameSpaceOem = 0xF, ///< OEM Objects Namespace | ||
EObjNameSpaceOem = 0xF, ///< OEM Objects Namespace | ||
EObjNameSpaceSmbios, ///< SMBIOS Objects Namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CM_OBJECT_ID name space ID (as defined at the top of this file) is 4 bits wide. So this addition causes an overflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change this.
@param [IN] Str Pointer to the string | ||
@param [OUT] StrRef The string field reference of the added string | ||
**/ | ||
#define STRING_TABLE_ADD_STRING(StrTable, String, StringRef) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This macro isn't used anywhere in this patchset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, this patchset doesn't include the generators that use this macro.
I will drop it from the patchset and bring it back when I introduce the other generators (type3/13 etc).
58f8eb2
to
777b9c1
Compare
@gmahadevan Thanks a lot for driving this effort and for this patch series. |
EObjNameSpaceArm, ///< ARM Objects Namespace | ||
EObjNameSpaceOem = 0xF, ///< OEM Objects Namespace | ||
EObjNameSpaceOem = 0xF, ///< OEM Objects Namespace | ||
EObjNameSpaceSmbios, ///< SMBIOS Objects Namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not introduce a new namespace. Can we use EObjNameSpaceArchCommon instead?
The reason for this is that we do not want duplication of information between some common ACPI and SMBIOS objects.
A good example is the information required for generating Type4 and Type7 SMBIOS tables. Most of the information therein is already available with the CM objects that we have defined for supporting PPTT table generation.
I understand that there is some additional information required for Type4 and Type7, but that can be added to the existing CM objects wherever necessary. These additional fields can remain unpopulated should
an implementation desire only ACPI tables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that makes sense.
} | ||
} | ||
|
||
DEBUG ((DEBUG_ERROR, "%a: Returning %r\n", __FUNCTION__, Status)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe these logs could be changed to DEBUG_VERBOSE. Also these log changes should be a separate patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that, this was an accident.
ETableGeneratorNameSpaceStd, \ | ||
TableId \ | ||
) | ||
#define MAX_SMBIOS_HANDLES (255) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we introduce a PCD PcdMaxSmbiosHandles to configure this? If so, please add a note to indicate that a large value may degrade the performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can do that.
/** Add a new entry to the SMBIOS table Map. | ||
@param [in] Smbios SMBIOS Protocol pointer. | ||
@param [in] SmbiosHandle SMBIOS Handle to be added, if the value SMBIOS_HANDLE_PI_RESERVED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the documentation for the SmbiosHandle parameter is out of sync with the code. Also, do we need a pointer to the SMBIOS handle?
If the intention is that we might need to extend the functionality to generate the handle in this function and return, then please add a comment to that effect. Then again the order of invocation of the table installation and handle generation would need to change in BuildXXX functions.
Which brings us back to should we support the handle generation in this function in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
acknowledged. This was a vestige of the initial implementation.
No, this code isn't meant to generate new handles, your code that setup the dispatcher ensures that any dependency between tables is taken care of.
IN EFI_SYSTEM_TABLE *SystemTable | ||
STATIC | ||
VOID | ||
AcpiTableProtocolReady ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be part of another patch (may be before this patch)?
Also can you explain why we need this notification? Can we not use the DEPEX dependency to dispatch?
Or is this required to satisfy the dispatch for a use case wherein only ACPI tables are present and SMBIOS support is not provided ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also can we keep the code from line 1186 to 1237 as part of the original DynamicTableManagerDxeInitialize() ?
I see that should not need changing, right?
Furthermore, the same code is again repeated in SmbiosProtocolReady(), see line 1281 to 1332.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, in systems that don't do ACPI but only SMBIOS the driver won't get dispatched and the SMBIOS tables don't get installed hence the move toward notification and not just DEPEX.
I can remove this from this patch and introduce the Protocol notification and split in the next patch in the series.
DynamicTablesPkg: Split the ACPI and SMBIOS table generators
Are you ok with splitting the ACPI and SMBIOS table generators into their own files? Or would you rather just have everything in DynamicTablesManagerDxe.c ?
@samimujawar
if there is a generator for that table that hasn't been called yet. | ||
@param [in] Smbios Pointer to the SMBIOS protocol. | ||
@param [in] SmbiosHandle Pointer to an SMBIOS handle (either generated by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comment in SmbiosTableFactory::AddSmbiosHandle()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
acknowledged
@return SMBIOS handle of the table associated with SmbiosTableId and | ||
CmObjectToken if found. Otherwise, returns 0xFFFF. | ||
**/ | ||
typedef UINT16 (EFIAPI *EDKII_DYNAMIC_TABLE_FACTORY_SMBIOS_TABLE_GET_HANDLE_EX)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use SMBIOS_HANDLE as the return type? If so please also update the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
acknowledged
777b9c1
to
0a568f5
Compare
Thanks for the review @samimujawar , please have a look at the latest push and let me know, I have dropped the generator patches and the patch for adding a new namespace. |
if (EFI_ERROR (Status)) { | ||
ASSERT_EFI_ERROR (Status); | ||
return Status; | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be
return Status;
&SmbiosTableInfo, | ||
&SmbiosTableCount | ||
); | ||
if (EFI_ERROR (Status)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this condition should be modified to
if (EFI_ERROR (Status)) {
if (Status != EFI_NOT_FOUND)) {
// If the SMBIOS table list is not found the platform may not be publishing SMBIOS tables
// Log a warning and return success.
DEBUG (( DEBUG_WARNING, "WARNING: Failed to get SMBIOS table List. Status = %r\n", Status));
Status = EFI_SUCCESS;
} else {
DEBUG ((
DEBUG_ERROR,
"ERROR: Failed to get SMBIOS Table List. Status = %r\n",
Status
));
}
return Status;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if (Status != EFI_NOT_FOUND))
test looks to be backwards? From the comment, we want to be warning and returning success if the table list is not found and returning the error otherwise, but the code is doing the opposite.
**/ | ||
typedef EFI_STATUS (*SMBIOS_TABLE_GENERATOR_BUILD_TABLE) ( | ||
IN CONST SMBIOS_TABLE_GENERATOR *Generator, | ||
IN CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *CONST TableFactoryProtocol, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CmObjToken is not documented.
**/ | ||
typedef EFI_STATUS (*SMBIOS_TABLE_GENERATOR_FREE_TABLE) ( | ||
IN CONST SMBIOS_TABLE_GENERATOR *Generator, | ||
IN CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *CONST TableFactoryProtocol, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the documentation for this parameter.
**/ | ||
typedef EFI_STATUS (*SMBIOS_TABLE_GENERATOR_BUILD_TABLEEX) ( | ||
IN CONST SMBIOS_TABLE_GENERATOR *Generator, | ||
IN CONST EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL *CONST TableFactoryProtocol, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the documentation for the function pointer parameters. Same for the other function pointers below.
EFI_EVENT AcpiEvent; | ||
EFI_EVENT SmbiosEvent; | ||
|
||
AcpiEvent = EfiCreateProtocolNotifyEvent ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to also explain in the commit message why the Protocol Notify messages are being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to run the CI on the patches, but this is would be only to clean some small bits. Otherwise the structure looks good to me
/** | ||
Initialise the SMBIOS table dispatcher. | ||
@param SmbiosTableInfo Pointer to the list of SMBIOS tables to be installed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: missing @param [in]
is less than the Object size for the | ||
requested object. | ||
**/ | ||
extern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it have be extern ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think extern can be dropped.
|
||
Found = FALSE; | ||
|
||
// Update the dospatcher state to dispatched. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dispatcher
// for installation and update the dispatcher status. | ||
for (Index = 0; Index < SmbiosTableCount; Index++) { | ||
TableType = SmbiosTableInfo[Index].TableType; | ||
ASSERT (mSmBiosDispatcher[TableType].State != StPresent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to add an ASSERT for
TableType < MAX_SMBIOS_TABLES
// First dispatch the default ordered SMBIOS tables | ||
for (Index = 0; Index < SmbiosTableCount; Index++) { | ||
TableType = SmbiosTableInfo[Index].TableType; | ||
if (mSmBiosDispatcher[TableType].Order != OrderDef) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should assert that TableType < MAX_SMBIOS_TABLES
. Same comment for InitSmbiosTableDispatcher()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have addressed all the feedback for the first 4 patches in this series and generated a PR at #513. If PR#513[https://github.com//pull/513] looks good then we can merge that in the staging branch.
Once @gmahadevan has addressed the remaining changes in this PR, we can merge this PR as well.
Also note, I have aligned the dynamictables-reorg branch with tianocore/edk2/master branch.
0a6087a
to
e4140a5
Compare
f1cc78e
to
f862e52
Compare
Dynamic Tables Framework currently supports Arm Architecture. This patch introduces a new staging branch for Dynamic Tables Framework to: - Reorganise the code to streamline adoption by other architectures - Introduce Dynamic Tables support for RISC-V architecture - Integrate Dynamic SMBIOS support. The description is in the Readme.md file. Please create the following branches: 1. edk2-staging Repo URL: https://github.com/tianocore/edk2-staging.git Branch Name: dynamictables-reorg 2. edk2-platforms Repo URL: https://github.com/tianocore/edk2-platforms.git Branch Name: devel-dynamictables-reorg Signed-off-by: Sami Mujawar <[email protected]> Reviewed-by: Laszlo Ersek <[email protected]> Reviewed-by: Leif Lindholm <[email protected]>
SMBIOS Structure 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. Therefore, define a new type 'SMBIOS_TABLE_TYPE' that represents a SMBIOS structure type and include it in the definition of CM_STD_OBJ_SMBIOS_TABLE_INFO. Acked-by: Abner Chang <[email protected]> Signed-off-by: Sami Mujawar <[email protected]> Cc: Alexei Fedorov <[email protected]> Cc: Pierre Gondois <[email protected]> Cc: Girish Mahadevan <[email protected]> Cc: Jeff Brasen <[email protected]> Cc: Ashish Singhal <[email protected]> Cc: Nick Ramirez <[email protected]> Cc: William Watson <[email protected]> Cc: Abner Chang <[email protected]> Cc: Samer El-Haj-Mahmoud <[email protected]> Cc: Jose Marinho <[email protected]>
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 <[email protected]> Cc: Alexei Fedorov <[email protected]> Cc: Pierre Gondois <[email protected]> Cc: Girish Mahadevan <[email protected]> Cc: Jeff Brasen <[email protected]> Cc: Ashish Singhal <[email protected]> Cc: Nick Ramirez <[email protected]> Cc: William Watson <[email protected]> Cc: Abner Chang <[email protected]> Cc: Samer El-Haj-Mahmoud <[email protected]> Cc: Jose Marinho <[email protected]>
Update the SMBIOS table dispatcher dependency table to add the table dependencies for SMBIOS table Type 19, Type 20, Type 27, Type 35 and Type 37. The SMBIOS table Type 35 can have dependency on 6 other SMBIOS tables. Therefore, increase the MAX_SMBIOS_DEPENDENCY to 6, and also update the SMBIOS table dispatcher table accordingly. Signed-off-by: Sami Mujawar <[email protected]> Cc: Alexei Fedorov <[email protected]> Cc: Pierre Gondois <[email protected]> Cc: Girish Mahadevan <[email protected]> Cc: Jeff Brasen <[email protected]> Cc: Ashish Singhal <[email protected]> Cc: Nick Ramirez <[email protected]> Cc: William Watson <[email protected]> Cc: Abner Chang <[email protected]> Cc: Samer El-Haj-Mahmoud <[email protected]> Cc: Jose Marinho <[email protected]>
Some SMBIOS tables do not have a fixed dependency and can depend on any other SMBIOS tables. Therefore, the SMBIOS dispatcher cannot define a fixed sequence for dispatching these tables. A possible solution is to defer the dispatch of such SMBIOS tables towards the end, assuming that the dependent SMBIOS tables would have been dispatched by then. Therefore, introduce a dispatch order attribute such that SMBIOS tables that have a fixed dependency sequence are configured as Default Ordered, and the SMBIOS tables that do not have a fixed dependency have an Order attribute specifying an Order Level. An Order Level is used to sequence the dispatch of Ordered SMBIOS tables. The Default Ordered SMBIOS tables are dispatched first and a dependency walk is performed to dispatch the dependent tables by iterating through the SMBIOS_TABLE_DISPATCHER.Dependency[]. Once all Default ordered SMBIOS tables have been dispatched, the Ordered SMBIOS tables would be scheduled for dispatch in increasing order as of the Order Level, e.g. OrderL1, OrderL2, ... Note: The dispatcher does not perform a dependency walk for the Ordered SMBIOS tables as the expectation is that the dependent SMBIOS tables would be already dispatched. A top level dispatch function DispatchSmbiosTables() has been introduced to schedule the dispatch of Default Ordered and Ordered SMBIOS tables. Signed-off-by: Sami Mujawar <[email protected]> Cc: Alexei Fedorov <[email protected]> Cc: Pierre Gondois <[email protected]> Cc: Girish Mahadevan <[email protected]> Cc: Jeff Brasen <[email protected]> Cc: Ashish Singhal <[email protected]> Cc: Nick Ramirez <[email protected]> Cc: William Watson <[email protected]> Cc: Abner Chang <[email protected]> Cc: Samer El-Haj-Mahmoud <[email protected]> Cc: Jose Marinho <[email protected]>
Add a new definition for invalid or non-existent handle. Signed-off-by: Girish Mahadevan <[email protected]>
Add the SMBIOS Table generator code to the DynamicTablesPkg. This change includes adding new logic to the DynamicTableManager to process and add SMBIOS tables and augmenting the existing SMBIOS Factory generator to include installing multiple SMBIOS tables . Signed-off-by: Girish Mahadevan <[email protected]>
There are 2 changes in this commit: 1. Use ProtocolNotify Event for generating the SMBIOS and ACPI tables and don't include the ACPI or SMBIOS protocols in the Depex section for DynamicTableManagerDxe Driver. This is because if a platforms doesn't do ACPI based boot or if doesn't want to install SMBIOS tables we still want the DynamicTableManager to be dispatched to install the SMBIOS or ACPI tables respectively. 2. Split the SMBIOS and ACPI table generators into their own files. Signed-off-by: Girish Mahadevan <[email protected]>
f862e52
to
defe05e
Compare
Hello @samimujawar @pierregondois |
Is there any specific reason why splitting the table generators is the final commit? It feels messy to add the SMBIOS generator then to immediately move it in the next commit. Moving the ACPI table generator out of DynamicTableManagerDxe.c as the first commit then adding the SMBIOS generator feels a neater approach. |
@param [in] GeneratorId Smbios Table Generator Id. | ||
@retval EFI_SUCCESS Successfully added/generated the handle. | ||
@retval EFI_OUT_OF_RESOURCESNULL Failure to add/generate the handle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "EFI_OUT_OF_RESOURCESNULL".
@param [in] CmObjectToken The CM Object token used to generate the SMBIOS | ||
record. | ||
@retval EFI_SUCCESS Success. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return value documentation is incorrect; this function is returning a pointer.
@param [in] CmObjectToken Token of the CM_OBJECT used to build the SMBIOS Table. | ||
@return SMBIOS handle of the table associated with SmbiosTableId and | ||
CmObjectToken if found. Otherwise, returns 0xFFFF. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be Otherwise, returns SMBIOS_HANDLE_INVALID.
?
@param [in] CmObjectToken Token of the CM_OBJECT used to build the SMBIOS Table. | ||
@return SMBIOS handle of the table associated with SmbiosTableId and | ||
CmObjectToken if found. Otherwise, returns 0xFFFF. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be Otherwise, returns SMBIOS_HANDLE_INVALID.
?
@param [in] CmObjectToken The CM Object token used to generate the SMBIOS | ||
record. | ||
@retval EFI_SUCCESS Success. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return value documentation is incorrect; this function is returning a pointer.
@gmahadevan I will fixup the above feedback and merge the changes shortly. |
22c9da5
to
2d1c8e7
Compare
@gmahadevan I have pushed the changes on the staging branch. |
Coming this PR as the changes have been merged at https://github.com/tianocore/edk2-staging/commits/dynamictables-reorg/ |
v2 of this PR.
Dropped the 3 generators.
Dropped the commit to add a new namespace.
Dropped the extra commit to define an extern call (not needed)
Commits included:
DynamicTablesPkg: Split the ACPI and SMBIOS table generators
DynamicTablesPkg: Add SMBIOS table generation
MdePkg/SmBios.h: Add New definition for Invalid Handle
DynamicTablesPkg: Add Ordered dispatch support for SMBIOS tables
DynamicTablesPkg: Update SMBIOS dispatcher dependency table
DynamicTablesPkg: Add SMBIOS table dispatcher
DynamicTablesPkg: Define a SMBIOS Structure/Table type
v1
Initial set of changes to add support for SMBIOS table generation that has the mods to the DynamicTablesPkg framework to install SMBIOS tables and 3 SMBIOS table generators for Type 16/17/19
DynamicTablesPkg: Smbios Memory Array Mapped Address (Type 19)
DynamicTablesPkg: Smbios Physical Memory Array (Type 16)
DynamicTablesPkg: Smbios Memory Device (Type 17)
DynamicTablesPkg: Introduce new namespace for SMBIOS Objects
DynamicTablesPkg: Split the ACPI and SMBIOS table generators
DynamicTablesPkg: Add SMBIOS table generation
DynamicTablesPkg: Add Ordered dispatch support for SMBIOS tables
DynamicTablesPkg: Update SMBIOS dispatcher dependency table
DynamicTablesPkg: Add extern call to build SMBIOS table
DynamicTablesPkg: Add SMBIOS table dispatcher
DynamicTablesPkg: Define a SMBIOS Structure/Table type
DynamicTablesPkg: SmbiosStringLib: Add new helper macro