Hi again,

Replies inline,

Hi Joey,
Thanks for the review, I answered inline:


On 9/24/21 9:56 AM, Joey Gouly wrote:

Hi,

This looks good to me!

[...]

+
+/** A parser for EArmObjFixedFeatureFlags.
+*/
+STATIC CONST CM_OBJ_PARSER CmArmFixedFeatureFlagsParser[] = {
+ {"Flags", 4, "0x%x", NULL}
+};
+
+/** A parser for EArmObjItsGroup.
+*/
+STATIC CONST CM_OBJ_PARSER CmArmItsGroupNodeParser[] = {
+ {"GTBlockTimerFrameToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL},

This should just be Token, not GTBlockTimerFrameToken.

The name of the field is 'GTBlockTimerFrameToken', cf
https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Include/ArmNameSpaceObjects.h#L394
I am not sure I understand why this should 'Token' instead.

You linked to the `CmArmGTBlockInfo` struct, but I was talking about `CmArmItsGroupNode`:
https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Include/ArmNameSpaceObjects.h#L464

+ {"ItsIdCount", 4, "0x%x", NULL},
+ {"ItsIdToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL}
+};
+

[...]

diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.h b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.h
new file mode 100644
index 000000000000..e229df7095d9
--- /dev/null
+++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.h

[...]

+/**
+ The CM_OBJ_PARSER structure describes the fields of an CmObject and
+ provides means for the parser to interpret and trace appropriately.
+
+ ParseAcpi() uses the format string specified by 'Format' for tracing
+ the field data.
+*/
+typedef struct CmObjParser CM_OBJ_PARSER;
+struct CmObjParser {
+
+ /// String describing the Cm Object
+ CONST CHAR8* NameStr;
+
+ /// The length of the field.
+ UINT32 Length;
+
+ /// Optional Print() style format string for tracing the data. If not
+ /// used this must be set to NULL.
+ CONST CHAR8* Format;
+
+ /// Optional pointer to a print formatter function which
+ /// is typically used to trace complex field information.
+ /// If not used this must be set to NULL.
+ /// The Format string is passed to the PrintFormatter function
+ /// but may be ignored by the implementation code.
+ FNPTR_PRINT_FORMATTER PrintFormatter;
+
+ /// Optional pointer to print the fields of another CM_OBJ_PARSER
+ /// structure. This is useful to print sub-structures.
+ CONST CM_OBJ_PARSER *SubObjParser;
+
+ /// Count of items in the SubObj.
+ UINTN SubObjItemCount;

The SubObjParser doesn't actually seem to be used by any of the objects? (Unless I misread when reading the list of them..)

The SubObjParser field is effectively not currently used. It will be
used in a later patch,
cf the 'UsageCounterRegister' field of
https://edk2.groups.io/g/devel/message/76954

Ok, makes sense!

Thanks,
Joey
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.