[Qemu-devel] Missing singlestep for already-translated code?

[Qemu-devel] Missing singlestep for already-translated code?

[Jun Koi]

Hi,

I am looking into the singlestep command in monitor interface, and it
seems that we only take into account the singlestep flag when we are
translating code.
So for the already-translated code, we will miss singlestep?

Thanks,
Jun

[Qemu-devel] Re: Missing singlestep for already-translated code?

[takasi-y]

Hi,
> So for the already-translated code, we will miss singlestep?
At least SH4(and mips?) shows such behaviour.
I think a patch below enables single stepping in such case, too.
But, I'm not sure if this behaviour is on purpose, nor this patch is correct.
/yoshii

diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index 3537f8c..dfa724a 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -300,7 +300,7 @@ static void gen_goto_tb(DisasContext * ctx, int n, target_ulong dest)
     tb = ctx->tb;
 
     if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) &&
-	!ctx->singlestep_enabled) {
+	!ctx->singlestep_enabled && !singlestep) {
 	/* Use a direct jump if in same page and singlestep not enabled */
         tcg_gen_goto_tb(n);
         tcg_gen_movi_i32(cpu_pc, dest);

[Qemu-devel] Re: Missing singlestep for already-translated code?

[Jun Koi]

On Tue, Apr 13, 2010 at 6:21 PM,  <takasi-y@ops.dti.ne.jp> wrote:
> Hi,
>> So for the already-translated code, we will miss singlestep?
> At least SH4(and mips?) shows such behaviour.
> I think a patch below enables single stepping in such case, too.
> But, I'm not sure if this behaviour is on purpose, nor this patch is correct.
> /yoshii
>
> diff --git a/target-sh4/translate.c b/target-sh4/translate.c
> index 3537f8c..dfa724a 100644
> --- a/target-sh4/translate.c
> +++ b/target-sh4/translate.c
> @@ -300,7 +300,7 @@ static void gen_goto_tb(DisasContext * ctx, int n, target_ulong dest)
>     tb = ctx->tb;
>
>     if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) &&
> -       !ctx->singlestep_enabled) {
> +       !ctx->singlestep_enabled && !singlestep) {
>        /* Use a direct jump if in same page and singlestep not enabled */
>         tcg_gen_goto_tb(n);
>         tcg_gen_movi_i32(cpu_pc, dest);
>

The first glance: because you are patching translate.c, I dont think
you fixed the problem.

Thanks,
J

[Qemu-devel] Re: Missing singlestep for already-translated code?

[Jan Kiszka]

Jun Koi wrote:
> Hi,
> 
> I am looking into the singlestep command in monitor interface, and it
> seems that we only take into account the singlestep flag when we are
> translating code.
> So for the already-translated code, we will miss singlestep?

This feature is broken. For TCG, it should at least flush the
translation buffer, and for KVM it has to enable single-stepping in the
kernel. That's what happens automatically when you call cpu_single_step.
I guess 'singlestep' wants to be somehow orthogonal to this. But this is
the wrong approach.

Does anyone actually used this feature or still does so? It looks fairly
redundant to me, kind of a poor-man's gdb front-end as part of the
monitor console.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: Missing singlestep for already-translated code?

[Alexander Graf]

On 13.04.2010, at 15:36, Jan Kiszka wrote:

> Jun Koi wrote:
>> Hi,
>> 
>> I am looking into the singlestep command in monitor interface, and it
>> seems that we only take into account the singlestep flag when we are
>> translating code.
>> So for the already-translated code, we will miss singlestep?
> 
> This feature is broken. For TCG, it should at least flush the
> translation buffer, and for KVM it has to enable single-stepping in the
> kernel. That's what happens automatically when you call cpu_single_step.
> I guess 'singlestep' wants to be somehow orthogonal to this. But this is
> the wrong approach.
> 
> Does anyone actually used this feature or still does so? It looks fairly
> redundant to me, kind of a poor-man's gdb front-end as part of the
> monitor console.

Not sure what it does, but I use -singlestep quite a lot to get register dumps for instructions when using -d cpu.

Alex

Re: [Qemu-devel] Re: Missing singlestep for already-translated code?

[Jan Kiszka]

Alexander Graf wrote:
> On 13.04.2010, at 15:36, Jan Kiszka wrote:
> 
>> Jun Koi wrote:
>>> Hi,
>>>
>>> I am looking into the singlestep command in monitor interface, and it
>>> seems that we only take into account the singlestep flag when we are
>>> translating code.
>>> So for the already-translated code, we will miss singlestep?
>> This feature is broken. For TCG, it should at least flush the
>> translation buffer, and for KVM it has to enable single-stepping in the
>> kernel. That's what happens automatically when you call cpu_single_step.
>> I guess 'singlestep' wants to be somehow orthogonal to this. But this is
>> the wrong approach.
>>
>> Does anyone actually used this feature or still does so? It looks fairly
>> redundant to me, kind of a poor-man's gdb front-end as part of the
>> monitor console.
> 
> Not sure what it does, but I use -singlestep quite a lot to get register dumps for instructions when using -d cpu.

Ah, "singlestep" is not about stopping the VM after each instruction but
about limiting the TB length to a single instruction. Badly named and
poorly documented.

In that case, the dynamic switch should already be fine by adding a
tb_flush() on enable. Still, someone should also patch at least the docs.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: Missing singlestep for already-translated code?

[Jun Koi]

On Wed, Apr 14, 2010 at 12:28 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Alexander Graf wrote:
>> On 13.04.2010, at 15:36, Jan Kiszka wrote:
>>
>>> Jun Koi wrote:
>>>> Hi,
>>>>
>>>> I am looking into the singlestep command in monitor interface, and it
>>>> seems that we only take into account the singlestep flag when we are
>>>> translating code.
>>>> So for the already-translated code, we will miss singlestep?
>>> This feature is broken. For TCG, it should at least flush the
>>> translation buffer, and for KVM it has to enable single-stepping in the
>>> kernel. That's what happens automatically when you call cpu_single_step.
>>> I guess 'singlestep' wants to be somehow orthogonal to this. But this is
>>> the wrong approach.
>>>
>>> Does anyone actually used this feature or still does so? It looks fairly
>>> redundant to me, kind of a poor-man's gdb front-end as part of the
>>> monitor console.
>>
>> Not sure what it does, but I use -singlestep quite a lot to get register dumps for instructions when using -d cpu.
>
> Ah, "singlestep" is not about stopping the VM after each instruction but
> about limiting the TB length to a single instruction. Badly named and
> poorly documented.
>
> In that case, the dynamic switch should already be fine by adding a
> tb_flush() on enable. Still, someone should also patch at least the docs.
>

Do you have any comment on the below patch?

Thanks,
J

diff --git a/monitor.c b/monitor.c
index 5659991..dfa9820 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1190,8 +1190,13 @@ 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 TB 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] Re: Missing singlestep for already-translated code?

[Jan Kiszka]

Jun Koi wrote:
> On Wed, Apr 14, 2010 at 12:28 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Alexander Graf wrote:
>>> On 13.04.2010, at 15:36, Jan Kiszka wrote:
>>>
>>>> Jun Koi wrote:
>>>>> Hi,
>>>>>
>>>>> I am looking into the singlestep command in monitor interface, and it
>>>>> seems that we only take into account the singlestep flag when we are
>>>>> translating code.
>>>>> So for the already-translated code, we will miss singlestep?
>>>> This feature is broken. For TCG, it should at least flush the
>>>> translation buffer, and for KVM it has to enable single-stepping in the
>>>> kernel. That's what happens automatically when you call cpu_single_step.
>>>> I guess 'singlestep' wants to be somehow orthogonal to this. But this is
>>>> the wrong approach.
>>>>
>>>> Does anyone actually used this feature or still does so? It looks fairly
>>>> redundant to me, kind of a poor-man's gdb front-end as part of the
>>>> monitor console.
>>> Not sure what it does, but I use -singlestep quite a lot to get register dumps for instructions when using -d cpu.
>> Ah, "singlestep" is not about stopping the VM after each instruction but
>> about limiting the TB length to a single instruction. Badly named and
>> poorly documented.
>>
>> In that case, the dynamic switch should already be fine by adding a
>> tb_flush() on enable. Still, someone should also patch at least the docs.
>>
> 
> Do you have any comment on the below patch?
> 
> Thanks,
> J
> 
> diff --git a/monitor.c b/monitor.c
> index 5659991..dfa9820 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1190,8 +1190,13 @@ 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 TB 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 {

Add braces to the loop, format the patch properly (subject, description,
signed off), repost it, and it should be accepted. And some extensions
to the docs of both the command line switch and the monitor command
would be welcome as well (as separate patch).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: Missing singlestep for already-translated code?

[Aurelien Jarno]

Jun Koi a écrit :
> On Wed, Apr 14, 2010 at 12:28 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Alexander Graf wrote:
>>> On 13.04.2010, at 15:36, Jan Kiszka wrote:
>>>
>>>> Jun Koi wrote:
>>>>> Hi,
>>>>>
>>>>> I am looking into the singlestep command in monitor interface, and it
>>>>> seems that we only take into account the singlestep flag when we are
>>>>> translating code.
>>>>> So for the already-translated code, we will miss singlestep?
>>>> This feature is broken. For TCG, it should at least flush the
>>>> translation buffer, and for KVM it has to enable single-stepping in the
>>>> kernel. That's what happens automatically when you call cpu_single_step.
>>>> I guess 'singlestep' wants to be somehow orthogonal to this. But this is
>>>> the wrong approach.
>>>>
>>>> Does anyone actually used this feature or still does so? It looks fairly
>>>> redundant to me, kind of a poor-man's gdb front-end as part of the
>>>> monitor console.
>>> Not sure what it does, but I use -singlestep quite a lot to get register dumps for instructions when using -d cpu.
>> Ah, "singlestep" is not about stopping the VM after each instruction but
>> about limiting the TB length to a single instruction. Badly named and
>> poorly documented.
>>
>> In that case, the dynamic switch should already be fine by adding a
>> tb_flush() on enable. Still, someone should also patch at least the docs.
>>

What's the real point of flushing the tb to get it retranslated again?
It will be retranslated in the exact same way.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: Missing singlestep for already-translated code?

[Jan Kiszka]

Aurelien Jarno wrote:
> Jun Koi a écrit :
>> On Wed, Apr 14, 2010 at 12:28 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> Alexander Graf wrote:
>>>> On 13.04.2010, at 15:36, Jan Kiszka wrote:
>>>>
>>>>> Jun Koi wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I am looking into the singlestep command in monitor interface, and it
>>>>>> seems that we only take into account the singlestep flag when we are
>>>>>> translating code.
>>>>>> So for the already-translated code, we will miss singlestep?
>>>>> This feature is broken. For TCG, it should at least flush the
>>>>> translation buffer, and for KVM it has to enable single-stepping in the
>>>>> kernel. That's what happens automatically when you call cpu_single_step.
>>>>> I guess 'singlestep' wants to be somehow orthogonal to this. But this is
>>>>> the wrong approach.
>>>>>
>>>>> Does anyone actually used this feature or still does so? It looks fairly
>>>>> redundant to me, kind of a poor-man's gdb front-end as part of the
>>>>> monitor console.
>>>> Not sure what it does, but I use -singlestep quite a lot to get register dumps for instructions when using -d cpu.
>>> Ah, "singlestep" is not about stopping the VM after each instruction but
>>> about limiting the TB length to a single instruction. Badly named and
>>> poorly documented.
>>>
>>> In that case, the dynamic switch should already be fine by adding a
>>> tb_flush() on enable. Still, someone should also patch at least the docs.
>>>
> 
> What's the real point of flushing the tb to get it retranslated again?
> It will be retranslated in the exact same way.

Nope. AFAIU, 'singlestep' will enforce single-instruction TBs.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: Missing singlestep for already-translated code?

[Aurelien Jarno]

Jan Kiszka a écrit :
> Aurelien Jarno wrote:
>> Jun Koi a écrit :
>>> On Wed, Apr 14, 2010 at 12:28 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> Alexander Graf wrote:
>>>>> On 13.04.2010, at 15:36, Jan Kiszka wrote:
>>>>>
>>>>>> Jun Koi wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I am looking into the singlestep command in monitor interface, and it
>>>>>>> seems that we only take into account the singlestep flag when we are
>>>>>>> translating code.
>>>>>>> So for the already-translated code, we will miss singlestep?
>>>>>> This feature is broken. For TCG, it should at least flush the
>>>>>> translation buffer, and for KVM it has to enable single-stepping in the
>>>>>> kernel. That's what happens automatically when you call cpu_single_step.
>>>>>> I guess 'singlestep' wants to be somehow orthogonal to this. But this is
>>>>>> the wrong approach.
>>>>>>
>>>>>> Does anyone actually used this feature or still does so? It looks fairly
>>>>>> redundant to me, kind of a poor-man's gdb front-end as part of the
>>>>>> monitor console.
>>>>> Not sure what it does, but I use -singlestep quite a lot to get register dumps for instructions when using -d cpu.
>>>> Ah, "singlestep" is not about stopping the VM after each instruction but
>>>> about limiting the TB length to a single instruction. Badly named and
>>>> poorly documented.
>>>>
>>>> In that case, the dynamic switch should already be fine by adding a
>>>> tb_flush() on enable. Still, someone should also patch at least the docs.
>>>>
>> What's the real point of flushing the tb to get it retranslated again?
>> It will be retranslated in the exact same way.
> 
> Nope. AFAIU, 'singlestep' will enforce single-instruction TBs.
> 

Ah ok, you mean it flushes the already translate TB. It makes sense now.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net