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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3176 bytes --]

Alexander Graf wrote:
> On 21.04.2010, at 12:04, Jun Koi wrote:
> 
>> On Tue, Apr 20, 2010 at 8:44 PM, Alexander Graf <agraf@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 <junkoi2004@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.
> 
> I agree. And I like the patch as is.
> 
> Acked-by: Alexander Graf <agraf@suse.de>
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]