From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 83339 invoked by alias); 9 Oct 2017 13:03:19 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 82904 invoked by uid 89); 9 Oct 2017 13:03:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=simulator, cards, graphics, icp X-HELO: mail-pf0-f169.google.com Received: from mail-pf0-f169.google.com (HELO mail-pf0-f169.google.com) (209.85.192.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Oct 2017 13:03:11 +0000 Received: by mail-pf0-f169.google.com with SMTP id m28so8775144pfi.11 for ; Mon, 09 Oct 2017 06:03:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=foh1qQpYOWnz0OnDubCbJCWcF08l+BAmwudPcG6PJh8=; b=bXN11+tqV/7qJ9D/b18UyRyaNYdSHLgJrKw61KQnb+SgvGvzRXDEJMjkSaAaa/6teb os7q6NS9pvyCtc9Q+tPcLPrCqmDnGvTDVbE4IUxq3H9bTcUB2/oihfC+Hlex+DlVuwEq K0kZegOJ+TrG08s7j0JSamuts7K72+JXaEqVrUilLGS8+2Bk+ce3h5uKjHxH9vASpdrY S9yuNf68Tp5Cwbjw/2v3YB03QA101Q3AMBqWqkn23Z8WTTp1ZGQt0Tney4TcNoxqcey+ FRjdUvnCg6r3Ux9AdJEU7EX/Kbyx8zhoua70VJ+aV+cvoa0XbyUJtPOeh224nhX6bgMq qa3w== X-Gm-Message-State: AMCzsaWR/QXH0XpU1RuBAjVKnAen3Xf+oNPxhyDN8E8bEiKluWEkIc5M YTfxk5VJDEb+S2F9XI3KmZ8= X-Google-Smtp-Source: AOwi7QATvRkI90Nx6KCBzj8jLlwewwpN2zcgeR059DW16omT1uTe5w3q6BfWcP5ZFlMrE6NRK8qTpw== X-Received: by 10.98.35.194 with SMTP id q63mr9953899pfj.15.1507554189275; Mon, 09 Oct 2017 06:03:09 -0700 (PDT) Received: from localhost (g248.61-45-56.ppp.wakwak.ne.jp. [61.45.56.248]) by smtp.gmail.com with ESMTPSA id v22sm11478879pgb.65.2017.10.09.06.03.08 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 09 Oct 2017 06:03:08 -0700 (PDT) Date: Mon, 09 Oct 2017 13:03:00 -0000 From: Stafford Horne To: Simon Marchi Cc: GDB patches , Openrisc , Mike Frysinger Subject: Re: [PATCH v5 3/6] sim: or1k: add or1k target to sim Message-ID: <20171009130305.GC2958@lianli.shorne-pla.net> References: <20171005134912.26799-1-shorne@gmail.com> <20171005134912.26799-4-shorne@gmail.com> <2a5f900c-99e7-cb4b-9211-b9dab8414de5@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2a5f900c-99e7-cb4b-9211-b9dab8414de5@polymtl.ca> User-Agent: Mutt/1.9.1 (2017-09-22) X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00196.txt.bz2 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 > > Peter Gavin > > > > * 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 . */ > > + > > +#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