From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (NAM10-MW2-obe.outbound.protection.outlook.com [40.107.94.49]) by mx.groups.io with SMTP id smtpd.web09.7095.1642649073120791931 for ; Wed, 19 Jan 2022 19:24:33 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@amd.com header.s=selector1 header.b=HxuoERGJ; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: amd.com, ip: 40.107.94.49, mailfrom: abdullateef.attar@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=CYqeuFrcvXMNT0XDIYH89tbF2fD76F2uN5KO9nl1q9XxoLY17wbDprTMuZgIRdUCtUTyxidN0GGk/4NiaRDosAE90EiymdRcxXhYVFjHyDUbEliHOEUcuOlYEsR5srv7Y+YFkLx2rIPrEt9BYEEOq+lf1qrt8p9lgpMpDwWJhcCeSs0TnzzseSLmLS5Z5sbcCj7p0bW/mC/QaUpdBtEu6K7de8+X/Svlxnmh2mhGQdemhm++kWcexnWyyk4iPx/X1McyZRfA4Us1BTFxfKacW9LGM0oFtgMIe6aMaojPYJ90CfBlcQvm0iATWiEdxDzM36MUdEYBrMC7UiJl/XnjvQ== 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=bGfx5t/OpTTUMtq5pjC97KBJqSI421bYKjlZS380TE4=; b=mNGiRvNxnvn93KA0UTAgmf+19Jf2Ob7BPUEUYQjJ8NoRxYPO2aIXNcEFdJGW29tWl8DawP7K3LXD85yN1KIP/NPq8skCYj5wMPYW89Dzuc7q6VyhRJXTv1t1T1Ula7/wtVyXkTqAQU966T34NIw6KSNMuLWYOENlkUWvsWJNLNcWpH1I7ZhJIO+PB15cnXiaD6o1KVq40GBkNK8DHwlwmtdsRUndgJ/QXnj7ysfVszSKBP3YKWjVu9/JgRshlEbmRmafjTZcFUgEvLu7Qc8wUP1METhPS7v6hrHIGjohFpj/zuxwfgqa2YqdQtpuDR73wTndMgCJukLzjHXim3O/gw== 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=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=bGfx5t/OpTTUMtq5pjC97KBJqSI421bYKjlZS380TE4=; b=HxuoERGJ5ztUtWl+vYhl3SLYMxMpiYcfGV0TH+wUH5UJShI85LdsUcGDAwhZ2UdEURAFta0Pj8ZH2KuB7tvNFXD8yR/8G2QlLaPCO7UR2nuefqIhTNiPwGS3ZuTZPb22WBhFz2oihA6yKFbQubFLCeaGxdkQTOU/iUuhWxvSrJA= Received: from BN9PR12MB5225.namprd12.prod.outlook.com (2603:10b6:408:11e::8) by BYAPR12MB3015.namprd12.prod.outlook.com (2603:10b6:a03:df::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4888.12; Thu, 20 Jan 2022 03:24:30 +0000 Received: from BN9PR12MB5225.namprd12.prod.outlook.com ([fe80::f4ec:165b:3903:35e3]) by BN9PR12MB5225.namprd12.prod.outlook.com ([fe80::f4ec:165b:3903:35e3%4]) with mapi id 15.20.4909.008; Thu, 20 Jan 2022 03:24:30 +0000 From: "Attar, AbdulLateef (Abdul Lateef)" To: "devel@edk2.groups.io" , "sami.mujawar@arm.com" CC: Ray Ni , Zhichao Gao , 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: AQHYDVPXiSGiIY5lXkKEhL1X0xkNeKxqjaIAgACw1vA= Date: Thu, 20 Jan 2022 03:24:30 +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; authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: c4914a8f-f6bb-4c6a-e286-08d9dbc45f15 x-ms-traffictypediagnostic: BYAPR12MB3015:EE_ x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: jxu4VQgjYuuncvxeLB7wirfthaCaZtuSxPCYVtNwirSPaXddzJD3eyzdDh7n7QswrNwdLzpKVYxaY1BmhRvgUB8Qykx62dC7va20MB9GAeG2YBJCbsqFkk/GXkn1SORbDMwz/85QDFuwzntGzS4p1V8loCiljs4K+bnDfJdXChmXaIoyN/pCm8CwQrR9ntBrC9hFi/JHSqkX9Vz3zYnBiC8rWTWRHa0VsYZSSHDEWUxUMRem/B21+BsGHSqQDZgrJW0Hj2sHun0BXbhCoi/lY2iTaXMpCTAvfISk4Ub6ARqiwGf3yn9qCpVmZhRSqsaIEK1H6A9r18Ccoecye+YmUNekY7P4Py7FRW/25y1YEgtY8Dtn/kVbGJ5sqWQqyQwRcQmkYiUccpP7VIWp41iru4wbZyV+JECcBWZMNRqUcURu7mcuCeuiyMb+1bAo8/4mzKecqPrftH831CrmuiZgLnSNdbGyWLg6Ojt4yck3h2o3Jika/esJZw+oS4Pm1m40tC3SSNscVsS4FyaEjoFn4IinCa4kwDrLk1gw+XVwyaqEktcfOT6CMmotGJlgYis6Fhlq69kq83RTpKTRneqcEA74RAl8oHtF6Rqgxc27eb5+bDbOg4qKxIuprJD2cU68NSTjC0+lGfbp+lOh0uWcfhhGWq7ycN1jQGilM3dU/VyDq+3tyDAP5Aw4mtVxcz+dlOZsoPYHHFVnxJMu+8zeRBL99nBGE99h7kXAmOjPE6F8Z0336FQa/WFM/KVXDCnb8uSyIakFGc2za6fTbekTlOtWDOs9pprRICFaKWn4x+A= x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BN9PR12MB5225.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(366004)(86362001)(7696005)(508600001)(66556008)(66946007)(8936002)(76116006)(8676002)(54906003)(4326008)(83380400001)(2906002)(9686003)(38070700005)(66476007)(66446008)(71200400001)(64756008)(76236003)(186003)(52536014)(316002)(38100700002)(110136005)(30864003)(26005)(33656002)(166002)(5660300002)(6506007)(55016003)(122000001)(53546011)(579004);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ejwJ2jqgUGlsd6vEQ2QZ8nPcsdUFXVy/OFMm2NOt5gL9Bc8+2piIYBQn6IIY?= =?us-ascii?Q?BV6DQvxdS516f+bgwncDKGDJXdXxXkgWp1QKFQBmdG6kYLiL39vTNxbwZUCH?= =?us-ascii?Q?ZoV4qnIxVvd76ZNvBYl5IV5CTmAK8L/l7n6wUE9XBf5KR1herZeOQXDt3sbJ?= =?us-ascii?Q?+yZfH8Bdx3ViilAfaPs7TLHoboSkViwyT4O/XBWft71tOLXkFkd51VblAnSi?= =?us-ascii?Q?MCJgt8SODAU0gTq1Q86EoEgI7fDhrC7uUwB/BbBCxWR2YaoLhCAqeYYx7FxI?= =?us-ascii?Q?44K7wPTL+kpdbzbt4YaXuMr+08cE2pcK1Oe1IN5UvkAQvvMMspGZtha+fxZF?= =?us-ascii?Q?tLkQRSadyzPVjUGa+H04vIj+XBzfNQTKHxOP3qNZ41HGIOQAUDi01FughqzD?= =?us-ascii?Q?5wd5RmTIU7AjYROQIxQf+MsZF7Zt8XL4faG5pZ5EcMGip7taYZ8k1rEmwLD2?= =?us-ascii?Q?mIr5f57RMYQWJ0+kA33Cyb/xrZmWydmpQXpCUGCvYZgYMQj9wdJtNv+moj8M?= =?us-ascii?Q?iwbRVCFP6I+89dclkqUtVzQ8Ki7IP+wzzCD98QG1w6Rv2sRvBIhrt074Sb3O?= =?us-ascii?Q?5wj2SaJtrwgM8TPk9HWV1FqrGhtsDaMJcSaOC6gs+hfx/reZL+APMckf2M87?= =?us-ascii?Q?nafLodLL7GsBudPME6Zy1QA6qvLk2b2/HtfHuU2jY0OS5FXLCe/F46jeas1I?= =?us-ascii?Q?EnMjwyw6cF9JcdT4pKXAmrK881W1YuhgerIMSyRcDGUZ76r7RGw4kKz/lf6k?= =?us-ascii?Q?q7nkXKp54TnQJuR+Syo06FjRsl4iY97l3rk7wk710Mgqyrhu5iwecbFkqgk4?= =?us-ascii?Q?NPQZ/ZyEponTUOLPRmu63vGXnVJ9xKu70k0XxO8JEMsRYxZT+SQwV+rJKs5X?= =?us-ascii?Q?cRoewHHbm8WOtjnNxLxlmUX4NkTiMrQVO9i2oXfJSmLW4J/dyGB7l59YaNFA?= =?us-ascii?Q?W1CZyn9Eq/HB2kkbefK4dMDim1LrIyi0828CcKp1pWO8/ul3kZ3/QUk/XgSk?= =?us-ascii?Q?7eYqJMAjGj9pTTnpRodlBvgDjui26NL9MpMpwk0CrvKSiGD1frklAxbl5KwE?= =?us-ascii?Q?UfgfGdN1tiVse+rTH5bvH9TebWrxQ8cGSMufehcAuYgrZGX7gVCYE9HY9gtS?= =?us-ascii?Q?hW7eaF958aEkRw0piT5a4Ko7VLB81rFDuPwBRlYTQS48/z1b+VGsMv1QuVBL?= =?us-ascii?Q?O+YPY05xaBogH83U20qshFe7EvQf3Ecr+Cv8lEks/+AbZwnR6iHmj+11+uYe?= =?us-ascii?Q?BXqe9xXOxEu05zyfMg+LNlArU/5QDk8aSAFa7vKVO9XlxMjO7/d/VXzJyqOP?= =?us-ascii?Q?CZTyy4SMKXJten3yroh5IahjzuDEfnLkkfxmYscoPTG+M7yc0iF/2UIxnrnW?= =?us-ascii?Q?kVMq4TgyOvhYtsXig18V5bH5VoWQxGNSbaRAgAIurVG2qHJBeIhjr0TRXfPi?= =?us-ascii?Q?dY4wOupXPKGT2tF1EtP/FG2+iK0GvSQ+DwfkSxFW7Rvr0Xusy7ZBfl48Xv0w?= =?us-ascii?Q?35DOu+O0ZvdknqmgyjSqPGjSwm9h6EELSjct/LqagEat7PLArVNDbgwc5wt+?= =?us-ascii?Q?lTMX54TjYnNDtdZ8sqV7+brl3f2FPGN9iNqdAqTgMp4XZppsb8eAhesAiGd7?= =?us-ascii?Q?7D4But1GAWDF7b2A8zYgCvU=3D?= MIME-Version: 1.0 X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5225.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: c4914a8f-f6bb-4c6a-e286-08d9dbc45f15 X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jan 2022 03:24:30.3885 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: +PRqbLPHd13FCOfYPiJgTILd2i6mJz6LV6HJOTyiw0yftldeFcmX2Ro0NwmlP9jurqGCZeU+IPON2zDo0v/xog== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB3015 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_BN9PR12MB522585ADC83EC58E98F29433E05A9BN9PR12MB5225namp_" --_000_BN9PR12MB522585ADC83EC58E98F29433E05A9BN9PR12MB5225namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable [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@ed= k2.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_BN9PR12MB522585ADC83EC58E98F29433E05A9BN9PR12MB5225namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

[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.group= s.io> On Behalf Of Sami Mujawar via groups.io
Sent: 19 January 2022 22:17
To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com&= gt;; devel@edk2.groups.io
Cc: Ray Ni <ray.ni@intel.com>; Zhichao Gao <zhichao.gao@int= el.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_PARSER.ItemPtr is used to get a re= ference 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_BN9PR12MB522585ADC83EC58E98F29433E05A9BN9PR12MB5225namp_--