|
From: | Stefan Weil |
Subject: | Re: [Qemu-devel] Re: [PATCH] flush TB on singlestep command |
Date: | Tue, 27 Apr 2010 21:55:59 +0200 |
User-agent: | Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100411 Iceowl/1.0b1 Icedove/3.0.4 |
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 <address@hidden> wrote:On 20.04.2010, at 13:38, Jan Kiszka wrote:Alexander Graf wrote:On 20.04.2010, at 09:18, Jan Kiszka wrote: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.Jun Koi wrote:Thank you for the explanation of this code.Qemu has a command named singlestep, which reduces the translated codeblock to be only one instruction. This new patch flushes TBs both when singlestep is on and off. Signed-off-by: Jun Koi <address@hidden> diff --git a/monitor.c b/monitor.c index 5659991..2b2005b 100644 --- a/monitor.c +++ b/monitor.c@@ -1187,13 +1187,26 @@ static void do_log(Monitor *mon, const QDict *qdict)cpu_set_log(mask); } +/* flush all the TBs to force new code generation */ +static void flush_all_tb(void) +{ + CPUState *env; + + for (env = first_cpu; env != NULL; env = env->next_cpu) { + tb_flush(env); + } +} +The smaller your patch are, the more people pick on it. :) I was about to suggest moving this close to tb_flush, but then Irealized that the env argument of that service is misleading. In fact,it already flushes the one and only translation buffer pool.Let's just pass mon->mon_cpu to tb_flush and skip the redundant loop.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); }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 theday 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 <address@hidden>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, Janflush_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
[Prev in Thread] | Current Thread | [Next in Thread] |