* Re: [PATCH v2 1/1] ppc/mmu_helper.c: do not truncate 'ea' in booke206_invalidate_ea_tlb()
2021-11-10 20:25 ` [PATCH v2 1/1] " Daniel Henrique Barboza
@ 2021-11-10 20:27 ` Daniel Henrique Barboza
2021-11-10 21:00 ` Philippe Mathieu-Daudé
2021-11-11 10:34 ` Cédric Le Goater
2 siblings, 0 replies; 5+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-10 20:27 UTC (permalink / raw)
To: qemu-devel; +Cc: f4bug, qemu-ppc, clg, Alexander Graf, david
I forgot to add Alexander in the CC. Sorry for the extra noise.
Daniel
On 11/10/21 17:25, 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 'vaddr'.
>
> [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..e0c4950dda 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)
> + vaddr ea)
> {
> int i;
> int ways = booke206_tlb_ways(env, tlbn);
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/1] ppc/mmu_helper.c: do not truncate 'ea' in booke206_invalidate_ea_tlb()
2021-11-10 20:25 ` [PATCH v2 1/1] " Daniel Henrique Barboza
2021-11-10 20:27 ` Daniel Henrique Barboza
@ 2021-11-10 21:00 ` Philippe Mathieu-Daudé
2021-11-11 10:34 ` Cédric Le Goater
2 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-10 21:00 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Cédric Le Goater, qemu-ppc@nongnu.org list:PowerPC,
qemu-devel@nongnu.org Developers, David Gibson
On Wed, Nov 10, 2021 at 9:25 PM Daniel Henrique Barboza
<danielhb413@gmail.com> 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 'vaddr'.
>
> [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(-)
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/1] ppc/mmu_helper.c: do not truncate 'ea' in booke206_invalidate_ea_tlb()
2021-11-10 20:25 ` [PATCH v2 1/1] " Daniel Henrique Barboza
2021-11-10 20:27 ` Daniel Henrique Barboza
2021-11-10 21:00 ` Philippe Mathieu-Daudé
@ 2021-11-11 10:34 ` Cédric Le Goater
2 siblings, 0 replies; 5+ messages in thread
From: Cédric Le Goater @ 2021-11-11 10:34 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel; +Cc: qemu-ppc, f4bug, david
On 11/10/21 21:25, 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 'vaddr'.
>
> [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(-)
It looks correct. I am not sure why we introduced PPC_TLBIVAX since
we don't use it for the instruction. But e200 needs it AFAICT.
Any how,
Queued for 6.2
Thanks,
C.
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 2cb98c5169..e0c4950dda 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)
> + vaddr ea)
> {
> int i;
> int ways = booke206_tlb_ways(env, tlbn);
>
^ permalink raw reply [flat|nested] 5+ messages in thread