Re: [PATCH] PRU Simulator port

[Dimitar Dimitrov <dimitar at dinux dot eu>]

On петък, 16 декември 2016 г. 15:33:24 EET Mike Frysinger wrote:
> On 09 Dec 2016 22:39, Dimitar Dimitrov wrote:
> > I'll change it. I assume my copyright papers for GDB will cover this
> > simulator patch?
> i think they want sep papers.  who knows!
> 
> try this small form (it has directions in it):
> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob_plain;f=doc/Copyrigh
> t/request-assign.future;hb=HEAD
Thanks, I've started the process.

> > > > +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.
> i don't believe this is possible today with gdb.  when gdb makes a request
> to the sim to do things like set breakpoints and decode instructinos, it
> follows the same paths as the sim which means it'll break gdb :(.
> 
> i wanted to do the same thing for Blackfin where there's a chunk of SRAM
> that can only be executed, but trying to do data reads/stores would fail.
May I conditionally enforce this restriction only if "(STATE_OPEN_KIND (sd) == SIM_OPEN_STANDALONE)" ? The standalone simulator is invaluable for regression checking Binutils and GCC, and I would like to catch those kinds of errors.

> 
> >    - Check that the load/store burst length is valid (i.e. do not access
> >    beyond the last CPU register).
> shouldn't this be handled by limiting the memory region addr+size ?
PRU LBBO instruction can load up to 120 bytes into a group of consecutive registers. This check guards the destination register group, not the source memory address.

Consider an example, where r10 holds a valid address in the middle of a large array. This would be a valid instruction for loading 32 bytes (8 words) into r20-r27:
    ldi &r20, r10, 0, 32
And this would be invalid because it would try to load 8 words into r29-r36, while PRU registers are only r0-r31:
    ldi &r29, r10, 0, 32


> 
> >    - Optionally catch NULL pointer dereferences.
> 
> hmm, seems like this would be better as a common sim option ?  but also,
> this runs into the same problems as i described above i think.
Again, may I enable it only for standalone simulator?

> 
> > > > +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.
> that was going to be my next question :).  if you have time, then yeah,
> changing all your libgloss syscall #'s to use the default ones would be
> best.
I will do that.

> -mike