From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 3F0C481EFB for ; Fri, 11 Nov 2016 23:48:34 -0800 (PST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP; 11 Nov 2016 23:48:37 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,625,1473145200"; d="scan'208,217";a="785561968" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by FMSMGA003.fm.intel.com with ESMTP; 11 Nov 2016 23:48:32 -0800 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 11 Nov 2016 23:48:32 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 11 Nov 2016 23:48:31 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.239]) with mapi id 14.03.0248.002; Sat, 12 Nov 2016 15:48:29 +0800 From: "Yao, Jiewen" To: Pete Batard , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger Thread-Index: AQHSPDN5qWBNkmWS6kux4COS3Hty5aDUcwQA//+KDYCAANctoA== Date: Sat, 12 Nov 2016 07:48:28 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C50386CF9E0@shsmsx102.ccr.corp.intel.com> References: <22709fc2-7dec-f9eb-43f7-d06405349b7e@akeo.ie> <74D8A39837DF1E4DA445A8C0B3885C50386CF903@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 0/2] MdeModulePkg/EbcDxe: add EBC Debugger 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: Sat, 12 Nov 2016 07:48:34 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable HI Pete I am glad that you like it. :) I met some problem on extracting patch from email. So I reviewed the code i= n https://github.com/pbatard/EbcDebugger and I assume they are same. However, the master contains some non-EBC-debugger related code. The edk2-diff branch contains an old version of EBC driver code. Next time, would you please post your V2 patches to a new branch, help me d= o the review efficiently? Now I only checked the diff file. Here is some suggestion for your consider= ation: 1) EFI_DEBUGGER_CONFIGURATION_PROTOCOL Previously, we document a way to consume this protocol. In user manual: https://sourceforge.net/projects/efidevkit/files/Documents/EBC%20Debugger%2= 0User%20Manual.pdf/download Appendix A. Configuring the EBC Debugger under EFI Shell. But I cannot find the EbcDebuggerConfig application in EDKI. It brings you = some confusing because you think no one is consuming this protocol. So I post a EDK-I style APPLICATION to my personal git-hub - https://github= .com/jyao1/EbcDebuggerApp You can take a look. It is also BDS license code. In order to make feature complete and match the user manual, I think we can= : 1.A) Add DebuggerConfiguration.h to MdeModulePkg/Include/Protocol. 1.B) Keep EFI_DEBUGGER_CONFIGURATION_PROTOCOL in Ebc driver. 1.C) Port EbcDebuggerApp to EDKII and add it to MdeModulePkg/Application di= r. Care need to be taken that EbcDebuggerApp should not depend on ShellPkg,= the ARGC and ARGV can be got from SHELL_PARAMETERS_PROTOCOL. 1.D) I found the APP need EdbCommon.h to enable/disable debug , so I think = we need expose some fields to the debug configuration protocol, such as: UINT32 EfiDebuggerRevision; UINT32 EbcVmRevision; UINT32 FeatureFlags; 2) The code in EbdDebugger\* I compare them with the previous one. Looks good to me. 3) The update for original EbdDxe driver. 3.1) EFI_EBC_DEBUGGER_CODE() Yes, I have similar thought as Mike. In EDK-I, we do not have good infrastructure to enable/disable features. Us= ing MACRO is the only choice. In EDK-II, MACRO is not encouraged because it cannot cover build. We may consider 2 options: 3.1.1) Use PCD - such as PcdEbcDebuggerEnable. Then we can put all EbdDebugger related feature into this PCD. This is an easy way, but it causes debugger code build every time. As summary, we can: 3.1.1.A) Add a FeaturePcd:PcdEbcDebuggerEnable to MdeModulePkg.dec, and use= this FeaturePcd to replace EFI_EBC_DEBUGGER_CODE. 3.1.2) Use Library - such EbcDebuggerLib. The API in this library class is similar to the function in EbcDebuggerHook= .h. We can provide 2 instances: one is EbcDebuggerLib - real function one. The = other is EbdDebuggerLibNull - null function one. The latter can be used by = default to bring minimal build impact. Personally, my preference is 3.1.2) because it can make code clean and alig= n with our other debugger feature. I found EbcDebugger code is using the OPCODE defined in EbcExecute.h. Maybe= a better way is to move this UEFI specification define OPCODE to MdePkg Eb= c.h. So that the EbcDebugger code does not need depend on EbcDriver. As summary, we can: 3.1.2.A) Move UEFI specification defined EBC OPCODE from EbcExecute.h to Md= ePkg\Include\Protocol\Ebc.h 3.1.2.B) Add EbcDebuggerLib.h library class to MdeModulePkg/Include/Library 3.1.2.C) Add EbcDebuggerNullLib library instance to MdeModulePkg/Library/ 3.1.2.D) Add EbcDebuggerLib library instance to MdeModulePkg/Library/ 3.2) EbcExecute(), I cannot find below original code. Would you please doub= le check? // // Verify the opcode is in range. Otherwise generate an exception. // if ((*VmPtr->Ip & OPCODE_M_OPCODE) >=3D (sizeof (mVmOpcodeTable) / size= of (mVmOpcodeTable[0]))) { EbcDebugSignalException (EXCEPT_EBC_INVALID_OPCODE, EXCEPTION_FLAG_FA= TAL, VmPtr); Status =3D EFI_UNSUPPORTED; goto Done; } 3.3) ExecuteBREAK(), I cannot find below original code. Would you please do= uble check? case 3: VmPtr->StopFlags |=3D STOPFLAG_BREAKPOINT; VmPtr->Ip +=3D 2; <=3D=3D=3D=3D=3D=3D=3D=3D here // // See if someone has registered a handler // EbcDebugSignalException ( EXCEPT_EBC_BREAKPOINT, EXCEPTION_FLAG_NONE, VmPtr ); return EFI_SUCCESS; <=3D=3D=3D=3D=3D=3D=3D=3D here break; Thank You Yao Jiewen From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pete= Batard Sent: Saturday, November 12, 2016 8:44 AM To: Yao, Jiewen ; edk2-devel@lists.01.org Subject: Re: [edk2] [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger Hi Yao - good to hear from you. On 2016.11.11 23:48, Yao, Jiewen wrote: > I have worked on this for EDK-I, but just did not have resource to do the= porting for EDK-II. Thank you for doing this. I'll be glad if I can be of help. The EBC Debugger has been tremendously useful during my development of an EBC assembler as well as the troubleshooting of related EBC code. So, while I understand that porting the EBC Debugger may not have been a high priority for EDK2, I'm very thankful for the work you have done on this in the past! > Please give me some time, and I will review the patch soon. Sounds good. I'll be trying to post a v2 early next week, that takes into account the points Mike made. By the way, if you have some ideas for what you'd prefer for the PCD, or any other potential improvements, don't hesitate to let me know, since you're probably a lot more familiar with all of this than I am. Regards, /Pete _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel