Re: [PATCH v5 3/6] sim: or1k: add or1k target to sim

Stafford Horne
2017-10-09T13:03:19Z

On Sat, Oct 07, 2017 at 05:14:42PM -0400, Simon Marchi wrote:
> On 2017-10-05 09:49 AM, Stafford Horne wrote:
> > This adds the OpenRISC 32-bit sim target. The OpenRISC sim is a CGEN
> > based sim so the bulk of the code is generated from the .cpu files by
> > CGEN. The engine decode and execute logic in mloop uses scache with
> > pseudo-basic-block extraction and supports both full and fast (switch)
> > modes.
> >
> > The sim does not implement an mmu at the moment. The sim does implement
> > fpu instructions via the common sim-fpu implementation.
> >
> > sim/ChangeLog:
> >
> > 2017-09-13 Stafford Horne <shorne@gmail.com>
> > Peter Gavin <pgavin@gmail.com>
> >
> > * configure.tgt: Add or1k sim.
> > * or1k/Makefile.in: New file.
> > * or1k/configure.ac: New file.> * or1k/mloop.in: New file.
> > * or1k/or1k-sim.h: New file.
> > * or1k/or1k.c: New file.
> > * or1k/sim-if.c: New file.
> > * or1k/sim-main.h: New file.
> > * or1k/traps.c: New file.
> > ---
> > sim/configure.tgt | 4 +
> > sim/or1k/Makefile.in | 147 +++++++++++++++++++++
> > sim/or1k/configure.ac | 17 +++
> > sim/or1k/mloop.in | 242 ++++++++++++++++++++++++++++++++++
> > sim/or1k/or1k-sim.h | 95 ++++++++++++++
> > sim/or1k/or1k.c | 355 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > sim/or1k/sim-if.c | 281 +++++++++++++++++++++++++++++++++++++++
> > sim/or1k/sim-main.h | 81 ++++++++++++
> > sim/or1k/traps.c | 299 ++++++++++++++++++++++++++++++++++++++++++
> > 9 files changed, 1521 insertions(+)
> > create mode 100644 sim/or1k/Makefile.in
> > create mode 100644 sim/or1k/configure.ac
> > create mode 100644 sim/or1k/mloop.in
> > create mode 100644 sim/or1k/or1k-sim.h
> > create mode 100644 sim/or1k/or1k.c
> > create mode 100644 sim/or1k/sim-if.c
> > create mode 100644 sim/or1k/sim-main.h
> > create mode 100644 sim/or1k/traps.c
> >
> > diff --git a/sim/configure.tgt b/sim/configure.tgt
> > index c958fb3174..82b0e89240 100644
> > --- a/sim/configure.tgt
> > +++ b/sim/configure.tgt
> > @@ -76,6 +76,10 @@ case "${target}" in
> > msp430*-*-*)
> > SIM_ARCH(msp430)
> > ;;
> > + or1k-*-* | or1knd-*-*)
> > + SIM_ARCH(or1k)
> > + sim_testsuite=yes
>
> sim_testsuite seems like it has been removed some time ago.

Right, removed.

> > + ;;
> > rl78-*-*)
> > SIM_ARCH(rl78)
> > ;;
> > diff --git a/sim/or1k/Makefile.in b/sim/or1k/Makefile.in
> > new file mode 100644
> > index 0000000000..fdc0f01ae1
> > --- /dev/null
> > +++ b/sim/or1k/Makefile.in
> > @@ -0,0 +1,147 @@
> > +# Makefile template for configure for the or1k simulator
> > +# Copyright (C) 1996-2017 Free Software Foundation, Inc.
>
> You used 1996 as the starting date for the copyright for all the files,
> is it right? If your code was originally copied from existing code with
> that date, or if the code really dates from 1996, that's fine. Otherwise,
> it should probably be 2017 or the year the work started.

It was started in the last few years, not 1996. But I will put 2017 as the
year it finally gets commited (hopefully).

> > diff --git a/sim/or1k/or1k-sim.h b/sim/or1k/or1k-sim.h
> > new file mode 100644
> > index 0000000000..4cb6dd8b4c
> > --- /dev/null
> > +++ b/sim/or1k/or1k-sim.h
> > @@ -0,0 +1,95 @@
> > +/* OpenRISC simulator support code header
> > + Copyright (C) 1996-2017 Free Software Foundation, Inc.
> > +
> > + This file is part of GDB, the GNU debugger.
> > +
> > + This program is free software; you can redistribute it and/or modify
> > + it under the terms of the GNU General Public License as published by
> > + the Free Software Foundation; either version 3 of the License, or
> > + (at your option) any later version.
> > +
> > + This program is distributed in the hope that it will be useful,
> > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + GNU General Public License for more details.
> > +
> > + You should have received a copy of the GNU General Public License
> > + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> > +
> > +#ifndef OR1K_SIM_H
> > +#define OR1K_SIM_H
> > +
> > +#include "symcat.h"
> > +
> > +/* GDB register numbers. */
> > +#define PPC_REGNUM 32
> > +#define PC_REGNUM 33
> > +#define SR_REGNUM 34
> > +
> > +/* Misc. profile data. */
> > +typedef struct
> > +{
> > +} OR1K_MISC_PROFILE;
> > +
> > +/* Nop codes used in nop simulation. */
> > +#define NOP_NOP 0x0
> > +#define NOP_EXIT 0x1
> > +#define NOP_REPORT 0x2
> > +#define NOP_PUTC 0x4
> > +#define NOP_CNT_RESET 0x5
> > +#define NOP_GET_TICKS 0x6
> > +#define NOP_GET_PS 0x7
> > +#define NOP_TRACE_ON 0x8
> > +#define NOP_TRACE_OFF 0x9
> > +#define NOP_RANDOM 0xa
> > +#define NOP_OR1KSIM 0xb
> > +#define NOP_EXIT_SILENT 0xc
> > +
> > +#define NUM_SPR 0x20000
> > +#define SPR_GROUP_SHIFT 11
> > +#define SPR_GROUP_FIRST(group) (((UWI) SPR_GROUP_##group) << SPR_GROUP_SHIFT)
> > +#define SPR_ADDR(group,index) (SPR_GROUP_FIRST(group) | ((UWI) SPR_INDEX_##group##_##index))
>
> Unless sim/ has some special rule allowing things to overflow 80 columns, these
> lines will have to be wrapped.

OK

> > +
> > +/* Define word getters and setter helpers based on those from sim/common/cgen-mem.h. */
> > +#define GETTWI GETTSI
> > +#define SETTWI SETTSI
> > +
> > +void or1k_cpu_init (SIM_DESC sd, sim_cpu * current_cpu, const USI or1k_vr,
>
> Throughout, there are spurious after * in types.

OK, there are a few cases `char * const *argv, char * const *envp)` which
look strange, but I see other examples with similar format. I would
usually do 'const char **argv` but is the before-mentioned ok?

> > +#define CHECK_SPR_FIELD(GROUP, INDEX, FIELD, test) \
> > + do { \
> > + USI field = GET_H_##SYS##_##INDEX##_##FIELD (); \
> > + if (!(test)) { \
> > + sim_io_eprintf(sd, "WARNING: unsupported %s field in %s register: 0x%x\n", \
>
> 80 columns.

OK, also noted the other issues like 'do {' should have '{' on the next
line.

> > + #FIELD, #INDEX, field); \
> > + } \
> > + } while (0)
> > +
> > + /* Set flags indicating if we are in a delay slot or not. */
> > + current_cpu->next_delay_slot = 0;
> > + current_cpu->delay_slot = 0;> > +
> > + /* Verify any user passed fields and warn on configurations we don't
> > + support. */
> > + CHECK_SPR_FIELD (SYS, UPR, UP, field == 1);
> > + CHECK_SPR_FIELD (SYS, UPR, DCP, field == 0);
> > + CHECK_SPR_FIELD (SYS, UPR, ICP, field == 0);
> > + CHECK_SPR_FIELD (SYS, UPR, DMP, field == 0);
> > + CHECK_SPR_FIELD (SYS, UPR, MP, field == 0);
> > + CHECK_SPR_FIELD (SYS, UPR, IMP, field == 0);
> > + CHECK_SPR_FIELD (SYS, UPR, DUP, field == 0);
> > + CHECK_SPR_FIELD (SYS, UPR, PCUP, field == 0);
> > + CHECK_SPR_FIELD (SYS, UPR, PICP, field == 0);
> > + CHECK_SPR_FIELD (SYS, UPR, PMP, field == 0);
> > + CHECK_SPR_FIELD (SYS, UPR, TTP, field == 0);
> > + CHECK_SPR_FIELD (SYS, UPR, CUP, field == 0);
> > +
> > + CHECK_SPR_FIELD (SYS, CPUCFGR, NSGR, field == 0);
> > + CHECK_SPR_FIELD (SYS, CPUCFGR, CGF, field == 0);
> > + CHECK_SPR_FIELD (SYS, CPUCFGR, OB32S, field == 1);
> > + CHECK_SPR_FIELD (SYS, CPUCFGR, OF32S, field == 1);
> > + CHECK_SPR_FIELD (SYS, CPUCFGR, OB64S, field == 0);
> > + CHECK_SPR_FIELD (SYS, CPUCFGR, OF64S, field == 0);
> > + CHECK_SPR_FIELD (SYS, CPUCFGR, OV64S, field == 0);
> > +
> > +#undef CHECK_SPR_FIELD
> > +
> > + /* Configure the fpu operations and mark fpu available. */
> > + cgen_init_accurate_fpu (current_cpu, CGEN_CPU_FPU (current_cpu),
> > + or1k32bf_fpu_error);
> > + SET_H_SYS_CPUCFGR_OF32S (1);
> > +
> > + /* Set the UPR[UP] flag, even if the user tried to unset it, as we always
> > + support the Unit Present Register. */
> > + SET_H_SYS_UPR_UP (1);
> > +
> > + /* Set the supervisor register to indicate we are in suprevisor mode and
>
> "suprevisor"

OK.

> > +/* Build an address value used for load and store instructions. For
> > + example, the instruction 'l.lws rD, I(rA)' will require to load data
> > + from the 4 byte address represented by rA + I. Here the argument base
> > + is rA, offset is I and the size is the address size in bytes. */
> > +
> > +USI
> > +or1k32bf_make_load_store_addr (sim_cpu * current_cpu, USI base, SI offset,
> > + int size)
> > +{
> > + SIM_DESC sd = CPU_STATE (current_cpu);
> > +
> > + USI addr = base + offset;
> > +
> > + if (GET_H_SYS_SR_LEE ())
> > + {
> > + switch (size)
> > + {
> > +
> > + case 4:
> > + break;
> > +
> > + case 2:
> > + addr ^= 0x2;
> > + break;
> > +
> > + case 1:
> > + addr ^= 0x3;
> > + break;
> > +
> > + default:
> > + SIM_ASSERT (0);
> > + return 0;
> > + }
> > + }
>
> That code looks strange to me. Enabling byte reordering (SR[LEE]) makes it so that each
> group of 4 bytes appears swapped in memory? Is this something a program can turn on/off
> at any time? There isn't much detail about that in the spec it seems.

I have added some more comments to this in the next patch series. I am not
to sure as to the 'why' for this functionality. But I assume its probably
to interact with hardware like graphics cards which only support
little-endian addressing. The SR[LEE] would only be available to change in
supervisor mode, so not just any program could change it.

The above code only handles the address translation (note for half-word
access openrisc requires the address to be half-word aligned).

> > +/* Implement the OpenRISC exception function. This is mostly used by the
> > + CGEN generated files. For example, this is used when handling a
> > + overflow exception during a multiplication instruction. */
> > +
> > +void
> > +or1k32bf_exception (sim_cpu * current_cpu, USI pc, USI exnum)
> > +{
> > + SIM_DESC sd = CPU_STATE (current_cpu);
> > +
> > + if (exnum == EXCEPT_TRAP)
> > + {
> > + /* Trap, used for breakpoints, sends control back to gdb breakpoint
> > + handling. */
> > + sim_engine_halt (sd, current_cpu, NULL, pc, sim_stopped, SIM_SIGTRAP);
> > + }
> > + else
> > + {
> > +
> > + /* Calculate the exception program counter. */
> > + switch (exnum)
> > + {
> > + case EXCEPT_RESET:
> > + break;
> > +
> > + case EXCEPT_FPE:
> > + case EXCEPT_SYSCALL:
> > + SET_H_SYS_EPCR0 (pc + 4 - (current_cpu->delay_slot ? 4 : 0));
> > + break;
> > +
> > + case EXCEPT_BUSERR:
> > + case EXCEPT_ALIGN:
> > + case EXCEPT_ILLEGAL:
> > + case EXCEPT_RANGE:
> > + SET_H_SYS_EPCR0 (pc - (current_cpu->delay_slot ? 4 : 0));
> > + break;
> > +
> > + default:
> > + sim_io_error (sd, "unexpected exception 0x%x raised at PC 0x%08x",
> > + exnum, pc);
> > + break;
> > + }
> > +
> > + /* Store the curent SR into ESR0. */
>
> "curent"
>

OK.

FYI, also the testsuite patch aas a few long lines like:

/* The sign extension produces unexpected results here. */
SHOULD_BE_LESS_THAN_UNSIGNED_I 0xFFFFFFFF - 1, 0xFFFF /* 0xFFFF gets sign-extended to 0xFFFFFFFF. */
SHOULD_BE_LESS_THAN_UNSIGNED_I 0xFFFF7FFF, 0x8000 /* 0x8000 gets sign-extended to 0xFFFF8000. */

I guess it should be like this? I don't mind the inline comments, but I
guess convention is convention?

/* 0xFFFF gets sign-extended to 0xFFFFFFFF. */
SHOULD_BE_LESS_THAN_UNSIGNED_I 0xFFFFFFFF - 1, 0xFFFF
/* 0x8000 gets sign-extended to 0xFFFF8000. */
SHOULD_BE_LESS_THAN_UNSIGNED_I 0xFFFF7FFF, 0x8000

Thank you,

Stafford