Re: [Qemu-devel] [PATCH 1/2] Support for MIPS64 user mode emulation

[Khansa Butt]

please see inline comments highlighted in red color.

On Wed, Apr 13, 2011 at 2:32 AM, Aurelien Jarno <address@hidden> wrote:

[I don't know very well linux-user, it would be nice to Cc: Riku Voipio,
 the linux-user maintainer for the next version.]

On Sat, Apr 09, 2011 at 04:02:31PM +0500, Khansa Butt wrote:
> From e96e20e50cada1c9e1b65de5925281cdd5659746 Mon Sep 17 00:00:00 2001
> From: Ehsan-ul-Haq & Khansa Butt <address@hidden>
> Date: Sat, 9 Apr 2011 10:51:22 +0500
> Subject: [PATCH 1/2] Support for MIPS64 user mode emulation
>
>
> Signed-off-by: Ehsan-ul-Haq, Abdul Qadeer, Abdul Waheed, Khansa Butt <
> address@hidden>
> ---
>  configure                             |    1 +
>  default-configs/mips64-linux-user.mak |    1 +
>  linux-user/elfload.c                  |    2 +-
>  linux-user/main.c                     |   29 +++++++++++++++++++++++++++--
>  linux-user/mips64/syscall.h           |    3 +++
>  linux-user/signal.c                   |    3 ++-
>  target-mips/translate.c               |    1 +
>  7 files changed, 36 insertions(+), 4 deletions(-)
>  create mode 100644 default-configs/mips64-linux-user.mak
>
> diff --git a/configure b/configure
> index ae97e11..d1f7867 100755
> --- a/configure
> +++ b/configure
> @@ -1039,6 +1039,7 @@ m68k-linux-user \
>  microblaze-linux-user \
>  microblazeel-linux-user \
>  mips-linux-user \
> +mips64-linux-user \
>  mipsel-linux-user \
>  ppc-linux-user \
>  ppc64-linux-user \
> diff --git a/default-configs/mips64-linux-user.mak
> b/default-configs/mips64-linux-user.mak
> new file mode 100644
> index 0000000..1598bfc
> --- /dev/null
> +++ b/default-configs/mips64-linux-user.mak
> @@ -0,0 +1 @@
> +# Default configuration for mips64-linux-user
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index fe5410e..2832a33 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1384,7 +1384,7 @@ static void load_elf_image(const char *image_name, int
> image_fd,
>              vaddr_po = TARGET_ELF_PAGEOFFSET(vaddr);
>              vaddr_ps = TARGET_ELF_PAGESTART(vaddr);
>
> -            error = target_mmap(vaddr_ps, eppnt->p_filesz + vaddr_po,
> +            error = target_mmap(vaddr_ps, eppnt->p_memsz + vaddr_po,

What is the goal of this change? If the mmapped aread is bigger than the
file size rounded up to te page size, it will cause a SIGBUS.

>                                  elf_prot, MAP_PRIVATE | MAP_FIXED,
>                                  image_fd, eppnt->p_offset - vaddr_po);
>              if (error == -1) {
> diff --git a/linux-user/main.c b/linux-user/main.c
> index e651bfd..a7f4955 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -1937,6 +1937,14 @@ static int do_store_exclusive(CPUMIPSState *env)
>      int d;
>
>      addr = env->lladdr;
> +#if defined(TARGET_MIPS64)
> +/* For MIPS64 on 32 bit host there is a need to make
> +* the page accessible to which the above 'addr' is belonged */
> +#if HOST_LONG_BITS == 32
> +    int flag = PAGE_VALID | PAGE_READ | PAGE_WRITE | PAGE_WRITE_ORG;
> +    page_set_flags(addr, addr + 4096, flag);
> +#endif
> +#endif

I don't really see the reason why this should be done that way. Are you
trying to run MIPS32 binaries compiled for 8kB page size?

this change is needed when we run MIPS64 ELF on 32 bit x86 host. MIPS64 ELF contains 36 bit address.

 load_elf_image() at /home/khansa/testpatch/qemu/linux-user/elfload.c: QEMU  contains these lines

           /* Round addresses to page boundaries.  */

            loaddr &= qemu_host_page_mask;

            hiaddr = HOST_PAGE_ALIGN(hiaddr);

when QEMU run on 32 bit x86 the above two variables are rounded to 32 bit value while these should be 36 bits as these come from MIPS64 ELF.and then for these rounded address l1_map is initialized in page_find_alloc().

in case of SCD(store condition double ) instruction of MIPS64r2 when we have to check load linked address its again 36 bit so it will make an index(addr >> TARGET_PAGE_BITS) for which l1_map is no valid entry, returning 0 value and we got segmentation fault. this is the reason we did following changes in main.c do_store_exclusive()

 +#if HOST_LONG_BITS == 32

> +    int flag = PAGE_VALID | PAGE_READ | PAGE_WRITE | PAGE_WRITE_ORG;
> +    page_set_flags(addr, addr + 4096, flag);
> +#endif

Please give comments on this

 

>      page_addr = addr & TARGET_PAGE_MASK;
>      start_exclusive();
>      mmap_lock();
> @@ -1978,7 +1986,8 @@ static int do_store_exclusive(CPUMIPSState *env)
>  void cpu_loop(CPUMIPSState *env)
>  {
>      target_siginfo_t info;
> -    int trapnr, ret;
> +    int trapnr;
> +    abi_long ret;
>      unsigned int syscall_num;
>
>      for(;;) {
> @@ -1987,7 +1996,11 @@ void cpu_loop(CPUMIPSState *env)
>          cpu_exec_end(env);
>          switch(trapnr) {
>          case EXCP_SYSCALL:
> +#if defined(TARGET_MIPS64)
> +            syscall_num = env->active_tc.gpr[2] - 5000;
> +#else
>              syscall_num = env->active_tc.gpr[2] - 4000;
> +#endif
>              env->active_tc.PC += 4;
>              if (syscall_num >= sizeof(mips_syscall_args)) {

I don't think you can do it like that. mips_syscall_args corresponds to
a table of 32-bit syscalls, which have totally different numbers than
64-bit syscalls. It's not a simple shift by 1000. See for example
pread64 is 4200 is 32-bit, 5016 in 64-bit.

>                  ret = -ENOSYS;
> @@ -2008,12 +2021,22 @@ void cpu_loop(CPUMIPSState *env)
>                  default:
>                      break;
>                  }
> +#if defined(TARGET_MIPS64)
> +                ret = do_syscall(env, env->active_tc.gpr[2],
> +                                 env->active_tc.gpr[4],
> +                                 env->active_tc.gpr[5],
> +                                 env->active_tc.gpr[6],
> +                                 env->active_tc.gpr[7],
> +                                 env->active_tc.gpr[8],
> +                                 env->active_tc.gpr[9]);
> +#else
>                  ret = do_syscall(env, env->active_tc.gpr[2],
>                                   env->active_tc.gpr[4],
>                                   env->active_tc.gpr[5],
>                                   env->active_tc.gpr[6],
>                                   env->active_tc.gpr[7],
>                                   arg5, arg6/*, arg7, arg8*/);
> +#endif
>              }
>              if (ret == -TARGET_QEMU_ESIGRETURN) {
>                  /* Returning from a successful sigreturn syscall.
> @@ -2935,7 +2958,9 @@ int main(int argc, char **argv, char **envp)
>  #endif
>  #elif defined(TARGET_MIPS)
>  #if defined(TARGET_ABI_MIPSN32) || defined(TARGET_ABI_MIPSN64)
> -        cpu_model = "20Kc";
> +        /* we use this model so that we can decode MIPS64r2
> +           reserved instruction */
> +        cpu_model = "MIPS64R2-generic";
>  #else
>          cpu_model = "24Kf";
>  #endif
> diff --git a/linux-user/mips64/syscall.h b/linux-user/mips64/syscall.h
> index 668a2b9..ec65653 100644
> --- a/linux-user/mips64/syscall.h
> +++ b/linux-user/mips64/syscall.h
> @@ -218,4 +218,7 @@ struct target_pt_regs {
>
>
>
> +/* Nasty hack: define a fake errno value for use by sigreturn.  */
> +#define TARGET_QEMU_ESIGRETURN 255
> +
>  #define UNAME_MACHINE "mips64"
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index ce033e9..a3d49dd 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -2413,7 +2413,8 @@ void sparc64_get_context(CPUSPARCState *env)
>  #endif
>  #elif defined(TARGET_ABI_MIPSN64)
>
> -# warning signal handling not implemented
> +/* This line is commented out to avoid compile time error */
> +/* # warning signal handling not implemented */

Wouldn't it be possible to implement signal handling instead?

>  static void setup_frame(int sig, struct target_sigaction *ka,
>   target_sigset_t *set, CPUState *env)
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 0f93e2a..63c2563 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -12694,6 +12694,7 @@ void cpu_reset (CPUMIPSState *env)
>          env->hflags |= MIPS_HFLAG_FPU;
>      }
>  #ifdef TARGET_MIPS64
> +    env->hflags |=  MIPS_HFLAG_UX;
>      if (env->active_fpu.fcr0 & (1 << FCR0_F64)) {
>          env->hflags |= MIPS_HFLAG_F64;
>      }
> --
> 1.7.3.4

--
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net