[Qemu-devel] [PATCH] PPC64: Fix timebase

[Qemu-devel] [PATCH] PPC64: Fix timebase

[Alexander Graf]

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

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

Re: [Qemu-devel] [PATCH] PPC64: Fix timebase

[Aurelien Jarno]

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

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] PPC64: Fix timebase

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH] PPC64: Fix timebase

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

Re: [Qemu-devel] [PATCH] PPC64: Fix timebase

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH] PPC64: Fix timebase

[Alexander Graf]

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

[Qemu-devel] [PATCH] PPC64: Fix timebase

[Alexander Graf]

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

V1 -> V2:

  - adjust user targets too
  - do an explicit cast for target_ulong
---
 darwin-user/main.c     |    4 ++--
 hw/ppc.c               |    4 ++--
 linux-user/main.c      |    4 ++--
 target-ppc/cpu.h       |    2 +-
 target-ppc/op_helper.c |    2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/darwin-user/main.c b/darwin-user/main.c
index 5fd314e..2f4a0f8 100644
--- a/darwin-user/main.c
+++ b/darwin-user/main.c
@@ -82,9 +82,9 @@ static inline uint64_t cpu_ppc_get_tb (CPUState *env)
     return 0;
 }
 
-uint32_t cpu_ppc_load_tbl (CPUState *env)
+uint64_t cpu_ppc_load_tbl (CPUState *env)
 {
-    return cpu_ppc_get_tb(env) & 0xFFFFFFFF;
+    return cpu_ppc_get_tb(env);
 }
 
 uint32_t cpu_ppc_load_tbu (CPUState *env)
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/linux-user/main.c b/linux-user/main.c
index 12502ad..5aa9dac 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -1068,9 +1068,9 @@ static inline uint64_t cpu_ppc_get_tb (CPUState *env)
     return 0;
 }
 
-uint32_t cpu_ppc_load_tbl (CPUState *env)
+uint64_t cpu_ppc_load_tbl (CPUState *env)
 {
-    return cpu_ppc_get_tb(env) & 0xFFFFFFFF;
+    return cpu_ppc_get_tb(env);
 }
 
 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);
diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index e3bd29c..e7bcd71 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -68,7 +68,7 @@ void helper_store_dump_spr (uint32_t sprn)
 
 target_ulong helper_load_tbl (void)
 {
-    return cpu_ppc_load_tbl(env);
+    return (target_ulong)cpu_ppc_load_tbl(env);
 }
 
 target_ulong helper_load_tbu (void)
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH] PPC64: Fix timebase

[Aurelien Jarno]

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.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] PPC64: Fix timebase

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH] PPC64: Fix timebase

[Andreas Färber]

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