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

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

[Jun Koi]

(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 <junkoi2004@gmail.com>



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 {

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

[Stefan Weil]

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 <junkoi2004@gmail.com>
>
>
>
> 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

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 <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);
+    }
+}
+
 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 <weil@mail.berlios.de> 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 <junkoi2004@gmail.com>
>>
>>
>>
>> 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
>
>

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

[Jan Kiszka]

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

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.

Jan


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

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

[Alexander Graf]

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.

According to your above comment the cache is global, but I don't think we should rely on that.


Alex

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

[Jan Kiszka]

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

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.

Jan


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

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

[Alexander Graf]

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.

Alex

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

[Jun Koi]

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.

thanks,
J

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

[Alexander Graf]

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>


Alex

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

[Jan Kiszka]

[-- 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 --]

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 <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

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

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

[Jan Kiszka]

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

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 <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
> 
> 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?

> 
> 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.
> 	...

If we need this knob, then this version please (not wanting to flush is
likely the corner case).

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

Right, and the qemu-options.hx needs update as well to explain that
"singlestep" has nothing to do with debugger single-stepping.

Jan


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

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

[Stefan Weil]

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 <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
>>
>> 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).

Regards,
Stefan

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

[Jun Koi]

On Wed, Apr 28, 2010 at 4:55 AM, Stefan Weil <weil@mail.berlios.de> 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 <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
>>>
>>> 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

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

[Stefan Weil]

Am 28.04.2010 01:50, schrieb Jun Koi:
> On Wed, Apr 28, 2010 at 4:55 AM, Stefan Weil <weil@mail.berlios.de> wrote:
>> 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


TCI stands for Tiny Code Interpreter and is a new way
to run QEMU's Tiny Code. Links with more information:

http://lists.gnu.org/archive/html/qemu-devel/2009-09/msg01710.html
http://repo.or.cz/w/qemu/ar7.git/blob_plain/master:/tcg/bytecode/README

Regards,
Stefan

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

[Jun Koi]

On Thu, Apr 22, 2010 at 4:20 AM, Stefan Weil <weil@mail.berlios.de> 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 <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
>
> 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

When dont you want to flush TBs??

Thanks,
J