Message ID | CAFEAcA84QTW+p57pccbgGnp5v_=deZT7g52ure+=s96WdM0oXw@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | booke206_tlb_ways() returning 0 | expand |
Le 26/03/2020 à 15:22, Peter Maydell a écrit : > Hi; Coverity points out a possible issue in booke206_get_tlbm() > (this is CID 1421942): > > > static inline ppcmas_tlb_t *booke206_get_tlbm(CPUPPCState *env, const int tlbn, > target_ulong ea, int way) > { > int r; > uint32_t ways = booke206_tlb_ways(env, tlbn); > int ways_bits = ctz32(ways); > int tlb_bits = ctz32(booke206_tlb_size(env, tlbn)); > int i; > > way &= ways - 1; > ea >>= MAS2_EPN_SHIFT; > ea &= (1 << (tlb_bits - ways_bits)) - 1; > r = (ea << ways_bits) | way; > > Here if booke206_tlb_ways() returns zero, then ways_bits() > will be 32 and the shift left 'ea << ways_bits' is undefined > behaviour. > > My first assumption was that booke206_tlb_ways() is not supposed > to ever return 0 (it's looking at what I think are read-only > system registers, and it doesn't make much sense to have > a zero-way TLB). So I tried adding an assert: > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 88d94495554..aedb6bcb265 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -2403,6 +2403,7 @@ static inline int booke206_tlb_ways(CPUPPCState > *env, int tlbn) > { > uint32_t tlbncfg = env->spr[SPR_BOOKE_TLB0CFG + tlbn]; > int r = tlbncfg >> TLBnCFG_ASSOC_SHIFT; > + assert(r > 0); > return r; > } > > However, this isn't right, because it causes one of the check-acceptance > tests to fail, with this backtrace: > > #3 0x00007ffff074d412 in __GI___assert_fail (assertion=0x5555560a0d7d > "r > 0", file=0x5555560a0d40 > "/home/petmay01/linaro/qemu-from-laptop/qemu/target/ppc/cpu.h", > line=2406, function=0x5555560a1720 <__PRETTY_FUNCTION__.20811> > "booke206_tlb_ways") at assert.c:101 > #4 0x0000555555a9939b in booke206_tlb_ways (env=0x555556e52a30, > tlbn=2) at /home/petmay01/linaro/qemu-from-laptop/qemu/target/ppc/cpu.h:2406 > #5 0x0000555555a9b3ac in mmubooke206_get_physical_address > (env=0x555556e52a30, ctx=0x7fffd77008a0, address=9223380835095282947, > rw=0, access_type=0, mmu_idx=1) at > /home/petmay01/linaro/qemu-from-laptop/qemu/target/ppc/mmu_helper.c:1093 > #6 0x0000555555a9c25d in get_physical_address_wtlb > (env=0x555556e52a30, ctx=0x7fffd77008a0, eaddr=9223380835095282947, > rw=0, access_type=0, mmu_idx=1) at > /home/petmay01/linaro/qemu-from-laptop/qemu/target/ppc/mmu_helper.c:1455 > #7 0x0000555555a9c82b in cpu_ppc_handle_mmu_fault > (env=0x555556e52a30, address=9223380835095282947, rw=0, mmu_idx=1) > at /home/petmay01/linaro/qemu-from-laptop/qemu/target/ppc/mmu_helper.c:1597 > #8 0x0000555555a9f975 in ppc_cpu_tlb_fill (cs=0x555556e49560, > addr=9223380835095282947, size=1, access_type=MMU_DATA_LOAD, > mmu_idx=1, probe=false, retaddr=140735610345781) at > /home/petmay01/linaro/qemu-from-laptop/qemu/target/ppc/mmu_helper.c:3053 > #9 0x00005555558e1422 in tlb_fill (cpu=0x555556e49560, > addr=9223380835095282947, size=1, access_type=MMU_DATA_LOAD, > mmu_idx=1, retaddr=140735610345781) at > /home/petmay01/linaro/qemu-from-laptop/qemu/accel/tcg/cputlb.c:1017 > #10 0x00005555558e279b in load_helper (env=0x555556e52a30, > addr=9223380835095282947, oi=1, retaddr=140735610345781, op=MO_8, > code_read=false, full_load=0x5555558e2b3a <full_ldub_mmu>) at > /home/petmay01/linaro/qemu-from-laptop/qemu/accel/tcg/cputlb.c:1534 > #11 0x00005555558e2b80 in full_ldub_mmu (env=0x555556e52a30, > addr=9223380835095282947, oi=1, retaddr=140735610345781) > at /home/petmay01/linaro/qemu-from-laptop/qemu/accel/tcg/cputlb.c:1624 > #12 0x00005555558e2bb4 in helper_ret_ldub_mmu (env=0x555556e52a30, > addr=9223380835095282947, oi=1, retaddr=140735610345781) > at /home/petmay01/linaro/qemu-from-laptop/qemu/accel/tcg/cputlb.c:1630 > #13 0x00007fff900fd9c6 in code_gen_buffer () > #14 0x00005555558f9915 in cpu_tb_exec (cpu=0x555556e49560, > itb=0x7fff900fd780 <code_gen_buffer+1038163>) > at /home/petmay01/linaro/qemu-from-laptop/qemu/accel/tcg/cpu-exec.c:172 > #15 0x00005555558fa732 in cpu_loop_exec_tb (cpu=0x555556e49560, > tb=0x7fff900fd780 <code_gen_buffer+1038163>, last_tb=0x7fffd7701078, > tb_exit=0x7fffd7701070) at > /home/petmay01/linaro/qemu-from-laptop/qemu/accel/tcg/cpu-exec.c:619 > #16 0x00005555558faa4c in cpu_exec (cpu=0x555556e49560) at > /home/petmay01/linaro/qemu-from-laptop/qemu/accel/tcg/cpu-exec.c:732 > #17 0x00005555558bcf29 in tcg_cpu_exec (cpu=0x555556e49560) at > /home/petmay01/linaro/qemu-from-laptop/qemu/cpus.c:1405 > #18 0x00005555558bd77f in qemu_tcg_cpu_thread_fn (arg=0x555556e49560) > at /home/petmay01/linaro/qemu-from-laptop/qemu/cpus.c:1713 > #19 0x0000555555f2ff3f in qemu_thread_start (args=0x555556e8dd10) at > /home/petmay01/linaro/qemu-from-laptop/qemu/util/qemu-thread-posix.c:519 > #20 0x00007ffff0b156db in start_thread (arg=0x7fffd7704700) at > pthread_create.c:463 > #21 0x00007ffff083e88f in clone () at > ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 > > So under what circumstances can booke206_tlb_ways() > validly return 0? Should booke206_get_tlbm() cope with a > zero return, or can it assert() that it will never > call booke206_tlb_ways() in a way that will cause one? It seems the value of booke206_tlb_ways() is the associativity (TLBnCFG_ASSOC_SHIFT) that seems to be generated by gen_tlbncfg() and set in init_proc_e500(). And used values are: 2, 4 ,16, and 64, but only for tlbn 0 and 1. According to PowerISA 2.06: booke206_tlb_size() booke206_tlb_ways() 0 0 No TLB present 0 1 TLB geometry is completely implementation-defined. MAS0 ESEL is ignored 0 > 1 TLB geometry and number of entries is implementation defined,... n > 0 n or 0 TLB is fully associative In mmubooke206_get_physical_address(), helper_booke206_tlbsx(), booke206_invalidate_ea_tlb() and helper_booke206_tlbilx3(), booke206_get_tlbm() is not called if ways is 0. In booke206_cur_tlb(), the function is only called with tlbn number provided by SPR_BOOKE_MAS0, so we can guess we are using the ones with the initialized values. It is also called in mmubooke_create_initial_mapping() but only for tlbn 1. But r can be 0 if ea and way are 0 (in mmubooke206_get_physical_address() if ways > 0, way will start with value 0 and ea >> ways_bit can be 0). I think it's why your assert() is triggered. Thanks, Laurent
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 88d94495554..aedb6bcb265 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -2403,6 +2403,7 @@ static inline int booke206_tlb_ways(CPUPPCState *env, int tlbn) { uint32_t tlbncfg = env->spr[SPR_BOOKE_TLB0CFG + tlbn]; int r = tlbncfg >> TLBnCFG_ASSOC_SHIFT; + assert(r > 0); return r; }