From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 83020 invoked by alias); 4 Sep 2017 21:49:08 -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 82971 invoked by uid 89); 4 Sep 2017 21:49:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=validations X-HELO: mail-pg0-f41.google.com Received: from mail-pg0-f41.google.com (HELO mail-pg0-f41.google.com) (74.125.83.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 04 Sep 2017 21:48:58 +0000 Received: by mail-pg0-f41.google.com with SMTP id 188so857582pgb.2 for ; Mon, 04 Sep 2017 14:48:58 -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=KnXecnFelGp4mYmRv20Ikte1LQ5GJtkYtCuKsusPLSs=; b=sCkngTXPG7MpJ7yd6gFwY2xSLqvBCBZggRpUup4yCx1LwXZdaf0h/6xzLpj9gkVvJA KrmixN0L8DF2yEhdtgQiDMBFEbrVUOW3GYymVrdoHBk/TkpbQCy7UHsaHVjWzGiGuTuG o6BpWql7gWDsihKY6XPMdU//6/yMwImAzgggAiXqKQTprBtQt9uJr2HiCNEAZbIGM1kP 7ZhYmK/f35lzzyyQtLJ4MBHnbvPl4wz7BPdJMVdDPuq4C61pZWpyuBr9YFLCAR18ptZR NxCo3XhiMMY2DLhqPyrTE8GnSqjg3DglXWXNi8D0ouFs8nb0gm3h5IcasOAGKi5PNlot 3gfQ== X-Gm-Message-State: AHPjjUiGNjQu8R4pncPcu4STGtcFK1D5vHFblXtt2CNzxtPPUzYfZo9l eS7LZMChAaHBvbprXnxCvw== X-Google-Smtp-Source: ADKCNb6t/s+0mfPHkTih8bT2qWaDszcdEAmxPJJeU+dJg8wLz7N8xL3LHc8QWFKbICiqf7G+MbOwmA== X-Received: by 10.98.74.217 with SMTP id c86mr1645164pfj.225.1504561737131; Mon, 04 Sep 2017 14:48:57 -0700 (PDT) Received: from localhost (g186.58-98-166.ppp.wakwak.ne.jp. [58.98.166.186]) by smtp.gmail.com with ESMTPSA id q73sm7075316pfl.65.2017.09.04.14.48.56 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 04 Sep 2017 14:48:56 -0700 (PDT) Date: Mon, 04 Sep 2017 21:49:00 -0000 From: Stafford Horne To: Simon Marchi Cc: GDB patches , Openrisc , Mike Frysinger Subject: Re: [PATCH v4 3/5] sim: or1k: add or1k target to sim Message-ID: <20170904214854.GN2609@lianli.shorne-pla.net> References: <617acff0490f4ae9ffcf961fb55b2ef1@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <617acff0490f4ae9ffcf961fb55b2ef1@polymtl.ca> User-Agent: Mutt/1.8.3 (2017-05-23) X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00071.txt.bz2 On Mon, Sep 04, 2017 at 11:14:13PM +0200, Simon Marchi wrote: > Hi Stafford, > > I started to look at this patch, and I have to say that it's quite difficult > for an outsider like me to follow. I can see what the code is doing, and > can correlate some parts with the openrisc spec. But since there are no > comments at all, I have no idea what the intents are, so I can't really tell > whether the code is right or wrong wrt the intent. Maybe it's just because > of my lack of experience in the area, and somebody familiar with the sim > code would find it obvious. Still, I think adding some comments would help > future contributors. Fair enough, to me I am also an outsider having inherited this. It took me some time to figure this all out as well. I will try to add some comments. > For example, in or1k_cpu_init: > > > +void > > +or1k_cpu_init (SIM_DESC sd, sim_cpu * current_cpu, const USI or1k_vr, > > + const USI or1k_upr, const USI or1k_cpucfgr) > > +{ > > + SET_H_SYS_VR (or1k_vr); > > + SET_H_SYS_UPR (or1k_upr); > > + SET_H_SYS_CPUCFGR (or1k_cpucfgr); > > + > > +#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", \ > > + #FIELD, #INDEX, field); > > \ > > + } > > \ > > + } while (0) > > + > > + current_cpu->next_delay_slot = 0; > > + current_cpu->delay_slot = 0; > > + > > + 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, OB64S, field == 0); > > + CHECK_SPR_FIELD (SYS, CPUCFGR, OF64S, field == 0); > > + CHECK_SPR_FIELD (SYS, CPUCFGR, OV64S, field == 0); > > Here, I think a comment should explain what these checks are about. I > understand they are checking for CPU features, making sure they are or > aren't there, but why? These registers it checks, the UPR (unit present register) and CPUCFGR (CPU configuration register), specify how the system has been configured. I.e. which features are available. Here we do some sanity checks to make sure its not configured to enable modules that are not available in the sim. Some flags can be configurable, but the ones checked above cannot be. The configuration of the sim can be passed in the by user as we can see in sim_open(), thats why these validations are needed. > > + > > +#undef CHECK_SPR_FIELD > > + > > + SET_H_SYS_UPR_UP (1); > > This, looks fishy to me, but maybe there's a good reason for it to be there? > In the checks above we made sure that the UP bit of the UPR register was > set. Why do we need to set it here? This is here to configure the UPR[UP] (UPR is present flag) to 1, even in case we didnt set it above when we do: SET_H_SYS_UPR (or1k_upr); The call to SET_H_SYS_UPR() sets all of the flags in the UPR as sent in from the user arguments. If they didn't set UPR[UP] we make sure it gets set. > I understand that there can't be (and we don't want) a comment for each > source line, but at least some high level ones. > Understood, Ill try to make the major points commented. Especially where some of the more complicated code generation macros are used. -Stafford