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

[Daniel Henrique Barboza]

On 9/22/21 08:23, Matheus K. Ferst wrote:
> 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.

This is correct. The reason why PMCC = 0b00 is being handled is because a PMU
kernel selftest exercises this case expecting write failures.

The other 3 cases (all other three PMCC values) aren't being considered. For
instance, the case you just mentioned would require not just MMCR0, but all
the PMCs and MMCR2 to also have problem state read/write forbidden. PMCC 0b11
would remove PMC5 and PMC6 from the Performance Monitor altogether. All of
this is described in the ISA but it's not being used by the PMU/EBB test
case I've been using.

I'll take your advice and add a comment in the code/commit message explaining 
the
reasoning why PMCC 0b00 is the only case being handled. We can add additional
PMCC controls in a follow-up work of this implementation.


Thanks,


Daniel

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