target-openrisc: bugfixes for debugging with GDB+Qemu on OpenRISC
PATCH 1/1 target-openrisc: bugfixes for debugging with GDB+Qemu on OpenRISC
This patch fixes two bugs in Qemu for OpenRISC, and enables more functionality from or1k-elf-gdb:
1) Fixed the decoding of "system" instructions (starting with 0x2) in dec_sys() in translate.c. In particular, the l.trap instruction is now correctly decoded, which enables for singlestepping and breakpoints to be set in GDB.
2) Fixed a memory read error when debugging kernels inside Qemu and the OpenRISC MMU is enabled
--- target-openrisc/cpu.h | 1 + target-openrisc/mmu.c | 2 +- target-openrisc/translate.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-)
target-openrisc/cpu.h +1 -0
@@ -20,6 +20,7 @@#ifndef CPU_OPENRISC_H #define CPU_OPENRISC_H +#define TARGET_HAS_ICE #define TARGET_LONG_BITS 32#define ELF_MACHINE EM_OPENRISC
target-openrisc/mmu.c +1 -1
@@ -219,7 +219,7 @@ hwaddr openrisc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)hwaddr phys_addr;int prot; -if (cpu_openrisc_get_phys_addr (cpu, &phys_addr, &prot, addr,0 )) { +if (cpu_openrisc_get_phys_nommu (cpu, &phys_addr, &prot, addr,0 )) {return -1 ; }
target-openrisc/translate.c +1 -1
@@ -1320,7 +1320,7 @@ static void dec_sys(DisasContext *dc, uint32_t insn)#ifdef OPENRISC_DISAS uint32_t K16 ;#endif - op0 =extract32 (insn,16 ,8 ); + op0 =extract32 (insn,16 ,10 );#ifdef OPENRISC_DISAS K16 =extract32 (insn,0 ,16 );#endif
Signature · 1 line
2.1.3
ping
Quoted 57 lines · David Morrison
This patch fixes two bugs in Qemu for OpenRISC, and enables more functionality from or1k-elf-gdb:
1) Fixed the decoding of "system" instructions (starting with
0x2) indec_sys()intranslate.c. In particular, the l.trap instruction is now correctly decoded, which enables for singlestepping and breakpoints to be set in GDB.2) Fixed a memory read error when debugging kernels inside Qemu and the OpenRISC MMU is enabled
Signed-off-by David R. Morrison <[email protected]>--- target-openrisc/cpu.h | 1 + target-openrisc/mmu.c | 2 +- target-openrisc/translate.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-)target-openrisc/cpu.h +1
-0@@ -20,6 +20,7 @@#ifndef CPU_OPENRISC_H #define CPU_OPENRISC_H +#define TARGET_HAS_ICE #define TARGET_LONG_BITS 32#define ELF_MACHINE EM_OPENRISCtarget-openrisc/mmu.c +1
-1@@ -219,7 +219,7 @@ hwaddr openrisc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)hwaddr phys_addr;int prot; -if (cpu_openrisc_get_phys_addr (cpu, &phys_addr, &prot, addr,0 )) { +if (cpu_openrisc_get_phys_nommu (cpu, &phys_addr, &prot, addr,0 )) {return -1 ; }target-openrisc/translate.c +1
-1@@ -1320,7 +1320,7 @@ static void dec_sys(DisasContext *dc, uint32_t insn)#ifdef OPENRISC_DISAS uint32_t K16 ;#endif - op0 =extract32 (insn,16 ,8 ); + op0 =extract32 (insn,16 ,10 );#ifdef OPENRISC_DISAS K16 =extract32 (insn,0 ,16 );#endif
Signature · 4 lines
David R. Morrison, PhD Inverse Limit [email protected] +1-217-417-9445
Quoted 10 lines · David Morrison
This patch fixes two bugs in Qemu for OpenRISC, and enables more functionality from or1k-elf-gdb:
1) Fixed the decoding of "system" instructions (starting with
0x2) indec_sys()intranslate.c. In particular, the l.trap instruction is now correctly decoded, which enables for singlestepping and breakpoints to be set in GDB.2) Fixed a memory read error when debugging kernels inside Qemu and the OpenRISC MMU is enabled
Thanks for this patch; comments below.
Quoted 12 lines
Signed-off-by David R. Morrison <[email protected]>--- target-openrisc/cpu.h | 1 + target-openrisc/mmu.c | 2 +- target-openrisc/translate.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-)target-openrisc/cpu.h +0
-0
target-openrisc/cpu.h line 25
Quoted 6 lines
target-openrisc/cpu.h +1
-0@@ -20,6 +20,7 @@#ifndef CPU_OPENRISC_H #define CPU_OPENRISC_H +#define TARGET_HAS_ICE #define TARGET_LONG_BITS 32#define ELF_MACHINE EM_OPENRISC
This looks like a correct change, but it should be in its own patch. (The general principle is that each unrelated bug fix should get a patch and thus a git commit of its own.)
target-openrisc/mmu.c hwaddr openrisc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) · line 222
Quoted 10 lines
target-openrisc/mmu.c +1
-1@@ -219,7 +219,7 @@ hwaddr openrisc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)hwaddr phys_addr;int prot; -if (cpu_openrisc_get_phys_addr (cpu, &phys_addr, &prot, addr,0 )) { +if (cpu_openrisc_get_phys_nommu (cpu, &phys_addr, &prot, addr,0 )) {
This looks wrong -- we won't do the virtual-to-physical translation on the addresses provided by the debugger if we use the _nommu() function. You definitely need to be doing a v-to-p translation here somehow.
Quoted 8 lines
target-openrisc/mmu.c +0
-0return -1 ; }target-openrisc/translate.c +0
-0
target-openrisc/translate.c static void dec_sys(DisasContext *dc, uint32_t insn) · line 1326
Quoted 8 lines
target-openrisc/translate.c +1
-1@@ -1320,7 +1320,7 @@ static void dec_sys(DisasContext *dc, uint32_t insn)#ifdef OPENRISC_DISAS uint32_t K16 ;#endif - op0 =extract32 (insn,16 ,8 ); + op0 =extract32 (insn,16 ,10 );#ifdef OPENRISC_DISAS K16 =extract32 (insn,0 ,16 );#endif
This change should also go in a patch of its own, since it's not related to either the HAS_ICE fix or the change to get_phys_page_debug().
thanks
-- PMM
Quoted 7 lines · Peter Maydell
Quoted 6 lines · David Morrison
target-openrisc/cpu.h +0
-0@@ -20,6 +20,7 @@#ifndef CPU_OPENRISC_H #define CPU_OPENRISC_H
Quoted 5 lines
target-openrisc/cpu.h line 25
Quoted 3 lines
target-openrisc/cpu.h +1
-0+#define TARGET_HAS_ICE #define TARGET_LONG_BITS 32#define ELF_MACHINE EM_OPENRISCThis looks like a correct change
...actually, on second thought, we should do this the other way round and just get rid of TARGET_HAS_ICE altogether. The only two targets which don't define it are openrisc and unicore32, and both of those actually do have the breakpoint handling code, so failing to define it was just a bug. I'll cook up a patch...
-- PMM
Hi, Peter,
Thanks for the response. I'll split out the changes into separate commits and resubmit. I do have one question here:
Quoted 9 lines
Quoted 9 lines
target-openrisc/mmu.c +0
-0@@ -219,7 +219,7 @@ hwaddr openrisc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)hwaddr phys_addr;int prot;
Quoted 8 lines
target-openrisc/mmu.c hwaddr openrisc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) · line 222
Quoted 2 lines
target-openrisc/mmu.c +1
-1-if (cpu_openrisc_get_phys_addr (cpu, &phys_addr, &prot, addr,0 )) { +if (cpu_openrisc_get_phys_nommu (cpu, &phys_addr, &prot, addr,0 )) {This looks wrong -- we won't do the virtual-to-physical translation on the addresses provided by the debugger if we use the
_nommu()function. You definitely need to be doing a v-to-p translation here somehow.
I was similarly puzzled by this. However, I've been comparing Qemu to or1ksim, which does not appear to do translation for the debugger; see the following code excerpt from the or1ksim source:
https://github.com/openrisc/or1ksim/blob/or1k-master/debug/rsp-server.c#L1546
rsp_read_mem (struct rsp_buf *buf)
{
...
for (off = 0; off < len; off++)
{
unsigned char ch; /* The byte at the address */
/* Check memory area is valid */
...
// Get the memory direct - no translation.
ch = eval_direct8 (addr + off, 0, 0);
buf->data[off * 2] = hexchars[ch >> 4];
buf->data[off * 2 + 1] = hexchars[ch & 0xf];
}buf->data[off * 2] = 0; /* End of string */ buf->len = strlen (buf->data); put_packet (buf);
} /* rsp_read_mem () */
Moreover, in Qemu if you perform the translation and use GDB to debug, it returns bogus values for the memory read, whereas not performing the translation appears to work correctly. Am I doing something wrong here, or is this possibly a bug in the or1k toolchain instead?
Thanks for your help!
David
Signature · 4 lines
David R. Morrison, PhD Inverse Limit [email protected] +1-217-417-9445
Quoted 25 lines · David Morrison
Hi, Peter,
Thanks for the response. I'll split out the changes into separate commits and resubmit. I do have one question here:
Quoted 10 lines
Quoted 10 lines
target-openrisc/mmu.c +0
-0@@ -219,7 +219,7 @@ hwaddr openrisc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) hwaddr phys_addr;int prot;Quoted 9 lines
target-openrisc/mmu.c hwaddr openrisc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) · line 222
Quoted 2 lines
target-openrisc/mmu.c +1
-1-if (cpu_openrisc_get_phys_addr (cpu, &phys_addr, &prot, addr,0 )) { +if (cpu_openrisc_get_phys_nommu (cpu, &phys_addr, &prot, addr,0 )) {This looks wrong -- we won't do the virtual-to-physical translation on the addresses provided by the debugger if we use the
_nommu()function. You definitely need to be doing a v-to-p translation here somehow.
Quoted 2 lines
I was similarly puzzled by this. However, I've been comparing Qemu to or1ksim, which does not appear to do translation for the debugger
Well, it depends what semantics you want to define for the debug stub. "Addresses are always physical addresses" isn't unreasonable as a choice, but it's not what QEMU has picked -- we always use virtual addresses if the MMU is enabled.
Quoted 3 lines
Moreover, in Qemu if you perform the translation and use GDB to debug, it returns bogus values for the memory read, whereas not performing the translation appears to work correctly.
Are you trying to give gdb physical addresses? That isn't the way we define the gdbstub to work: you need to give it virtual addresses.
-- PMM