From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O4Wrw-0006IN-V4 for qemu-devel@nongnu.org; Wed, 21 Apr 2010 06:09:21 -0400 Received: from [140.186.70.92] (port=54542 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O4Wrv-0006Fe-1m for qemu-devel@nongnu.org; Wed, 21 Apr 2010 06:09:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O4Wrt-0004g7-CX for qemu-devel@nongnu.org; Wed, 21 Apr 2010 06:09:18 -0400 Received: from mail-yw0-f198.google.com ([209.85.211.198]:57479) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O4Wrt-0004fM-9B for qemu-devel@nongnu.org; Wed, 21 Apr 2010 06:09:17 -0400 Received: by ywh36 with SMTP id 36so3337160ywh.4 for ; Wed, 21 Apr 2010 03:09:16 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <40763F40-E48C-4380-94E0-E637F00A5C81@suse.de> References: <4BC8D2E8.3030309@mail.berlios.de> <4BCD5560.6070004@web.de> <4BCD924D.1080909@web.de> <40763F40-E48C-4380-94E0-E637F00A5C81@suse.de> From: Jun Koi Date: Wed, 21 Apr 2010 19:04:00 +0900 Message-ID: Subject: Re: [Qemu-devel] Re: [PATCH] flush TB on singlestep command Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Jan Kiszka , qemu-devel@nongnu.org On Tue, Apr 20, 2010 at 8:44 PM, Alexander Graf 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 cod= e >>>>> block to be only one instruction. >>>>> This new patch flushes TBs both when singlestep is on and off. >>>>> >>>>> Signed-off-by: Jun Koi >>>>> >>>>> >>>>> 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) >>>>> =A0 =A0cpu_set_log(mask); >>>>> } >>>>> >>>>> +/* flush all the TBs to force new code generation */ >>>>> +static void flush_all_tb(void) >>>>> +{ >>>>> + =A0 =A0CPUState *env; >>>>> + >>>>> + =A0 =A0for (env =3D first_cpu; env !=3D NULL; env =3D env->next_cpu= ) { >>>>> + =A0 =A0 =A0 =A0tb_flush(env); >>>>> + =A0 =A0} >>>>> +} >>>>> + >>>> 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) >>>>> { >>>>> =A0 =A0const char *option =3D qdict_get_try_str(qdict, "option"); >>>>> + >>>>> =A0 =A0if (!option || !strcmp(option, "on")) { >>>>> =A0 =A0 =A0 =A0singlestep =3D 1; >>>>> + =A0 =A0 =A0 =A0flush_all_tb(); >>>>> =A0 =A0} else if (!strcmp(option, "off")) { >>>>> =A0 =A0 =A0 =A0singlestep =3D 0; >>>>> + =A0 =A0 =A0 =A0flush_all_tb(); >>>>> =A0 =A0} else { >>>>> =A0 =A0 =A0 =A0monitor_printf(mon, "unexpected option %s\n", option); >>>>> =A0 =A0} >>>>> >>>> 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 t= he current vcpu would still not affect the others, while the singlestep swi= tch 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 eno= ugh. thanks, J