From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=38785 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OvZci-0007E7-SR for Qemu-devel@nongnu.org; Tue, 14 Sep 2010 13:48:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OvZch-0007yp-BP for Qemu-devel@nongnu.org; Tue, 14 Sep 2010 13:48:52 -0400 Received: from mail-qy0-f180.google.com ([209.85.216.180]:37179) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OvZch-0007yk-8O for Qemu-devel@nongnu.org; Tue, 14 Sep 2010 13:48:51 -0400 Received: by qyk31 with SMTP id 31so6560193qyk.4 for ; Tue, 14 Sep 2010 10:48:50 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20100914170501.GA16141@laped.lan> References: <20100914170501.GA16141@laped.lan> From: Blue Swirl Date: Tue, 14 Sep 2010 17:48:30 +0000 Message-ID: Subject: Re: [Qemu-devel] PowerPC code generation and the program counter Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: Stu Grossman , Qemu-devel@nongnu.org On Tue, Sep 14, 2010 at 5:05 PM, Edgar E. Iglesias wrote: > On Tue, Sep 14, 2010 at 04:10:27PM +0000, Blue Swirl wrote: >> On Mon, Sep 13, 2010 at 4:51 AM, Stu Grossman w= rote: >> > I've been using qemu-12.4 to trace accesses to non-existent addresses,= but I've >> > found that the PC is incorrect when cpu_abort() is called from within = the >> > unassigned memory helper routines (unassigned_mem_read[bwl] and >> > unassigned_mem_write[bwl]). =C2=A0Even nearby instructions (plus or mi= nus 10 >> > instructions or so) don't match the type of load or store being done, = so this >> > isn't a PC being current_instr+4 kind of thing. >> >> If PC is not updated to match the value at the access instruction, it >> will point to the last instruction that did update PC, or start of the >> translation block (TB). >> >> > I ended up modifying the GEN_LD* and GEN_ST* macros (in target-ppc/tra= nslate.c) >> > to call gen_update_nip(ctx, ctx->nip - 4). =C2=A0This fixed the above = problem, which >> > has helped enormously. >> > >> > Since I'm not a qemu expert, I was wondering about several things: >> > >> > =C2=A0 =C2=A0 =C2=A0 =C2=A01) Was it really necessary to add gen_updat= e_nip to the load and store >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 instructions in order to get the co= rrect PC? =C2=A0Could the correct PC >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 have been derived some other way, w= ithout a runtime cost for all >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 basic loads and stores? >> >> This is the way used by Sparc. There save_state() updates PC, NPC and >> forces lazy flag calculation. > > Hi! > > There might be reasons you might need that logic in SPARC, but in general > I dont think the PC needs to be up to date at generated load/stores for > qemu to cope with MMU exceptions. Maybe the reason is NPC and the flags, or unassigned access problem. >> It may be possible to avoid updating the state, if TB generation was >> limited to allow only one instruction which can update the state per >> TB. But shorter TBs will also decrease performance, so the trade-off >> should be evaluated. >> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A02) As the current code lacks that fix, the = basic load and store >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 instructions will save an incorrect= PC if an exception occurs. =C2=A0If >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 so, how come nobody noticed this be= fore? =C2=A0I think that exceptions >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 would have srr0 pointing at the las= t instruction which called >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 gen_update_nip. =C2=A0So when the t= arget returns from a data exception, >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 it might re-execute some instructio= ns. =C2=A0Possibly harmless, but could >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 lead to subtle bugs... >> >> Yes. Also, page fault handlers are not interested in the exact >> location, only the page. Because we ensure that TBs will never cross >> page boundaries, the page will be correct. > > > I'm not sure I follow you here, but in general, MMU exception handlers > need to know the exact address of the instruction that caused the excepti= on. Right, I was in a severe state of confusion. >> > >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0Thanks, Stu >> > >> > Here's the patch if anybody is interested: >> >> Please resubmit with git send-email, with Signed-off-by line and a >> short description. I think it should be applied. > > > I don't think the patch is needed. > > IIUC: > When a load/store in QEMU causes a memory abort, QEMU will retranslate th= e > current TB in a "search" mode that creates side tables that make it possi= ble > to map host PC to guest PC and restore the state. See translate-all.c: > cpu_restore_state(). > > For cases when that logic is not applied, the translator needs to generat= e > code to put the required state up to date. For example for exceptions tha= t > the translator itself generates. > > Accesses to unmapped addresses might be a case that is not handled today. > IMO, we should treat it similary to a memory abort and cpu_restore_state(= ) > in the unmapped access exception code path. If this indeed is the case, maybe the fix will be generic and also Sparc won't need to save state.