Re: [Qemu-devel] Re: [PATCH] flush TB on singlestep command

[Alexander Graf]

On 20.04.2010, at 13:38, Jan Kiszka wrote:

> Alexander Graf wrote:
>> On 20.04.2010, at 09:18, Jan Kiszka wrote:
>> 
>>> Jun Koi wrote:
>>>> Thank you for the explanation of this code.
>>>> 
>>>> Qemu has a command named singlestep, which reduces the translated code
>>>> block to be only one instruction.
>>>> This new patch flushes TBs both when singlestep is on and off.
>>>> 
>>>> Signed-off-by: Jun Koi <address@hidden>
>>>> 
>>>> 
>>>> diff --git a/monitor.c b/monitor.c
>>>> index 5659991..2b2005b 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -1187,13 +1187,26 @@ static void do_log(Monitor *mon, const QDict 
>>>> *qdict)
>>>>    cpu_set_log(mask);
>>>> }
>>>> 
>>>> +/* flush all the TBs to force new code generation */
>>>> +static void flush_all_tb(void)
>>>> +{
>>>> +    CPUState *env;
>>>> +
>>>> +    for (env = first_cpu; env != NULL; env = env->next_cpu) {
>>>> +        tb_flush(env);
>>>> +    }
>>>> +}
>>>> +
>>> The smaller your patch are, the more people pick on it. :)
>>> 
>>> I was about to suggest moving this close to tb_flush, but then I
>>> realized that the env argument of that service is misleading. In fact,
>>> it already flushes the one and only translation buffer pool.
>>> 
>>>> static void do_singlestep(Monitor *mon, const QDict *qdict)
>>>> {
>>>>    const char *option = qdict_get_try_str(qdict, "option");
>>>> +
>>>>    if (!option || !strcmp(option, "on")) {
>>>>        singlestep = 1;
>>>> +        flush_all_tb();
>>>>    } else if (!strcmp(option, "off")) {
>>>>        singlestep = 0;
>>>> +        flush_all_tb();
>>>>    } else {
>>>>        monitor_printf(mon, "unexpected option %s\n", option);
>>>>    }
>>>> 
>>> Let's just pass mon->mon_cpu to tb_flush and skip the redundant loop.
>> 
>> That doesn't help, no? singlestep is a global variable. Flushing only the 
>> current vcpu would still not affect the others, while the singlestep switch 
>> would.
> 
> tb_flush uses env only to dump some state when a problem occurred.
> 
>> 
>> According to your above comment the cache is global, but I don't think we 
>> should rely on that.
> 
> It might make sense to define some tb_flush_all() as tb_flush(first_cpu)
> for now to establish the infrastructure. Then we are prepared for the
> day the tb_flush implementation may change.

Right. But then the call to tb_flush_all here is still correct.

Alex