From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O4qRG-0004bG-Im for qemu-devel@nongnu.org; Thu, 22 Apr 2010 03:03:06 -0400 Received: from [140.186.70.92] (port=34682 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O4qRB-0004a3-BX for qemu-devel@nongnu.org; Thu, 22 Apr 2010 03:03:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O4qQx-0004T2-9v for qemu-devel@nongnu.org; Thu, 22 Apr 2010 03:02:56 -0400 Received: from fmmailgate03.web.de ([217.72.192.234]:39674) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O4qQw-0004SE-25 for qemu-devel@nongnu.org; Thu, 22 Apr 2010 03:02:46 -0400 Message-ID: <4BCFF490.6060407@web.de> Date: Thu, 22 Apr 2010 09:02:40 +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> <4BCED6C6.4070509@web.de> <4BCF4FE2.50401@mail.berlios.de> In-Reply-To: <4BCF4FE2.50401@mail.berlios.de> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig8D34919B281AF0FAAE9A3C37" Sender: jan.kiszka@web.de List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: qemu-devel@nongnu.org, Alexander Graf , Jun Koi This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig8D34919B281AF0FAAE9A3C37 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Stefan Weil wrote: > Jan Kiszka schrieb: >> Alexander Graf wrote: >> =20 >>> On 21.04.2010, at 12:04, Jun Koi wrote: >>> >>> =20 >>>> On Tue, Apr 20, 2010 at 8:44 PM, Alexander Graf wrot= e: >>>> =20 >>>>> On 20.04.2010, at 13:38, Jan Kiszka wrote: >>>>> >>>>> =20 >>>>>> Alexander Graf wrote: >>>>>> =20 >>>>>>> On 20.04.2010, at 09:18, Jan Kiszka wrote: >>>>>>> >>>>>>> =20 >>>>>>>> Jun Koi wrote: >>>>>>>> =20 >>>>>>>>> Thank you for the explanation of this code. >>>>>>>>> >>>>>>>>> Qemu has a command named singlestep, which reduces the translat= ed 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 = 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 =3D first_cpu; env !=3D NULL; env =3D env->next_c= pu) { >>>>>>>>> + tb_flush(env); >>>>>>>>> + } >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> =20 >>>>>>>> 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. >>>>>>>> >>>>>>>> =20 >>>>>>>>> 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); >>>>>>>>> } >>>>>>>>> >>>>>>>>> =20 >>>>>>>> Let's just pass mon->mon_cpu to tb_flush and skip the redundant = loop. >>>>>>>> =20 >>>>>>> That doesn't help, no? singlestep is a global variable. Flushing = only the current vcpu would still not affect the others, while the single= step switch would. >>>>>>> =20 >>>>>> tb_flush uses env only to dump some state when a problem occurred.= >>>>>> >>>>>> =20 >>>>>>> According to your above comment the cache is global, but I don't = think we should rely on that. >>>>>>> =20 >>>>>> It might make sense to define some tb_flush_all() as tb_flush(firs= t_cpu) >>>>>> for now to establish the infrastructure. Then we are prepared for = the >>>>>> day the tb_flush implementation may change. >>>>>> =20 >>>>> Right. But then the call to tb_flush_all here is still correct. >>>>> =20 >>>> So what is the final solution do you want? >>>> >>>> I still think that having flush_all_tb() like in the last patch is g= ood enough. >>>> =20 >>> I agree. And I like the patch as is. >>> >>> Acked-by: Alexander Graf >>> >>> =20 >> Sorry, nack for keeping this service in /monitor.c/. But a bonus ack i= f >> 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 >=20 > 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? >=20 > What about this syntax for the singlestep monitor command: >=20 > 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. >=20 > Or, if you prefer to flush by default: >=20 > 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). >=20 > 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 --------------enig8D34919B281AF0FAAE9A3C37 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 iEYEARECAAYFAkvP9JMACgkQitSsb3rl5xTA2QCdHuGMAdTJVVsePAxEdMMe9LL9 aDcAnimGiE6aaug7L2RIGEHQNr9Yl6WW =ZwbA -----END PGP SIGNATURE----- --------------enig8D34919B281AF0FAAE9A3C37--