Patchwork i386 debugging stubs: Consider segment bases

login
register
mail settings
Submitter Eddie Kohler
Date Sept. 25, 2010, 12:25 a.m.
Message ID <4C9D415F.6090909@cs.ucla.edu>
Download mbox | patch
Permalink /patch/65714/
State New
Headers show

Comments

Eddie Kohler - Sept. 25, 2010, 12:25 a.m.
Hi,

QEMU has a bug that complicates GDB debugging of i386 targets when the
current code or data segment has a nonzero base.  A fix is attached.

If the current code segment has a nonzero base, breakpoints don't work
as expected, because the breakpoint detector does not consider segment
bases.

If the current data segment has a nonzero base, memory inspection doesn't
work, because cpu_get_phys_page_debug does not consider segment bases.

A tiny 'operating system' demonstrating the problem is here:

http://read.cs.ucla.edu/~kohler/qemu-gdbseg-demo.tgz

The README enclosed in that tarball gives steps on how to replicate the
breakpoint problem.  The 'kernel' runs with segment base 0x10000000, so
that linear address 0xF0001000 is translated into physical address
0x00001000.  But breakpoints (which should use virtual addresses) at
a linear address (e.g. 0xF0100000) are ignored.  You can stop execution
using a physical address, but all the addresses reported
back to GDB are linear addresses, so this isn't consistent.

This is a real problem that prevents us from using unpatched QEMU in
classwork.  Any comments on the fix??  (A version was initially posted
several years ago.)

Thanks,
Eddie Kohler



From 6784824c7576514456a989192e07e63352bdb4ae Mon Sep 17 00:00:00 2001
From: Eddie Kohler <ekohler@gmail.com>
Date: Fri, 24 Sep 2010 16:42:27 -0700
Subject: [PATCH] i386 debugging stubs: Consider segment bases

- Access dumpable memory relative to the current data segment base.
- Detect breakpoints relative to the current code segment base.
---
 target-i386/helper.c    |    1 +
 target-i386/translate.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)
Jan Kiszka - Sept. 25, 2010, 7:22 a.m.
Am 25.09.2010 02:25, Eddie Kohler wrote:
> Hi,
> 
> QEMU has a bug that complicates GDB debugging of i386 targets when the
> current code or data segment has a nonzero base.  A fix is attached.
> 
> If the current code segment has a nonzero base, breakpoints don't work
> as expected, because the breakpoint detector does not consider segment
> bases.
> 
> If the current data segment has a nonzero base, memory inspection doesn't
> work, because cpu_get_phys_page_debug does not consider segment bases.
> 
> A tiny 'operating system' demonstrating the problem is here:
> 
> http://read.cs.ucla.edu/~kohler/qemu-gdbseg-demo.tgz
> 
> The README enclosed in that tarball gives steps on how to replicate the
> breakpoint problem.  The 'kernel' runs with segment base 0x10000000, so
> that linear address 0xF0001000 is translated into physical address
> 0x00001000.  But breakpoints (which should use virtual addresses) at
> a linear address (e.g. 0xF0100000) are ignored.  You can stop execution
> using a physical address, but all the addresses reported
> back to GDB are linear addresses, so this isn't consistent.
> 
> This is a real problem that prevents us from using unpatched QEMU in
> classwork.  Any comments on the fix??  (A version was initially posted
> several years ago.)

This is just a workaround for the core issue: lacking segment support in
gdb and its remote protocol. Assuming that an address passed via the
current protocol refers to CS and DS is broken. Your patch e.g. requires
that CS.base == DS.base, which is not always the case with truly
segmented OSes. And it prevents setting breakpoints in inactive segments.

We really need to fix gdb instead, i.e. transfer the segment states so
that the frontend can linearize as required. And gdb needs to be
extended to support segment:offset addressing syntax.

For the time being, you should be able to workaround the gdb limitation
by setting two breakpoints: one on the linear address and another one on
the CS offset. Not nice, but used to work for us.

Jan
Eddie Kohler - Sept. 25, 2010, 8:35 a.m.
Thanks for the response.  I agree the patch is a workaround, but it is a 
useful workaround, and I'd still argue for including it.

The patch doesn't *require* that CS.base == DS.base.  Breakpoints 
correctly and exclusively use CS.base.  However, any memory examination 
uses DS.base, and you're right that the user might "want" to examine 
some other segment.  A GDB fix would involve changing the gdb remote 
protocol as well as GDB itself and the GDB user interface.  Google says 
you've been thinking about that for a while now -- is it going well?

> For the time being, you should be able to workaround the gdb limitation
> by setting two breakpoints: one on the linear address and another one on
> the CS offset. Not nice, but used to work for us.

I don't mind the double-breakpoint as much, but memory examination would 
still be broken, yes?

I don't understand the comment about "prevents setting breakpoints on 
inactive segments."  The code for setting breakpoints has not changed.

Do you think the patch would actually make debugging WORSE on any OS? 
Or have any other undesirable effects, or make it harder to DTRT when 
GDB is ready?  It seems safe & useful to me; & it's 2 LOC!

Eddie
Jan Kiszka - Sept. 26, 2010, 6:44 a.m.
Am 25.09.2010 10:35, Eddie Kohler wrote:
> Thanks for the response.  I agree the patch is a workaround, but it is a
> useful workaround, and I'd still argue for including it.

Nope, sorry, I have to vote against this.

> 
> The patch doesn't *require* that CS.base == DS.base.  Breakpoints

It does. There are several parts in QEMU that use cpu_memory_rw_debug
for reading or writing code segment: the disassembler, KVM when
manipulating soft breakpoints, and also the TB flushing on breakpoint
changes relies on cpu_get_phys_page_debug and would break when using DS
as base.

> correctly and exclusively use CS.base.  However, any memory examination
> uses DS.base, and you're right that the user might "want" to examine
> some other segment.  A GDB fix would involve changing the gdb remote
> protocol as well as GDB itself and the GDB user interface.  Google says
> you've been thinking about that for a while now -- is it going well?

It's on a long list of things that would be nice to work on...

> 
>> For the time being, you should be able to workaround the gdb limitation
>> by setting two breakpoints: one on the linear address and another one on
>> the CS offset. Not nice, but used to work for us.
> 
> I don't mind the double-breakpoint as much, but memory examination would
> still be broken, yes?

Issue "monitor info registers", extract the segment base, add it to the
variable address you are interested in, and issue a print request. It is
definitely not impossible, just "a bit" unhandy.

> 
> I don't understand the comment about "prevents setting breakpoints on
> inactive segments."  The code for setting breakpoints has not changed.

It has because you unconditionally subtract the CS base from the passed
address. If you want to set a breakpoint on some other CS, you would
have to account for their base offsets and pass a weirdly "corrected"
address from gdb. That's really no sane interface, specifically long-term.

> 
> Do you think the patch would actually make debugging WORSE on any OS? Or
> have any other undesirable effects, or make it harder to DTRT when GDB
> is ready?  It seems safe & useful to me; & it's 2 LOC!

The pathes change the interface to gdb by re-defining the semantics of
the passed addresses in way that is not future-proof, and they are buggy.

Jan
Eddie Kohler - Sept. 26, 2010, 5:19 p.m.
OK, thanks.  I understand how you're relying on the current behavior.

I'd rather not change all of QEMU and GDB in one step, but I'd like to 
address this.  QEMU documentation implies, and new users expect, that 
debugging uses virtual addresses, not the segmentation-specific "linear 
addresses" that are actually used now.

- How about a maintenance packet type that changed behavior to what I 
would prefer (breakpoints and memory access use virtual addresses, not 
linear addresses)?

- We could add a "segment identifier" parameter to 
cpu_get_phys_page_debug, ignored on all targets but i386 at first.  Then 
we could pass information through to cpu_get_phys_page_debug about what 
kind of address is being translated.  This change could be propagated to 
cpu_memory_rw_debug (now or later).  Would you object?

Eddie
Jan Kiszka - Sept. 27, 2010, 6:29 a.m.
Am 26.09.2010 19:19, Eddie Kohler wrote:
> OK, thanks.  I understand how you're relying on the current behavior.
> 
> I'd rather not change all of QEMU and GDB in one step,

The first step is changing gdb anyway.

> but I'd like to
> address this.  QEMU documentation implies, and new users expect, that
> debugging uses virtual addresses, not the segmentation-specific "linear
> addresses" that are actually used now.
> 
> - How about a maintenance packet type that changed behavior to what I
> would prefer (breakpoints and memory access use virtual addresses, not
> linear addresses)?
> 
> - We could add a "segment identifier" parameter to
> cpu_get_phys_page_debug, ignored on all targets but i386 at first.  Then
> we could pass information through to cpu_get_phys_page_debug about what
> kind of address is being translated.  This change could be propagated to
> cpu_memory_rw_debug (now or later).  Would you object?

These changes would establish a temporary interface for an incomplete
workaround, and that even with impact on non-x86 code. I would prefer if
you could invest your time on the gdb side instead. Anything improved
there is not lost - in contrast to the modifications of qemu. I would
try to support any effort in this direction.

BTW, gdb has nice Python binding these days, so some extensions may also
start their life as a helper script.

Jan

Patch

diff --git a/target-i386/helper.c b/target-i386/helper.c
index e134340..0bfd4a9 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -831,6 +831,7 @@  target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr)
     target_phys_addr_t paddr;
     uint32_t page_offset;
     int page_size;
+    addr += env->segs[R_DS].base;
 
     if (env->cr[4] & CR4_PAE_MASK) {
         target_ulong pdpe_addr;
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 7b6e3c2..d9e5b79 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7816,7 +7816,7 @@  static inline void gen_intermediate_code_internal(CPUState *env,
     for(;;) {
         if (unlikely(!QTAILQ_EMPTY(&env->breakpoints))) {
             QTAILQ_FOREACH(bp, &env->breakpoints, entry) {
-                if (bp->pc == pc_ptr &&
+                if (bp->pc == pc_ptr - dc->cs_base &&
                     !((bp->flags & BP_CPU) && (tb->flags & HF_RF_MASK))) {
                     gen_debug(dc, pc_ptr - dc->cs_base);
                     break;