| Message ID | g2lfdaac4d51004142110tacdb33b8h8b434cf63fea598a@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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 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
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.
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 {