From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.27897.1632726870639054768 for ; Mon, 27 Sep 2021 00:14:31 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DCEC6D6E; Mon, 27 Sep 2021 00:14:23 -0700 (PDT) Received: from [10.57.95.149] (unknown [10.57.95.149]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 1B2093F70D; Mon, 27 Sep 2021 00:14:22 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v1 06/10] DynamicTablesPkg: Add Configuration Manager Object parser To: Joey Gouly , "devel@edk2.groups.io" , Sami Mujawar , Alexei Fedorov References: <20210623110525.6171-1-Pierre.Gondois@arm.com> <20210623110525.6171-7-Pierre.Gondois@arm.com> From: "PierreGondois" Message-ID: Date: Mon, 27 Sep 2021 08:14:21 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US 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. > >> + {"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 > >> + >> + /// Count of items >> + UINTN ItemCount; >> +} CM_OBJ_PARSER_ARRAY; >> + >> +#endif // CONFIGURATION_MANAGER_OBJECT_PARSER_H_ >> diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf b/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf >> index 5435f74aa0b8..abbf4bc38cab 100644 >> --- a/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf >> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf >> @@ -15,6 +15,8 @@ [Defines] >> LIBRARY_CLASS = TableHelperLib >> >> [Sources] >> + ConfigurationManagerObjectParser.c >> + ConfigurationManagerObjectParser.h >> TableHelper.c >> >> [Packages] > Need to update the copyright year. > > Otherwise: > Reviewed-by: Joey Gouly > > Thanks, > Joey Regards, Pierre