Re: [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger

["Yao, Jiewen" <jiewen.yao@intel.com>]

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-bounces@lists.01.org] On Behalf Of Pete Batard
Sent: Saturday, November 12, 2016 8:44 AM
To: Yao, Jiewen <jiewen.yao@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