Re: [Qemu-devel] PowerPC code generation and the program counter

[Edgar E. Iglesias]

On Tue, Sep 14, 2010 at 04:10:27PM +0000, Blue Swirl wrote:
> On Mon, Sep 13, 2010 at 4:51 AM, Stu Grossman <address@hidden> 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.


> 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.


> >
> >        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.

Cheers