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

["Matheus K. Ferst" <matheus.ferst@eldorado.org.br>]

On 03/09/2021 17:31, Daniel Henrique Barboza wrote:
> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
> 
> 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>
> ---
>   target/ppc/cpu.h       | 10 ++++++++++
>   target/ppc/cpu_init.c  |  4 ++--
>   target/ppc/spr_tcg.h   |  2 ++
>   target/ppc/translate.c | 37 +++++++++++++++++++++++++++++++++++++
>   4 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 500205229c..f68bb8d8aa 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -342,6 +342,16 @@ typedef struct ppc_v3_pate_t {
>   #define MSR_RI   1  /* Recoverable interrupt                        1        */
>   #define MSR_LE   0  /* Little-endian mode                           1 hflags */
> 
> +/* PMU bits */
> +#define MMCR0_FC    PPC_BIT(32)         /* Freeze Counters  */
> +#define MMCR0_PMAO  PPC_BIT(56)         /* Perf Monitor Alert Ocurred */
> +#define MMCR0_PMAE  PPC_BIT(37)         /* Perf Monitor Alert Enable */
> +#define MMCR0_EBE   PPC_BIT(43)         /* Perf Monitor EBB Enable */
> +#define MMCR0_FCECE PPC_BIT(38)         /* FC on Enabled Cond or Event */
> +#define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
> +/* MMCR0 userspace r/w mask */
> +#define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
> +
>   /* LPCR bits */
>   #define LPCR_VPM0         PPC_BIT(0)
>   #define LPCR_VPM1         PPC_BIT(1)
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index ad7abc6041..9efc6c2d87 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6867,7 +6867,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
>   static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>   {
>       spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_MMCR0_ureg, SPR_NOACCESS,
>                    &spr_read_ureg, &spr_write_ureg,
>                    0x00000000);
>       spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> @@ -6975,7 +6975,7 @@ static void register_power8_pmu_sup_sprs(CPUPPCState *env)
>   static void register_power8_pmu_user_sprs(CPUPPCState *env)
>   {
>       spr_register(env, SPR_POWER_UMMCR2, "UMMCR2",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_MMCR2_ureg, SPR_NOACCESS,
>                    &spr_read_ureg, &spr_write_ureg,
>                    0x00000000);
>       spr_register(env, SPR_POWER_USIER, "USIER",
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 0be5f347d5..30cb6c3fdc 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -32,6 +32,8 @@ void spr_write_lr(DisasContext *ctx, int sprn, int gprn);
>   void spr_read_ctr(DisasContext *ctx, int gprn, int sprn);
>   void spr_write_ctr(DisasContext *ctx, int sprn, int gprn);
>   void spr_read_ureg(DisasContext *ctx, int gprn, int sprn);
> +void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn);
> +void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn);
>   void spr_read_tbl(DisasContext *ctx, int gprn, int sprn);
>   void spr_read_tbu(DisasContext *ctx, int gprn, int sprn);
>   void spr_read_atbl(DisasContext *ctx, int gprn, int sprn);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 171b216e17..b2ead144d1 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -519,6 +519,43 @@ void spr_read_ureg(DisasContext *ctx, int gprn, int sprn)
>       gen_load_spr(cpu_gpr[gprn], sprn + 0x10);
>   }
> 
> +void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
> +{
> +    TCGv t0 = tcg_temp_new();
> +
> +    /*
> +     * Filter out all bits but FC, PMAO, and PMAE, according
> +     * to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
> +     * fourth paragraph.
> +     */
> +    gen_load_spr(t0, SPR_POWER_MMCR0);
> +    tcg_gen_andi_tl(t0, t0, MMCR0_UREG_MASK);
> +    tcg_gen_mov_tl(cpu_gpr[gprn], t0);

 From the other patches, it seems that the focus is in the MMCR0[PMCC] = 
0b00 case, but I would note that the PMCC field description says that 
when MMCR0[PMCC] = 0b01, "Group A is not allowed to be read or written 
in problem state." If this case doesn't matter for this initial 
implementation, it'd be nice to leave a comment (XXX/TODO/etc.) saying 
that it's not handled. Otherwise, I think we'll need a helper or add 
both PMCC bits to hflags.

> +
> +    tcg_temp_free(t0);
> +}
> +
> +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);
> +    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)
>   {
> --
> 2.31.1
> 


-- 
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>