From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mx.groups.io with SMTP id smtpd.web10.9157.1642667823981584505 for ; Thu, 20 Jan 2022 00:37:04 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="unable to parse pub key" header.i=@intel.com header.s=intel header.b=E9JVOdqy; spf=pass (domain: intel.com, ip: 134.134.136.24, mailfrom: zhichao.gao@intel.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1642667824; x=1674203824; h=from:to:cc:subject:date:message-id:references: in-reply-to:mime-version; bh=LgpehUHACe3jDZeZbUNKKdhRAy49bnXnsSnemRJeMiw=; b=E9JVOdqyWxP28Fy/F3HjMGf0Rq1UpqjA/2TddfkzHr4MJrkVAf9O6TcO jJsqdF9c411XfW/UsZRsb9AG4my742cDfiBOGv3aPMwYjEIx/47sfBqFD nDa6Rp/9FeeoENNR3BbrpuBsPF1LK+JESFf7HAsx2s1fBAl1iaSP0r/JP sdaVc4SAbzGM+3V9BsruMZc+OS/6uz4hUSuMp/PkycmjtEVTn7lyfYfN5 n6sriTYHOaxtgLKKmI9JqJarNX/H/V18q891u03bBVKlGYK6jckHIyO2X qENmtdKxz3PHBaktZFASfDejMcWuvg1/JCI6XFfC8BHMN4STdTZJBE+++ A==; X-IronPort-AV: E=McAfee;i="6200,9189,10232"; a="245095125" X-IronPort-AV: E=Sophos;i="5.88,301,1635231600"; d="scan'208,217";a="245095125" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Jan 2022 00:37:03 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.88,301,1635231600"; d="scan'208,217";a="532679545" Received: from fmsmsx604.amr.corp.intel.com ([10.18.126.84]) by orsmga008.jf.intel.com with ESMTP; 20 Jan 2022 00:37:02 -0800 Received: from fmsmsx602.amr.corp.intel.com (10.18.126.82) by fmsmsx604.amr.corp.intel.com (10.18.126.84) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.20; Thu, 20 Jan 2022 00:37:02 -0800 Received: from FMSEDG603.ED.cps.intel.com (10.1.192.133) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.20 via Frontend Transport; Thu, 20 Jan 2022 00:37:02 -0800 Received: from NAM10-BN7-obe.outbound.protection.outlook.com (104.47.70.103) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2308.20; Thu, 20 Jan 2022 00:37:01 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=UmQZ+lQ00fBZoBXRjBaVZwVVg88piH2gMa16HnK6NFScMFdyP95d6aMPVnMKMohonWwNsS4obqJKgWc5R8Cj3LPVlSWPHJKwCvcomKx/YtdJonvUm0U/yDnbqsd5ffkgGf6yt01fS8/Flj8IXPeLgG50lfzd7NjpGh2biAU2eWeMU0bamU/Si2v9EgYHmvBafry888MsTE9ofsezHYib2XqpmZt2neRQ7T9CyqFWa4tsZDJgetCXHVbGOrorX9veZDz9ZAwn5Mu8hPLQZvW0uASZKxztwKxF7c8QdxWh4rNgNweBEB8fsrL3h2VOixkEYGzpOxUSaRAqcPT7VVfrBw== 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=aJtc7MoRQIrWw1VqH3Pu+4m3xIZWqLdNi8rrXN8fuio=; b=LghuQ9c5B6/GZAjn/yybgZJdXYMYySZc+MNmP0r5TU3jLOhSCiPU2ZWB1ELVAsj7qm+p5g8U5deiawcGB8RBgTMrZiqrqIxb9rFXs3UVNDwPN5XjSXVOMnPYyK1/0zy08SyR1VN3gNELRcbiWZIY20fAJgA5c0zt3sq2ECC0iA+X5gKe1p/zmpzf8vFV+masFyt2oqP8QsRMvzVJim5qTuOdkv4MYCIwKzrwkN/Qkmm7cr7WKBujZHJtOdNjNRsKdwmakFVh4Hg7xpyLr8+8ncSYurMYjhngQP1YiyjN+N8wG5Kyw8g3i+TGsVUaVTUyT8aIlYSlJZw2kUYDvwuBjA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none Received: from DM4PR11MB5277.namprd11.prod.outlook.com (2603:10b6:5:388::23) by DM6PR11MB2634.namprd11.prod.outlook.com (2603:10b6:5:c6::28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4909.8; Thu, 20 Jan 2022 08:36:59 +0000 Received: from DM4PR11MB5277.namprd11.prod.outlook.com ([fe80::6445:d5c9:8cc3:165f]) by DM4PR11MB5277.namprd11.prod.outlook.com ([fe80::6445:d5c9:8cc3:165f%7]) with mapi id 15.20.4888.014; Thu, 20 Jan 2022 08:36:59 +0000 From: "Gao, Zhichao" To: "Attar, AbdulLateef (Abdul Lateef)" , "devel@edk2.groups.io" , "sami.mujawar@arm.com" CC: "Ni, Ray" , nd Subject: Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser Thread-Topic: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser Thread-Index: AQHYDVPehUEVe7He/0iWCc8Yrcww8axqjaIAgACyLgCAAFEtkA== Date: Thu, 20 Jan 2022 08:36:59 +0000 Message-ID: References: <20211219144437.3721-1-abdattar@amd.com> <20211219144437.3721-2-abdattar@amd.com> <7dac633c-4bae-460d-0998-7da08b459023@arm.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_d4243a53-6221-4f75-8154-e4b33a5707a1_ActionId=e9e9a742-f934-4149-afcb-1ba4749f5ea6; MSIP_Label_d4243a53-6221-4f75-8154-e4b33a5707a1_ContentBits=0; MSIP_Label_d4243a53-6221-4f75-8154-e4b33a5707a1_Enabled=true; MSIP_Label_d4243a53-6221-4f75-8154-e4b33a5707a1_Method=Privileged; MSIP_Label_d4243a53-6221-4f75-8154-e4b33a5707a1_Name=Public-AIP 2.0; MSIP_Label_d4243a53-6221-4f75-8154-e4b33a5707a1_SetDate=2022-01-20T03:21:03Z; MSIP_Label_d4243a53-6221-4f75-8154-e4b33a5707a1_SiteId=3dd8961f-e488-4e60-8e11-a82d994e183d dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.6.200.16 authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 8b9b8687-5acb-459d-0ecd-08d9dbf0063e x-ms-traffictypediagnostic: DM6PR11MB2634:EE_ x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:9508; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: YZ6UUvacXkI++cQLtQZ4a6ZADHZMGhfRLWcxMZlkpAZRGWFTieQIjspwXBKyP2519HJjPzZS73OZXQxHgXHDgja0tRL6BVoevjcH1fmZRSUGpfRAJyqUO5tP4VhXrLT5QBLJsO2dlCYbnyIJRXDH5jOvkyGBlcQsEEB9PRGpzdaVwCuBXIsq0xjUhGr4yEOB/Z3er30cOddKsuMMS8fStjif5bH3yIEyAniSti9esDOEnC24g3auy+JD3bdEgSpPSmpd/vrq/ZaiFrBOCmZuFy5cRtGqwqjgswsn/1t8HsjEl24UDiq6P8F8vlh1XTs5jxbQaT4RCPnxo7uJMwfbCxQ4AJnIIzMnQ3wQI1qiv0P2ou+htSvklyN92OjWft3C5CnG4yeL+RgID0Ib+Q0os2zEsnpNMURTHMIkVjXsjJeMe1ilyJ+Yms+qCvtQJuvCi/m7cZqj6g6D5rjgtq9z2rfBLWVAz182P4gaZD6j2ILjB3NMEfcLxqQHWo/EX7tdHDaTtTXftw/N8PzHkXpz0Zavy8ND3IYPbYZ2GJvNf8u1fukANaqoWhYIr6IkQjycmpjw1Ea1JEyfMnOkJL1VWCBmIcKtaGZcBblO0voYAXM8yHgu9SaQV+AJxuS0G8HEGwRoU2sFfg78770VB7lTIYtujnTF6tr9Nah8MjuKwSKsjGcN+f7jVWRgDmXINFnbeoUmuU56HUVMXtkLRydl8Rt6aHzG0JjD7JqG0n28R+LYI5VHzKuOtXs1L/2qu9x3KLQLkd8xReBuEXXD4uFiHejLCY2Avo7ddzwFZ/3cvPSbUlmAkVGhMj+AjhzjKXYmJ7Bi6qjvXj/b/j4D712kkg== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM4PR11MB5277.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(366004)(2906002)(8936002)(38070700005)(7696005)(30864003)(508600001)(52536014)(8676002)(66946007)(83380400001)(66556008)(66446008)(166002)(76116006)(71200400001)(64756008)(66476007)(76236003)(55016003)(122000001)(33656002)(53546011)(6506007)(186003)(4326008)(38100700002)(54906003)(5660300002)(316002)(82960400001)(86362001)(9686003)(9326002)(110136005)(26005)(579004);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?zYDEVsAMH7Axu8847ioPeoGxxoM0bVtgjeJ2Rtny5WJSOC8tObLTDnsGgNG9?= =?us-ascii?Q?IRZY48GTKOwTHCOfGPvlW5R6+2mNsCffe/jnyKp0n3OuQcpMcIzXZxwOio7Q?= =?us-ascii?Q?sbYrvxr2tZvd+mSn12+NZRAxeaw33QFp1CPEUXEJUNjED+JdcfwRz5wlC00Z?= =?us-ascii?Q?bq7VEcd0rt0Ofcr5Db78PeAeYI4BJHDg+PnFiLGSSJvbfRt/vOLVjsRqVcHU?= =?us-ascii?Q?vwlEQdj+aBhGk4Tuy5+MmJ/L7XQjCq0JeAe/2xMh/aVVS2opKZjWsTh6As/0?= =?us-ascii?Q?sUtnoc6o753hqNt03hT0C5cYp1sPdIjoXafZNG6z8xXAsSbTeV3gpQk0AHWh?= =?us-ascii?Q?q/Wb4rYjSVO+mJ+RzqyvXA/cLTl4z1Yg7PXnJNJudKSqX6shCB8SdnN6l3MM?= =?us-ascii?Q?oMtRoUgyxitP/ujMFGOqaweasdIfCcGawPLU3pbmxYKbuzNSZtGn1vzgOJTY?= =?us-ascii?Q?kJI2VfFP/9OCyFgAH6hwDIheBSt6O63Gtc0nazlpg/DFv0JTt52rauZyLI+9?= =?us-ascii?Q?DVgfQ7nuZ8GaiKUl5s2DcDMyO00f5Yu0PjcG6UmS9BtBImZk1g0QlctDzPBT?= =?us-ascii?Q?Ica0VokcxiROcjsVYGAu3PYP2iZ38UoHPi+uB336hPPuvqbMJhRnBwP6gUEQ?= =?us-ascii?Q?OZ//YzZmXCipZJZ/fmGipD87MvM/XLu4spzMhMFY9+gY1bzrLKncOVc3Xzhb?= =?us-ascii?Q?vOdA3Y9EUNPOs1QI5QAO4KK/WaOck+lPVY1GWUjcSB49RVkLoTE05TSyj7Hl?= =?us-ascii?Q?0sB0aHbzGtngatfJC3Y4C4931w4XH9rKoD9p8FBRQPDaCrgzevPPQddeGDvD?= =?us-ascii?Q?RMnEVQORl00vJe1kHVMWzVm6watG+uLjtQV8QP4uvbe1mg8h9LeC9dUkAJwF?= =?us-ascii?Q?KRoqMSgGAdAJDpfPsEAt9mPtbLnQPb/3oIxxoDqBjRREGPUWD25HJv0J800e?= =?us-ascii?Q?yzVeA828uF/lKl+Uf97Osn5Zwc41TA07ikZnCrCUzyn1YiYUaQZj4gwyunEe?= =?us-ascii?Q?kjyxwoUh2LutbeQA9vT1QBEFGUoKhsskQ1+uIoWF+oEGMaPmIpJy5b7J48W2?= =?us-ascii?Q?oD23BLNoj85UXRvpwkP0SdIGAoAQ3B4PI0rS73/mYcv9GrPc5imxbHwZJ3VM?= =?us-ascii?Q?ryxcR3SaPN91C6TvmtCbSKhr11clggKs8woTrF0SlUC8SShZmh5ZDYmQdCbD?= =?us-ascii?Q?GNpDTahFeaj7rcqVUDa6oHqez8cFm0/os0pKSdll2c8vRBf/z+iUS/fa3Vyb?= =?us-ascii?Q?/tee4OqE01DIv/PFafqa1aWz2QlfxFM5+I1kwmm2NEHwO+OaYmcNgnN40HyR?= =?us-ascii?Q?K6jRooTonfmgDfEzj+QugL4iKw61MbAufk9wYWK1dMamP3uZ2zaA0YbwqJEj?= =?us-ascii?Q?OTMt+aJT5Y+TQ3HzWbA5kG30lAHOP6WtFyqB7RE5ysrmKuTKrpvUcIm7dYd4?= =?us-ascii?Q?uPHVYtjc3Ldvs6eZHQgXpfPqhMybOExSO75ldVm4xOhdiH9o0j61pE5xcZiI?= =?us-ascii?Q?saC946zdw+OaU+2Cp+5Hd3jAvLxQ5FNszSnCSlId2yxA6ZPPYkcvVRh4wHS6?= =?us-ascii?Q?7PKjetoqYd0NUbAXWbPkmf3l4f6M7BoCVWwkM3EcdqTB9rKYbvCj9UP7EfPT?= =?us-ascii?Q?YjfgENOYENhOFn/GHhy4os0=3D?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5277.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 8b9b8687-5acb-459d-0ecd-08d9dbf0063e X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jan 2022 08:36:59.2219 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: VpscEOL7Qp8LO7B8RIDM8avCzKUvgRCtS3JZgKoZG0OX8/2aNQSB4XvcMFD0HAjT55Jd9lsOavlCJLKYJWulBA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB2634 Return-Path: zhichao.gao@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_DM4PR11MB5277422969973DF54D6985C4F65A9DM4PR11MB5277namp_" --_000_DM4PR11MB5277422969973DF54D6985C4F65A9DM4PR11MB5277namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi, Sorry for the late response. Many works interrupt my review plan. The BZ creation and update is part of edk2 develop process EDK II Developme= nt Process * tianocore/tianocore.github.io Wiki * GitHub. It give a good history of the code change and it can be traced by the edk2 = stable release page. Back to the patch itself. Because it is a reuse the structure of the ACPI_P= ARSER, I think it is fine to take Length check into consideration for secur= ity thinking. For the ItemPtr, it is used to be used outside the parser. So the patch loo= ks fine to me. Anything I miss consideration, please feel free to point out. Thanks, Zhichao From: Attar, AbdulLateef (Abdul Lateef) Sent: Thursday, January 20, 2022 11:25 AM To: devel@edk2.groups.io; sami.mujawar@arm.com Cc: Ni, Ray ; Gao, Zhichao ; nd Subject: RE: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSE= R bitfield parser [Public] Hi Sami, Why Bugzilla ticket required? Also please see inline for my response [Abdul]. Thanks AbduL From: devel@edk2.groups.io > On Behalf Of Sami Mujawar via groups.io Sent: 19 January 2022 22:17 To: Attar, AbdulLateef (Abdul Lateef) >; devel@edk2.groups.io Cc: Ray Ni >; Zhichao Gao >; nd > Subject: Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSE= R bitfield parser [CAUTION: External Email] Hi Abdul, Please also create a bugzilla ticket and add a reference to it in the commi= t message. Regards, Sami Mujawar On 19/01/2022 04:44 PM, Sami Mujawar wrote: 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/Sh= ellPkg/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 *Pt= r, 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 bitf= ield. + 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 th= e ACPI table bitfields. + + This function can optionally be used to parse ACPI tables and fetch spec= ific + field values. The ItemPtr member of the ACPI_PARSER structure (where use= d) + 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 descri= bes + 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 th= at + 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/Sh= ellPkg/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 th= e ACPI table bitfields. + + This function can optionally be used to parse ACPI tables and fetch spec= ific + field values. The ItemPtr member of the ACPI_PARSER structure (where use= d) + 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 descri= bes + 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 th= at + 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 =3D=3D 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 =3D 0; + Offset =3D 0; + + // Increment the Indent + gIndent +=3D Indent; + + CopyMem ((VOID *)&BitsData, (VOID *)Ptr, Length); + if (Trace && (AsciiName !=3D NULL)) { + HighLight =3D GetColourHighlighting (); + + if (HighLight) { + OriginalAttribute =3D 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 =3D 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 !=3D NULL) { + *Parser[Index].ItemPtr =3D NULL; + } + + // We don't parse past the end of the max length specified + continue; + } + + if (Parser[Index].Length =3D=3D 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 =3D=3D 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] [Abdul] Reserved bits should be having some length, 1 to 31, it cannot be 0= bit length. I tested with FADT flags field which is having Reserved bits. [/Abdul] + } + + if (GetConsistencyChecking () && + (Offset !=3D Parser[Index].Offset)) + { + IncrementErrorCount (); + Print ( + L"\nERROR: %a: Offset Mismatch for %s\n" + L"CurrentOffset =3D %d FieldOffset =3D %d\n", + AsciiName, + Parser[Index].NameStr, + Offset, + Parser[Index].Offset + ); + } + + // extract Bitfield data for the current item + Data =3D (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 !=3D NULL) { + Parser[Index].PrintFormatter (Parser[Index].Format, (UINT8 *)&Data= ); + } else if (Parser[Index].Format !=3D 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 =3D %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 !=3D NULL)) + { + Parser[Index].FieldValidator ((UINT8 *)&Data, Parser[Index].Contex= t); + } + + Print (L"\n"); + } // if (Trace) + + if (Parser[Index].ItemPtr !=3D NULL) { + *Parser[Index].ItemPtr =3D (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 bette= r 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_PARS= ER structure. [/SAMI] + } + + Offset +=3D Parser[Index].Length; + } // for + + // Decrement the Indent + gIndent -=3D Indent; + return Offset; +} --_000_DM4PR11MB5277422969973DF54D6985C4F65A9DM4PR11MB5277namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi,

 

Sorry for the late = response. Many works interrupt my review plan.

The BZ creation and= update is part of edk2 develop process EDK II Development Process · tianocore/tian= ocore.github.io Wiki · GitHub.

It give a good history of the code change and it can= be traced by the edk2 stable release page.

 

Back to the patch itself. Because it is a reuse the = structure of the ACPI_PARSER, I think it is fine to take Length check into = consideration for security thinking.

For the ItemPtr, it is used to be used outside the p= arser. So the patch looks  fine to me.

 

Anything I miss consideration, please feel free to p= oint out.

 

Thanks,

Zhichao<= /span>

 

 

From:= Attar, AbdulLateef (Abdul Lateef) <Abd= ulLateef.Attar@amd.com>
Sent: Thursday, January 20, 2022 11:25 AM
To: devel@edk2.groups.io; sami.mujawar@arm.com
Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@i= ntel.com>; nd <nd@arm.com>
Subject: RE: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACP= I_PARSER bitfield parser

 

[Public]

 

Hi Sami,=

   &= nbsp;            Why= Bugzilla ticket required?

Also please see inl= ine for my response [Abdul].

Thanks

AbduL

 

From:= devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sami Mujawar via groups.io
Sent: 19 January 2022 22:17
To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; devel@edk2.groups.io
Cc: Ray Ni <ray.ni@int= el.com>; Zhichao Gao <zhichao.gao@intel.com>; nd <nd@= arm.com>
Subject: Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACP= I_PARSER bitfield parser

 

[CAUTION: External Email]

Hi Abdul,

Please also create a bugzilla ticket and add a reference to it in the co= mmit message.

Regards,

Sami Mujawar

 

On 19/01/2022 04:44 PM, Sami Mujawar wrote:

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.g=
ao@intel.com>
Cc: Sami Mujawar <sami.muja=
war@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 reserv=
ed.


 


+  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)(UINT=
8 *Ptr, VOID *Context);


   the field data. If the field is more complex and requires=
 additional


 


   processing for formatting and representation a print form=
atter function


 


   can be specified in 'PrintFormatter'.


 


+


 


+  ParseAcpiBitFields() uses AcpiParser structure to parse the bi=
t fields.


 


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


 


+  Also, the Offset field will be considered as starting offset o=
f the bitfield.


 


+


 


   The PrintFormatter function may choose to use the format =
string


 


   specified by 'Format' or use its own internal format stri=
ng.


 


 


 


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


 


 


   /// The length of the field.


 


   /// (Byte Length column from ACPI table spec)<=
/pre>

 


+  /// Length(in bits) of the bitfield if used with ParseAcpiBitF=
ields().


 


   UINT32        &nb=
sp;          Length;


 


 


 


   /// The offset of the field from the start of the table.<=
o:p>


 


   /// (Byte Offset column from ACPI table spec)<=
/pre>

 


+  /// The Bit offset of the field if used with ParseAcpiBitField=
s().


 


   UINT32        &nb=
sp;          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.<=
o:p>


 


+


 


+  The ACPI table buffer is parsed using the ACPI table parser in=
formation


 


+  specified by a pointer to an array of ACPI_PARSER elements. Th=
is parser


 


+  function iterates through each item on the ACPI_PARSER array a=
nd 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 fi=
eld data


 


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

 


+


 


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


 


+           &nb=
sp;            =
   table.


 


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


 


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


 


+           &nb=
sp;            =
   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


 


+           &nb=
sp;            =
   describes the table being parsed.


 


+  @param [in] ParserItems  Number of items in the ACPI_PARS=
ER array.


 


+


 


+  @retval Number of bits parsed.


 


+**/


 


+UINT32


 


+EFIAPI


 


+ParseAcpiBitFields (


 


+  IN BOOLEAN        &nbs=
p;   Trace,


 


+  IN UINT32         =
;    Indent,


 


+  IN CONST CHAR8        *Asci=
iName 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 Pa=
rser 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 reserv=
ed.


 


+  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.<=
o:p>


 


+


 


+  The ACPI table buffer is parsed using the ACPI table parser in=
formation


 


+  specified by a pointer to an array of ACPI_PARSER elements. Th=
is parser


 


+  function iterates through each item on the ACPI_PARSER array a=
nd 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 fi=
eld data


 


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

 


+


 


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


 


+           &nb=
sp;            =
   table.


 


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


 


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


 


+           &nb=
sp;            =
   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


 


+           &nb=
sp;            =
   describes the table being parsed.


 


+  @param [in] ParserItems  Number of items in the ACPI_PARS=
ER array.


 


+


 


+  @retval Number of bits parsed.


 


+**/


 


+UINT32


 


+EFIAPI


 


+ParseAcpiBitFields (


 


+  IN BOOLEAN        &nbs=
p;   Trace,


 


+  IN UINT32         =
;    Indent,


 


+  IN CONST CHAR8        *Asci=
iName 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 =3D=3D 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 =3D 0;


 


+  Offset         &n=
bsp;  =3D 0;


 


+


 


+  // Increment the Indent


 


+  gIndent +=3D Indent;


 


+


 


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


 


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


 


+    HighLight =3D GetColourHighlighting ();=



 


+


 


+    if (HighLight) {


 


+      OriginalAttribute =3D gST->ConOut-&=
gt;Mode->Attribute;


 


+      gST->ConOut->SetAttribute (=



 


+           &nb=
sp;         gST->ConOut,


 


+           &nb=
sp;         EFI_TEXT_ATTR (


 


+           &nb=
sp;           EFI_YELLOW,=



 


+           &nb=
sp;           ((OriginalA=
ttribute&(BIT4|BIT5|BIT6))>>4)


 


+           &nb=
sp;           )


 


+           &nb=
sp;         );


 


+    }


 


+


 


+    Print (


 


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

 


+      gIndent,


 


+      "",


 


+      (OUTPUT_FIELD_COLUMN_WIDTH - gIndent),=



 


+      AsciiName


 


+      );


 


+    if (HighLight) {


 


+      gST->ConOut->SetAttribute (gST-&=
gt;ConOut, OriginalAttribute);


 


+    }


 


+  }


 


+


 


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


 


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


 


+      // For fields outside the buffer lengt=
h provided, reset any pointers


 


+      // which were supposed to be updated b=
y this function call


 


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



 


+        *Parser[Index].ItemPtr =3D=
 NULL;


 


+      }


 


+


 


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


 


+      continue;


 


+    }


 


+


 


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

 


+      // don't parse the bitfield whose leng=
th 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. Co= nsidering this, Parser[Index].Length =3D=3D 0 would mean an incorrect an in= correct description in ACPI_PARSER, right?
If so, the default case for the switch statement below would print "ER= ROR: %a: CANNOT PARSE THIS FIELD, Field Length". Can you check this, p= lease?
[/SAMI]
[Abdul] Reserved bits should be having som= e length, 1 to 31, it cannot be 0bit length.

I tested with FADT = flags field which is having Reserved bits.

[/Abdul]= 

+    }
 
+
 
+    if (GetConsistencyChecking () &&
 
+        (Offset !=3D Parser[Index]=
.Offset))
 
+    {
 
+      IncrementErrorCount ();

 


+      Print (


 


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


 


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


 


+        AsciiName,

 


+        Parser[Index].NameStr,


 


+        Offset,


 


+        Parser[Index].Offset<=
/o:p>


 


+        );


 


+    }


 


+


 


+    // extract Bitfield data for the current item=



 


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


 


+


 


+    if (Trace) {


 


+      // if there is a Formatter function le=
t the function handle


 


+      // the printing else if a Format is sp=
ecified in the table use


 


+      // the Format for printing<=
/pre>

 


+      PrintFieldName (2, Parser[Index].NameS=
tr);


 


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


 


+        Parser[Index].PrintFormatt=
er (Parser[Index].Format, (UINT8 *)&Data);


 


+      } else if (Parser[Index].Format !=3D N=
ULL) {


 


+        // convert bit length to b=
yte length


 


+        switch ((Parser[Index].Len=
gth + 7) >> 3) {


 


+          // print the d=
ata depends on byte size


 


+          case 1:


 


+            Du=
mpUint8 (Parser[Index].Format, (UINT8 *)&Data);


 


+            br=
eak;


 


+          case 2:


 


+            Du=
mpUint16 (Parser[Index].Format, (UINT8 *)&Data);


 


+            br=
eak;


 


+          case 3:


 


+          case 4:


 


+            Du=
mpUint32 (Parser[Index].Format, (UINT8 *)&Data);


 


+            br=
eak;


 


+          case 5:


 


+          case 6:


 


+          case 7:


 


+          case 8:


 


+            Du=
mpUint64 (Parser[Index].Format, (UINT8 *)&Data);


 


+            br=
eak;


 


+          default:<=
/o:p>


 


+            Pr=
int (


 


+           &nb=
sp;  L"\nERROR: %a: CANNOT PARSE THIS FIELD, Field Length =3D %d\=
n",


 


+           &nb=
sp;  AsciiName,


 


+           &nb=
sp;  Parser[Index].Length


 


+           &nb=
sp;  );


 


+        } // switch

 


+      }


 


+


 


+      // Validating only makes sense if we a=
re tracing


 


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


 


+      if (GetConsistencyChecking () &&am=
p;


 


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


 


+      {


 


+        Parser[Index].FieldValidat=
or ((UINT8 *)&Data, Parser[Index].Context);


 


+      }


 


+


 


+      Print (L"\n");

 


+    } // if (Trace)


 


+


 


+    if (Parser[Index].ItemPtr !=3D NULL) {<=
/pre>

 


+      *Parser[Index].ItemPtr =3D (VOID *)(UI=
NT8 *)&Data;

[SAMI]  ACPI_PAR= SER.ItemPtr is used to get a reference to the field data in the original AC= PI 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 bette= r 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_PARS= ER structure.
[/SAMI]

 
+    }
 
+
 
+    Offset +=3D Parser[Index].Length;
 
+  } // for
 
+
 
+  // Decrement the Indent
 
+  gIndent -=3D Indent;
 
+  return Offset;
 
+}
 

 

 

--_000_DM4PR11MB5277422969973DF54D6985C4F65A9DM4PR11MB5277namp_--