Re: [PATCH 08/25] accel/tcg: Rename @start argument of tlb_reset_dirty*()

[Richard Henderson <richard.henderson@linaro.org>]

On 10/1/25 01:21, Philippe Mathieu-Daudé wrote:
> Generally we want to clarify terminology and avoid confusions,
> prefering @start with (exclusive) @end, and base @addr with
> @length (for inclusive range).
> 
> Here as tlb_reset_dirty() and tlb_reset_dirty_range_all()
> operate on a range, rename @start as @addr.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/cputlb.h | 4 ++--
>   accel/tcg/cputlb.c    | 6 +++---
>   system/physmem.c      | 8 ++++----
>   3 files changed, 9 insertions(+), 9 deletions(-)

Eh.  I suppose.

What we *really* want is to abolish both end and length.  They both lead to the silliness 
that is system/memory.h using Int128 for ranges.  Both end and length allow an empty 
range, which we don't actually need, but do not allow a range covering the entire 2**64.

We should be using first/last, where first==last is a 1 byte inclusive range, and first==0 
&& last==UINT64_MAX is the entire 2**64.

I'm not sure where you're going with this other than swapping names.


r~

> 
> diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h
> index db7cbf97826..995fe31ef75 100644
> --- a/include/exec/cputlb.h
> +++ b/include/exec/cputlb.h
> @@ -32,8 +32,8 @@ void tlb_unprotect_code(ram_addr_t ram_addr);
>   
>   #ifndef CONFIG_USER_ONLY
>   /* Called with rcu_read_lock held. */
> -void tlb_reset_dirty(CPUState *cpu, uintptr_t start, uintptr_t length);
> -void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length);
> +void tlb_reset_dirty(CPUState *cpu, uintptr_t addr, uintptr_t length);
> +void tlb_reset_dirty_range_all(ram_addr_t addr, ram_addr_t length);
>   #endif
>   
>   /**
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 2a6aa01c57c..6807328df82 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -916,7 +916,7 @@ static inline void copy_tlb_helper_locked(CPUTLBEntry *d, const CPUTLBEntry *s)
>    * We must take tlb_c.lock to avoid racing with another vCPU update. The only
>    * thing actually updated is the target TLB entry ->addr_write flags.
>    */
> -void tlb_reset_dirty(CPUState *cpu, uintptr_t start, uintptr_t length)
> +void tlb_reset_dirty(CPUState *cpu, uintptr_t addr, uintptr_t length)
>   {
>       int mmu_idx;
>   
> @@ -929,12 +929,12 @@ void tlb_reset_dirty(CPUState *cpu, uintptr_t start, uintptr_t length)
>   
>           for (i = 0; i < n; i++) {
>               tlb_reset_dirty_range_locked(&desc->fulltlb[i], &fast->table[i],
> -                                         start, length);
> +                                         addr, length);
>           }
>   
>           for (i = 0; i < CPU_VTLB_SIZE; i++) {
>               tlb_reset_dirty_range_locked(&desc->vfulltlb[i], &desc->vtable[i],
> -                                         start, length);
> +                                         addr, length);
>           }
>       }
>       qemu_spin_unlock(&cpu->neg.tlb.c.lock);
> diff --git a/system/physmem.c b/system/physmem.c
> index 000bde90c2e..098824ad1d2 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -882,16 +882,16 @@ found:
>       return block;
>   }
>   
> -void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
> +void tlb_reset_dirty_range_all(ram_addr_t addr, ram_addr_t length)
>   {
>       CPUState *cpu;
> -    ram_addr_t start1;
> +    ram_addr_t start, start1;
>       RAMBlock *block;
>       ram_addr_t end;
>   
>       assert(tcg_enabled());
> -    end = TARGET_PAGE_ALIGN(start + length);
> -    start &= TARGET_PAGE_MASK;
> +    end = TARGET_PAGE_ALIGN(addr + length);
> +    start = addr & TARGET_PAGE_MASK;
>   
>       RCU_READ_LOCK_GUARD();
>       block = qemu_get_ram_block(start);