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

[Alexander Graf]

On 21.04.2010, at 12:04, Jun Koi wrote:

> On Tue, Apr 20, 2010 at 8:44 PM, Alexander Graf <address@hidden> wrote:
>> 
>> 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.
> 
> So what is the final solution do you want?
> 
> I still think that having flush_all_tb() like in the last patch is good 
> enough.

I agree. And I like the patch as is.

Acked-by: Alexander Graf <address@hidden>


Alex