cputlb: Fix tlb_vaddr_to_host

[PATCH] cputlb: Fix tlb_vaddr_to_host

Posted by Richard Henderson 4 years, 6 months ago

Using uintptr_t instead of target_ulong meant that, for 64-bit guest
and 32-bit host, we truncated the guest address comparator and so may
not hit the tlb when we should.

Fixes: 4811e9095c0
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

Fixes aarch64 emulation on arm32 host, after our recent changes
to tlb probing.


r~

---
 accel/tcg/cputlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 6f4194df96..5eebddcca8 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1189,7 +1189,7 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
                         MMUAccessType access_type, int mmu_idx)
 {
     CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
-    uintptr_t tlb_addr, page;
+    target_ulong tlb_addr, page;
     size_t elt_ofs;
 
     switch (access_type) {
-- 
2.17.1

Re: [PATCH] cputlb: Fix tlb_vaddr_to_host

Posted by David Hildenbrand 4 years, 6 months ago

On 23.10.19 17:49, Richard Henderson wrote:
> Using uintptr_t instead of target_ulong meant that, for 64-bit guest
> and 32-bit host, we truncated the guest address comparator and so may
> not hit the tlb when we should.
> 
> Fixes: 4811e9095c0
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> Fixes aarch64 emulation on arm32 host, after our recent changes
> to tlb probing.
> 
> 
> r~
> 
> ---
>   accel/tcg/cputlb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 6f4194df96..5eebddcca8 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1189,7 +1189,7 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
>                           MMUAccessType access_type, int mmu_idx)
>   {
>       CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
> -    uintptr_t tlb_addr, page;
> +    target_ulong tlb_addr, page;
>       size_t elt_ofs;
>   
>       switch (access_type) {
> 

probe_access() seems to be fine

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [PATCH] cputlb: Fix tlb_vaddr_to_host

Posted by Philippe Mathieu-Daudé 4 years, 6 months ago

On 10/23/19 5:49 PM, Richard Henderson wrote:
> Using uintptr_t instead of target_ulong meant that, for 64-bit guest
> and 32-bit host, we truncated the guest address comparator and so may
> not hit the tlb when we should.
> 
> Fixes: 4811e9095c0
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> Fixes aarch64 emulation on arm32 host, after our recent changes
> to tlb probing.
> 
> 
> r~
> 
> ---
>   accel/tcg/cputlb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 6f4194df96..5eebddcca8 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1189,7 +1189,7 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
>                           MMUAccessType access_type, int mmu_idx)
>   {
>       CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
> -    uintptr_t tlb_addr, page;
> +    target_ulong tlb_addr, page;
>       size_t elt_ofs;
>   
>       switch (access_type) {
> 

Clang catched this with -Wshorten-64-to-32 but there are so many other 
warnings to fix that we can not use it :(

accel/tcg/cputlb.c:620:26: error: implicit conversion loses integer 
precision: 'size_t' (aka 'unsigned long') to 'unsigned int' 
[-Werror,-Wshorten-64-to-32]
         unsigned int n = tlb_n_entries(env, mmu_idx);
                      ~   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
accel/tcg/cputlb.c:788:13: error: implicit conversion loses integer 
precision: 'uintptr_t' (aka 'unsigned long') to 'unsigned int' 
[-Werror,-Wshorten-64-to-32]
     index = tlb_index(env, mmu_idx, vaddr_page);
           ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Maybe we could try to clean accel/tcg/ and use it there at least.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>