| Message ID | 20210207232310.2505283-6-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | exec: Remove "tcg/tcg.h" from "exec/cpu_ldst.h" | expand |
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>
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... > >> #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> >
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> >>
On 2/8/21 5:52 AM, Philippe Mathieu-Daudé wrote: > 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... Certainly you can, though it's not especially pretty: #ifdef CONFIG_DEBUG_TCG void tlb_assert_iotlb_entry_for_ptr_present (CPUArchState *env, int ptr_mmu_idx, uint64_t ptr, MMUAccessType ptr_access, uintptr_t index); #else static inline void tlb_assert_iotlb_entry_for_ptr_present (CPUArchState *env, int ptr_mmu_idx, uint64_t ptr, MMUAccessType ptr_access, uintptr_t index) { } #endif r~
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); #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; }
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. --- 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(-)