From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CF6D7821BA for ; Tue, 20 Dec 2016 08:35:13 -0800 (PST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP; 20 Dec 2016 08:35:13 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,379,1477983600"; d="scan'208,217";a="20789240" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga002.jf.intel.com with ESMTP; 20 Dec 2016 08:35:12 -0800 Received: from fmsmsx157.amr.corp.intel.com (10.18.116.73) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 20 Dec 2016 08:35:12 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX157.amr.corp.intel.com (10.18.116.73) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 20 Dec 2016 08:35:12 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.54]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.159]) with mapi id 14.03.0248.002; Wed, 21 Dec 2016 00:35:09 +0800 From: "Yao, Jiewen" To: Evan Lloyd , "Ni, Ruiyu" , "Carsey, Jaben" , Sami Mujawar CC: "Carsey, Jaben" , "edk2-devel@ml01.01.org" , Leif Lindholm , "ard.biesheuvel@linaro.org" Thread-Topic: [edk2] [PATCH] ShellPkg: Add acpiview tool to dump ACPI tables Thread-Index: AQHSV8nhfrd+vdgk1UaQNKJ6nLAROaEPCe4g///hgwCAACKtgIABFlEg//+P4ACAAJMo8IAACSiAgAAA/BCAAAB9AIAAlgsg Date: Tue, 20 Dec 2016 16:35:09 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A8C7026@shsmsx102.ccr.corp.intel.com> References: <20161216182547.616-1-evan.lloyd@arm.com> <734D49CCEBEEF84792F5B80ED585239D5B836C04@SHSMSX103.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5B8371E1@SHSMSX103.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A8C6C52@shsmsx102.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5B839479@SHSMSX103.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A8C6CBD@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [PATCH] ShellPkg: Add acpiview tool to dump ACPI tables X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Dec 2016 16:35:14 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thank you, Lloyd. Comment inline. From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Evan= Lloyd Sent: Tuesday, December 20, 2016 9:21 PM To: Yao, Jiewen ; Ni, Ruiyu ; Car= sey, Jaben ; Sami Mujawar Cc: Carsey, Jaben ; edk2-devel@ml01.01.org; Leif Li= ndholm ; ard.biesheuvel@linaro.org Subject: Re: [edk2] [PATCH] ShellPkg: Add acpiview tool to dump ACPI tables Hi Jiewen. Some responses inline below: > From: Yao, Jiewen [mailto:jiewen.yao@intel.com] > Sent: 20 December 2016 05:54 > To: Ni, Ruiyu; Carsey, Jaben; Evan Lloyd > Cc: Carsey, Jaben; edk2-devel@ml01.01.org;= Leif Lindholm; ard.biesheuvel@linaro.org > Subject: RE: [edk2] [PATCH] ShellPkg: Add acpiview tool to dump ACPI tabl= es > > I like this idea - Shell.efi should only support commands defined by Shel= l spec. > Then we should not port code to be a shell library. > > I have concern on ShellPkg\Library\UefiDpLib, because DP depends on Timer= Lib and TimerLib is a platform specific library. > I do not object the idea to move DP from performancepkg to shellPkg. > I would suggest DP had better be a standard shell application, rather tha= n a shell lib and included in a shell bin. > > > Now back to acpiview: > First, it is great to have such tool in UEFI shell. Thanks. We hoped people would see the benefit. > > Second, I found the function is incomplete. > 1) Some ACPI defined tables are not complete. Take MADT as example,= it only supports EFI_ACPI_6_1_GIC/EFI_ACPI_6_1_GICD/EFI_ACPI_6_1_GIC_MSI_F= RAME/EFI_ACPI_6_1_GICR/EFI_ACPI_6_1_GIC_ITS. > 2) Some ACPI defined tables are missing. Such as BGRT, FPDT. At this stage we have only added tables for which we had an immediate use, = as available effort is limited. In addition, we did not want to submit code we were not able to test. Clea= rly, our platforms use GIC, not APIC. Also we are not well placed to judge the best representation for some fiel= ds, e.g. are APIC interrupt numbers normally decimal or hex? [Jiewen] Got it. As you mention, I hope it can be in SHELL spec, too. So that we can have a standard way to dump all table information. For X86 system, I have written similar tool to dump X86 related info. I just check in to https://github.com/jyao1/EdkiiShellTool/tree/master/Acpi= ToolPkg. It is also BSD license code. I tried by best to dump info for *all* the ACP= I table. But I do not validate ARM system. We complement to each other. :) And mine does not have any consistency *check* function so far. Your #2 below is very good. > 3) I found some ACPI reserved tables are added here. Such as DBG2, = IORT. Is there any problem with dumping the reserved tables? [Jiewen] No problem at all. This question is combined with 4). And I hope it can be complete as well. > 4) But not all ACPI reserved tables are added. Such as HPET, TPM2. We only wrote code for tables we could test. We have no example hardware/f= irmware with those tables. [Jiewen] Same as 2). I have written tool for X64 system, but not for ARM. > May I know what criteria we are using to select which feature is supporte= d or not supported? As stated above, we only provided the tables/sections that we needed to dum= p and could test. >>From the commit message: """ Many tables are not explicitly handled, in part because no examples are ava= ilable for our testing. The program is designed to be extended to new tables with minimal effort, a= nd contributions are invited. """ [Jiewen] Got it and agree. > Or it is just something from start and still need some work to make it co= mplete? Yes, this is just a starting point and very far from complete. We found it detected a number of problems for us, so we felt it worth contr= ibuting. If we can get people to use it, that may mean fewer problems for us to debu= g. ;-) [Jiewen] See my last comment. > > Third, I found we need a long ? ?switch (*AcpiTableSignature) case? to du= mp/parse ACPI table specific function. > Do you think it will be better, that we provide a register function to le= t each ACPI table register a parser? > For example: RegisterDumpAcpiTable (EFI_ACPI_1_0_APIC_SIGNATURE, DumpAcpi= MADT); > Then the core logic just need use a table-driven way to find out the corr= esponding parser function by a SIGNATURE. You are right, that would be a much better abstract design, and would make = adding a table simpler. We discussed this in our pre-release review. The trade-off is that you need to maintain a dynamic registry of handlers, = and write your own lookup code. Using a switch eliminates the registry, and relies on the compiler for effi= cient look up. We would be very happy with this change, should someone have the time avail= able to contribute it. [Jiewen] Thank you considering that. Actually, I have such code for X86 sys= tem. I have a generic library for registration - https://github.com/jyao1/EdkiiS= hellTool/tree/master/AcpiToolPkg/Library/DumpAcpi/DumpAcpiTableFuncLib So that every individual table can register itself by calling RegisterDumpA= cpiTable() in the library constructor. Our review also suggested changing the switch to just set a function pointe= r, with a single call at the end. That would make each switch case much simpler. Would that be an acceptable= "half-way house"? [Jiewen] Let me summarize current status: 1) We know there is strong requirement to dump ACPI table in UEFI she= ll environment, for debug purposes. 2) UEFI shell specification does not contain any ACPI dump command. (= Although it has SmbiosDump) 3) ACPICA.ORG has sample code to dump ACPI. > > >Binary can be downloaded from: https://acpica.org/downloads/uefi-suppo= rt > > >Source can be downloaded from: https://github.com/acpica/acpica 4) This patch provides dump ACPI information with consistency check. = But limitation is: it only supports the limited table on ARM platform. NO s= upport for X86 platform. 5) I have similar code to dump ACPI. But the limitation is: it only v= alidated on X86 platform. It is not validated on ARM platform. So I would like to propose: 1) DOCUMENTATION: Can we co-work to submit an ECR for SHELL, to add A= cpiDump command? So that people may get standard dump log in the future. 2) CODE: It seems both of our code is POC quality and has partial val= idation only. It might not be suitable to check in EDKII immediately. I would like to propose we co-work in EDKII staging tree - https://github.c= om/tianocore/edk2-staging/ (We have 4 features there.) We can align our design, then I can help validate X86 and you can help vali= date ARM. Then we can submit a complete patch to EDKII ShellPkg. What about your idea? > > Finally, I agree with Ruiyu that a standalone tool might be more proper. Agreed in other response. > > Thank you > Yao Jiewen >> > > ... > > > >> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759") IMPORTANT NOTICE: The contents of this email and any attachments are confid= ential and may also be privileged. If you are not the intended recipient, p= lease notify the sender immediately and do not disclose the contents to any= other person, use it for any purpose, or store or copy the information in = any medium. Thank you. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel