| Message ID | 1418862393-10691-1-git-send-email-dmorrison@invlim.com |
|---|---|
| State | New |
| Headers | show |
ping On 12/17/2014 04:26 PM, David Morrison wrote: > 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 > > Signed-off-by: David R. Morrison <dmorrison@invlim.com> > --- > target-openrisc/cpu.h | 1 + > target-openrisc/mmu.c | 2 +- > target-openrisc/translate.c | 2 +- > 3 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/target-openrisc/cpu.h b/target-openrisc/cpu.h > index 69b96c6..6b08af6 100644 > --- a/target-openrisc/cpu.h > +++ b/target-openrisc/cpu.h > @@ -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 > > diff --git a/target-openrisc/mmu.c b/target-openrisc/mmu.c > index 750a936..bbd05f1 100644 > --- a/target-openrisc/mmu.c > +++ b/target-openrisc/mmu.c > @@ -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; > } > > diff --git a/target-openrisc/translate.c b/target-openrisc/translate.c > index 407bd97..d36278f 100644 > --- a/target-openrisc/translate.c > +++ b/target-openrisc/translate.c > @@ -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 >
On 18 December 2014 at 00:26, David Morrison <dmorrison@invlim.com> wrote: > 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 Thanks for this patch; comments below. > Signed-off-by: David R. Morrison <dmorrison@invlim.com> > --- > target-openrisc/cpu.h | 1 + > target-openrisc/mmu.c | 2 +- > target-openrisc/translate.c | 2 +- > 3 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/target-openrisc/cpu.h b/target-openrisc/cpu.h > index 69b96c6..6b08af6 100644 > --- a/target-openrisc/cpu.h > +++ b/target-openrisc/cpu.h > @@ -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.) > diff --git a/target-openrisc/mmu.c b/target-openrisc/mmu.c > index 750a936..bbd05f1 100644 > --- a/target-openrisc/mmu.c > +++ b/target-openrisc/mmu.c > @@ -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. > return -1; > } > > diff --git a/target-openrisc/translate.c b/target-openrisc/translate.c > index 407bd97..d36278f 100644 > --- a/target-openrisc/translate.c > +++ b/target-openrisc/translate.c > @@ -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
On 5 January 2015 at 18:15, Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 December 2014 at 00:26, David Morrison <dmorrison@invlim.com> wrote: >> --- a/target-openrisc/cpu.h >> +++ b/target-openrisc/cpu.h >> @@ -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 ...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: > >> diff --git a/target-openrisc/mmu.c b/target-openrisc/mmu.c >> index 750a936..bbd05f1 100644 >> --- a/target-openrisc/mmu.c >> +++ b/target-openrisc/mmu.c >> @@ -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. > 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
On 5 January 2015 at 18:41, David Morrison <dmorrison@invlim.com> wrote: > Hi, Peter, > > Thanks for the response. I'll split out the changes into separate commits > and resubmit. I do have one question here: > >> >>> diff --git a/target-openrisc/mmu.c b/target-openrisc/mmu.c >>> index 750a936..bbd05f1 100644 >>> --- a/target-openrisc/mmu.c >>> +++ b/target-openrisc/mmu.c >>> @@ -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. >> > > 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. > 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
diff --git a/target-openrisc/cpu.h b/target-openrisc/cpu.h index 69b96c6..6b08af6 100644 --- a/target-openrisc/cpu.h +++ b/target-openrisc/cpu.h @@ -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 diff --git a/target-openrisc/mmu.c b/target-openrisc/mmu.c index 750a936..bbd05f1 100644 --- a/target-openrisc/mmu.c +++ b/target-openrisc/mmu.c @@ -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; } diff --git a/target-openrisc/translate.c b/target-openrisc/translate.c index 407bd97..d36278f 100644 --- a/target-openrisc/translate.c +++ b/target-openrisc/translate.c @@ -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 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 Signed-off-by: David R. Morrison <dmorrison@invlim.com> --- target-openrisc/cpu.h | 1 + target-openrisc/mmu.c | 2 +- target-openrisc/translate.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-)