Re: [PATCH v2 6/6] exec/cpu_ldst: Move tlb* declarations to "exec/exec-all.h"

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Keep MMU functions in "exec/cpu_ldst.h", and move TLB functions
> to "exec/exec-all.h". As tlb_addr_write() is only called in
> accel/tcg/cputlb.c, make move it there as a static function.
>
> Doing so we removed the "tcg/tcg.h" dependency on "exec/cpu_ldst.h".
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/exec/cpu_ldst.h | 28 ----------------------------
>  include/exec/exec-all.h | 16 ++++++++++++++++
>  accel/tcg/cputlb.c      |  9 +++++++++
>  3 files changed, 25 insertions(+), 28 deletions(-)
>
> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
> index ef54cb7e1f8..c1753a64dfd 100644
> --- a/include/exec/cpu_ldst.h
> +++ b/include/exec/cpu_ldst.h
> @@ -291,34 +291,6 @@ static inline void cpu_stq_le_mmuidx_ra(CPUArchState 
> *env, abi_ptr addr,
>  
>  #else
>  
> -/* Needed for TCG_OVERSIZED_GUEST */
> -#include "tcg/tcg.h"
> -
> -static inline target_ulong tlb_addr_write(const CPUTLBEntry *entry)
> -{
> -#if TCG_OVERSIZED_GUEST
> -    return entry->addr_write;
> -#else
> -    return qatomic_read(&entry->addr_write);
> -#endif
> -}
> -
> -/* Find the TLB index corresponding to the mmu_idx + address pair.  */
> -static inline uintptr_t tlb_index(CPUArchState *env, uintptr_t mmu_idx,
> -                                  target_ulong addr)
> -{
> -    uintptr_t size_mask = env_tlb(env)->f[mmu_idx].mask >> 
> CPU_TLB_ENTRY_BITS;
> -
> -    return (addr >> TARGET_PAGE_BITS) & size_mask;
> -}
> -
> -/* Find the TLB entry corresponding to the mmu_idx + address pair.  */
> -static inline CPUTLBEntry *tlb_entry(CPUArchState *env, uintptr_t mmu_idx,
> -                                     target_ulong addr)
> -{
> -    return &env_tlb(env)->f[mmu_idx].table[tlb_index(env, mmu_idx, addr)];
> -}
> -
>  uint32_t cpu_ldub_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>                              int mmu_idx, uintptr_t ra);
>  int cpu_ldsb_mmuidx_ra(CPUArchState *env, abi_ptr addr,
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index c5e8e355b7f..8e54b537189 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -297,6 +297,22 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>                    hwaddr paddr, int prot,
>                    int mmu_idx, target_ulong size);
>  
> +/* Find the TLB index corresponding to the mmu_idx + address pair.  */
> +static inline uintptr_t tlb_index(CPUArchState *env, uintptr_t mmu_idx,
> +                                  target_ulong addr)
> +{
> +    uintptr_t size_mask = env_tlb(env)->f[mmu_idx].mask >> 
> CPU_TLB_ENTRY_BITS;
> +
> +    return (addr >> TARGET_PAGE_BITS) & size_mask;
> +}
> +
> +/* Find the TLB entry corresponding to the mmu_idx + address pair.  */
> +static inline CPUTLBEntry *tlb_entry(CPUArchState *env, uintptr_t mmu_idx,
> +                                     target_ulong addr)
> +{
> +    return &env_tlb(env)->f[mmu_idx].table[tlb_index(env, mmu_idx, addr)];
> +}
> +

I wonder if throwing these into exec-all is worth it, could we not use
the cputlb.h so we avoid too much kitchen sink for a header (after all
we are trying to avoid recompilations and not everything needs detailed
access to the tlb structures).

>  /*
>   * Find the iotlbentry for ptr.  This *must* be present in the TLB
>   * because we just found the mapping.
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index a6247da34a0..084d19b52d7 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -429,6 +429,15 @@ void tlb_flush_all_cpus_synced(CPUState *src_cpu)
>      tlb_flush_by_mmuidx_all_cpus_synced(src_cpu, ALL_MMUIDX_BITS);
>  }
>  
> +static inline target_ulong tlb_addr_write(const CPUTLBEntry *entry)
> +{
> +#if TCG_OVERSIZED_GUEST
> +    return entry->addr_write;
> +#else
> +    return qatomic_read(&entry->addr_write);
> +#endif
> +}

You can drop the inline, compiler should know best what to do in this case.

>  void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int 
> ptr_mmu_idx,
>                                              uint64_t ptr,
>                                              MMUAccessType ptr_access,


-- 
Alex Bennée