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

Mike Frysinger
2017-02-13T05:36:16Z

[-- 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 --]