Re: [PATCH v3 18/51] cpu: Use interval-tree for CPUWatchpoint

On 7/14/26 06:02, Ilya Leoshkevich wrote:
> 
> 
> On 7/10/26 22:53, Richard Henderson wrote:
>> Use a balanced binary tree rather than a simple list for watchpoints.
>> Using an interval tree makes it easy to probe for any overlapping address.
>>
>> Signed-off-by: Richard Henderson <[email protected]>
>> ---
>>   include/exec/breakpoint.h |   7 +-
>>   include/hw/core/cpu.h     |   3 +-
>>   accel/tcg/cpu-exec.c      |   8 +-
>>   accel/tcg/watchpoint.c    | 155 +++++++++++++++++++++-----------------
>>   hw/core/cpu-common.c      |   1 -
>>   system/watchpoint.c       |  43 ++++++-----
>>   target/arm/hyp_gdbstub.c  |   3 +-
>>   7 files changed, 119 insertions(+), 101 deletions(-)
>>
>> diff --git a/include/exec/breakpoint.h b/include/exec/breakpoint.h
>> index bb7cbc626d..e0826a1a2d 100644
>> --- a/include/exec/breakpoint.h
>> +++ b/include/exec/breakpoint.h
>> @@ -9,7 +9,6 @@
>>   #define EXEC_BREAKPOINT_H
>>   #include "qemu/interval-tree.h"
>> -#include "qemu/queue.h"
>>   #include "exec/vaddr.h"
>>   #include "exec/memattrs.h"
>> @@ -36,13 +35,11 @@ struct CPUBreakpoint {
>>   };
>>   struct CPUWatchpoint {
>> -    vaddr vaddr;
>> -    vaddr len;
>> +    IntervalTreeNode itree;
>>       vaddr hitaddr;
>>       MemTxAttrs hitattrs;
>> -    int flags; /* BP_* */
>> +    BreakpointFlags flags;
> 
> Should this go into 1/51?
> 
>>       unsigned id;
>> -    QTAILQ_ENTRY(CPUWatchpoint) entry;
>>   };
>>   int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, BreakpointFlags flags,
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index b8a1419860..7e19ea418d 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -525,8 +525,7 @@ struct CPUState {
>>       /* ice debug support */
>>       IntervalTreeRoot breakpoints;
>> -
>> -    QTAILQ_HEAD(, CPUWatchpoint) watchpoints;
>> +    IntervalTreeRoot watchpoints;
>>       CPUWatchpoint *watchpoint_hit;
>>       void *opaque;
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index 2762cf6705..ab299387e3 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -682,10 +682,12 @@ static inline bool cpu_handle_halt(CPUState *cpu)
>>   static inline void cpu_handle_debug_exception(CPUState *cpu)
>>   {
>>       const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops;
>> -    CPUWatchpoint *wp;
>> +    IntervalTreeNode *n;
>> -    if (!cpu->watchpoint_hit) {
> 
> Isn't it reasonable to continue checking for this, or can it never be
> NULL if we come here?

Intermediate steps.

Before this patch we walk the entire watchpoint list performing this same clearing in 
cpu_check_watchpoint.  With this patch, we no longer walk every element in 
cpu_check_watchpoint, but have not yet eliminated some manual checks for BP_WATCHPOINT_HIT 
in the target code.  By the end of the patch series, this loop is gone completely.

Helpful if I add a /* Will be cleaned up */ comment at this point in the series?


r~