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

[Glauber Costa]

On Tue, Oct 14, 2008 at 3:45 PM, Jan Kiszka <address@hidden> wrote:
> 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.

fair.

>
>> 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...
Actually we don't need a conversion. We just need an implementation,
and the conversion
happens through time, as old code gets replaced. But we still need the
$SOMEONE, and it's not
exactly my priority right now.

>
> Jan
>
>



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."