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

On 7/14/26 16:55, Richard Henderson wrote:
> 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~

I think it's fine as is, I just haven't read the series until the
end yet. Thanks for the explanation.