From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101670 invoked by alias); 9 Dec 2016 20:40:10 -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 101636 invoked by uid 89); 9 Dec 2016 20:40:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=1.9 required=5.0 tests=AWL,BAYES_00,BODY_8BITS,GARBLED_BODY,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy==d0=b2=d1, =d0=bd=d0=b8=d0=ba, tip, optionally?= X-HELO: server28.host.bg Received: from server28.host.bg (HELO server28.host.bg) (87.120.40.98) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 09 Dec 2016 20:39:58 +0000 Received: from [95.87.234.74] (port=58354 helo=tpdeb.localnet) by server28.host.bg with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.87) (envelope-from ) id 1cFRxR-001q0X-AT; Fri, 09 Dec 2016 22:39:53 +0200 From: Dimitar Dimitrov To: Mike Frysinger Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] PRU Simulator port Date: Fri, 09 Dec 2016 20:40:00 -0000 Message-ID: <9877120.H2PyqMF4WO@tpdeb> User-Agent: KMail/5.2.3 (Linux/4.8.0-1-amd64; KDE/5.27.0; x86_64; ; ) In-Reply-To: <20161206231731.GG10558@vapier.lan> References: <20161205211108.26616-1-dimitar@dinux.eu> <20161206231731.GG10558@vapier.lan> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" X-Get-Message-Sender-Via: server28.host.bg: authenticated_id: dimitar@dinux.eu X-IsSubscribed: yes X-SW-Source: 2016-12/txt/msg00255.txt.bz2 On =D0=B2=D1=82=D0=BE=D1=80=D0=BD=D0=B8=D0=BA, 6 =D0=B4=D0=B5=D0=BA=D0=B5= =D0=BC=D0=B2=D1=80=D0=B8 2016 =D0=B3. 18:17:31 EET Mike Frysinger wrote: > On 05 Dec 2016 23:11, Dimitar Dimitrov wrote: >=20 > a very good start! Thanks. I'll rework and send v2.=20 >=20 ... > > --- /dev/null > > +++ b/sim/pru/configure.ac > >=20 > > +SIM_AC_OPTION_ENDIAN >=20 > can PRU be little or big endian ? or is it always one or the other ? >=20 > if it's always one or the other, then you should pass in LITTLE or BIG he= re. I'll hard-code it to LITTLE because that has been my assumption while porti= ng. If someday TI folks decide to build a big-endian PRU, then I'll revise. > > --- /dev/null > > +++ b/sim/pru/interp.c > >=20 > > + This file is part of GDB, the GNU debugger. >=20 > should say: > This file is part of simulators. I'll change it. I assume my copyright papers for GDB will cover this simula= tor patch? >=20 ... >=20 > > +/* 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 const int abort_on_dmem_zero_access =3D 0; >=20 > seems like this should be a debug option so people can change it on > the fly ? you could leverage the sim-options.h API to change the > value based on command line flags. Thanks for the tip. I'll add an "--abort-on-dmem-zero" option. >=20 > > +static uint32_t > > +pru_extract_unsigned_integer (uint8_t *addr, int len) > > +{ > > ... > > + if (len > (int) sizeof (unsigned long)) >=20 > change the prototype so that len is a size_t instead of len, then you > don't need this cast. size_t is a better type anyways. >=20 > > + printf ("That operation is not available on integers of more than " >=20 > sim's should never use printf. you can use sim_io_eprintf instead. >=20 > then again, the only caller of this func already does a len check which > means this scenario should never happen. use sim_io_error instead and > that'll trigger an exit/abort. after all, if you let this code continue > to run, you'll clobber random memory. >=20 > > + "%ld bytes.", sizeof (unsigned long)); >=20 > sizeof returns a size_t which means this should be %zu, not %ld I'll remove the check as caller already sanitizes the parameter. >=20 >=20 > > +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 >=3D 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); > > + } >=20 > 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. - Check that the load/store burst length is valid (i.e. do not access be= yond the last CPU register). - Optionally catch NULL pointer dereferences. > > +static void > > +pru_sim_xin_mac (SIM_DESC sd, SIM_CPU *cpu, unsigned int rd_regn, > > + unsigned int rdb, unsigned int length) > > +{ > > + if (rd_regn < 25 || (rd_regn * 4 + rdb + length) > (27 + 1) * 4) > > + { > > + fprintf (stderr, "XIN MAC: invalid transfer regn=3D%u.%u, > > length=3D%u\n", + rd_regn, rdb, length); > > + RAISE_SIGILL (); > > + return; >=20 > use sim_io_error instead i think >=20 > comes up multiple times in this file I'll switch to ASSERT, SIM_ASSERT, sim_io_error and sim_io_eprintf. >=20 > > +static void > > +pru_sim_syscall (SIM_DESC sd, SIM_CPU *cpu) >=20 > 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 si= m_syscall. >=20 Regards, Dimitar