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

[Peter Maydell <peter.maydell@linaro.org>]

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