From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50788) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gNgFz-00062Y-BQ for qemu-devel@nongnu.org; Fri, 16 Nov 2018 10:42:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gNgFy-00035Y-GP for qemu-devel@nongnu.org; Fri, 16 Nov 2018 10:42:07 -0500 From: Aaron Lindsay Date: Fri, 16 Nov 2018 15:41:57 +0000 Message-ID: <20181116154137.GC23658@quinoa.localdomain> References: <20181105185046.2802-1-aaron@os.amperecomputing.com> <20181105185046.2802-3-aaron@os.amperecomputing.com> In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v7 02/12] target/arm: Reorganize PMCCNTR accesses List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "qemu-arm@nongnu.org" , Alistair Francis , Wei Huang , Peter Crosthwaite , Richard Henderson , "qemu-devel@nongnu.org" , Michael Spradling , Digant Desai , Aaron Lindsay , Aaron Lindsay On Nov 16 14:50, Peter Maydell wrote: > On 5 November 2018 at 18:51, Aaron Lindsay = wrote: > > pmccntr_read and pmccntr_write contained duplicate code that was alread= y > > being handled by pmccntr_sync. Consolidate the duplicated code into two > > functions: pmccntr_op_start and pmccntr_op_finish. Add a companion to > > c15_ccnt in CPUARMState so that we can simultaneously save both the > > architectural register value and the last underlying cycle count - this > > ensures time isn't lost and will also allow us to access the 'old' > > architectural register value in order to detect overflows in later > > patches. > > > > Signed-off-by: Aaron Lindsay > > Signed-off-by: Aaron Lindsay >=20 >=20 > > /** > > - * pmccntr_sync > > + * pmccntr_op_start/finish > > + * @env: CPUARMState > > + * > > + * Convert the counter in the PMCCNTR between its delta form (the typi= cal mode > > + * when it's enabled) and the guest-visible value. These two calls mus= t always > > + * surround any action which might affect the counter. > > + */ > > +void pmccntr_op_start(CPUARMState *env); > > +void pmccntr_op_finish(CPUARMState *env); > > + > > +/** > > + * pmu_op_start/finish > > * @env: CPUARMState > > * > > - * Synchronises the counter in the PMCCNTR. This must always be called= twice, > > - * once before any action that might affect the timer and again afterw= ards. > > - * The function is used to swap the state of the register if required. > > - * This only happens when not in user mode (!CONFIG_USER_ONLY) > > + * Convert all PMU counters between their delta form (the typical mode= when > > + * they are enabled) and the guest-visible values. These two calls mus= t > > + * surround any action which might affect the counters, and the return= value > > + * from pmu_op_start must be supplied as the second argument to pmu_op= _finish. > > */ > > -void pmccntr_sync(CPUARMState *env); > > +void pmu_op_start(CPUARMState *env); > > +void pmu_op_finish(CPUARMState *env); >=20 > The comment says that pmu_op_start returns a value and pmu_op_finish > has a second argument, but the prototypes disagree... Doh, I updated the comment for pmccntr_op_* but missed pmu_op_*. The last sentence should end after the comma: > > + * Convert all PMU counters between their delta form (the typical mode= when > > + * they are enabled) and the guest-visible values. These two calls mus= t > > + * surround any action which might affect the counters. > > */ > Otherwise > Reviewed-by: Peter Maydell > so if this turns out to be the only problem I can fix it up when > I apply it to my tree, if you provide suitable new comment text. I've also updated the comment text in my tree, so if/when there is a v8 of this patchset, it won't be lost. -Aaron