Re: [PATCH 11/24] exec: Move tlb_reset_dirty*() declarations to 'exec/cputlb.h'

[Anton Johansson]

On 18/04/24, Philippe Mathieu-Daudé wrote:
> Declare tlb_reset_dirty() and tlb_reset_dirty_range_all()
> in "exec/cputlb.h". Restrict tlb_reset_dirty_range_all()
> to TCG accel.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/exec/cputlb.h   | 12 +++++++++++-
>  include/exec/exec-all.h |  3 ---
>  include/exec/ram_addr.h |  1 +
>  system/physmem.c        |  2 ++
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h
> index 3594f904b4..dc92befb93 100644
> --- a/include/exec/cputlb.h
> +++ b/include/exec/cputlb.h
> @@ -49,6 +49,9 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
>  void tlb_protect_code(ram_addr_t ram_addr);
>  void tlb_unprotect_code(ram_addr_t ram_addr);
>  
> +void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length);
> +void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length);
> +
>  /**
>   * iotlb_to_section:
>   * @cpu: CPU performing the access
> @@ -69,6 +72,13 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,
>  
>  #endif /* CONFIG_USER_ONLY */
>  
> -#endif /* CONFIG_TCG */
> +#else /* !CONFIG_TCG */
> +
> +static inline void tlb_reset_dirty_range_all(ram_addr_t start,
> +                                             ram_addr_t length)
> +{
> +}
> +
> +#endif /* !CONFIG_TCG */

The only use of tlb_reset_dirty_range_all() is in
cpu_physical_memory_dirty_bits_cleared() surrounded by an
if (tcg_enabled()).  Would stubbing the latter function be better?

>  
>  #endif
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 778c82ba8e..6f46015ab4 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -640,9 +640,6 @@ static inline void mmap_lock(void) {}
>  static inline void mmap_unlock(void) {}
>  #define WITH_MMAP_LOCK_GUARD()
>  
> -void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length);
> -void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length);
> -
>  #endif
>  
>  #endif
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 3fc83587c0..f06ae9b516 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -26,6 +26,7 @@
>  #include "exec/ramlist.h"
>  #include "exec/ramblock.h"
>  #include "exec/exec-all.h"
> +#include "cputlb.h"
>  #include "qemu/rcu.h"
>  
>  extern uint64_t total_dirty_pages;
> diff --git a/system/physmem.c b/system/physmem.c
> index 38d3ede9f9..7a7876a375 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -850,6 +850,7 @@ found:
>      return block;
>  }
>  
> +#ifdef CONFIG_TCG
>  void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
>  {
>      CPUState *cpu;
> @@ -869,6 +870,7 @@ void tlb_reset_dirty_range_all(ram_addr_t start, 
> ram_addr_t length)
>          tlb_reset_dirty(cpu, start1, length);
>      }
>  }
> +#endif

This function only depends on qemu_get_ramblock which is statically
defined in physmem.c although it doesnt depend on anything in physmem.c
as far as I can tell. Thoughts on moving qemu_get_ramblock to a common
.c or .h and moving the tlb_reset_dirty_range_all definition to
cputlb.c?