From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60047) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRYZJ-0008Q7-U8 for qemu-devel@nongnu.org; Mon, 25 Jul 2016 01:36:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bRYZF-0002Np-4w for qemu-devel@nongnu.org; Mon, 25 Jul 2016 01:36:45 -0400 Received: from gate.crashing.org ([63.228.1.57]:38663) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRYZE-0002Ma-1x for qemu-devel@nongnu.org; Mon, 25 Jul 2016 01:36:41 -0400 Message-ID: <1469423736.5978.13.camel@kernel.crashing.org> From: Benjamin Herrenschmidt Date: Mon, 25 Jul 2016 15:15:36 +1000 In-Reply-To: <441c89df-0830-ab0c-6298-89374b1cbe9d@twiddle.net> References: <1469364141.8568.251.camel@kernel.crashing.org> <441c89df-0830-ab0c-6298-89374b1cbe9d@twiddle.net> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] TCG problem with cpu_{st,ld}x_data ? List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson , qemu-devel@nongnu.org Cc: Paolo Bonzini , Christian Borntraeger On Mon, 2016-07-25 at 06:04 +0530, Richard Henderson wrote: > I noticed a related problem recently, while working on the cmpxchg patc= h set. >=20 > In my opinion, we should (1) merge GETRA and GETPC so there's no confus= ion=C2=A0 > between the two, (2) push all adjustment down to the final moment befor= e use,=C2=A0 > perhaps in cpu_restore_state. >=20 > Thus a null value would be properly retained until checked, and one can= easily=C2=A0 > call the memory helper functions without confusion. Ok, after a bit more scrubbing of the code I think I understand what you mean. Now assuming we fix that, there is still a problem if the target code, su= ch as the PPC code, calls a helper that might cause a fault without first updating the PC in the env, right ? IE. On powerpc for example, that mean= s that any instruction using a helper that might potentially do loads or st= ores needs to first call gen_update_nip(). (The same way we seem to do before generating other exceptions such as tr= aps, well provided we do it correctly everywhere, I need to double check). Either that, or change the helpers to capture the PC using GETPC/GETRA fr= om the first level of helper function (so as to ensure the return address is correct). Am I right ? IE. Even if we fix the 0 vs. -2 problem, I still need this patch: --- a/target-ppc/translate.c +++ b/target-ppc/translate.c @@ -6916,6 +6916,7 @@ static void gen_lve##name(DisasContext *ctx) = \ if (size > 1) { = \ tcg_gen_andi_tl(EA, EA, ~(size - 1)); = \ } = \ + gen_update_nip(ctx, ctx->nip - 4); = \ rs =3D gen_avr_ptr(rS(ctx->opcode)); = \ gen_helper_lve##name(cpu_env, rs, EA); = \ tcg_temp_free(EA); = \ @@ -6937,6 +6938,7 @@ static void gen_stve##name(DisasContext *ctx) = \ if (size > 1) { = \ tcg_gen_andi_tl(EA, EA, ~(size - 1)); = \ } = \ + gen_update_nip(ctx, ctx->nip - 4); = \ rs =3D gen_avr_ptr(rS(ctx->opcode)); = \ gen_helper_stve##name(cpu_env, rs, EA); = \ tcg_temp_free(EA); = \ (And possibly others I haven't yet audited) Cheers, Ben.