Re: [Qemu-ppc] [PATCH] PPC: Introduce the Virtual Time Base (VTB) SPR register

[Cyril Bur]

On Thu, 2015-02-26 at 12:20 +0100, Alexander Graf wrote:
> 
> On 26.02.15 02:03, Cyril Bur wrote:
> > This patch adds basic support for the VTB.
> > 
> > PowerISA:
> > The Virtual Time Base (VTB) is a 64-bit incrementing counter.
> > Virtual Time Base increments at the same rate as the Time Base until its 
> > value
> > becomes 0xFFFF_FFFF_FFFF_FFFF (2 64 - 1); at the next increment its value
> > becomes 0x0000_0000_0000_0000. There is no interrupt or other indication 
> > when
> > this occurs.
> > 
> > The operation of the Virtual Time Base has the following additional
> > properties.
> > 1. Loading a GPR from the Virtual Time Base has no effect on the accuracy of
> > the Virtual Time Base.
> > 2. Copying the contents of a GPR to the Virtual Time Base replaces the
> > contents of the Virtual Time Base with the contents of the GPR.
> > 
> > Signed-off-by: Cyril Bur <address@hidden>
> > ---
> >  target-ppc/cpu.h            |  1 +
> >  target-ppc/translate_init.c | 17 +++++++++++++++++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> > index b706b9f..abc3545 100644
> > --- a/target-ppc/cpu.h
> > +++ b/target-ppc/cpu.h
> > @@ -1624,6 +1624,7 @@ static inline int cpu_mmu_index (CPUPPCState *env)
> >  #define SPR_MPC_MD_DBRAM1     (0x32A)
> >  #define SPR_RCPU_L2U_RA3      (0x32B)
> >  #define SPR_TAR               (0x32F)
> > +#define SPR_VTB               (0x351)
> >  #define SPR_440_INV0          (0x370)
> >  #define SPR_440_INV1          (0x371)
> >  #define SPR_440_INV2          (0x372)
> > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > index df1a62c..c2636f5 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -295,6 +295,13 @@ static void spr_read_purr (DisasContext *ctx, int 
> > gprn, int sprn)
> >  #endif
> >  #endif
> >  
> > +/* Virtual Time Base access. */
> > +__attribute__ ((unused))
> > +static void spr_read_vtb(DisasContext *ctx, int gprn, int sprn)
> > +{
> > +    spr_read_tbl(ctx, gprn, SPR_TBL);
> 
> Why indirectly call spr_read_tbl()? You can just use spr_read_tbl as
> handler for vtb. We don't implement HV mode properly yet anyway - and
> when we do we can just add an offset in spr_read_tbl.
> 
Absolutely, makes more sense.

> > +}
> > +
> >  #if !defined(CONFIG_USER_ONLY)
> >  /* IBAT0U...IBAT0U */
> >  /* IBAT0L...IBAT7L */
> > @@ -873,6 +880,15 @@ static void gen_tbl (CPUPPCState *env)
> >                   0x00000000);
> >  }
> >  
> > +/* Virtual Time Base */
> > +static void gen_spr_vtb(CPUPPCState *env)
> > +{
> > +    spr_register(env, SPR_VTB, "VTB",
> > +                 &spr_read_vtb, SPR_NOACCESS,
> > +                 &spr_read_vtb, SPR_NOACCESS,
> 
> The first 2 handlers are for user mode, the latter 2 are for supervisor
> mode. According to the spec, VTB is only readable from supervisor mode
> and writable from HV mode.

Yes, I didn't think there I guess I assumed it could be accessed like
the TB, will fix.
> 
> Also, please always CC qemu-devel on patches to PPC QEMU code, so that
> everyone gets the chance to see patches.

Sorry, will do for next version.

Thanks for feedback.

Cyril
> 
> 
> Alex
> 
> > +                 0x00000000);
> > +}
> > +
> >  /* Softare table search registers */
> >  static void gen_6xx_7xx_soft_tlb (CPUPPCState *env, int nb_tlbs, int 
> > nb_ways)
> >  {
> > @@ -7881,6 +7897,7 @@ static void init_proc_book3s_64(CPUPPCState *env, int 
> > version)
> >          gen_spr_power8_pmu_sup(env);
> >          gen_spr_power8_pmu_user(env);
> >          gen_spr_power8_tm(env);
> > +        gen_spr_vtb(env);
> >      }
> >      if (version < BOOK3S_CPU_POWER8) {
> >          gen_spr_book3s_dbg(env);
> > 
>