Brian,
Sorry, I missed this patch.
Thank you finding and fixing this bug. Your patch looks good to me. One
minimal comment is to remove the word "qemu" from the code comment, because we
will avoid to use any specific platform information in core code.
> + if ((NewDr7 & ~BIT10) == 0) { // H/w sets bit 10, qemu doesn't
I suggest you may say "some simulator platform doesn't".
PS: CR4 seems has the similar issue reported by you on
DEBUG_LOAD_IMAGE_METHOD_SOFT_INT3 path. Are you interesting in fixing it
together in your patch?
Thanks!
Jeff
-----Original Message-----
From: Brian J. Johnson [mailto:[email protected]]
Sent: Saturday, March 14, 2015 12:41 AM
To: [email protected]
Cc: Fan, Jeff
Subject: Re: [edk2] [PATCH] PeCoffExtraActionLibDebug: Restore debug registers
in PeCoffExtraActionLibDebug
Ping?
CCing the SourceLevelDebugPkg maintainer.
Brian
On 03/05/2015 09:26 AM, Brian J. Johnson wrote:
> PeCoffExtraActionLibDebug uses the debug registers to pass module load
> information to the DebugAgent, then restores the old register values.
> However, it was missing code to restore Dr7 in the
> DEBUG_LOAD_IMAGE_METHOD_SOFT_INT3 case. This broke hardware
> breakpoints and watchpoints.
>
> Restore the values correctly in the DEBUG_LOAD_IMAGE_METHOD_SOFT_INT3
> case, as well as the DEBUG_LOAD_IMAGE_METHOD_IO_HW_BREAKPOINT case.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Brian J. Johnson <[email protected]>
> ---
>
> This fix has been built with OvmfPkg/OvmfPkgIa32X64.dsc, using the
> GCC48 toolchain under Ubuntu. It has been tested (32- and 64-bit
> mode) using qemu with tcg and a proprietary DebugAgentLib. We've also
> used it on hardware in a proprietary BIOS with various Intel CPUs,
> using the SourceLevelDebugPkg DebugAgentLib (an older version). It's
> been built successfully with the WINDDK compilers, but I'm not really
> in a position to build with MSVC.
>
> A note on Dr7 bit 10: the Intel SDM says that this bit always reads
> as 1, and that's the behavior I see on hardware. However, on
> simulators (including qemu in tcg mode) it often reads as 0. So I
> just ignore it in the patch below when determining if Dr7 needs to be
> restored.
>
> Thanks,
> Brian
>
> .../PeCoffExtraActionLib.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git
> a/SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraAct
> ionLib.c
> b/SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraAct
> ionLib.c
> index 9bf76bf..be26293 100644
> ---
> a/SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraAct
> ionLib.c
> +++
> b/SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraAct
> ionLib.c @@ -164,8 +164,14 @@ PeCoffLoaderExtraActionCommon (
> if (AsmReadCr4 () == (Cr4 | BIT3)) {
> AsmWriteCr4 (Cr4);
> }
> - if (NewDr7 == 0x20000480) {
> - AsmWriteDr7 (Dr7);
> + if (LoadImageMethod == DEBUG_LOAD_IMAGE_METHOD_IO_HW_BREAKPOINT) {
> + if (NewDr7 == 0x20000480) {
> + AsmWriteDr7 (Dr7);
> + }
> + } else if (LoadImageMethod == DEBUG_LOAD_IMAGE_METHOD_SOFT_INT3) {
> + if ((NewDr7 & ~BIT10) == 0) { // H/w sets bit 10, qemu doesn't
> + AsmWriteDr7 (Dr7);
> + }
> }
> //
> // Restore original IDT entry for INT1 if it was hooked.
>
> ----------------------------------------------------------------------
> -------- Dive into the World of Parallel Programming The Go Parallel
> Website, sponsored by Intel and developed in partnership with Slashdot
> Media, is your hub for all things parallel software development, from
> weekly thought leadership blogs to news, videos, case studies,
> tutorials and more. Take a look and join the conversation now.
> http://goparallel.sourceforge.net/
> _______________________________________________
> edk2-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
