From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O4XP8-0002hb-9y for qemu-devel@nongnu.org; Wed, 21 Apr 2010 06:43:38 -0400 Received: from [140.186.70.92] (port=57080 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O4XP5-0002ek-To for qemu-devel@nongnu.org; Wed, 21 Apr 2010 06:43:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O4XP3-0002yh-Ad for qemu-devel@nongnu.org; Wed, 21 Apr 2010 06:43:35 -0400 Received: from fmmailgate01.web.de ([217.72.192.221]:51691) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O4XP2-0002xa-WE for qemu-devel@nongnu.org; Wed, 21 Apr 2010 06:43:33 -0400 Message-ID: <4BCED6C6.4070509@web.de> Date: Wed, 21 Apr 2010 12:43:18 +0200 From: Jan Kiszka MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH] flush TB on singlestep command References: <4BC8D2E8.3030309@mail.berlios.de> <4BCD5560.6070004@web.de> <4BCD924D.1080909@web.de> <40763F40-E48C-4380-94E0-E637F00A5C81@suse.de> <297C546E-0AE2-4EAD-AEAA-3FABAB7786FA@suse.de> In-Reply-To: <297C546E-0AE2-4EAD-AEAA-3FABAB7786FA@suse.de> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig7559DB19E9BFA8106E283B71" Sender: jan.kiszka@web.de List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-devel@nongnu.org, Jun Koi This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig7559DB19E9BFA8106E283B71 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Alexander Graf wrote: > On 21.04.2010, at 12:04, Jun Koi wrote: >=20 >> 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= code >>>>>>> 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 QD= ict *qdict) >>>>>>> cpu_set_log(mask); >>>>>>> } >>>>>>> >>>>>>> +/* flush all the TBs to force new code generation */ >>>>>>> +static void flush_all_tb(void) >>>>>>> +{ >>>>>>> + CPUState *env; >>>>>>> + >>>>>>> + for (env =3D first_cpu; env !=3D NULL; env =3D 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 f= act, >>>>>> it already flushes the one and only translation buffer pool. >>>>>> >>>>>>> static void do_singlestep(Monitor *mon, const QDict *qdict) >>>>>>> { >>>>>>> const char *option =3D qdict_get_try_str(qdict, "option"); >>>>>>> + >>>>>>> if (!option || !strcmp(option, "on")) { >>>>>>> singlestep =3D 1; >>>>>>> + flush_all_tb(); >>>>>>> } else if (!strcmp(option, "off")) { >>>>>>> singlestep =3D 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 lo= op. >>>>> That doesn't help, no? singlestep is a global variable. Flushing on= ly the current vcpu would still not affect the others, while the singlest= ep 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 th= ink 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 th= e >>>> 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 goo= d enough. >=20 > I agree. And I like the patch as is. >=20 > Acked-by: Alexander Graf >=20 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 --------------enig7559DB19E9BFA8106E283B71 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iEYEARECAAYFAkvO1s0ACgkQitSsb3rl5xQcvwCePZplWxvBzyh2NZsB8PJriACy 4VoAniyOVrPZumpaig3VsvNUOY3+NwaL =fHqB -----END PGP SIGNATURE----- --------------enig7559DB19E9BFA8106E283B71--