[Qemu-devel] [PATCH] target-openrisc: bugfixes for debugging with GDB+Qemu on OpenRISC

[Qemu-devel] [PATCH] target-openrisc: bugfixes for debugging with GDB+Qemu on OpenRISC

[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)
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
-- 
2.1.3

Re: [Qemu-devel] [PATCH] target-openrisc: bugfixes for debugging with GDB+Qemu on OpenRISC

[David Morrison]

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
>

-- 
David R. Morrison, PhD
Inverse Limit
dmorrison@invlim.com
+1-217-417-9445

Re: [Qemu-devel] [PATCH] target-openrisc: bugfixes for debugging with GDB+Qemu on OpenRISC

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] target-openrisc: bugfixes for debugging with GDB+Qemu on OpenRISC

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] target-openrisc: bugfixes for debugging with GDB+Qemu on OpenRISC

[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:

>
>> 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

-- 
David R. Morrison, PhD
Inverse Limit
dmorrison@invlim.com
+1-217-417-9445

Re: [Qemu-devel] [PATCH] target-openrisc: bugfixes for debugging with GDB+Qemu on OpenRISC

[Peter Maydell]

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