From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from EUR04-DB3-obe.outbound.protection.outlook.com (EUR04-DB3-obe.outbound.protection.outlook.com [40.107.6.42]) by mx.groups.io with SMTP id smtpd.web11.201.1642610674681875036 for ; Wed, 19 Jan 2022 08:44:35 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@armh.onmicrosoft.com header.s=selector2-armh-onmicrosoft-com header.b=4Djpc3px; spf=pass (domain: arm.com, ip: 40.107.6.42, mailfrom: sami.mujawar@arm.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=StRYNIf9GhHV64PwO65teKNOMRwqwEJgycluPcL0vQY=; b=4Djpc3pxQZgAerUZVXhXT3dcC2ns2X19Ge41BOXqqwNIZ/xhehkKxcBAfJWqj7TA6aQZ0FQZJtL1+kFV7GbytdwgXOIb6/DSX+jWrRqot8nfvICS+rd8asv+sfuacq6chfqOEzqJFmKG7Wd5CMem0enLtq8nBbvRKfGJZtjq+Sc= Received: from AS8PR04CA0178.eurprd04.prod.outlook.com (2603:10a6:20b:331::33) by DB6PR0802MB2568.eurprd08.prod.outlook.com (2603:10a6:4:9f::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4888.11; Wed, 19 Jan 2022 16:44:30 +0000 Received: from VE1EUR03FT017.eop-EUR03.prod.protection.outlook.com (2603:10a6:20b:331:cafe::2c) by AS8PR04CA0178.outlook.office365.com (2603:10a6:20b:331::33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4909.8 via Frontend Transport; Wed, 19 Jan 2022 16:44:29 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 63.35.35.123) smtp.mailfrom=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com;dmarc=pass action=none header.from=arm.com; Received-SPF: Pass (protection.outlook.com: domain of arm.com designates 63.35.35.123 as permitted sender) receiver=protection.outlook.com; client-ip=63.35.35.123; helo=64aa7808-outbound-1.mta.getcheckrecipient.com; Received: from 64aa7808-outbound-1.mta.getcheckrecipient.com (63.35.35.123) by VE1EUR03FT017.mail.protection.outlook.com (10.152.18.90) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4909.7 via Frontend Transport; Wed, 19 Jan 2022 16:44:29 +0000 Received: ("Tessian outbound 31aeb3346a45:v113"); Wed, 19 Jan 2022 16:44:29 +0000 X-CheckRecipientChecked: true X-CR-MTA-CID: 808c2f9a8d5e57e3 X-CR-MTA-TID: 64aa7808 Received: from 2cce5a86ec55.1 by 64aa7808-outbound-1.mta.getcheckrecipient.com id A8874FCD-12E1-4588-8D7E-945F0F1AA686.1; Wed, 19 Jan 2022 16:44:19 +0000 Received: from EUR02-HE1-obe.outbound.protection.outlook.com by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id 2cce5a86ec55.1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384); Wed, 19 Jan 2022 16:44:19 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=LVv5kBnq3S0lSvxgmS67W/u4SZ02nB7xmGdm0BZpgPKdWYqQmdMg/5dG5vh8SOB9QKuFEooqDXo0O06CJ7SpHmKd3MxbpSBIDrc3Jhke1BbZWYlr5bMlQ9azwpSBL7uaMFXUrb48RL96bpyEBnZOgezBDM4GfWcVd5N0lGU4OzJbxdVaDXXbjdR4YIC+VMvyfvM4iuc9GsOI+CaLDomX7TiU05F43HwcaMDtQyM7B9p/6/OSROQKdyPVZwma4OQ9vU5LHJda/OqgawpK6crWufdeO1EUKsp9IStnxA5xp0/jrDSBNroFhIGPMbsnLpbQVOS/mNUA88Jsf/lrSAM2wQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=StRYNIf9GhHV64PwO65teKNOMRwqwEJgycluPcL0vQY=; b=D8Ik5t2tx0nezurf0X4l5+TmeXtaxVWyNeQIr2ZCFXed9TTXYpsPvFaypPpRinOWE9S+y/VhywtV0EAHViDUZuXZQ6gXGXeUen3BGVPQuwOhP1FE+NqWOWrJ4YPAe3eETQDBAHG8Y4d6UDJJIZ/T9ASvEHpsOGSQ/jX2vOKWJXphwb4bQRXs4yn7umpMJv/rkqrokGwX7epb403QDCpyA4NEulM90kxUzbbYU9PDlcjkxBfoccST0eoXRkNMA988Bic4iwGuazlVZ2OFxt0c0c/lywGbELMubSMAh/gsUQ8wZK0RWkucS/VKv7iwGRcNausOukI5hzyc4Yr4+xn7PA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=StRYNIf9GhHV64PwO65teKNOMRwqwEJgycluPcL0vQY=; b=4Djpc3pxQZgAerUZVXhXT3dcC2ns2X19Ge41BOXqqwNIZ/xhehkKxcBAfJWqj7TA6aQZ0FQZJtL1+kFV7GbytdwgXOIb6/DSX+jWrRqot8nfvICS+rd8asv+sfuacq6chfqOEzqJFmKG7Wd5CMem0enLtq8nBbvRKfGJZtjq+Sc= Authentication-Results-Original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; Received: from AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) by HE1PR0802MB2571.eurprd08.prod.outlook.com (2603:10a6:3:e2::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4888.11; Wed, 19 Jan 2022 16:44:16 +0000 Received: from AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::34b6:4579:9e43:6b95]) by AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::34b6:4579:9e43:6b95%5]) with mapi id 15.20.4888.014; Wed, 19 Jan 2022 16:44:16 +0000 Subject: Re: [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser To: Abdul Lateef Attar , devel@edk2.groups.io Cc: Ray Ni , Zhichao Gao , nd References: <20211219144437.3721-1-abdattar@amd.com> <20211219144437.3721-2-abdattar@amd.com> From: "Sami Mujawar" Message-ID: <7dac633c-4bae-460d-0998-7da08b459023@arm.com> Date: Wed, 19 Jan 2022 16:44:11 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.0.1 In-Reply-To: <20211219144437.3721-2-abdattar@amd.com> X-ClientProxiedBy: LNXP265CA0057.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:5d::21) To AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: 85b11b6d-3c1f-411e-1613-08d9db6af649 X-MS-TrafficTypeDiagnostic: HE1PR0802MB2571:EE_|VE1EUR03FT017:EE_|DB6PR0802MB2568:EE_ X-Microsoft-Antispam-PRVS: x-checkrecipientrouted: true NoDisclaimer: true X-MS-Oob-TLC-OOBClassifiers: OLM:10000;OLM:10000; X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam-Untrusted: BCL:0; X-Microsoft-Antispam-Message-Info-Original: vtp8S9JHxO+7AA+/dhcRsgXp91B5mPNdqCRIwFNzMYvFAIyknXwrUVOoSMh8JVISJn7XMsAfSIMEZQWYNLQHyqgo/Nx+wByGWbxAHF8mgLU/b5vxPTZRBX77Qyp7Oylo68bwyVUA7cZ5v/ljhjsn9BwP/LLr8Mr5cgiOHX5+9o4pRZtG9HfZagM41FhgBYaYzqY4ljePGWoJAbD8NTBovLIxrsiM76dh/ogjE+WUvKpcn8EMEL4ffclub+7mjakQkFoL88pZCH6XR7uAOsjtfTV/8NEm2/etfg4KV0ZizzTAOKNeS4LQVlUj1Wv+rzPXYeY+nHaoFEBWYKgklj6lSsL9lTIutXGrWjxAn5fUqk783vErPHzuHyvyuRagm13/x6jPjC8ZJ5zlKcEUfXlqM1vU61Z4CDuOwx3ErF+S0Inx1vMVSFgSQAZo6MWRpKjESV3li9Qm6lhOVlhhpQ3l8GoiBL2psiVi/ZP6tSElNmvgbpuKpP6DSqszNSDQUzNWT+wVGBUQCdi8PFcC3Y8oPNcNi9xyMdtb0bMYDJ8l5JkhiUMrBV5+NgTsVW8AAUpDfC1xSfKOsJLH570s1djHhVT+8A4ztn/B15L+8X2aroMhHJG/8wEOYWoucpiF6D775raW7gizww/UEkwmrkro2zGIJWRBeHawhHUP5cipIlFBwrbZhsfsbRbLJUnYUZuSU8X4/aL9ZudTsv5HIiA8bByYQbmaQDEDlUmD0dPU4HmBdW1+cc7GxGSJpf+FMsfQ238PFw+sCKIRQJLWPPdMNQ== X-Forefront-Antispam-Report-Untrusted: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:AS8PR08MB6806.eurprd08.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(366004)(66946007)(83380400001)(33964004)(31696002)(6506007)(316002)(6512007)(8676002)(36756003)(6666004)(38350700002)(66556008)(186003)(66476007)(26005)(53546011)(86362001)(30864003)(44832011)(2616005)(2906002)(38100700002)(5660300002)(6486002)(54906003)(31686004)(52116002)(508600001)(4326008)(8936002)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0802MB2571 Original-Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; Return-Path: Sami.Mujawar@arm.com X-EOPAttributedMessage: 0 X-MS-Exchange-Transport-CrossTenantHeadersStripped: VE1EUR03FT017.eop-EUR03.prod.protection.outlook.com X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id-Prvs: 10c267ac-ab02-4a0f-8938-08d9db6aee36 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: zz7tk3LhvtGtGec4PanBCnL5Ib0ZAL6h8k7vsSVLoo5lE+sGZz1I5FxUD2ehtPXJqAdMD1SBanfvyFTj1MG+Ywh9x3fCLN6cMFQPwRdlm869W6AaTogTJEmdxzOub9xlL7mMWxI+zKBLB50sRFSCLNOmUaWxYOkb2mG6sc8kaWl3SCXxc+cQ8NzUG8rEygxLQWgnNh1pB+jjEDjEdRohvorzrZudrurvRNX4ZxViDHcw61vXD8N7rOBlI66Q8Bv0EOmFiu3+r2vjKnyrdQARnmOpObH4t0nntgVuFBAEj/1r4tezWMwCQAShQu89UU1GGDquGkXUaedUx4YaB3o4xvprW5xctsUZeHDEa2Lbk4xzUnzkFbjvseN5WvCCRnU/WVkSRaR1HIJ3Q/GO7qc377NipO58DqRCbUU8HDodOKb9LC+4IyLafSeMgn9OfALDVPDaxXsfvs/ZZivoIDcwUbkZ5eGKK6kL5vxNszAoBuWy022l8625frpSZqjmPbtKSv4M3ZuucUotuPLvfykv2Je/v53rPWxEcO4KbcYJoXT6js6PhwJAbyQTtyVsDeBwlmImFE+aJMK3v6w1iSQiHiQaOaHgCBaJ6pLzP66eEdmqGWvmwWfLowR63tsOUjr7nYYicR0ZQWLM8sXmvXZNaD3fM+pZWUaJ1tsLsBlqZFzk+W9mXz9U3cDsdIVXYKfyp5QGn+vCEai4Tga0/j+cHrG1JNnNz8SYcF8ml/VoQ04= X-Forefront-Antispam-Report: CIP:63.35.35.123;CTRY:IE;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:64aa7808-outbound-1.mta.getcheckrecipient.com;PTR:ec2-63-35-35-123.eu-west-1.compute.amazonaws.com;CAT:NONE;SFS:(4636009)(36840700001)(46966006)(81166007)(6666004)(6506007)(86362001)(26005)(2906002)(336012)(83380400001)(70206006)(47076005)(356005)(70586007)(508600001)(4326008)(186003)(36756003)(316002)(44832011)(54906003)(6512007)(31696002)(8936002)(36860700001)(2616005)(53546011)(6486002)(8676002)(30864003)(82310400004)(31686004)(5660300002)(33964004)(43740500002);DIR:OUT;SFP:1101; X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jan 2022 16:44:29.3230 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 85b11b6d-3c1f-411e-1613-08d9db6af649 X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f34e5979-57d9-4aaa-ad4d-b122a662184d;Ip=[63.35.35.123];Helo=[64aa7808-outbound-1.mta.getcheckrecipient.com] X-MS-Exchange-CrossTenant-AuthSource: VE1EUR03FT017.eop-EUR03.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0802MB2568 Content-Type: multipart/alternative; boundary="------------8AC8C231867F411ECD5B7C0A" Content-Language: en-GB --------------8AC8C231867F411ECD5B7C0A Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Hi Abdul, Thank you for providing a patch to add this feature to Acpiview. I have some minor feedback marked inline as [SAMI]. Regards, Sami Mujawar On 19/12/2021 02:44 PM, Abdul Lateef Attar wrote: > Adds ParseAcpiBitFields() which is based on > ParseAcpi() and capable of parsing the bit fields. > Supports parsing of UINT8, UINT16, UINT32 and UINT64 byte data. > > Cc: Ray Ni > Cc: Zhichao Gao > Cc: Sami Mujawar > Signed-off-by: Abdul Lateef Attar > --- > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 45 +++++ > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 185 ++++++++++++++++++++ > 2 files changed, 230 insertions(+) > > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > index 5c916a4720b8..83266e8ec2d3 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > @@ -2,6 +2,7 @@ > Header file for ACPI parser > > > > Copyright (c) 2016 - 2020, Arm Limited. All rights reserved. > > + Copyright (c) 2021, AMD Incorporated. All rights reserved. > > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > > > > @@ -251,6 +252,11 @@ typedef VOID (EFIAPI *FNPTR_FIELD_VALIDATOR)(UINT8 *Ptr, VOID *Context); > the field data. If the field is more complex and requires additional > > processing for formatting and representation a print formatter function > > can be specified in 'PrintFormatter'. > > + > > + ParseAcpiBitFields() uses AcpiParser structure to parse the bit fields. > > + It considers Length as a number of bits that need to be parsed. > > + Also, the Offset field will be considered as starting offset of the bitfield. > > + > > The PrintFormatter function may choose to use the format string > > specified by 'Format' or use its own internal format string. > > > > @@ -264,10 +270,12 @@ typedef struct AcpiParser { > > > /// The length of the field. > > /// (Byte Length column from ACPI table spec) > > + /// Length(in bits) of the bitfield if used with ParseAcpiBitFields(). > > UINT32 Length; > > > > /// The offset of the field from the start of the table. > > /// (Byte Offset column from ACPI table spec) > > + /// The Bit offset of the field if used with ParseAcpiBitFields(). > > UINT32 Offset; > > > > /// Optional Print() style format string for tracing the data. If not > > @@ -364,6 +372,43 @@ ParseAcpi ( > IN UINT32 ParserItems > > ); > > > > +/** > > + This function is used to parse an ACPI table bitfield buffer. > > + > > + The ACPI table buffer is parsed using the ACPI table parser information > > + specified by a pointer to an array of ACPI_PARSER elements. This parser > > + function iterates through each item on the ACPI_PARSER array and logs the ACPI table bitfields. > > + > > + This function can optionally be used to parse ACPI tables and fetch specific > > + field values. The ItemPtr member of the ACPI_PARSER structure (where used) > > + is updated by this parser function to point to the selected field data > > + (e.g. useful for variable length nested fields). > > + > > + @param [in] Trace Trace the ACPI fields TRUE else only parse the > > + table. > > + @param [in] Indent Number of spaces to indent the output. > > + @param [in] AsciiName Optional pointer to an ASCII string that describes > > + the table being parsed. > > + @param [in] Ptr Pointer to the start of the buffer. > > + @param [in] Length Length of the buffer pointed by Ptr. > > + @param [in] Parser Pointer to an array of ACPI_PARSER structure that > > + describes the table being parsed. > > + @param [in] ParserItems Number of items in the ACPI_PARSER array. > > + > > + @retval Number of bits parsed. > > +**/ > > +UINT32 > > +EFIAPI > > +ParseAcpiBitFields ( > > + IN BOOLEAN Trace, > > + IN UINT32 Indent, > > + IN CONST CHAR8 *AsciiName OPTIONAL, > > + IN UINT8 *Ptr, > > + IN UINT32 Length, > > + IN CONST ACPI_PARSER *Parser, > > + IN UINT32 ParserItems > > + ); > > + > > /** > > This is a helper macro to pass parameters to the Parser functions. > > > > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > index cb193a5ea449..94ee26f42ab9 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > @@ -2,12 +2,14 @@ > ACPI parser > > > > Copyright (c) 2016 - 2021, Arm Limited. All rights reserved. > > + Copyright (c) 2021, AMD Incorporated. All rights reserved. > > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > > > > #include > > #include > > #include > > +#include > > #include "AcpiParser.h" > > #include "AcpiView.h" > > #include "AcpiViewConfig.h" > > @@ -752,3 +754,186 @@ ParseAcpiHeader ( > > > return BytesParsed; > > } > > + > > +/** > > + This function is used to parse an ACPI table bitfield buffer. > > + > > + The ACPI table buffer is parsed using the ACPI table parser information > > + specified by a pointer to an array of ACPI_PARSER elements. This parser > > + function iterates through each item on the ACPI_PARSER array and logs the ACPI table bitfields. > > + > > + This function can optionally be used to parse ACPI tables and fetch specific > > + field values. The ItemPtr member of the ACPI_PARSER structure (where used) > > + is updated by this parser function to point to the selected field data > > + (e.g. useful for variable length nested fields). > > + > > + @param [in] Trace Trace the ACPI fields TRUE else only parse the > > + table. > > + @param [in] Indent Number of spaces to indent the output. > > + @param [in] AsciiName Optional pointer to an ASCII string that describes > > + the table being parsed. > > + @param [in] Ptr Pointer to the start of the buffer. > > + @param [in] Length Length in bytes of the buffer pointed by Ptr. > > + @param [in] Parser Pointer to an array of ACPI_PARSER structure that > > + describes the table being parsed. > > + @param [in] ParserItems Number of items in the ACPI_PARSER array. > > + > > + @retval Number of bits parsed. > > +**/ > > +UINT32 > > +EFIAPI > > +ParseAcpiBitFields ( > > + IN BOOLEAN Trace, > > + IN UINT32 Indent, > > + IN CONST CHAR8 *AsciiName OPTIONAL, > > + IN UINT8 *Ptr, > > + IN UINT32 Length, > > + IN CONST ACPI_PARSER *Parser, > > + IN UINT32 ParserItems > > + ) > > +{ > > + UINT32 Index; > > + UINT32 Offset; > > + BOOLEAN HighLight; > > + UINTN OriginalAttribute; > > + > > + UINT64 Data; > > + UINT64 BitsData; > > + > > + if ((Length == 0) || (Length > 8)) { > > + IncrementErrorCount (); > > + Print ( > > + L"\nERROR: Bitfield Length(%d) is zero or exceeding the 64 bit limit.\n", > > + Length > > + ); > > + return 0; > > + } > > + > > + // > > + // set local variables to suppress incorrect compiler/analyzer warnings > > + // > > + OriginalAttribute = 0; > > + Offset = 0; > > + > > + // Increment the Indent > > + gIndent += Indent; > > + > > + CopyMem ((VOID *)&BitsData, (VOID *)Ptr, Length); > > + if (Trace && (AsciiName != NULL)) { > > + HighLight = GetColourHighlighting (); > > + > > + if (HighLight) { > > + OriginalAttribute = gST->ConOut->Mode->Attribute; > > + gST->ConOut->SetAttribute ( > > + gST->ConOut, > > + EFI_TEXT_ATTR ( > > + EFI_YELLOW, > > + ((OriginalAttribute&(BIT4|BIT5|BIT6))>>4) > > + ) > > + ); > > + } > > + > > + Print ( > > + L"%*a%-*a :\n", > > + gIndent, > > + "", > > + (OUTPUT_FIELD_COLUMN_WIDTH - gIndent), > > + AsciiName > > + ); > > + if (HighLight) { > > + gST->ConOut->SetAttribute (gST->ConOut, OriginalAttribute); > > + } > > + } > > + > > + for (Index = 0; Index < ParserItems; Index++) { > > + if ((Offset + Parser[Index].Length) > (Length * 8)) { > > + // For fields outside the buffer length provided, reset any pointers > > + // which were supposed to be updated by this function call > > + if (Parser[Index].ItemPtr != NULL) { > > + *Parser[Index].ItemPtr = NULL; > > + } > > + > > + // We don't parse past the end of the max length specified > > + continue; > > + } > > + > > + if (Parser[Index].Length == 0) { > > + // don't parse the bitfield whose length is zero > > + continue; [SAMI] Is there a use-case where the bitfield length will be zero? I think any unused bits would be represented as Reserved. Considering this, Parser[Index].Length == 0 would mean an incorrect an incorrect description inACPI_PARSER, right? If so, the default case for the switch statement below would print "ERROR: %a: CANNOT PARSE THIS FIELD, Field Length". Can you check this, please? [/SAMI] > + } > > + > > + if (GetConsistencyChecking () && > > + (Offset != Parser[Index].Offset)) > > + { > > + IncrementErrorCount (); > > + Print ( > > + L"\nERROR: %a: Offset Mismatch for %s\n" > > + L"CurrentOffset = %d FieldOffset = %d\n", > > + AsciiName, > > + Parser[Index].NameStr, > > + Offset, > > + Parser[Index].Offset > > + ); > > + } > > + > > + // extract Bitfield data for the current item > > + Data = (BitsData >> Parser[Index].Offset) & ~(~0ULL << Parser[Index].Length); > > + > > + if (Trace) { > > + // if there is a Formatter function let the function handle > > + // the printing else if a Format is specified in the table use > > + // the Format for printing > > + PrintFieldName (2, Parser[Index].NameStr); > > + if (Parser[Index].PrintFormatter != NULL) { > > + Parser[Index].PrintFormatter (Parser[Index].Format, (UINT8 *)&Data); > > + } else if (Parser[Index].Format != NULL) { > > + // convert bit length to byte length > > + switch ((Parser[Index].Length + 7) >> 3) { > > + // print the data depends on byte size > > + case 1: > > + DumpUint8 (Parser[Index].Format, (UINT8 *)&Data); > > + break; > > + case 2: > > + DumpUint16 (Parser[Index].Format, (UINT8 *)&Data); > > + break; > > + case 3: > > + case 4: > > + DumpUint32 (Parser[Index].Format, (UINT8 *)&Data); > > + break; > > + case 5: > > + case 6: > > + case 7: > > + case 8: > > + DumpUint64 (Parser[Index].Format, (UINT8 *)&Data); > > + break; > > + default: > > + Print ( > > + L"\nERROR: %a: CANNOT PARSE THIS FIELD, Field Length = %d\n", > > + AsciiName, > > + Parser[Index].Length > > + ); > > + } // switch > > + } > > + > > + // Validating only makes sense if we are tracing > > + // the parsed table entries, to report by table name. > > + if (GetConsistencyChecking () && > > + (Parser[Index].FieldValidator != NULL)) > > + { > > + Parser[Index].FieldValidator ((UINT8 *)&Data, Parser[Index].Context); > > + } > > + > > + Print (L"\n"); > > + } // if (Trace) > > + > > + if (Parser[Index].ItemPtr != NULL) { > > + *Parser[Index].ItemPtr = (VOID *)(UINT8 *)&Data; [SAMI] ACPI_PARSER.ItemPtr is used to get a reference to the field data in the original ACPI table data. In the current case, Parser[Index].ItemPtr is being set to a local variable (i.e. Data). I don't think this is what we want. I think it would be better to not supportACPI_PARSER.ItemPtr for BitFields. I would recommend adding a comment to clarify that ItemPtr is not supported for BitFields in this function, as well as the documentation for ACPI_PARSER structure. [/SAMI] > > + } > > + > > + Offset += Parser[Index].Length; > > + } // for > > + > > + // Decrement the Indent > > + gIndent -= Indent; > > + return Offset; > > +} > --------------8AC8C231867F411ECD5B7C0A Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit

Hi Abdul,

Thank you for providing a patch to add this feature to Acpiview.

I have some minor feedback marked inline as [SAMI].

Regards,

Sami Mujawar


On 19/12/2021 02:44 PM, Abdul Lateef Attar wrote:
Adds ParseAcpiBitFields() which is based on
ParseAcpi() and capable of parsing the bit fields.
Supports parsing of UINT8, UINT16, UINT32 and UINT64 byte data.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
---
 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h |  45 +++++
 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 185 ++++++++++++++++++++
 2 files changed, 230 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index 5c916a4720b8..83266e8ec2d3 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -2,6 +2,7 @@
   Header file for ACPI parser

 

   Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.

+  Copyright (c) 2021, AMD Incorporated. All rights reserved.

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/

 

@@ -251,6 +252,11 @@ typedef VOID (EFIAPI *FNPTR_FIELD_VALIDATOR)(UINT8 *Ptr, VOID *Context);
   the field data. If the field is more complex and requires additional

   processing for formatting and representation a print formatter function

   can be specified in 'PrintFormatter'.

+

+  ParseAcpiBitFields() uses AcpiParser structure to parse the bit fields.

+  It considers Length as a number of bits that need to be parsed.

+  Also, the Offset field will be considered as starting offset of the bitfield.

+

   The PrintFormatter function may choose to use the format string

   specified by 'Format' or use its own internal format string.

 

@@ -264,10 +270,12 @@ typedef struct AcpiParser {
 

   /// The length of the field.

   /// (Byte Length column from ACPI table spec)

+  /// Length(in bits) of the bitfield if used with ParseAcpiBitFields().

   UINT32                   Length;

 

   /// The offset of the field from the start of the table.

   /// (Byte Offset column from ACPI table spec)

+  /// The Bit offset of the field if used with ParseAcpiBitFields().

   UINT32                   Offset;

 

   /// Optional Print() style format string for tracing the data. If not

@@ -364,6 +372,43 @@ ParseAcpi (
   IN UINT32             ParserItems

   );

 

+/**

+  This function is used to parse an ACPI table bitfield buffer.

+

+  The ACPI table buffer is parsed using the ACPI table parser information

+  specified by a pointer to an array of ACPI_PARSER elements. This parser

+  function iterates through each item on the ACPI_PARSER array and logs the ACPI table bitfields.

+

+  This function can optionally be used to parse ACPI tables and fetch specific

+  field values. The ItemPtr member of the ACPI_PARSER structure (where used)

+  is updated by this parser function to point to the selected field data

+  (e.g. useful for variable length nested fields).

+

+  @param [in] Trace        Trace the ACPI fields TRUE else only parse the

+                           table.

+  @param [in] Indent       Number of spaces to indent the output.

+  @param [in] AsciiName    Optional pointer to an ASCII string that describes

+                           the table being parsed.

+  @param [in] Ptr          Pointer to the start of the buffer.

+  @param [in] Length       Length of the buffer pointed by Ptr.

+  @param [in] Parser       Pointer to an array of ACPI_PARSER structure that

+                           describes the table being parsed.

+  @param [in] ParserItems  Number of items in the ACPI_PARSER array.

+

+  @retval Number of bits parsed.

+**/

+UINT32

+EFIAPI

+ParseAcpiBitFields (

+  IN BOOLEAN            Trace,

+  IN UINT32             Indent,

+  IN CONST CHAR8        *AsciiName OPTIONAL,

+  IN UINT8              *Ptr,

+  IN UINT32             Length,

+  IN CONST ACPI_PARSER  *Parser,

+  IN UINT32             ParserItems

+  );

+

 /**

    This is a helper macro to pass parameters to the Parser functions.

 

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
index cb193a5ea449..94ee26f42ab9 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
@@ -2,12 +2,14 @@
   ACPI parser

 

   Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.

+  Copyright (c) 2021, AMD Incorporated. All rights reserved.

   SPDX-License-Identifier: BSD-2-Clause-Patent

 **/

 

 #include <Uefi.h>

 #include <Library/UefiLib.h>

 #include <Library/UefiBootServicesTableLib.h>

+#include <Library/BaseMemoryLib.h>

 #include "AcpiParser.h"

 #include "AcpiView.h"

 #include "AcpiViewConfig.h"

@@ -752,3 +754,186 @@ ParseAcpiHeader (
 

   return BytesParsed;

 }

+

+/**

+  This function is used to parse an ACPI table bitfield buffer.

+

+  The ACPI table buffer is parsed using the ACPI table parser information

+  specified by a pointer to an array of ACPI_PARSER elements. This parser

+  function iterates through each item on the ACPI_PARSER array and logs the ACPI table bitfields.

+

+  This function can optionally be used to parse ACPI tables and fetch specific

+  field values. The ItemPtr member of the ACPI_PARSER structure (where used)

+  is updated by this parser function to point to the selected field data

+  (e.g. useful for variable length nested fields).

+

+  @param [in] Trace        Trace the ACPI fields TRUE else only parse the

+                           table.

+  @param [in] Indent       Number of spaces to indent the output.

+  @param [in] AsciiName    Optional pointer to an ASCII string that describes

+                           the table being parsed.

+  @param [in] Ptr          Pointer to the start of the buffer.

+  @param [in] Length       Length in bytes of the buffer pointed by Ptr.

+  @param [in] Parser       Pointer to an array of ACPI_PARSER structure that

+                           describes the table being parsed.

+  @param [in] ParserItems  Number of items in the ACPI_PARSER array.

+

+  @retval Number of bits parsed.

+**/

+UINT32

+EFIAPI

+ParseAcpiBitFields (

+  IN BOOLEAN            Trace,

+  IN UINT32             Indent,

+  IN CONST CHAR8        *AsciiName OPTIONAL,

+  IN UINT8              *Ptr,

+  IN UINT32             Length,

+  IN CONST ACPI_PARSER  *Parser,

+  IN UINT32             ParserItems

+  )

+{

+  UINT32   Index;

+  UINT32   Offset;

+  BOOLEAN  HighLight;

+  UINTN    OriginalAttribute;

+

+  UINT64  Data;

+  UINT64  BitsData;

+

+  if ((Length == 0) || (Length > 8)) {

+    IncrementErrorCount ();

+    Print (

+      L"\nERROR: Bitfield Length(%d) is zero or exceeding the 64 bit limit.\n",

+      Length

+      );

+    return 0;

+  }

+

+  //

+  // set local variables to suppress incorrect compiler/analyzer warnings

+  //

+  OriginalAttribute = 0;

+  Offset            = 0;

+

+  // Increment the Indent

+  gIndent += Indent;

+

+  CopyMem ((VOID *)&BitsData, (VOID *)Ptr, Length);

+  if (Trace && (AsciiName != NULL)) {

+    HighLight = GetColourHighlighting ();

+

+    if (HighLight) {

+      OriginalAttribute = gST->ConOut->Mode->Attribute;

+      gST->ConOut->SetAttribute (

+                     gST->ConOut,

+                     EFI_TEXT_ATTR (

+                       EFI_YELLOW,

+                       ((OriginalAttribute&(BIT4|BIT5|BIT6))>>4)

+                       )

+                     );

+    }

+

+    Print (

+      L"%*a%-*a :\n",

+      gIndent,

+      "",

+      (OUTPUT_FIELD_COLUMN_WIDTH - gIndent),

+      AsciiName

+      );

+    if (HighLight) {

+      gST->ConOut->SetAttribute (gST->ConOut, OriginalAttribute);

+    }

+  }

+

+  for (Index = 0; Index < ParserItems; Index++) {

+    if ((Offset + Parser[Index].Length) > (Length * 8)) {

+      // For fields outside the buffer length provided, reset any pointers

+      // which were supposed to be updated by this function call

+      if (Parser[Index].ItemPtr != NULL) {

+        *Parser[Index].ItemPtr = NULL;

+      }

+

+      // We don't parse past the end of the max length specified

+      continue;

+    }

+

+    if (Parser[Index].Length == 0) {

+      // don't parse the bitfield whose length is zero

+      continue;
[SAMI] Is there a use-case where the bitfield length will be zero? I think any unused bits would be represented as Reserved. Considering this, Parser[Index].Length == 0 would mean an incorrect an incorrect description in ACPI_PARSER, right?
If so, the default case for the switch statement below would print "ERROR: %a: CANNOT PARSE THIS FIELD, Field Length". Can you check this, please?
[/SAMI]
+    }

+

+    if (GetConsistencyChecking () &&

+        (Offset != Parser[Index].Offset))

+    {

+      IncrementErrorCount ();

+      Print (

+        L"\nERROR: %a: Offset Mismatch for %s\n"

+        L"CurrentOffset = %d FieldOffset = %d\n",

+        AsciiName,

+        Parser[Index].NameStr,

+        Offset,

+        Parser[Index].Offset

+        );

+    }

+

+    // extract Bitfield data for the current item

+    Data = (BitsData >> Parser[Index].Offset) & ~(~0ULL << Parser[Index].Length);

+

+    if (Trace) {

+      // if there is a Formatter function let the function handle

+      // the printing else if a Format is specified in the table use

+      // the Format for printing

+      PrintFieldName (2, Parser[Index].NameStr);

+      if (Parser[Index].PrintFormatter != NULL) {

+        Parser[Index].PrintFormatter (Parser[Index].Format, (UINT8 *)&Data);

+      } else if (Parser[Index].Format != NULL) {

+        // convert bit length to byte length

+        switch ((Parser[Index].Length + 7) >> 3) {

+          // print the data depends on byte size

+          case 1:

+            DumpUint8 (Parser[Index].Format, (UINT8 *)&Data);

+            break;

+          case 2:

+            DumpUint16 (Parser[Index].Format, (UINT8 *)&Data);

+            break;

+          case 3:

+          case 4:

+            DumpUint32 (Parser[Index].Format, (UINT8 *)&Data);

+            break;

+          case 5:

+          case 6:

+          case 7:

+          case 8:

+            DumpUint64 (Parser[Index].Format, (UINT8 *)&Data);

+            break;

+          default:

+            Print (

+              L"\nERROR: %a: CANNOT PARSE THIS FIELD, Field Length = %d\n",

+              AsciiName,

+              Parser[Index].Length

+              );

+        } // switch

+      }

+

+      // Validating only makes sense if we are tracing

+      // the parsed table entries, to report by table name.

+      if (GetConsistencyChecking () &&

+          (Parser[Index].FieldValidator != NULL))

+      {

+        Parser[Index].FieldValidator ((UINT8 *)&Data, Parser[Index].Context);

+      }

+

+      Print (L"\n");

+    } // if (Trace)

+

+    if (Parser[Index].ItemPtr != NULL) {

+      *Parser[Index].ItemPtr = (VOID *)(UINT8 *)&Data;
[SAMI]  ACPI_PARSER.ItemPtr is used to get a reference to the field data in the original ACPI table data.
In the current case, Parser[Index].ItemPtr is being set to a local variable (i.e. Data). I don't think this is what we want. I think it would be better to not support ACPI_PARSER.ItemPtr for BitFields.
I would recommend adding a comment to clarify that ItemPtr is not supported for BitFields in this function, as well as the documentation for ACPI_PARSER structure.
[/SAMI]

+    }

+

+    Offset += Parser[Index].Length;

+  } // for

+

+  // Decrement the Indent

+  gIndent -= Indent;

+  return Offset;

+}

--------------8AC8C231867F411ECD5B7C0A--