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

[David Gibson]

On Fri, Sep 24, 2021 at 04:05:37PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 9/24/21 15:34, Matheus K. Ferst wrote:
> > On 24/09/2021 11:41, Daniel Henrique Barboza wrote:
> > > On 9/22/21 08:24, 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.
> > > > > 
> > > > > 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>
> > > > > ---
> > > > 
> > > > <snip>
> > > > 
> > > > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > > > index 0babde3131..c3e2e3d329 100644
> > > > > --- a/target/ppc/translate.c
> > > > > +++ b/target/ppc/translate.c
> > > > > @@ -401,6 +401,24 @@ void spr_write_generic(DisasContext *ctx, int 
> > > > > sprn, int gprn)
> > > > >       spr_store_dump_spr(sprn);
> > > > >   }
> > > > > 
> > > > > +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> > > > > +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> > > > > +{
> > > > > +    /*
> > > > > +     * helper_store_mmcr0 will make clock based operations that
> > > > > +     * will cause 'bad icount read' errors if we do not execute
> > > > > +     * gen_icount_io_start() beforehand.
> > > > > +     */
> > > > > +    gen_icount_io_start(ctx);
> > > > > +    gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
> > > > > +}
> > > > > +#else
> > > > > +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> > > > > +{
> > > > > +    spr_write_generic(ctx, sprn, gprn);
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > >   #if !defined(CONFIG_USER_ONLY)
> > > > >   void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
> > > > >   {
> > > > > @@ -596,7 +614,10 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int 
> > > > > sprn, int gprn)
> > > > >       tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
> > > > >       /* Keep all other bits intact */
> > > > >       tcg_gen_or_tl(t1, t1, t0);
> > > > > -    gen_store_spr(SPR_POWER_MMCR0, t1);
> > > > > +
> > > > > +    /* Overwrite cpu_gpr[gprn] and use spr_write_MMCR0() */
> > > > > +    tcg_gen_mov_tl(cpu_gpr[gprn], t1);
> > > > > +    spr_write_MMCR0(ctx, sprn + 0x10, gprn);
> > > > 
> > > > IIUC, this makes writing to MMCR0 change the GPR value and expose the 
> > > > unfiltered content of the SPR to problem state. It might be better to 
> > > > call the helper directly or create another method that takes a TCGv as 
> > > > an argument and call it from spr_write_MMCR0_ureg and spr_write_MMCR0.
> > > 
> > > I'm overwriting cpu_gpr[gprn] with t1, which is filtered by MMCR0_REG_MASK
> > > right before, to re-use spr_write_MMCR0() since its API requires a gprn
> > > index. The reason I'm re-using spr_write_MMCR0() here is to avoid code 
> > > repetition
> > > in spr_write_MMCR0_ureg(), which would need to repeat the same steps as
> > > spr_write_MMCR0 (calling icount_io_start(), calling the helper, and then 
> > > setting
> > > DISAS_EXIT_UPDATE in a later patch).
> > > 
> > > The idea behind is that all PMU user_write() functions works the same as 
> > > its
> > > privileged counterparts but with some form of filtering done beforehand. 
> > > Note
> > > that this is kind of true in the previous patch as well - gen_store_spr() 
> > > is
> > > similar to the privileged function MMCR0 was using (spr_write_generic()) 
> > > with
> > > the exception of an optional qemu_log().
> > > 
> > > Maybe I should've made this clear in the previous patch, using 
> > > spr_write_generic()
> > > and overwriting cpu_gpr[gprn] with the filtered t1 content back there.
> > > 
> > > Speaking of which, since t1 is being filtered by MMCR0_REG_MASK before 
> > > being used to
> > > overwrite cpu_gpr[gprn], I'm not sure how this is exposing unfiltered 
> > > content to
> > > problem state. Can you elaborate?
> > 
> > Suppose MMCR0 has the value 0x80000001 (FC and FCH) and problem state 
> > executes an mtspr with the value 0x4000000 (unset FC and set PMAE) in the 
> > GPR. The proposed code will do the following:
> > 
> >  > tcg_gen_andi_tl(t0, cpu_gpr[gprn], MMCR0_UREG_MASK);
> > 
> > t0 = GPR & MMCR0_UREG_MASK = 0x4000000 & 0x84000080 = 0x4000000
> > 
> >  > gen_load_spr(t1, SPR_POWER_MMCR0);
> > 
> > t1 = MMCR0 = 0x80000001
> > 
> >  > tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
> > 
> > t1 = t1 & ~MMCR0_UREG_MASK = 0x80000001 & ~0x84000080 = 0x1
> > 
> >  > tcg_gen_or_tl(t1, t1, t0);
> > 
> > t1 = t1 | t0 = 0x4000000 | 0x1 = 0x4000001
> > 
> >  > tcg_gen_mov_tl(cpu_gpr[gprn], t1);
> > 
> > GPR = 0x4000001
> > 
> > Now problem state knows that FCH is set.

Nice catch Matheus.

> I see. The problem is that overwriting the GPR is exposing bits outside
> of the MMCR0_UREG_MASK via GPR itself, something that wasn't happening
> in the previous patch because the filtering logic wasn't visible via
> userspace.

Right.  Note that even if it wasn't exposing privileged bits, I don't
think changing the GPR value would be correct behaviour, although I
suspect it would be unlikely to cause problems in practice.

> Thanks for clarifying. I'll fix it in the next version, probably by adding a
> common 'write_MMCR0' method that receives a TCGv and that can be shared
> for both privileged and user write() callbacks, like you suggested in your
> previous reply.

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

signature.asc
Description: PGP signature