RE: [PATCH] Add micromips support to the MIPS simulator

[Andrew Bennett <Andrew.Bennett@imgtec.com>]

Ping.

I was wondering if anyone has had a chance to look at this yet?

Many thanks,


Andrew

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Andrew Bennett
> Sent: 27 August 2015 16:05
> To: Mike Frysinger
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH] Add micromips support to the MIPS simulator
> 
> Firstly, sorry for the long delay in replying back to your review comments.
> 
> > > --- a/sim/mips/Makefile.in
> > > +++ b/sim/mips/Makefile.in
> > >
> > >  support.o: sim-main.h support.c $(SIM_EXTRA_DEPS)
> > >  idecode.o: sim-main.h idecode.c $(SIM_EXTRA_DEPS)
> > >  itable.o: sim-main.h itable.c $(SIM_EXTRA_DEPS)
> > > -m16run.o: sim-main.h m16_idecode.h m32_idecode.h $(SIM_EXTRA_DEPS)
> > > +m16run.o: sim-main.h m16_idecode.h m32_idecode.h m16run.c
> $(SIM_EXTRA_DEPS)
> > > +micromipsrun.o: sim-main.h micromips16_idecode.h micromips32_idecode.h \
> > > +	micromips_m32_idecode.h micromipsrun.c $(SIM_EXTRA_DEPS)
> >
> > pretty sure you don't need any of this hand maintained dependency info
> > anymore.
> > can you please try deleting it all instead ?
> 
> Done.  I had to keep in the micromipsrun.o, m16run.o and interp.o rules
> as these rely on files generated by igen.
> 
> > > --- a/sim/mips/configure.ac
> > > +++ b/sim/mips/configure.ac
> > >
> > > +      *:*micromips32*:*)
> > > +	# Run igen thrice, once for micromips32, once for micromips16,
> > > +        # and once for m32.
> > > +	ws="micromips_m32 micromips16 micromips32"
> >
> > indentation is broken slightly on the second comment line.  seems to come up
> a
> > few times ... you should fix them all.
> 
> This has been fixed.
> 
> >
> > > --- a/sim/mips/interp.c
> > > +++ b/sim/mips/interp.c
> > >
> > > +/* microMIPS ISA mode */
> > > +int isa_mode;
> >
> > why is this a global instead of being part of the sim state ?
> >
> > the comment also needs tweaking to match GNU style
> 
> This has now been moved to the sim_state structure.
> 
> > > --- /dev/null
> > > +++ b/sim/mips/micromipsrun.c
> > >
> > > +/*  Run function for the micromips simulator
> > > +
> > > +    Copyright (C) 2005-2013 Free Software Foundation, Inc.
> >
> > years needs updating
> 
> Done.
> 
> >
> > > +    This file is part of GDB, the GNU debugger.
> >
> > i think you mean:
> > 	This file is part of the GNU simulators.
> 
> Correct.  I have changed all files my patch changes to have this comment.
> 
> 
> > > +#define SD sd
> > > +#define CPU cpu
> >
> > unused ?
> 
> No, they are required for some of the macros used in the file so they need to
> stay in.
> 
> >
> > > +void
> > > +sim_engine_run (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int signal);
> >
> > you should include sim-engine.h instead of declaring the prototype yourself
> 
> Done.
> 
> > -mike
> 
> 
> I have attached the full updated patch, and I have inlined the changes I made
> to
> the original patch below.  The patch also changes CIA_{GET/SET} to
> CPU_PC_{GET/SET},
> and moves multi-run.o from sim_multi_obj to SIM_MULTI_OBJ to match the
> structure of
> the other SIM_*_OBJ variables.   Finally, I also need to add an extra
> ChangeLog
> entry to account for the change to the sim_state structure:
> 
> 	sim/mips/
> 
> 	* sim-main.h (sim_state): Add isa_mode field.
> 
> 
> Ok to commit?
> 
> 
> Regards,
> 
> 
> 
> Andrew
> 
> 
> 
> 
> diff --git a/sim/mips/Makefile.in b/sim/mips/Makefile.in
> index f4beb45..f02e1bd 100644
> --- a/sim/mips/Makefile.in
> +++ b/sim/mips/Makefile.in
> @@ -55,7 +55,9 @@ SIM_MICROMIPS_OBJ = \
>  	micromipsrun.o \
> 
> 
> -SIM_MULTI_OBJ = itable.o @sim_multi_obj@
> +SIM_MULTI_OBJ = @sim_multi_obj@ \
> +		itable.o \
> +		multi-run.o \
> 
>  MIPS_EXTRA_LIBS = @mips_extra_libs@
> 
> @@ -88,11 +90,11 @@ SIM_EXTRA_LIBS = $(MIPS_EXTRA_LIBS)
>  ## COMMON_POST_CONFIG_FRAG
> 
>  interp.o: $(srcdir)/interp.c config.h sim-main.h itable.h
> -cp1.o: $(srcdir)/cp1.c config.h sim-main.h
> 
> -mdmx.o: $(srcdir)/mdmx.c $(srcdir)/sim-main.h
> +m16run.o: sim-main.h m16_idecode.h m32_idecode.h m16run.c $(SIM_EXTRA_DEPS)
> 
> -dsp.o: $(srcdir)/dsp.c $(srcdir)/sim-main.h
> +micromipsrun.o: sim-main.h micromips16_idecode.h micromips32_idecode.h \
> +		micromips_m32_idecode.h micromipsrun.c $(SIM_EXTRA_DEPS)
> 
>  multi-run.o: multi-include.h tmp-mach-multi
> 
> @@ -199,43 +201,6 @@ tmp-igen: $(IGEN_INSN) $(IGEN_DC) ../igen/igen
> $(IGEN_INCLUDE)
>  	$(SHELL) $(srcdir)/../../move-if-change tmp-irun.c irun.c
>  	touch tmp-igen
> 
> -semantics.o: sim-main.h semantics.c $(SIM_EXTRA_DEPS)
> -engine.o: sim-main.h engine.c $(SIM_EXTRA_DEPS)
> -support.o: sim-main.h support.c $(SIM_EXTRA_DEPS)
> -idecode.o: sim-main.h idecode.c $(SIM_EXTRA_DEPS)
> -itable.o: sim-main.h itable.c $(SIM_EXTRA_DEPS)
> -m16run.o: sim-main.h m16_idecode.h m32_idecode.h m16run.c $(SIM_EXTRA_DEPS)
> -micromipsrun.o: sim-main.h micromips16_idecode.h micromips32_idecode.h \
> -	micromips_m32_idecode.h micromipsrun.c $(SIM_EXTRA_DEPS)
> -
> -m16_semantics.o: sim-main.h m16_semantics.c $(SIM_EXTRA_DEPS)
> -m16_support.o: sim-main.h m16_support.c $(SIM_EXTRA_DEPS)
> -m16_idecode.o: sim-main.h m16_idecode.c $(SIM_EXTRA_DEPS)
> -m16_icache.o: sim-main.h m16_icache.c $(SIM_EXTRA_DEPS)
> -
> -micromips_m32_semantics.o: sim-main.h micromips_m32_semantics.c \
> -	$(SIM_EXTRA_DEPS)
> -micromips_m32_support.o: sim-main.h micromips_m32_support.c $(SIM_EXTRA_DEPS)
> -micromips_m32_idecode.o: sim-main.h micromips_m32_idecode.c $(SIM_EXTRA_DEPS)
> -micromips_m32_icache.o: sim-main.h micromips_m32_icache.c $(SIM_EXTRA_DEPS)
> -
> -m32_semantics.o: sim-main.h m32_semantics.c $(SIM_EXTRA_DEPS)
> -m32_support.o: sim-main.h m32_support.c $(SIM_EXTRA_DEPS)
> -m32_idecode.o: sim-main.h m32_idecode.c $(SIM_EXTRA_DEPS)
> -m32_icache.o: sim-main.h m32_icache.c $(SIM_EXTRA_DEPS)
> -
> -micromips16_semantics.o: sim-main.h micromips16_semantics.c $(SIM_EXTRA_DEPS)
> -micromips16_support.o: sim-main.h micromips16_support.c $(SIM_EXTRA_DEPS)
> -micromips16_idecode.o: sim-main.h micromips16_idecode.c $(SIM_EXTRA_DEPS)
> -micromips16_icache.o: sim-main.h micromips16_icache.c $(SIM_EXTRA_DEPS)
> -
> -micromips32_semantics.o: sim-main.h micromips32_semantics.c $(SIM_EXTRA_DEPS)
> -micromips32_support.o: sim-main.h micromips32_support.c $(SIM_EXTRA_DEPS)
> -micromips32_idecode.o: sim-main.h micromips32_idecode.c $(SIM_EXTRA_DEPS)
> -micromips32_icache.o: sim-main.h micromips32_icache.c $(SIM_EXTRA_DEPS)
> -
> -$(SIM_MULTI_OBJ): sim-main.h $(SIM_EXTRA_DEPS)
> -
>  BUILT_SRC_FROM_M16 = \
>  	m16_icache.h \
>  	m16_icache.c \
> diff --git a/sim/mips/configure.ac b/sim/mips/configure.ac
> index e2a4871..a642326 100644
> --- a/sim/mips/configure.ac
> +++ b/sim/mips/configure.ac
> @@ -228,7 +228,7 @@ if test ${sim_gen} = MULTI; then
>    rm -f multi-include.h multi-run.c
>    sim_multi_flags=
>    sim_multi_src=
> -  sim_multi_obj=multi-run.o
> +  sim_multi_obj=
>    sim_multi_igen_configs=
>    sim_seen_default=no
> 
> @@ -318,7 +318,7 @@ __EOF__
>  	;;
>        *:*micromips32*:*)
>  	# Run igen thrice, once for micromips32, once for micromips16,
> -        # and once for m32.
> +	# and once for m32.
>  	ws="micromips_m32 micromips16 micromips32"
> 
>  	# The top-level function for the micromips simulator is
> @@ -330,7 +330,7 @@ __EOF__
>  	;;
>        *:*micromips64*:*)
>  	# Run igen thrice, once for micromips64, once for micromips16,
> -        # and once for m64.
> +	# and once for m64.
>  	ws="micromips_m64 micromips16 micromips64"
> 
>  	# The top-level function for the micromips simulator is
> @@ -436,8 +436,6 @@ AC_SUBST(sim_multi_flags)
>  AC_SUBST(sim_multi_igen_configs)
>  AC_SUBST(sim_multi_src)
>  AC_SUBST(sim_multi_obj)
> -
> -
>  #
>  # Add simulated hardware devices
>  #
> diff --git a/sim/mips/dsp.igen b/sim/mips/dsp.igen
> index 35c90d0..b8d5a6c 100644
> --- a/sim/mips/dsp.igen
> +++ b/sim/mips/dsp.igen
> @@ -4,7 +4,7 @@
>  // Copyright (C) 2005-2015 Free Software Foundation, Inc.
>  // Contributed by MIPS Technologies, Inc.  Written by Chao-ying Fu.
>  //
> -// This file is part of GDB, the GNU debugger.
> +// This file is part of the MIPS sim
>  //
>  // This program is free software; you can redistribute it and/or modify
>  // it under the terms of the GNU General Public License as published by
> diff --git a/sim/mips/dsp2.igen b/sim/mips/dsp2.igen
> index 06b5a68..a871026 100644
> --- a/sim/mips/dsp2.igen
> +++ b/sim/mips/dsp2.igen
> @@ -5,7 +5,7 @@
>  // Contributed by MIPS Technologies, Inc.
>  // Written by Chao-ying Fu (fu@mips.com).
>  //
> -// This file is part of GDB, the GNU debugger.
> +// This file is part of the MIPS sim
>  //
>  // This program is free software; you can redistribute it and/or modify
>  // it under the terms of the GNU General Public License as published by
> diff --git a/sim/mips/interp.c b/sim/mips/interp.c
> index 686de61..9dc8964 100644
> --- a/sim/mips/interp.c
> +++ b/sim/mips/interp.c
> @@ -146,9 +146,6 @@ static SIM_ADDR lsipmon_monitor_base = 0xBFC00200;
> 
>  static SIM_RC sim_firmware_command (SIM_DESC sd, char* arg);
> 
> -/* microMIPS ISA mode */
> -int isa_mode;
> -
>  #define MEM_SIZE (8 << 20)	/* 8 MBytes */
> 
> 
> diff --git a/sim/mips/micromips.igen b/sim/mips/micromips.igen
> index fdd0368..2c62376 100644
> --- a/sim/mips/micromips.igen
> +++ b/sim/mips/micromips.igen
> @@ -1,9 +1,9 @@
>  // Simulator definition for the micromips ASE.
> -// Copyright (C) 2005-2013 Free Software Foundation, Inc.
> +// Copyright (C) 2005-2015 Free Software Foundation, Inc.
>  // Contributed by Imagination Technologies, Ltd.
>  // Written by Andrew Bennett <andrew.bennett@imgtec.com>
>  //
> -// This file is part of GDB, the GNU debugger.
> +// This file is part of the MIPS sim.
>  //
>  // This program is free software; you can redistribute it and/or modify
>  // it under the terms of the GNU General Public License as published by
> @@ -53,7 +53,7 @@
> 
>  :function:::address_word:process_isa_mode:address_word target
>  {
> -  isa_mode = target & 0x1;
> +  SD->isa_mode = target & 0x1;
>    return (target & (-1 << 1));
>  }
> 
> @@ -1063,7 +1063,7 @@
>    address_word region = (NIA & MASK (63, 26));
>    NIA = do_micromips_jal (SD_, (region | (IMM_SHIFT_2BIT)) | ISA_MODE_MIPS32,
>  			  NIA, MICROMIPS_DELAYSLOT_SIZE_32);
> -  isa_mode = ISA_MODE_MIPS32;
> +  SD->isa_mode = ISA_MODE_MIPS32;
>  }
> 
>  000000,00000,5.RS,0000111100,111100:POOL32A:32::JR
> diff --git a/sim/mips/micromipsdsp.igen b/sim/mips/micromipsdsp.igen
> index c19de09..daa9f83 100644
> --- a/sim/mips/micromipsdsp.igen
> +++ b/sim/mips/micromipsdsp.igen
> @@ -1,9 +1,9 @@
>  // Simulator definition for the micromips DSP ASE.
> -// Copyright (C) 2005-2013 Free Software Foundation, Inc.
> +// Copyright (C) 2005-2015 Free Software Foundation, Inc.
>  // Contributed by Imagination Technologies, Ltd.
>  // Written by Andrew Bennett <andrew.bennett@imgtec.com>
>  //
> -// This file is part of GDB, the GNU debugger.
> +// This file is part of the MIPS sim.
>  //
>  // This program is free software; you can redistribute it and/or modify
>  // it under the terms of the GNU General Public License as published by
> diff --git a/sim/mips/micromipsrun.c b/sim/mips/micromipsrun.c
> index f07ad8e..7dd10d7 100644
> --- a/sim/mips/micromipsrun.c
> +++ b/sim/mips/micromipsrun.c
> @@ -1,10 +1,10 @@
>  /*  Run function for the micromips simulator
> 
> -    Copyright (C) 2005-2013 Free Software Foundation, Inc.
> +    Copyright (C) 2005-2015 Free Software Foundation, Inc.
>      Contributed by Imagination Technologies, Ltd.
>      Written by Andrew Bennett <andrew.bennett@imgtec.com>.
> 
> -    This file is part of GDB, the GNU debugger.
> +    This file is part of the MIPS sim.
> 
>      This program is free software; you can redistribute it and/or modify
>      it under the terms of the GNU General Public License as published by
> @@ -24,14 +24,11 @@
>  #include "micromips32_idecode.h"
>  #include "micromips_m32_idecode.h"
>  #include "bfd.h"
> -
> +#include "sim-engine.h"
> 
>  #define SD sd
>  #define CPU cpu
> 
> -void
> -sim_engine_run (SIM_DESC sd, int next_cpu_nr, int nr_cpus, int signal);
> -
>  address_word
>  micromips_instruction_decode (SIM_DESC sd, sim_cpu * cpu,
>  			      address_word cia,
> @@ -74,8 +71,8 @@ sim_engine_run (SIM_DESC sd, int next_cpu_nr, int nr_cpus,
>  {
>    micromips_m32_instruction_word instruction_0;
>    sim_cpu *cpu = STATE_CPU (sd, next_cpu_nr);
> -  micromips32_instruction_address cia = CIA_GET (cpu);
> -  isa_mode = ISA_MODE_MIPS32;
> +  micromips32_instruction_address cia = CPU_PC_GET (cpu);
> +  sd->isa_mode = ISA_MODE_MIPS32;
> 
>    while (1)
>      {
> @@ -87,17 +84,17 @@ sim_engine_run (SIM_DESC sd, int next_cpu_nr, int nr_cpus,
>  	 from the elf header.
>  	 2. Setting the correct isa mode after a MIPS32 jump or branch
>  	 instruction.  */
> -      if ((isa_mode == ISA_MODE_MIPS32)
> +      if ((sd->isa_mode == ISA_MODE_MIPS32)
>  	  && ((cia & 0x1) == ISA_MODE_MICROMIPS))
>  	{
> -	  isa_mode = ISA_MODE_MICROMIPS;
> +	  sd->isa_mode = ISA_MODE_MICROMIPS;
>  	  cia = cia & ~0x1;
>  	}
> 
>  #if defined (ENGINE_ISSUE_PREFIX_HOOK)
>        ENGINE_ISSUE_PREFIX_HOOK ();
>  #endif
> -      switch (isa_mode)
> +      switch (sd->isa_mode)
>  	{
>  	case ISA_MODE_MICROMIPS:
>  	  nia =
> @@ -122,9 +119,9 @@ sim_engine_run (SIM_DESC sd, int next_cpu_nr, int nr_cpus,
>        /* process any events */
>        if (sim_events_tick (sd))
>  	{
> -	  CIA_SET (CPU, cia);
> +	  CPU_PC_SET (cpu, cia);
>  	  sim_events_process (sd);
> -	  cia = CIA_GET (CPU);
> +	  cia = CPU_PC_GET (cpu);
>  	}
>      }
>  }
> diff --git a/sim/mips/mips3264r2.igen b/sim/mips/mips3264r2.igen
> index e003664..1c299c3 100644
> --- a/sim/mips/mips3264r2.igen
> +++ b/sim/mips/mips3264r2.igen
> @@ -4,7 +4,7 @@
>  // Copyright (C) 2004-2015 Free Software Foundation, Inc.
>  // Contributed by David Ung, of MIPS Technologies.
>  //
> -// This file is part of GDB, the GNU debugger.
> +// This file is part of the MIPS sim.
>  //
>  // This program is free software; you can redistribute it and/or modify
>  // it under the terms of the GNU General Public License as published by
> diff --git a/sim/mips/sim-main.h b/sim/mips/sim-main.h
> index 4bfc06c..42d8db3 100644
> --- a/sim/mips/sim-main.h
> +++ b/sim/mips/sim-main.h
> @@ -2,7 +2,7 @@
>     Copyright (C) 1997-2015 Free Software Foundation, Inc.
>     Contributed by Cygnus Support.
> 
> -This file is part of GDB, the GNU debugger.
> +This file is part of the MIPS sim.
> 
>  This program is free software; you can redistribute it and/or modify
>  it under the terms of the GNU General Public License as published by
> @@ -493,6 +493,9 @@ struct sim_state {
> 
>    sim_cpu *cpu[MAX_NR_PROCESSORS];
> 
> +  /* microMIPS ISA mode.  */
> +  int isa_mode;
> +
>    sim_state_base base;
>  };
> 
> diff --git a/sim/testsuite/sim/mips/hilo-hazard-4.s
> b/sim/testsuite/sim/mips/hilo-hazard-4.s
> index c489a4f..e83fbfa 100644
> --- a/sim/testsuite/sim/mips/hilo-hazard-4.s
> +++ b/sim/testsuite/sim/mips/hilo-hazard-4.s
> @@ -5,11 +5,11 @@
>  # ld:		-N -Ttext=0x80010000
>  # output:	pass\\n
> 
> -# Copyright (C) 2013 Imagination Technologies, Ltd.
> +# Copyright (C) 2013-2015 Imagination Technologies, Ltd.
>  # All rights reserved.
>  # Contributed by Andrew Bennett (andrew.bennett@imgtec.com)
>  #
> -# This file is part of the GNU simulators.
> +# This file is part of the MIPS sim.
>  #
>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by