Maybe we can wait for Mike's thought on PCD or library.
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yao, Jiewen Sent: Saturday, November 12, 2016 9:43 PM To: Pete Batard <p...@akeo.ie>; edk2-devel@lists.01.org Subject: Re: [edk2] [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger Comment inline. From: Pete Batard [mailto:p...@akeo.ie] Sent: Saturday, November 12, 2016 8:46 PM To: Yao, Jiewen <jiewen....@intel.com<mailto:jiewen....@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Subject: Re: [edk2] [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger On 2016.11.12 07:48, Yao, Jiewen wrote: > 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. Sorry, no, they are not the same. Please do not use that project, as it is missing some important parts (and also has changes that shouldn't be part of the current proposal). It was the project where I've been experimenting, before I cleaned things up submit this proposal, and I am planning to update it after this patch has been integrated to reflect the cleanup, and the other changes we are discussing. The actual github project to use for this proposal is: https://github.com/pbatard/edk2/tree/EBCDebugger > However, the master contains some non-EBC-debugger related code. Yes it does. Please disregard that earlier github project you found, and only use https://github.com/pbatard/edk2/tree/EBCDebugger. > The edk2-diff branch contains an old version of EBC driver code. The edk2-diff branch is only intended to show the changes between the old and new code, since this patch just provides the new sources under EBCDebugger/ wholesale, which makes it hard to see what was altered there. Also please note that the diff is only for the content under EBCDebugger/. > Next time, would you please post your V2 patches to a new branch, help > me do the review efficiently? I'll make sure to create a github branch for V2. [Jiewen] Thank you. > 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. Yeah. As I indicated in my notes I dropped the debugger configuration protocol, because I didn't see much use for it. > 2) (..) 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. Thanks, I will do that. Do you think maybe we could split adding the EBC debugger without the configuration protocol, and then work on a separate patch to add it back? Especially, if EbcDebuggerApp needs to be ported, I think I'd prefer to split these 2 tasks into 2 separate series of patch, even if it means that, for a while, the EBC debugger will not match its manual when it comes to configuration. [Jiewen] I think it is OK. I agree with you that this can be handled in different patch. Let's focus on current one at first. > 3.1.2) Use Library - such EbcDebuggerLib. > > Personally, my preference is 3.1.2) because it can make code clean and > align with our other debugger feature. Okay. I'll try to work with 3.1.2 then, as I don't have a strong preference. [Jiewen] Thank you. > 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; > } This code is unrelated to the EBC Debugger and was removed in 2009 in the official EDK2: https://github.com/tianocore/edk2/commit/ead7e7dc748750e88a1d1d5810c4550edeabb22f > 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; Similarly the 2 lines you point to have never been present in the EDK2 version I see of ExecuteBREAK(): https://github.com/tianocore/edk2/blob/53c71d097b13311e2bd8dda6ae54b5766a1c7d6d/MdeModulePkg/Universal/EbcDxe/EbcExecute.c#L1115-L1125 If the sections you point to need to be modified, I think that would be better left to a different patch, as these would apply to the generic EbcDxe module and not the EBC Debugger. [Jiewen] That makes sense. We can use separate patch to handle these 2 enhancement. I agree with you. 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
