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

[Jun Koi]

On Wed, Apr 28, 2010 at 4:55 AM, Stefan Weil <address@hidden> wrote:
> Am 22.04.2010 09:02, schrieb Jan Kiszka:
>>
>> Stefan Weil wrote:
>>>
>>> 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).
>>
>> Mind to tell us the use case?
>
> Typical use case: execution trace of some code which is
> run after OS boot with an explicit trigger.
>
> This can be loading of a linux kernel module, a user space
> application or kernel code which handles a rare event.
>
> I can enable logging and single stepping before that code
> starts. There is no need to re-translate existing TBs:
> they are faster than TBs with only single steps, so only
> the execution of the new code is slow, and only new TBs
> will appear in qemu.log which is exactly what I want.
>
> Typically, I use single stepping like this to examine a
> problem with QEMU's emulation or code generation. Two examples:
> some years ago aptitude crashed in mips emulation (fpu emulation
> problem), and now I use it to examine differences between
> native TCG and TCI (tiny code interpreter).

What is that TCI???

Thanks,
J