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

[Jun Koi]

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);
+    }
+}
+
 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);
     }





On Sat, Apr 17, 2010 at 6:13 AM, Stefan Weil <address@hidden> wrote:
> Jun Koi schrieb:
>> (Thanks to Jan for comments on the last patch)
>>
>> Qemu has a command named singlestep, which reduces the translated code
>> block to be only one instruction.
>> However, there is one flaw when this command is triggered via monitor
>> interface: we do not flush all the current TBs, so we will miss
>> single-step on already translated code.
>> This patch fixes the problem by flushing all the TB to force new code
>> generation.
>>
>> Signed-off-by: Jun Koi <address@hidden>
>>
>>
>>
>> diff --git a/monitor.c b/monitor.c
>> index 5659991..948b861 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1190,8 +1190,14 @@ static void do_log(Monitor *mon, const QDict
>> *qdict)
>> static void do_singlestep(Monitor *mon, const QDict *qdict)
>> {
>> const char *option = qdict_get_try_str(qdict, "option");
>> + CPUState *env;
>> +
>> if (!option || !strcmp(option, "on")) {
>> singlestep = 1;
>> + /* flush all the TBs to force new code generation */
>> + for (env = first_cpu; env != NULL; env = env->next_cpu) {
>> + tb_flush(env);
>> + }
>> } else if (!strcmp(option, "off")) {
>> singlestep = 0;
>> } else {
>
> Hi,
>
> sorry that my feedback comes rather late. I read the discussion,
> but had no time to answer.
>
> I wrote the code for the singlestep command line and monitor option
> (which already existed before as a compile time option) and still use
> it frequently. Up to now, I did not miss the tb flushing, but I see that
> it might be useful in certain cases.
>
> My typical use cases for "singlestep" are
>
> * Compare execution of same code (usually a user mode program or a kernel)
>  running in different environments (32 bit or 64 bit host, big or little
>  endian host, different host architectures).
>  In combination with -D in_asm,cpu it is possible to find wrong tcg code
>  by comparing the resulting qemu.log files.
>
> * Use singlestep to slow down the execution (yes, this is sometimes useful).
>
> In most cases, I use the command line option, but sometimes I use the
> monitor command, too.
>
> There is no logical relation between switching singlestep on or off and
> tb flushing. If there is the need for tb flush, I'd suggest to add a new
> monitor command.
>
> If the translated tbs should match the singlestep setting,
> you would also have to flush them when singlestep is disabled:
> in that case, the translated tbs only contain a single target instruction,
> so they are not very efficient - and they remain so even after
> singlestep was disabled.
>
> So either flush for singlestep on and off, or better add a new monitor
> command
> which flushes tbs.
>
> Regards,
> Stefan
>
>