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

[Alex Bennée]

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

> On 2/8/21 9:42 AM, Alex Bennée wrote:
>> 
>> 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.
>
> But then we can't make tlb_addr_write() static (next patch) and
> we still have to include "tcg/tcg.h" for the TCG_OVERSIZED_GUEST
> definition...

Hmm - yeah. I'm not keen on turning something into a function call when
the compiler should have all the information it needs with it. On the
other hand maybe we don't care for a debug assert.

Richard WDYT?

>
>> 
>>>  #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