From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x242.google.com (mail-wm0-x242.google.com [IPv6:2a00:1450:400c:c09::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6D54381EC9 for ; Sat, 12 Nov 2016 04:46:23 -0800 (PST) Received: by mail-wm0-x242.google.com with SMTP id a20so3118752wme.2 for ; Sat, 12 Nov 2016 04:46:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-transfer-encoding; bh=tfAZtcmMfZy2/y7vRlL9m/ZbUvckYRlH9uzjlxawRA4=; b=M/+265S73x1z9yAdGOSHRiKt/mIQRFGvNA3LvW1AGYjZu9/YpwO2frbaWA3AC6TJ9D ajecZYVpTzSXRRa2HAHaOnBAMW6YACAfjrxMSGvtBPfozBWXmlCedzN+EiEWwtOsFolX D1bRGLt5ykDPnyMivsuAnV2V5RSTlSilt1My+qdMcgDbO72NC4w2x+fOXbCAAwqNM1KQ +03k2RwQ4NCo9C1gfvsGKkunVWEsa6LV5/9UPBORloSgwFwrbCUT4s+Z7QJvFoWujsCQ VgQTxm8T1c0lgHEfhHMu/BVy5rGQIZcek5WJyKt8QWnMBBDjcY/B9wMgn1TPAZ3tYk7C edHw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=tfAZtcmMfZy2/y7vRlL9m/ZbUvckYRlH9uzjlxawRA4=; b=kq5eVyZSeNCSoMUQ8ps4/HbQIHW9H+v1S7KcCCJEgsoWfww+gurswnWR/ehEbN8CZU t2G7yaOWCeLFzAGTLAOSR1E/BRK8b1U+baAWYrLYzchfZBoCjjIAb8QvnNFfOTilr1Q1 NiA8wRi3rHB0XyjWpRo+SwSI++rYuw5Evmdq4vCB8wRgbuaqsAKsf8zyKK4uF2W9rq4B WU8sOqdiCuXMbdAr8ktIwefAn8ORQ18wiai1bQKN7ZEnI3xjDwUmP/oIXXADtRq+DqsU hBYkESr4ADO8U7r3ryrwYSbS3tyABhNhr+00SIJ78qFpuKqdLaPA21cKlwuWiZSykJIg lQPg== X-Gm-Message-State: ABUngvcd8Nljpjda+8WzqmhzaKUE6XwAqkLOsiw1L4F+dFcxjVJfK7IXAH9A4fmcrCrHmQ== X-Received: by 10.28.52.201 with SMTP id b192mr2376054wma.118.1478954785952; Sat, 12 Nov 2016 04:46:25 -0800 (PST) Received: from [10.0.0.101] ([84.203.54.175]) by smtp.googlemail.com with ESMTPSA id jx8sm16970942wjc.2.2016.11.12.04.46.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 12 Nov 2016 04:46:25 -0800 (PST) To: "Yao, Jiewen" , "edk2-devel@lists.01.org" References: <22709fc2-7dec-f9eb-43f7-d06405349b7e@akeo.ie> <74D8A39837DF1E4DA445A8C0B3885C50386CF903@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C50386CF9E0@shsmsx102.ccr.corp.intel.com> From: Pete Batard Message-ID: <283cb151-bde0-6866-2646-244a6dace583@akeo.ie> Date: Sat, 12 Nov 2016 12:46:23 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C50386CF9E0@shsmsx102.ccr.corp.intel.com> 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 12:46:23 -0000 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit 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