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

[Stefan Weil]

Jan Kiszka schrieb:
> Alexander Graf wrote:
>   
>> 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>
>>
>>     
>
> Sorry, nack for keeping this service in /monitor.c/. But a bonus ack if
> you avoid the needless loop when moving it to exec.c, adding a comment
> that current tb_flush has global, env-invariant scope.
>
> Thanks,
> Jan

flush_all_tb() is now called for singlestep on and off, that's fine.
But it's called always - no way to disable this call. That's not good.
Sometimes I don't want to flush all TBs when I switch singlestep mode
(that's the reason why I suggested a separate monitor command which
flushes all TBs - I still think that would be the best solution).

What about this syntax for the singlestep monitor command:

singlestep [on|off][,flush]
        Run the emulation in single step mode. In that mode, QEMU uses
        one translation block per target CPU instruction.
        If called with option off, the emulation returns to normal mode.
        If called with the optional parameter flush, existing translation
        blocks are flushed.

Or, if you prefer to flush by default:

singlestep [on|off][,noflush]
        Run the emulation in single step mode. In that mode, QEMU uses
        one translation block per instructions.
        ...

Please update qemu-monitor.hx, too (that should be done in any case).

Regards,
Stefan