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

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?

[...]

Reviewed-by: Ilya Leoskevich <[email protected]>