Re: [PATCH] PRU Simulator port

[Dimitar Dimitrov <dimitar at dinux dot eu>]

On вторник, 6 декември 2016 г. 18:17:31 EET Mike Frysinger wrote:
> On 05 Dec 2016 23:11, Dimitar Dimitrov wrote:
> 
> a very good start!
Thanks. I'll rework and send v2. 
> 
...
> > --- /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.
I'll hard-code it to LITTLE because that has been my assumption while porting.  If someday TI folks decide to build a big-endian PRU, then I'll revise.

> > --- /dev/null
> > +++ b/sim/pru/interp.c
> > 
> > +   This file is part of GDB, the GNU debugger.
> 
> should say:
>     This file is part of simulators.
I'll change it. I assume my copyright papers for GDB will cover this simulator patch?

> 
...
> 
> > +/* 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.
Thanks for the tip.  I'll add an "--abort-on-dmem-zero" option.
> 
> > +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
I'll remove the check as caller already sanitizes the parameter.

> 
> 
> > +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.
I believe the checks are needed. I let sim_core_write_1 handle the Data RAM bounds checking. On top of that, I want to:
   - Ensure that instruction memory (PC_ADDR_SPACE_MARKER) is not accessed by the CPU data path.
   - Check that the load/store burst length is valid (i.e. do not access beyond the last CPU register).
   - Optionally catch NULL pointer dereferences.

> > +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
I'll switch to ASSERT, SIM_ASSERT, sim_io_error and sim_io_eprintf.
> 
> > +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
I'll fix libgloss to use more standard syscalls, and then I'll switch to sim_syscall.
> 

Regards,
Dimitar