Patchwork PPC64: Fix timebase

login
register
mail settings
Submitter Alexander Graf
Date Dec. 21, 2009, 12:22 a.m.
Message ID <1261354932-28003-1-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/41517/
State New
Headers show

Comments

Alexander Graf - Dec. 21, 2009, 12:22 a.m.
On PPC we have a 64-bit time base. Usually (PPC32) this is accessed using
two separate 32 bit SPR accesses to SPR_TBU and SPR_TBL.

On PPC64 the SPR_TBL register acts as 64 bit though, so we get the full
64 bits as return value. If we only take the lower ones, fine. But Linux
wants to see all 64 bits or it breaks.

This patch makes PPC64 Linux work even after TB crossed the 32-bit boundary,
which usually happened a few seconds after bootup.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

To verify my assumptions of the above I used this test program:

  int main()
  {
      unsigned int tbu=0, tbl=0;
      unsigned long tb=0;

      asm("mftbu %0" : "=r" (tbu));
      asm("mftbl %0" : "=r" (tbl));
      asm("mftbl %0" : "=r" (tb));

      printf("TB: %#x %#x\n", tbu, tbl);
      printf("TB64: %#lx\n", tb);
  }

It produces the following output on a 970MP CPU:

$ ./mftb
TB: 0x238 0xd676bd6
TB64: 0x2380d676f75
---
 hw/ppc.c         |    4 ++--
 target-ppc/cpu.h |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
Aurelien Jarno - Dec. 21, 2009, 9:24 a.m.
On Mon, Dec 21, 2009 at 01:22:12AM +0100, Alexander Graf wrote:
> On PPC we have a 64-bit time base. Usually (PPC32) this is accessed using
> two separate 32 bit SPR accesses to SPR_TBU and SPR_TBL.
> 
> On PPC64 the SPR_TBL register acts as 64 bit though, so we get the full
> 64 bits as return value. If we only take the lower ones, fine. But Linux
> wants to see all 64 bits or it breaks.

Good catch! However, I think this patch it's not fully complete and can
be improved a bit
- it's probably better to return a target_ulong value from
  cpu_ppc_load_tbl() with an explicit cast here, so that we don't have
  an implicit cast from 64-bit to 32-bit on qemu-system-powerpc (GCC may
  warn on that with some flags or in future versions).
- the store function also has to be fixed.
- the same changes should be done for the alternate timebase.

> This patch makes PPC64 Linux work even after TB crossed the 32-bit boundary,
> which usually happened a few seconds after bootup.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> ---
> 
> To verify my assumptions of the above I used this test program:
> 
>   int main()
>   {
>       unsigned int tbu=0, tbl=0;
>       unsigned long tb=0;
> 
>       asm("mftbu %0" : "=r" (tbu));
>       asm("mftbl %0" : "=r" (tbl));
>       asm("mftbl %0" : "=r" (tb));
> 
>       printf("TB: %#x %#x\n", tbu, tbl);
>       printf("TB64: %#lx\n", tb);
>   }
> 
> It produces the following output on a 970MP CPU:
> 
> $ ./mftb
> TB: 0x238 0xd676bd6
> TB64: 0x2380d676f75
> ---
>  hw/ppc.c         |    4 ++--
>  target-ppc/cpu.h |    2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc.c b/hw/ppc.c
> index 5208039..b4bf2d3 100644
> --- a/hw/ppc.c
> +++ b/hw/ppc.c
> @@ -401,7 +401,7 @@ static inline uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk,
>      return muldiv64(vmclk, tb_env->tb_freq, get_ticks_per_sec()) + tb_offset;
>  }
>  
> -uint32_t cpu_ppc_load_tbl (CPUState *env)
> +uint64_t cpu_ppc_load_tbl (CPUState *env)
>  {
>      ppc_tb_t *tb_env = env->tb_env;
>      uint64_t tb;
> @@ -409,7 +409,7 @@ uint32_t cpu_ppc_load_tbl (CPUState *env)
>      tb = cpu_ppc_get_tb(tb_env, qemu_get_clock(vm_clock), tb_env->tb_offset);
>      LOG_TB("%s: tb %016" PRIx64 "\n", __func__, tb);
>  
> -    return tb & 0xFFFFFFFF;
> +    return tb;
>  }
>  
>  static inline uint32_t _cpu_ppc_load_tbu(CPUState *env)
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 2535cbc..2dc301d 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -741,7 +741,7 @@ int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def);
>  
>  /* Time-base and decrementer management */
>  #ifndef NO_CPU_IO_DEFS
> -uint32_t cpu_ppc_load_tbl (CPUPPCState *env);
> +uint64_t cpu_ppc_load_tbl (CPUPPCState *env);
>  uint32_t cpu_ppc_load_tbu (CPUPPCState *env);
>  void cpu_ppc_store_tbu (CPUPPCState *env, uint32_t value);
>  void cpu_ppc_store_tbl (CPUPPCState *env, uint32_t value);
> -- 
> 1.6.0.2
> 
> 
> 
>
Alexander Graf - Dec. 21, 2009, 9:39 a.m.
On 21.12.2009, at 10:24, Aurelien Jarno wrote:

> On Mon, Dec 21, 2009 at 01:22:12AM +0100, Alexander Graf wrote:
>> On PPC we have a 64-bit time base. Usually (PPC32) this is accessed using
>> two separate 32 bit SPR accesses to SPR_TBU and SPR_TBL.
>> 
>> On PPC64 the SPR_TBL register acts as 64 bit though, so we get the full
>> 64 bits as return value. If we only take the lower ones, fine. But Linux
>> wants to see all 64 bits or it breaks.
> 
> Good catch! However, I think this patch it's not fully complete and can
> be improved a bit
> - it's probably better to return a target_ulong value from
>  cpu_ppc_load_tbl() with an explicit cast here, so that we don't have
>  an implicit cast from 64-bit to 32-bit on qemu-system-powerpc (GCC may
>  warn on that with some flags or in future versions).

ppc.c is in hw, so I suspect it's in the target independent makefile part? Otherwise we should move all TB stuff to target-ppc.

> - the store function also has to be fixed.

Oh, right.

> - the same changes should be done for the alternate timebase.

Hum, probably. Right.

Alex
Aurelien Jarno - Dec. 21, 2009, 10:15 a.m.
On Mon, Dec 21, 2009 at 10:39:39AM +0100, Alexander Graf wrote:
> 
> On 21.12.2009, at 10:24, Aurelien Jarno wrote:
> 
> > On Mon, Dec 21, 2009 at 01:22:12AM +0100, Alexander Graf wrote:
> >> On PPC we have a 64-bit time base. Usually (PPC32) this is accessed using
> >> two separate 32 bit SPR accesses to SPR_TBU and SPR_TBL.
> >> 
> >> On PPC64 the SPR_TBL register acts as 64 bit though, so we get the full
> >> 64 bits as return value. If we only take the lower ones, fine. But Linux
> >> wants to see all 64 bits or it breaks.
> > 
> > Good catch! However, I think this patch it's not fully complete and can
> > be improved a bit
> > - it's probably better to return a target_ulong value from
> >  cpu_ppc_load_tbl() with an explicit cast here, so that we don't have
> >  an implicit cast from 64-bit to 32-bit on qemu-system-powerpc (GCC may
> >  warn on that with some flags or in future versions).
> 
> ppc.c is in hw, so I suspect it's in the target independent makefile part? Otherwise we should move all TB stuff to target-ppc.

Correct. then let's return uint64_t for cpu_ppc_load_tbl(), but do the
explicit cast in the helper.
Alexander Graf - Dec. 21, 2009, 11:12 a.m.
On 21.12.2009, at 10:24, Aurelien Jarno wrote:

> On Mon, Dec 21, 2009 at 01:22:12AM +0100, Alexander Graf wrote:
>> On PPC we have a 64-bit time base. Usually (PPC32) this is accessed using
>> two separate 32 bit SPR accesses to SPR_TBU and SPR_TBL.
>> 
>> On PPC64 the SPR_TBL register acts as 64 bit though, so we get the full
>> 64 bits as return value. If we only take the lower ones, fine. But Linux
>> wants to see all 64 bits or it breaks.
> 
> Good catch! However, I think this patch it's not fully complete and can
> be improved a bit
> - it's probably better to return a target_ulong value from
>  cpu_ppc_load_tbl() with an explicit cast here, so that we don't have
>  an implicit cast from 64-bit to 32-bit on qemu-system-powerpc (GCC may
>  warn on that with some flags or in future versions).
> - the store function also has to be fixed.

According to Book3:

It is not possible to write the entire 64-bit Time Base using a single instruction. The mttbl and mttbu extended mnemonics write the lower and upper halves of the Time Base (TBL and TBU), respectively, preserving the other half. These are extended mne- monics for the mtspr instruction; see page 83.

> - the same changes should be done for the alternate timebase.

I can't find traces of the alternative timebase in the docs :-(.

Alex
Alexander Graf - Dec. 21, 2009, 11:15 a.m.
On 21.12.2009, at 10:24, Aurelien Jarno wrote:

> On Mon, Dec 21, 2009 at 01:22:12AM +0100, Alexander Graf wrote:
>> On PPC we have a 64-bit time base. Usually (PPC32) this is accessed using
>> two separate 32 bit SPR accesses to SPR_TBU and SPR_TBL.
>> 
>> On PPC64 the SPR_TBL register acts as 64 bit though, so we get the full
>> 64 bits as return value. If we only take the lower ones, fine. But Linux
>> wants to see all 64 bits or it breaks.
> 
> Good catch! However, I think this patch it's not fully complete and can
> be improved a bit
> - it's probably better to return a target_ulong value from
>  cpu_ppc_load_tbl() with an explicit cast here, so that we don't have
>  an implicit cast from 64-bit to 32-bit on qemu-system-powerpc (GCC may
>  warn on that with some flags or in future versions).
> - the store function also has to be fixed.
> - the same changes should be done for the alternate timebase.

Uuuh:

__attribute__ (( unused ))
static void spr_read_atbl (void *opaque, int gprn, int sprn)
{
    gen_helper_load_atbl(cpu_gpr[gprn]);
}

And that attribute is correct. There is no caller.

Alex
Aurelien Jarno - Dec. 21, 2009, 3:05 p.m.
On Mon, Dec 21, 2009 at 12:15:42PM +0100, Alexander Graf wrote:
> 
> On 21.12.2009, at 10:24, Aurelien Jarno wrote:
> 
> > On Mon, Dec 21, 2009 at 01:22:12AM +0100, Alexander Graf wrote:
> >> On PPC we have a 64-bit time base. Usually (PPC32) this is accessed using
> >> two separate 32 bit SPR accesses to SPR_TBU and SPR_TBL.
> >> 
> >> On PPC64 the SPR_TBL register acts as 64 bit though, so we get the full
> >> 64 bits as return value. If we only take the lower ones, fine. But Linux
> >> wants to see all 64 bits or it breaks.
> > 
> > Good catch! However, I think this patch it's not fully complete and can
> > be improved a bit
> > - it's probably better to return a target_ulong value from
> >  cpu_ppc_load_tbl() with an explicit cast here, so that we don't have
> >  an implicit cast from 64-bit to 32-bit on qemu-system-powerpc (GCC may
> >  warn on that with some flags or in future versions).
> > - the store function also has to be fixed.
> > - the same changes should be done for the alternate timebase.

They are defined in the Book II, and corresponds to atbl and atbu
functions.

> Uuuh:
> 
> __attribute__ (( unused ))
> static void spr_read_atbl (void *opaque, int gprn, int sprn)
> {
>     gen_helper_load_atbl(cpu_gpr[gprn]);
> }
> 
> And that attribute is correct. There is no caller.
> 

Ok. I have committed a fix anyway, so that if someone enable it later,
he/she doesn't spend to much time fixing the bug.
Alexander Graf - Dec. 21, 2009, 4:04 p.m.
Aurelien Jarno wrote:
> On Mon, Dec 21, 2009 at 12:15:42PM +0100, Alexander Graf wrote:
>   
>> On 21.12.2009, at 10:24, Aurelien Jarno wrote:
>>
>>     
>>> On Mon, Dec 21, 2009 at 01:22:12AM +0100, Alexander Graf wrote:
>>>       
>>>> On PPC we have a 64-bit time base. Usually (PPC32) this is accessed using
>>>> two separate 32 bit SPR accesses to SPR_TBU and SPR_TBL.
>>>>
>>>> On PPC64 the SPR_TBL register acts as 64 bit though, so we get the full
>>>> 64 bits as return value. If we only take the lower ones, fine. But Linux
>>>> wants to see all 64 bits or it breaks.
>>>>         
>>> Good catch! However, I think this patch it's not fully complete and can
>>> be improved a bit
>>> - it's probably better to return a target_ulong value from
>>>  cpu_ppc_load_tbl() with an explicit cast here, so that we don't have
>>>  an implicit cast from 64-bit to 32-bit on qemu-system-powerpc (GCC may
>>>  warn on that with some flags or in future versions).
>>> - the store function also has to be fixed.
>>> - the same changes should be done for the alternate timebase.
>>>       
>
> They are defined in the Book II, and corresponds to atbl and atbu
> functions.
>
>   
>> Uuuh:
>>
>> __attribute__ (( unused ))
>> static void spr_read_atbl (void *opaque, int gprn, int sprn)
>> {
>>     gen_helper_load_atbl(cpu_gpr[gprn]);
>> }
>>
>> And that attribute is correct. There is no caller.
>>
>>     
>
> Ok. I have committed a fix anyway, so that if someone enable it later,
> he/she doesn't spend to much time fixing the bug.
>   

Thanks :-).

Alex
Andreas Färber - Dec. 21, 2009, 8:19 p.m.
Am 21.12.2009 um 11:15 schrieb Aurelien Jarno:

> On Mon, Dec 21, 2009 at 10:39:39AM +0100, Alexander Graf wrote:
>>
>> On 21.12.2009, at 10:24, Aurelien Jarno wrote:
>>
>>> On Mon, Dec 21, 2009 at 01:22:12AM +0100, Alexander Graf wrote:
>>>> On PPC we have a 64-bit time base. Usually (PPC32) this is  
>>>> accessed using
>>>> two separate 32 bit SPR accesses to SPR_TBU and SPR_TBL.
>>>>
>>>> On PPC64 the SPR_TBL register acts as 64 bit though, so we get  
>>>> the full
>>>> 64 bits as return value. If we only take the lower ones, fine.  
>>>> But Linux
>>>> wants to see all 64 bits or it breaks.
>>>
>>> Good catch! However, I think this patch it's not fully complete  
>>> and can
>>> be improved a bit
>>> - it's probably better to return a target_ulong value from
>>> cpu_ppc_load_tbl() with an explicit cast here, so that we don't have
>>> an implicit cast from 64-bit to 32-bit on qemu-system-powerpc (GCC  
>>> may
>>> warn on that with some flags or in future versions).
>>
>> ppc.c is in hw, so I suspect it's in the target independent  
>> makefile part? Otherwise we should move all TB stuff to target-ppc.
>
> Correct.

No. Just like hw/ppc_newworld.c, it is target dependent and built from  
Makefile.target (obj-ppc-y).

Andreas

> then let's return uint64_t for cpu_ppc_load_tbl(), but do the
> explicit cast in the helper.
>
> -- 
> Aurelien Jarno	                        GPG: 1024D/F1BCDB73
> aurelien@aurel32.net                 http://www.aurel32.net
>
>

Patch

diff --git a/hw/ppc.c b/hw/ppc.c
index 5208039..b4bf2d3 100644
--- a/hw/ppc.c
+++ b/hw/ppc.c
@@ -401,7 +401,7 @@  static inline uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk,
     return muldiv64(vmclk, tb_env->tb_freq, get_ticks_per_sec()) + tb_offset;
 }
 
-uint32_t cpu_ppc_load_tbl (CPUState *env)
+uint64_t cpu_ppc_load_tbl (CPUState *env)
 {
     ppc_tb_t *tb_env = env->tb_env;
     uint64_t tb;
@@ -409,7 +409,7 @@  uint32_t cpu_ppc_load_tbl (CPUState *env)
     tb = cpu_ppc_get_tb(tb_env, qemu_get_clock(vm_clock), tb_env->tb_offset);
     LOG_TB("%s: tb %016" PRIx64 "\n", __func__, tb);
 
-    return tb & 0xFFFFFFFF;
+    return tb;
 }
 
 static inline uint32_t _cpu_ppc_load_tbu(CPUState *env)
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 2535cbc..2dc301d 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -741,7 +741,7 @@  int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def);
 
 /* Time-base and decrementer management */
 #ifndef NO_CPU_IO_DEFS
-uint32_t cpu_ppc_load_tbl (CPUPPCState *env);
+uint64_t cpu_ppc_load_tbl (CPUPPCState *env);
 uint32_t cpu_ppc_load_tbu (CPUPPCState *env);
 void cpu_ppc_store_tbu (CPUPPCState *env, uint32_t value);
 void cpu_ppc_store_tbl (CPUPPCState *env, uint32_t value);