On Tue, Apr 20, 2010 at 8:44 PM, Alexander Graf <ag...@suse.de> 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 <junkoi2...@gmail.com>
>>>>>
>>>>>
>>>>> 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.
thanks,
J
