Re: [Qemu-devel] [Qemu-ppc] [PATCH] target-ppc: fix 32 bit build break in the page table lookup code

[Greg Kurz]

On Thu, 13 Feb 2014 15:53:16 +0100
Alexander Graf <address@hidden> wrote:
> 
> On 13.02.2014, at 04:00, Aneesh Kumar K.V
> <address@hidden> wrote:
> 
> > Greg Kurz <address@hidden> writes:
> > 
> >> The 396bb9874 commit reworked page table lookup to support kvm.
> >> Unfortunately this breaks 32 bit build:
> >> 
> >> target-ppc/mmu-hash64.h: In function ‘ppc_hash64_load_hpte0’:
> >> target-ppc/mmu-hash64.h:90:23: error: cast to pointer from integer of
> >> different size
> >> 
> >> target-ppc/mmu-hash64.h: In function ‘ppc_hash64_load_hpte1’:
> >> target-ppc/mmu-hash64.h:101:23: error: cast to pointer from integer of
> >> different size
> >> 
> >> The problem is that we have two cases to handle here:
> >> - the HTAB is external and it is accessed with a pointer
> >> - the HTAB is emulated and it is accessed with a hwaddr
> >> 
> >> Depending on the way the HTAB is controlled, we should use the
> >> appropriate type:
> >> - controlled by kvm, it is copied to an allocated buffer (pointer)
> >> - controlled by qemu with an allocated buffer (pointer)
> >> - controlled by qemu with soft mmu (hwaddr)
> >> 
> >> This patch introduces an explicit distinction between the two cases in
> >> the new page table lookup code.
> > 
> > Reviewed-by: Aneesh Kumar K.V <address@hidden>
> 
> I wouldn't mind something slightly lighter weight. How about this one
> instead? If you guys think it's better to have an actual type for the
> token I'd pull in this patch as is though.
> 
> 
> Alex
> 

I confess... I consider castings evil and favor explicit typing. :)

On the other hand, I have no strong opinions against your patch. The "token"
code is quite simple and risks of confusion are low, we can live with it.

Please add:

Reviewed-by: Greg Kurz <address@hidden>

Thanks for your time.

--
Greg

> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 4e574d9..3240427 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -340,7 +340,7 @@ uint64_t ppc_hash64_start_access(PowerPCCPU *cpu,
> target_ulong pte_index)
>       * accessible PTEG.
>       */
>      if (cpu->env.external_htab) {
> -        token = (uint64_t) cpu->env.external_htab + pte_offset;
> +        token = (uint64_t)(uintptr_t) cpu->env.external_htab +
> pte_offset; } else if (cpu->env.htab_base) {
>          token = cpu->env.htab_base + pte_offset;
>      }
> diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
> index 9c9ca1d..8fb2ae4 100644
> --- a/target-ppc/mmu-hash64.h
> +++ b/target-ppc/mmu-hash64.h
> @@ -85,22 +85,24 @@ void ppc_hash64_stop_access(uint64_t token);
>  static inline target_ulong ppc_hash64_load_hpte0(CPUPPCState *env,
>                                                   uint64_t token, int
> index) {
> -    index *= HASH_PTE_SIZE_64;
> +    uint64_t addr;
> +    addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
>      if (env->external_htab) {
> -        return  ldq_p((const void *)(token + index));
> +        return  ldq_p((const void *)(uintptr_t)addr);
>      } else {
> -        return ldq_phys(token + index);
> +        return ldq_phys(addr);
>      }
>  }
> 
>  static inline target_ulong ppc_hash64_load_hpte1(CPUPPCState *env,
>                                                   uint64_t token, int
> index) {
> -    index *= HASH_PTE_SIZE_64;
> +    uint64_t addr;
> +    addr = token + (index * HASH_PTE_SIZE_64) + HASH_PTE_SIZE_64/2;
>      if (env->external_htab) {
> -        return  ldq_p((const void *)(token + index +
> HASH_PTE_SIZE_64/2));
> +        return  ldq_p((const void *)(uintptr_t)addr);
>      } else {
> -        return ldq_phys(token + index + HASH_PTE_SIZE_64/2);
> +        return ldq_phys(addr);
>      }
>  }
> 
> 

-- 
Gregory Kurz                                     address@hidden
                                                 address@hidden
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.