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

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

[Pete Batard]

The EBC Debugger [1], which was present in Tianocore [2], is an 
invaluable tool for EBC development.
This patch adds it back into the EDK2, allowing, for instance, the 
compilation of an AARCH64 EBC debugger.

Note 1: The patch is split in two, so that the changes to the existing 
EbcDxe code are clearer.

Note 2: The diff between the original and the new sources can be found 
at [3]. Most of the changes were for whitespaces/API names/compiler 
warnings, with the notable exception of:
- Bumping of the EBCdebugger version to 1.0.
- Dropping of the DebuggerConfiguration protocol (which would require 
introducing a new global GUID and protocol). I didn't see it as 
particularly useful to caary on and would rather see if there is actual 
demand for it, before adding it back.
- Add of an EFIAPI qualifier for most of the support functions, 
especially the ones dealing with VPrint() ouput. This is required to 
avoid garbage text output in some instances.
- Fixing of the erroneous display of 32 and 64 bit indexes in the 
disassembly.
- Replacement of one assignation with CopyMem() to avoid an intrinsic 
memcpy().

Note 3: I tested the debugger built for AARCH64 and X64 using gcc on 
Linux (Debian/Sid) as well as the one built for IA32 and X64 using 
VS2015 on Windows. I haven't tested an IA64 version for lack of a toolchain.

Regards,

/Pete

[1] http://www.uefi.org/node/550
[2] https://github.com/tianocore/edk/tree/master/Sample/Universal/Ebc/Dxe
[3] 
https://github.com/pbatard/EbcDebugger/commit/906e87ed6ceab1c361ba6f681bef48179baf549e

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

[Kinney, Michael D]

Hi Pete,

I see the new INF files uses '..' in the [Sources]
section, which is not allowed.  Can we move that INF
file up one directory, so it can remove use of ..?

I also see that this code defined its own 
EFI_EBC_DEBUGGER_CODE macro.  Could these be changed
to the standard DEBUG_CODE() macro that can be enabled
and disabled with a PCD?  Or do you think we should add
a new Feature Flag PCD to enable/disable the EBC
debugger?

Thanks,

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pete
> Batard
> Sent: Friday, November 11, 2016 7:51 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger
> 
> The EBC Debugger [1], which was present in Tianocore [2], is an
> invaluable tool for EBC development.
> This patch adds it back into the EDK2, allowing, for instance, the
> compilation of an AARCH64 EBC debugger.
> 
> Note 1: The patch is split in two, so that the changes to the existing
> EbcDxe code are clearer.
> 
> Note 2: The diff between the original and the new sources can be found
> at [3]. Most of the changes were for whitespaces/API names/compiler
> warnings, with the notable exception of:
> - Bumping of the EBCdebugger version to 1.0.
> - Dropping of the DebuggerConfiguration protocol (which would require
> introducing a new global GUID and protocol). I didn't see it as
> particularly useful to caary on and would rather see if there is actual
> demand for it, before adding it back.
> - Add of an EFIAPI qualifier for most of the support functions,
> especially the ones dealing with VPrint() ouput. This is required to
> avoid garbage text output in some instances.
> - Fixing of the erroneous display of 32 and 64 bit indexes in the
> disassembly.
> - Replacement of one assignation with CopyMem() to avoid an intrinsic
> memcpy().
> 
> Note 3: I tested the debugger built for AARCH64 and X64 using gcc on
> Linux (Debian/Sid) as well as the one built for IA32 and X64 using
> VS2015 on Windows. I haven't tested an IA64 version for lack of a toolchain.
> 
> Regards,
> 
> /Pete
> 
> [1] http://www.uefi.org/node/550
> [2] https://github.com/tianocore/edk/tree/master/Sample/Universal/Ebc/Dxe
> [3]
> https://github.com/pbatard/EbcDebugger/commit/906e87ed6ceab1c361ba6f681bef48179ba
> f549e
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Pete Batard]

Hi Mike,

On 2016.11.11 17:41, Kinney, Michael D wrote:
> I see the new INF files uses '..' in the [Sources]
> section, which is not allowed.  Can we move that INF
> file up one directory, so it can remove use of ..?

Sure. I'll work on this and submit a V2.

> I also see that this code defined its own
> EFI_EBC_DEBUGGER_CODE macro.  Could these be changed
> to the standard DEBUG_CODE() macro that can be enabled
> and disabled with a PCD?  Or do you think we should add
> a new Feature Flag PCD to enable/disable the EBC
> debugger?

I've been wondering about keeping the macro as well, which I mostly 
carried over from Tiano. If PCD is the more appropriate EDK2 practice, 
then I agree that we probably want to go with that.

I do feel however that we would need a new feature flag, as some people 
may want to compile an EBC module with the current debug PCD 
functionality enabled, but without the EBC debugger, especially as, in 
essence, the EBC debugger is designed to be intrusive and will break the 
flow of regular EBC execution (such as on program entry or thunk calls).

I'll explore this a little bit further, and try have a PCD proposal for V2.

Regards,

/Pete

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

[Yao, Jiewen]

That is great news. Thank you Pete.

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.

Please give me some time, and I will review the patch soon.

Thank you
Yao Jiewen

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Pete Batard
> Sent: Friday, November 11, 2016 11:51 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger
> 
> The EBC Debugger [1], which was present in Tianocore [2], is an
> invaluable tool for EBC development.
> This patch adds it back into the EDK2, allowing, for instance, the
> compilation of an AARCH64 EBC debugger.
> 
> Note 1: The patch is split in two, so that the changes to the existing
> EbcDxe code are clearer.
> 
> Note 2: The diff between the original and the new sources can be found
> at [3]. Most of the changes were for whitespaces/API names/compiler
> warnings, with the notable exception of:
> - Bumping of the EBCdebugger version to 1.0.
> - Dropping of the DebuggerConfiguration protocol (which would require
> introducing a new global GUID and protocol). I didn't see it as
> particularly useful to caary on and would rather see if there is actual
> demand for it, before adding it back.
> - Add of an EFIAPI qualifier for most of the support functions,
> especially the ones dealing with VPrint() ouput. This is required to
> avoid garbage text output in some instances.
> - Fixing of the erroneous display of 32 and 64 bit indexes in the
> disassembly.
> - Replacement of one assignation with CopyMem() to avoid an intrinsic
> memcpy().
> 
> Note 3: I tested the debugger built for AARCH64 and X64 using gcc on
> Linux (Debian/Sid) as well as the one built for IA32 and X64 using
> VS2015 on Windows. I haven't tested an IA64 version for lack of a toolchain.
> 
> Regards,
> 
> /Pete
> 
> [1] http://www.uefi.org/node/550
> [2]
> https://github.com/tianocore/edk/tree/master/Sample/Universal/Ebc/Dxe
> [3]
> https://github.com/pbatard/EbcDebugger/commit/906e87ed6ceab1c361ba
> 6f681bef48179baf549e
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Pete Batard]

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

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

[Yao, Jiewen]

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

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

[Pete Batard]

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.

> 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.

> 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.

> 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.

Regards,

/Pete

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

[Yao, Jiewen]

Comment inline.

From: Pete Batard [mailto:pete@akeo.ie]
Sent: Saturday, November 12, 2016 8:46 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; 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

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

[Yao, Jiewen]

Maybe we can wait for Mike's thought on PCD or library.



From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Saturday, November 12, 2016 9:43 PM
To: Pete Batard <pete@akeo.ie>; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger

Comment inline.

From: Pete Batard [mailto:pete@akeo.ie]
Sent: Saturday, November 12, 2016 8:46 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@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

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

[Yao, Jiewen]

Hi Pete
I realize that the side-effect of library solution is that: we need update DSC file to add Library instance.

Now I am thinking the third options:
3.1.3) 2 INF without MACRO.
3.1.3.A) Remove EFI_EBC_DEBUGGER_CODE
3.1.3.B) EBC driver always calls EbcDebuggerHookXXX.
3.1.3.C) Create a EbcDebuggerHookNull.c and add it to EbcDxe.inf.
So that no change is needed for a platform, and no PCD is introduced.

3.1.3.D) Move EbcDebugger.inf from EbcDxe\EbcDebugger to EbcDxe
3.1.3.E) Do not include the new EbcDebuggerHookNull.c in EbcDebugger.inf
3.1.3.F) I still suggest we remove EbcInit.h and EbcExecute.h from Edb.h. The common definition should be in a better place.
So that if EDB is needed, a platform just includes EbcDebugger.inf, and EbdDebugger.inf does not include "..".


Thank you
Yao Jiewen


From: Yao, Jiewen
Sent: Sunday, November 13, 2016 3:50 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Pete Batard <pete@akeo.ie>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2] [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger

Maybe we can wait for Mike's thought on PCD or library.



From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Saturday, November 12, 2016 9:43 PM
To: Pete Batard <pete@akeo.ie<mailto:pete@akeo.ie>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Subject: Re: [edk2] [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger

Comment inline.

From: Pete Batard [mailto:pete@akeo.ie]
Sent: Saturday, November 12, 2016 8:46 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@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

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

[Pete Batard]

Hi Jiewen,

On 2016.11.12 21:44, Yao, Jiewen wrote:
> I realize that the side-effect of library solution is that: we need
> update DSC file to add Library instance.

Yes. I started looking at the library option, and I'm kind of wondering 
if we really want to put code in a library when it's unlikely that any 
of the hooks will be used anywhere but once, and in the very specific 
locations we have now.

I still don't mind going that route if that's what we decide, but 
creating a non-generic library, that is only going to be used to build a 
single application seems a bit like an overkill.

> Now I am thinking the third options:
> 3.1.3) 2 INF without MACRO.
> 3.1.3.A) Remove EFI_EBC_DEBUGGER_CODE
> 3.1.3.B) EBC driver always calls EbcDebuggerHookXXX.
> 3.1.3.C) Create a EbcDebuggerHookNull.c and add it to EbcDxe.inf.
> So that no change is needed for a platform, and no PCD is introduced.

This sound like a more elegant solution.

I like this option better than the others so far, so that's what I'll go 
with for v2.

> 3.1.3.F) I still suggest we remove EbcInit.h and EbcExecute.h from
> Edb.h. The common definition should be in a better place.

Agreed.

I'll try to move the common parts to MdePkg\Include\Protocol\Ebc.h in 
the new proposal, if that's still okay with you.

Regards,

/Pete