Re: [PATCH v3 03/15] target/ppc: PMU basic cycle count for pseries TCG

From: David Gibson <david@gibson.dropbear.id.au>
To: Daniel Henrique Barboza <danielhb413@gmail.com>
Cc: richard.henderson@linaro.org, qemu-devel@nongnu.org,
	groug@kaod.org, qemu-ppc@nongnu.org, clg@kaod.org,
	matheus.ferst@eldorado.org.br
Subject: Re: [PATCH v3 03/15] target/ppc: PMU basic cycle count for pseries TCG
Date: Tue, 7 Sep 2021 11:48:32 +1000	[thread overview]
Message-ID: <YTbE8LB5JwU3Yi0l@yekko> (raw)
In-Reply-To: <20210903203116.80628-4-danielhb413@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2002 bytes --]

On Fri, Sep 03, 2021 at 05:31:04PM -0300, Daniel Henrique Barboza wrote:
> This patch adds the barebones of the PMU logic by enabling cycle
> counting, done via the performance monitor counter 6. The overall logic
> goes as follows:
> 
> - a helper is added to control the PMU state on each MMCR0 write. This
> allows for the PMU to start/stop as the frozen counter bit (MMCR0_FC)
> is cleared or set;
> 
> - MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
> having to spin the PMU right at system init;
> 
> - the intended usage is to freeze the counters by setting MMCR0_FC, do
> any additional setting of events to be counted via MMCR1 (not
> implemented yet) and enable the PMU by zeroing MMCR0_FC. Software must
> freeze counters to read the results - on the fly reading of the PMCs
> will return the starting value of each one.
> 
> Since there will be more PMU exclusive code to be added next, put the
> PMU logic in its own helper to keep all in the same place. The name of
> the new helper file, power8_pmu.c, is an indicative that the PMU logic
> has been tested with the IBM POWER chip family, POWER8 being the oldest
> version tested. This doesn't mean that this PMU logic will break with
> any other PPC64 chip that implements Book3s, but since we can't assert
> that this PMU will work with all available Book3s emulated processors
> we're choosing to be explicit.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

LGTM, except for one nit:
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +
> +static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
> +                              uint64_t time_delta)
> +{
> +    /*
> +     * The pseries and pvn clock runs at 1Ghz, meaning that

s/pvn/pnv/ ?


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]