[PATCH] PRU Simulator port

[Mike Frysinger]

On 05 Dec 2016 23:11, Dimitar Dimitrov wrote:

a very good start!

> --- a/sim/configure.tgt
> +++ b/sim/configure.tgt
> @@ -76,6 +76,9 @@ case "${target}" in
>     msp430*-*-*)
>         SIM_ARCH(msp430)
>         ;;
> +   pru*-*-*)
> +       SIM_ARCH(pru)
> +       ;;
>     rl78-*-*)
>         SIM_ARCH(rl78)
>         ;;

you'll need to run `autoconf` in this dir to regen configure.
then include that in your commit.

> --- /dev/null
> +++ b/sim/pru/Makefile.in
> @@ -0,0 +1,25 @@
> +#    Makefile template for Configure for the MCore sim library.

looks like you'll need to update some of the comments -- this isn't mcore :)

> +#    Written by Cygnus Solutions.

Cygnus didn't write this :)

> +SIM_OBJS = \
> +	interp.o \
> +	$(SIM_NEW_COMMON_OBJS) \
> +	sim-resume.o

interp.o should come after $(SIM_NEW_COMMON_OBJS)

> --- /dev/null
> +++ b/sim/pru/configure.ac
>
> +SIM_AC_OPTION_ENDIAN

can PRU be little or big endian ?  or is it always one or the other ?

if it's always one or the other, then you should pass in LITTLE or BIG here.

> --- /dev/null
> +++ b/sim/pru/interp.c
>
> +   This file is part of GDB, the GNU debugger.

should say:
    This file is part of simulators.

> +#include <signal.h>

doesn't look like you use this, so drop the include

> +#include <assert.h>

don't use this header or assert().  use the existing SIM_ASSERT()
helpers instead.  those provide better error messages and integrate
with the sim framework.

> +#include <sys/param.h>

pretty sure you don't use stuff from it, so drop the include

> +#include "sim-utils.h"

your sim-main.h should include sim-base.h which includes sim-utils.h,
so no need to include it here

> +#include "sim-main.h"

you included this already like 5 lines up

> +#include "sim-base.h"

your sim-main.h should be including sim-base.h, so no need to include
it here

> +#include "sim-options.h"

you don't use these, so drop the include.  unless you do below ...

> +#define max(a,b) ((a) > (b) ? (a) : (b))

you don't need this -- sim-basics.h already provides it which is
included (eventually) by your sim-main.h

> +/* DMEM zero address is perfectly valid.  But if CRT leaves the first word
> +   alone, we can use it as a trap to catch NULL pointer access.  */
> +static const int abort_on_dmem_zero_access = 0;

seems like this should be a debug option so people can change it on
the fly ?  you could leverage the sim-options.h API to change the
value based on command line flags.

> +static uint32_t
> +pru_extract_unsigned_integer (uint8_t *addr, int len)
> +{
> ...
> +  if (len > (int) sizeof (unsigned long))

change the prototype so that len is a size_t instead of len, then you
don't need this cast.  size_t is a better type anyways.

> +    printf ("That operation is not available on integers of more than "

sim's should never use printf.  you can use sim_io_eprintf instead.

then again, the only caller of this func already does a len check which
means this scenario should never happen.  use sim_io_error instead and
that'll trigger an exit/abort.  after all, if you let this code continue
to run, you'll clobber random memory.

> +	    "%ld bytes.", sizeof (unsigned long));

sizeof returns a size_t which means this should be %zu, not %ld

> +static void
> +pru_store_unsigned_integer (uint8_t *addr, int len, uint32_t val)

same feedback on this func as the extract one above

> +static inline uint32_t
> +extract_regval (uint32_t val, uint32_t regsel)
> +{
> ...
> +    default:	      assert (0); return 0;

if you want this to abort, call sim_io_error instead with a good
error message

> +static inline void
> +write_regval (uint32_t val, uint32_t *reg, uint32_t regsel)
> +{
> ...
> +    default:	      assert (0); mask = sh = 0;

same here

> +static inline void
> +pru_reg2dmem (SIM_CPU *cpu, uint32_t addr, unsigned int nbytes,
> +	      int regn, int regb)
> +{
> +  if (abort_on_dmem_zero_access && addr < 4)
> +    {
> +      sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, write_map,
> +		       nbytes, addr, write_transfer,
> +		       SIM_SIGSEGV);
> +    }
> +  else if ((addr >= PC_ADDR_SPACE_MARKER)
> +	   || (addr + nbytes > PC_ADDR_SPACE_MARKER))
> +    {
> +      sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, write_map,
> +		       nbytes, addr, write_transfer,
> +		       SIM_SIGSEGV);
> +    }
> +  else if ((regn * 4 + regb + nbytes) > (32 * 4))
> +    {
> +      /* Register and load size are not valid.  */
> +      sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, write_map,
> +		       nbytes, addr, write_transfer,
> +		       SIM_SIGILL);
> +    }

do you really need to do all this ?  seems like the existing
sim_core_write_1 function already deals properly with writes
to out-of-bind addresses.

> +static inline void
> +pru_dmem2reg (SIM_CPU *cpu, uint32_t addr, unsigned int nbytes,
> +	      int regn, int regb)
> +{
> +  if (abort_on_dmem_zero_access && addr < 4)
> +    {
> +      sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, read_map,
> +		       nbytes, addr, read_transfer,
> +		       SIM_SIGSEGV);
> +    }
> +  else if ((addr >= PC_ADDR_SPACE_MARKER)
> +	   || (addr + nbytes > PC_ADDR_SPACE_MARKER))
> +    {
> +      /* This check is necessary because our IMEM "address space"
> +	 is not really accessible, yet we have mapped it as a generic
> +	 memory space.  */
> +      sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, read_map,
> +		       nbytes, addr, read_transfer,
> +		       SIM_SIGSEGV);
> +    }
> +  else if ((regn * 4 + regb + nbytes) > (32 * 4))
> +    {
> +      /* Register and load size are not valid.  */
> +      sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, read_map,
> +		       nbytes, addr, read_transfer,
> +		       SIM_SIGILL);
> +    }

same here

> +static void
> +set_initial_gprs (SIM_CPU *cpu)
> +{
> +  long space;

unused var -> delete

> +static inline unsigned int
> +regsel_width (uint32_t regsel)
> +{
> ...
> +    default: assert (0); return 0;

sim_io_error instead please

> +static void
> +pru_sim_xin_mac (SIM_DESC sd, SIM_CPU *cpu, unsigned int rd_regn,
> +		 unsigned int rdb, unsigned int length)
> +{
> +  if (rd_regn < 25 || (rd_regn * 4 + rdb + length) > (27 + 1) * 4)
> +    {
> +      fprintf (stderr, "XIN MAC: invalid transfer regn=%u.%u, length=%u\n",
> +	       rd_regn, rdb, length);
> +      RAISE_SIGILL ();
> +      return;

use sim_io_error instead i think

comes up multiple times in this file

> +static void
> +pru_sim_syscall (SIM_DESC sd, SIM_CPU *cpu)

seems like you should use sim_syscall instead of implementing your own
ad-hoc syscall ABI

> +void
> +sim_engine_run (SIM_DESC sd,
> +		int next_cpu_nr, /* ignore  */
> +		int nr_cpus, /* ignore  */
> +		int siggnal) /* ignore  */
> +{

sim_engine_run should be pretty small.  all the business work of
your sim should be in a new function instead.  so in the end, you'd
want something like:

  while (1)
    { 
      step_once (cpu);  // Name this whatever you want.
      if (sim_events_tick (sd))
        sim_events_process (sd);
    }

> +	  /* don't treat r30 and r31 as regular registers, they are I/O! */

GNU style says "Don't", and two spaces after "!"

> +void
> +sim_info (SIM_DESC sd, int verbose)

don't define this func -- let the common one do the work for you

> +SIM_DESC
> +sim_open (SIM_OPEN_KIND kind, host_callback *cb,
> +	  struct bfd *abfd, char * const *argv)
> +{
> ...
> +  cpu = STATE_CPU (sd, 0);

doesn't seem like you use this, so delete it

> +  sim_do_commandf (sd, "memory-region 0x%lx,0x%lx",
> +		   (unsigned long)0,
> +		   (unsigned long)DMEM_DEFAULT_SIZE);
> +  sim_do_commandf (sd, "memory-region 0x%lx,0x%lx",
> +		   (unsigned long)0x20000000,
> +		   (unsigned long)IMEM_DEFAULT_SIZE);

pretty sure you can drop the (unsigned long) cast and change %lx to %x

> +void
> +sim_close (SIM_DESC sd, int quitting)

don't define this func -- let the common one do the work for you

> +SIM_RC
> +sim_create_inferior (SIM_DESC sd, struct bfd *prog_bfd,
> +		     char * const *argv, char * const *env)
> +{

i think you need to deal with STATE_PROG_ARGV here.  grep for
'Standalone mode' in other sims as examples.

> --- /dev/null
> +++ b/sim/pru/pru.h
> +#ifndef PRU_H
> +#define PRU_H
> +
> +/* Copyright 2014-2016 Free Software Foundation, Inc.
> +   Contributed by Dimitar Dimitrov <dimitar@dinux.eu>

the ifdef should be after the file comment, not before it

> --- /dev/null
> +++ b/sim/pru/sim-main.h
> +#ifndef PRU_SIM_MAIN
> +#define PRU_SIM_MAIN
> +
> +/* Copyright 2014-2016 Free Software Foundation, Inc.

the ifdef should be after the file comment, not before it
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20161206/b82f1166/attachment.sig>