| Message ID | 1261354932-28003-1-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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 > > > >
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
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.
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
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
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 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
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 > >
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);
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(-)