Re: [PATCH 1/1] ppc/mmu_helper.c: do not truncate 'ea' in booke206_invalidate_ea_tlb()

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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.
>   */
>