| Message ID | 20211110184545.1981500-1-danielhb413@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] ppc/mmu_helper.c: do not truncate 'ea' in booke206_invalidate_ea_tlb() | expand |
+Alexander On 11/10/21 19:45, Daniel Henrique Barboza wrote: > 'tlbivax' is implemented by gen_tlbivax_booke206() via > gen_helper_booke206_tlbivax(). In case the TLB needs to be flushed, > booke206_invalidate_ea_tlb() is called. All these functions, but > booke206_invalidate_ea_tlb(), uses a 64-bit effective address 'ea'. > > booke206_invalidate_ea_tlb() uses an uint32_t 'ea' argument that > truncates the original 'ea' value for apparently no particular reason. > This function retrieves the tlb pointer by calling booke206_get_tlbm(), > which also uses a target_ulong address as parameter - in this case, a > truncated 'ea' address. All the surrounding logic considers the > effective TLB address as a 64 bit value, aside from the signature of > booke206_invalidate_ea_tlb(). > > Last but not the least, PowerISA 2.07B section 6.11.4.9 [2] makes it > clear that the effective address "EA" is a 64 bit value. > > Commit 01662f3e5133 introduced this code and no changes were made ever > since. An user detected a problem with tlbivax [1] stating that this > address truncation was the cause. This same behavior might be the source > of several subtle bugs that were never caught. > > For all these reasons, this patch assumes that this address truncation > is the result of a mistake/oversight of the original commit, and changes > booke206_invalidate_ea_tlb() 'ea' argument to target_ulong. > > [1] https://gitlab.com/qemu-project/qemu/-/issues/52 > [2] https://wiki.raptorcs.com/wiki/File:PowerISA_V2.07B.pdf > > Fixes: 01662f3e5133 ("PPC: Implement e500 (FSL) MMU") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/52 > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > target/ppc/mmu_helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c > index 2cb98c5169..21cdae4c6d 100644 > --- a/target/ppc/mmu_helper.c > +++ b/target/ppc/mmu_helper.c > @@ -1216,7 +1216,7 @@ void helper_booke206_tlbsx(CPUPPCState *env, target_ulong address) > } > > static inline void booke206_invalidate_ea_tlb(CPUPPCState *env, int tlbn, > - uint32_t ea) > + target_ulong ea) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> But I wonder if vaddr is not more appropriated here, see "hw/core/cpu.h": /** * vaddr: * Type wide enough to contain any #target_ulong virtual address. */
On 11/10/21 15:52, Philippe Mathieu-Daudé wrote: > +Alexander > > On 11/10/21 19:45, Daniel Henrique Barboza wrote: >> 'tlbivax' is implemented by gen_tlbivax_booke206() via >> gen_helper_booke206_tlbivax(). In case the TLB needs to be flushed, >> booke206_invalidate_ea_tlb() is called. All these functions, but >> booke206_invalidate_ea_tlb(), uses a 64-bit effective address 'ea'. >> >> booke206_invalidate_ea_tlb() uses an uint32_t 'ea' argument that >> truncates the original 'ea' value for apparently no particular reason. >> This function retrieves the tlb pointer by calling booke206_get_tlbm(), >> which also uses a target_ulong address as parameter - in this case, a >> truncated 'ea' address. All the surrounding logic considers the >> effective TLB address as a 64 bit value, aside from the signature of >> booke206_invalidate_ea_tlb(). >> >> Last but not the least, PowerISA 2.07B section 6.11.4.9 [2] makes it >> clear that the effective address "EA" is a 64 bit value. >> >> Commit 01662f3e5133 introduced this code and no changes were made ever >> since. An user detected a problem with tlbivax [1] stating that this >> address truncation was the cause. This same behavior might be the source >> of several subtle bugs that were never caught. >> >> For all these reasons, this patch assumes that this address truncation >> is the result of a mistake/oversight of the original commit, and changes >> booke206_invalidate_ea_tlb() 'ea' argument to target_ulong. >> >> [1] https://gitlab.com/qemu-project/qemu/-/issues/52 >> [2] https://wiki.raptorcs.com/wiki/File:PowerISA_V2.07B.pdf >> >> Fixes: 01662f3e5133 ("PPC: Implement e500 (FSL) MMU") >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/52 >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> target/ppc/mmu_helper.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c >> index 2cb98c5169..21cdae4c6d 100644 >> --- a/target/ppc/mmu_helper.c >> +++ b/target/ppc/mmu_helper.c >> @@ -1216,7 +1216,7 @@ void helper_booke206_tlbsx(CPUPPCState *env, target_ulong address) >> } >> >> static inline void booke206_invalidate_ea_tlb(CPUPPCState *env, int tlbn, >> - uint32_t ea) >> + target_ulong ea) > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > But I wonder if vaddr is not more appropriated here, > see "hw/core/cpu.h": Just saw other mmu related code in target/ppc that uses 'vaddr' in this context, so I believe it is appropriate to use it here. I'll send a v2. Thanks, Daniel > > /** > * vaddr: > * Type wide enough to contain any #target_ulong virtual address. > */ >
diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c index 2cb98c5169..21cdae4c6d 100644 --- a/target/ppc/mmu_helper.c +++ b/target/ppc/mmu_helper.c @@ -1216,7 +1216,7 @@ void helper_booke206_tlbsx(CPUPPCState *env, target_ulong address) } static inline void booke206_invalidate_ea_tlb(CPUPPCState *env, int tlbn, - uint32_t ea) + target_ulong ea) { int i; int ways = booke206_tlb_ways(env, tlbn);
'tlbivax' is implemented by gen_tlbivax_booke206() via gen_helper_booke206_tlbivax(). In case the TLB needs to be flushed, booke206_invalidate_ea_tlb() is called. All these functions, but booke206_invalidate_ea_tlb(), uses a 64-bit effective address 'ea'. booke206_invalidate_ea_tlb() uses an uint32_t 'ea' argument that truncates the original 'ea' value for apparently no particular reason. This function retrieves the tlb pointer by calling booke206_get_tlbm(), which also uses a target_ulong address as parameter - in this case, a truncated 'ea' address. All the surrounding logic considers the effective TLB address as a 64 bit value, aside from the signature of booke206_invalidate_ea_tlb(). Last but not the least, PowerISA 2.07B section 6.11.4.9 [2] makes it clear that the effective address "EA" is a 64 bit value. Commit 01662f3e5133 introduced this code and no changes were made ever since. An user detected a problem with tlbivax [1] stating that this address truncation was the cause. This same behavior might be the source of several subtle bugs that were never caught. For all these reasons, this patch assumes that this address truncation is the result of a mistake/oversight of the original commit, and changes booke206_invalidate_ea_tlb() 'ea' argument to target_ulong. [1] https://gitlab.com/qemu-project/qemu/-/issues/52 [2] https://wiki.raptorcs.com/wiki/File:PowerISA_V2.07B.pdf Fixes: 01662f3e5133 ("PPC: Implement e500 (FSL) MMU") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/52 Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- target/ppc/mmu_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)