HI Pete I am glad that you like it. :) I met some problem on extracting patch from email. So I reviewed the code in 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 do the review efficiently? Now I only checked the diff file. Here is some suggestion for your consideration: 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%20User%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 dir. 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. Using 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 align 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 Ebc.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 MdePkg\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 double check? // // Verify the opcode is in range. Otherwise generate an exception. // if ((*VmPtr->Ip & OPCODE_M_OPCODE) >= (sizeof (mVmOpcodeTable) / sizeof (mVmOpcodeTable[0]))) { EbcDebugSignalException (EXCEPT_EBC_INVALID_OPCODE, EXCEPTION_FLAG_FATAL, VmPtr); Status = EFI_UNSUPPORTED; goto Done; } 3.3) ExecuteBREAK(), I cannot find below original code. Would you please double check? case 3: VmPtr->StopFlags |= STOPFLAG_BREAKPOINT; VmPtr->Ip += 2; <======== here // // See if someone has registered a handler // EbcDebugSignalException ( EXCEPT_EBC_BREAKPOINT, EXCEPTION_FLAG_NONE, VmPtr ); return EFI_SUCCESS; <======== here break; Thank You Yao Jiewen From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Pete Batard Sent: Saturday, November 12, 2016 8:44 AM To: Yao, Jiewen <jiewen....@intel.com>; 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<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
