Re: [PATCH v2 1/2] PRU Simulator port

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 4725 bytes --]

On 27 Dec 2016 23:25, Dimitar Dimitrov wrote:
> The corresponding libgloss changes have not yet been mainlined.
> The PRU patches are available here:
> https://github.com/dinuxbg/gnupru/tree/for-next/patches/newlib-cygwin

have you sent them to the mailing list ?  i don't see them ...
	https://sourceware.org/ml/newlib/

> 2016-12-26  Dimitar Dimitrov <dimitar@dinux.eu>
> 
> 	* sim/MAINTAINERS.tgt: Add myself as PRU maintainer.

ChangeLog is relative to sim/, so should be "MAINTAINERS".
also, this file doesn't have a .tgt suffix.

> --- /dev/null
> +++ b/sim/pru/interp.c
>
> +#ifndef NUM_ELEM
> +#define NUM_ELEM(A) (sizeof (A) / sizeof (A)[0])
> +#endif

use the standard ARRAY_SIZE from libiberty.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 int abort_on_dmem_zero_access;

use bfd_boolean instead, and TRUE/FALSE values

> +extract_regval (uint32_t val, uint32_t regsel)
> +{
> ...
> +    default:	      sim_io_error(NULL, "invalid regsel");

space before the (

this comes up a few times in this file -- please fix them all

> +pru_reg2dmem (SIM_CPU *cpu, uint32_t addr, unsigned int nbytes,
> +	      int regn, int regb)
> +{
> +  /* GDB assumes unconditional access to all memories, so enable additional
> +     checks only  in standalone mode.  */

only one space before "in"

> +  bool standalone = (STATE_OPEN_KIND (CPU_STATE (cpu)) == SIM_OPEN_STANDALONE);
> +
> +  if (standalone && abort_on_dmem_zero_access && addr < 4)

i don't think you need the standalone check here.  you already have
the abort_on_dmem_zero_access check here.  it'll work in gdb too.

> +sim_step_once (SIM_DESC sd)
> ...
> +	default:
> +	  RAISE_SIGILL ();
> +	  sim_io_eprintf (sd, "Unknown opcode.\n");

RAISE_SIGILL never returns, so this sim_io_eprintf is never shown,
so you might as well delete it

> +SIM_DESC
> +sim_open (SIM_OPEN_KIND kind, host_callback *cb,
> +	  struct bfd *abfd, char * const *argv)
> ...
> +  sim_do_commandf (sd, "memory-region 0x%x,0x%x",
> +		   0,
> +		   DMEM_DEFAULT_SIZE);
> +  sim_do_commandf (sd, "memory-region 0x%x,0x%x",
> +		   0x20000000,
> +		   IMEM_DEFAULT_SIZE);

in other sims, we check the memory map before we add this

  /* Allocate external memory if none specified by user.
     Use address 4 here in case the user wanted address 0 unmapped.  */
  if (sim_core_read_buffer (sd, NULL, read_map, &c, 4, 1) == 0)
    {
      ...do the sim_do_commandf call...
    }

> --- /dev/null
> +++ b/sim/pru/pru.h
>
> +/* (void)0 is a guard against using RD as a left-hand side value.  */
> +#define RD      (void)0; rd_is_modified = 1; _RDVAL

the standard way to handle (void)0 is with a do{...}while(0) loop

looks like you want to make a helper call macro anyways.  something like
#define FOO(x) do { ...; _RDVAL = x; } while (0)

also, RD depends on _RDVAL which only exists inside a single func.
this should be moved to interp.c i think for the one user.

> +#define PC		(cpu->pru_cpu.pc)

use CPU ?

> +#define PC_byteaddr	((cpu->pru_cpu.pc << 2) | PC_ADDR_SPACE_MARKER)

use PC ?

> +#define DO_SYSCALL()	pru_sim_syscall(sd, cpu)

you use this once.  doesn't seem like a good use of a define.
i.e. delete this and inline the code in the one call site.

> +#define RAISE_SIGILL()	sim_engine_halt (sd, NULL, NULL, PC_byteaddr, \
> +					 sim_signalled, SIM_SIGILL);
> +#define RAISE_SIGINT()	sim_engine_halt (sd, NULL, NULL, PC_byteaddr, \
> +					 sim_signalled, SIM_SIGINT);

shouldn't you pass in cpu here ?

drop the semi-colons too

> +#define NUM_REGS 	33

you've got a space then a tab.  you should fix that.

> +#define PCREG_NUM	32
> +#define INST_SIZE 	4

you aren't using these.  delete them ?

> +/* Number of cycles spent for memory access.  No distinction is currently
> +   made between SRAM, DRAM and generic L3 slaves.  */
> +#define MEM_ACCESS_CYCLES 2

you only use this once.  does it need to be a define ?

> --- /dev/null
> +++ b/sim/pru/pru.isa
>
> +INSTRUCTION (xchg,
> +	     fprintf (stderr, "XCHG instruction not supported by sim\n");
> +	     RAISE_SIGILL ())
> +
> +INSTRUCTION (sxin,
> +	     fprintf (stderr, "SXIN instruction not supported by sim\n");
> +	     RAISE_SIGILL ())
> +
> +INSTRUCTION (sxout,
> +	     fprintf (stderr, "SXOUT instruction not supported by sim\n");
> +	     RAISE_SIGILL ())
> +
> +INSTRUCTION (sxchg,
> +	     fprintf (stderr, "SXCHG instruction not supported by sim\n");
> +	     RAISE_SIGILL ())

you should use sim_io_eprintf instead of fprintf
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]