On Tue, Sep 14, 2010 at 5:05 PM, Edgar E. Iglesias <edgar.igles...@gmail.com> 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 <stu.gross...@gmail.com> wrote: >> > 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]). Even nearby instructions (plus or minus 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/translate.c) >> > to call gen_update_nip(ctx, ctx->nip - 4). This fixed the above problem, >> > which >> > has helped enormously. >> > >> > Since I'm not a qemu expert, I was wondering about several things: >> > >> > 1) Was it really necessary to add gen_update_nip to the load and >> > store >> > instructions in order to get the correct PC? Could the correct >> > PC >> > have been derived some other way, without a runtime cost for all >> > 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. >> >> > 2) As the current code lacks that fix, the basic load and store >> > instructions will save an incorrect PC if an exception occurs. >> > If >> > so, how come nobody noticed this before? I think that exceptions >> > would have srr0 pointing at the last instruction which called >> > gen_update_nip. So when the target returns from a data >> > exception, >> > it might re-execute some instructions. Possibly harmless, but >> > could >> > 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 exception. Right, I was in a severe state of confusion. >> > >> > Thanks, 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 the > current TB in a "search" mode that creates side tables that make it possible > 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 generate > code to put the required state up to date. For example for exceptions that > 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.