Re: [PATCH v3 01/15] target/ppc: add user read functions for MMCR0 and MMCR2

From: David Gibson <david@gibson.dropbear.id.au>
To: Daniel Henrique Barboza <danielhb413@gmail.com>
Cc: Gustavo Romero <gustavo.romero@linaro.org>,
	Gustavo Romero <gromero@linux.ibm.com>,
	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 01/15] target/ppc: add user read functions for MMCR0 and MMCR2
Date: Tue, 7 Sep 2021 11:27:04 +1000	[thread overview]
Message-ID: <YTa/6KluMBiMN5n7@yekko> (raw)
In-Reply-To: <20210903203116.80628-2-danielhb413@gmail.com>

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

On Fri, Sep 03, 2021 at 05:31:02PM -0300, Daniel Henrique Barboza wrote:
> From: Gustavo Romero <gromero@linux.ibm.com>
> 
> We're going to add PMU support for TCG PPC64 chips, based on IBM POWER8+
> emulation and following PowerISA v3.1.
> 
> Let's start by handling the user read of UMMCR0 and UMMCR2. According to
> PowerISA 3.1 these registers omit some of its bits from userspace.
> 
> CC: Gustavo Romero <gustavo.romero@linaro.org>
> Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

LGTM except for one nit...

[snip]
> +void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn)
> +{
> +    TCGv t0 = tcg_temp_new();
> +
> +    /*
> +     * On read, filter out all bits that are not FCnP0 bits.
> +     * When MMCR0[PMCC] is set to 0b10 or 0b11, providing
> +     * problem state programs read/write access to MMCR2,
> +     * only the FCnP0 bits can be accessed. All other bits are
> +     * not changed when mtspr is executed in problem state, and
> +     * all other bits return 0s when mfspr is executed in problem
> +     * state, according to ISA v3.1, section 10.4.6 Monitor Mode
> +     * Control Register 2, p. 1316, third paragraph.
> +     */
> +    gen_load_spr(t0, SPR_POWER_MMCR2);
> +    tcg_gen_andi_tl(t0, t0, 0x4020100804020000UL);

A #define for this mask... and #defines with meaningful names for the
various bits it includes would be nice.

> +    tcg_gen_mov_tl(cpu_gpr[gprn], t0);
> +
> +    tcg_temp_free(t0);
> +}
> +
>  #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>  void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
>  {

-- 
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 --]