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

[Edgar E. Iglesias]

On Tue, Sep 14, 2010 at 05:48:30PM +0000, Blue Swirl wrote:
> On Tue, Sep 14, 2010 at 5:05 PM, Edgar E. Iglesias
> <address@hidden> 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 <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.
> 
> Maybe the reason is NPC and the flags, or unassigned access problem.

The lazy condition-code flags evaluation is a bit problematic at load/stores.

For CRIS (which implements the lazy CC flag optimization) I avoided
evaluating the flags at every load/store by instead putting the evaluation
in the slow path (target-cris/op_helper.c:tlb_fill()). The generated code
now only has to make sure that the condition-code bookkeeping is up to date
(i.e cc_op, operands etc).

It's a bit of a hack and I'm not 100% sure it's the right thing to do,
but it seems to be working fine :)


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

:)

Cheers