[Qemu-devel] Re: [PATCH 02/13] Refactor and enhance break/watchpoint API

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2694 bytes --]

Glauber Costa wrote:
>> Index: b/exec.c
>> ===================================================================
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -537,7 +537,6 @@ void cpu_exec_init(CPUState *env)
>>         cpu_index++;
>>     }
>>     env->cpu_index = cpu_index;
>> -    env->nb_watchpoints = 0;
>>     *penv = env;
>>  #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
>>     register_savevm("cpu_common", cpu_index, CPU_COMMON_SAVE_VERSION,
>> @@ -1311,107 +1310,150 @@ static void breakpoint_invalidate(CPUSta
>>  #endif
>>
>>  /* Add a watchpoint.  */
>> -int cpu_watchpoint_insert(CPUState *env, target_ulong addr, int type)
>> +int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
>> +                          int flags, CPUWatchpoint **watchpoint)
>>  {
>> -    int i;
>> +    CPUWatchpoint *wp;
>>
>> -    for (i = 0; i < env->nb_watchpoints; i++) {
>> -        if (addr == env->watchpoint[i].vaddr)
>> -            return 0;
>> -    }
>> -    if (env->nb_watchpoints >= MAX_WATCHPOINTS)
>> -        return -1;
>> +    wp = qemu_malloc(sizeof(*wp));
>> +    if (!wp)
>> +        return -ENOBUFS;
>> +
>> +    wp->vaddr = addr;
>> +    wp->len = len;
>> +    wp->flags = flags;
>> +
>> +    wp->next = env->watchpoints;
>> +    wp->prev = NULL;
>> +    if (wp->next)
>> +        wp->next->prev = wp;
>> +    env->watchpoints = wp;
>>
>> -    i = env->nb_watchpoints++;
>> -    env->watchpoint[i].vaddr = addr;
>> -    env->watchpoint[i].type = type;
>>     tlb_flush_page(env, addr);
>>     /* FIXME: This flush is needed because of the hack to make memory ops
>>        terminate the TB.  It can be removed once the proper IO trap and
>>        re-execute bits are in.  */
>>     tb_flush(env);
> 
>> Index: b/cpu-defs.h
>> +typedef struct CPUBreakpoint {
>> +    target_ulong pc;
>> +    int flags; /* BP_* */
>> +    struct CPUBreakpoint *prev, *next;
>> +} CPUBreakpoint;
>> +
>> +typedef struct CPUWatchpoint {
>> +    target_ulong vaddr;
>> +    target_ulong len;
>> +    int flags; /* BP_* */
>> +    struct CPUWatchpoint *prev, *next;
>> +} CPUWatchpoint;
>> +
> 
> Most of the time, you are transversing the list in a single direction.
> So any particular reason to use a double linked list?

When looking as this patch only, one may get along with a singly-linked
list. But patch 13 adds a use case where the back-reference pays off.

> By the way, /me thinks it is about time for us to have a generic
> linked list implementation

Probably - but $SOMEONE will have to do the time-consuming conversion
work to make QEMU really benefit from this...

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]