| Message ID | o2mfdaac4d51004191817h7158e19dy448b40502839a431@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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
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
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
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); }