Re: [RFC PATCH v2 5/6] accel/tcg: Refactor debugging tlb_assert_iotlb_entry_for_ptr_present()

["Alex Bennée" <alex.bennee@linaro.org>]

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

> Refactor debug code as tlb_assert_iotlb_entry_for_ptr_present() helper.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> What this code does is out of my league, but refactoring it allow
> keeping tlb_addr_write() local to accel/tcg/cputlb.c in the next
> patch.

The assertion that the table entry is current is just a simple
housekeeping one. The details of how the MTE implementation uses
(abuses?) the iotlb entries requires a closer reading of the code.

> ---
>  include/exec/exec-all.h |  9 +++++++++
>  accel/tcg/cputlb.c      | 14 ++++++++++++++
>  target/arm/mte_helper.c | 11 ++---------
>  target/arm/sve_helper.c | 10 ++--------
>  4 files changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index f933c74c446..c5e8e355b7f 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -296,6 +296,15 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
>  void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>                    hwaddr paddr, int prot,
>                    int mmu_idx, target_ulong size);
> +
> +/*
> + * Find the iotlbentry for ptr.  This *must* be present in the TLB
> + * because we just found the mapping.
> + */
> +void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx,
> +                                            uint64_t ptr,
> +                                            MMUAccessType ptr_access,
> +                                            uintptr_t index);

Probably worth making this an empty inline for the non CONFIG_DEBUG_TCG
case so we can eliminate the call to an empty function.

>  #else
>  static inline void tlb_init(CPUState *cpu)
>  {
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 8a7b779270a..a6247da34a0 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -429,6 +429,20 @@ void tlb_flush_all_cpus_synced(CPUState *src_cpu)
>      tlb_flush_by_mmuidx_all_cpus_synced(src_cpu, ALL_MMUIDX_BITS);
>  }
>  
> +void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx,
> +                                            uint64_t ptr,
> +                                            MMUAccessType ptr_access,
> +                                            uintptr_t index)
> +{
> +#ifdef CONFIG_DEBUG_TCG
> +    CPUTLBEntry *entry = tlb_entry(env, ptr_mmu_idx, ptr);
> +    target_ulong comparator = (ptr_access == MMU_DATA_LOAD
> +                               ? entry->addr_read
> +                               : tlb_addr_write(entry));
> +    g_assert(tlb_hit(comparator, ptr));
> +#endif
> +}
> +
>  static bool tlb_hit_page_mask_anyprot(CPUTLBEntry *tlb_entry,
>                                        target_ulong page, target_ulong mask)
>  {
> diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
> index 6cea9d1b506..f47d3b4570e 100644
> --- a/target/arm/mte_helper.c
> +++ b/target/arm/mte_helper.c
> @@ -111,15 +111,8 @@ static uint8_t *allocation_tag_mem(CPUARMState *env, int ptr_mmu_idx,
>       * matching tlb entry + iotlb entry.
>       */
>      index = tlb_index(env, ptr_mmu_idx, ptr);
> -# ifdef CONFIG_DEBUG_TCG
> -    {
> -        CPUTLBEntry *entry = tlb_entry(env, ptr_mmu_idx, ptr);
> -        target_ulong comparator = (ptr_access == MMU_DATA_LOAD
> -                                   ? entry->addr_read
> -                                   : tlb_addr_write(entry));
> -        g_assert(tlb_hit(comparator, ptr));
> -    }
> -# endif
> +    tlb_assert_iotlb_entry_for_ptr_present(env, ptr_mmu_idx, ptr,
> +                                           ptr_access, index);
>      iotlbentry = &env_tlb(env)->d[ptr_mmu_idx].iotlb[index];
>  
>      /* If the virtual page MemAttr != Tagged, access unchecked. */
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index c8cdf7618eb..a5708da0f2f 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -4089,14 +4089,8 @@ static bool sve_probe_page(SVEHostPage *info, bool nofault,
>      {
>          uintptr_t index = tlb_index(env, mmu_idx, addr);
>  
> -# ifdef CONFIG_DEBUG_TCG
> -        CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
> -        target_ulong comparator = (access_type == MMU_DATA_LOAD
> -                                   ? entry->addr_read
> -                                   : tlb_addr_write(entry));
> -        g_assert(tlb_hit(comparator, addr));
> -# endif
> -
> +        tlb_assert_iotlb_entry_for_ptr_present(env, mmu_idx, addr,
> +                                               access_type, index);
>          CPUIOTLBEntry *iotlbentry = &env_tlb(env)->d[mmu_idx].iotlb[index];
>          info->attrs = iotlbentry->attrs;
>      }

with the inline fix:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée