inorder/dtb: make sure DTB translate correct address

Information
Submitter:	Korey Sewell
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	ali, gblack, nate, stever

Description

inorder/dtb: make sure DTB translate correct address
The DTB expects the correct PC in the ThreadContext
but how if the memory accesses are speculative? Shouldn't
we send along the requestor's PC to the translate functions?

Testing Done

This isn't a review, just a thought on the question you're asking. If the access is speculative, is it ok to use a misspeculated pc since the instruction will be thrown out anyway?

Steve Reinhardt June 9, 2011, 4:58 a.m. (June 9, 2011, 4:58 a.m.)

Actually after briefly looking at the code, I wonder if we don't have a bigger problem here.  I'm assuming that the ThreadContext struct reflects the committed state of the machine, where we really want the state relative to this in-flight instruction.  Have we just been getting lucky so far?  Seems like in Alpha the TLB only uses the PC to see if it's in PAL mode or not, so maybe the pipeline gets flushed when we switch in and out of PAL mode and thus there's never an inconsistency there even in O3.  (Nate or Ali, can you confirm this?)

Korey, can you elaborate on the kind of errors you were running into without this change?

Technically I think perhaps the TLB should be using the ExecContext interface instead of the ThreadContext, but since ExecContext isn't a real object, that only works if you template the call... I wonder if we really should be using something like the ProxyThreadContext (see cpu/thread_context.hh) wrapped around a DynInst.  Does that make sense?

Ali Saidi June 9, 2011, 8:05 a.m. (June 9, 2011, 8:05 a.m.)

Steve, you're correct that entrances and exists from PAL are serialize the pipeline so it isn't an issue. 

The thread context is pretty much used to just read miscellaneous register. Due to them (rightly) not being renamed and updated at commit the tlb is getting non-speculative state only. Any instruction that is going to do something that needs to be visible to later instructions needs to be marked as serializing. In short, I don't think we need to change the current interface and the hwrei change is very broken.

Korey Sewell June 9, 2011, 11:02 a.m. (June 9, 2011, 11:02 a.m.)

Can you rephrase this: "I don't think we need to change the current interface and the hwrei change is very broken." Are you saying that the subsequent hwrei patch is not what we want or ???

I think the interface is broken and we get away with it more than it's correctly coded. We may get away with in alpha, but I would think seeding the TLB with the last committed PC rather than the current  PC that needs to be translated is wrong and would at some point break the code.

Steve,
I think when I caught this "error" it was when the InOrder wasnt respecting the serializeBefore flag correctly. Thus, you get a mixup in which instruction PC needs to be translated.

Diff:

Revision 2 (+10 -3)

Show changes

	src/arch/alpha/tlb.cc
	src/cpu/inorder/resources/cache_unit.cc

src/arch/alpha/tlb.cc (Diff revision 2)

The Request object has a PC in it which is used by the translateInst() function. 

Assuming that the PC object is always set correctly in Request, then translateData() can also use the Request to get the current PC value rather than the last committed state from ThreadContext.

Shouldn't you get rid of the cache_unit.cc changes now?  I thought that was the point.

This is still a hack, in my opinion; note that the comment on the _pc field in mem/request.hh says "for tracing/debugging", i.e., it's not intended to be architectural.  Also, it isn't always set (e.g., for device accesses), though for CPU accesses it should be.  So I'd say (1) it should work and (2) it's a much less ugly hack than what you had before, so assuming you do get rid of the cache_unit.cc changes I'd say it's fine.

I still think having a ProxyThreadContext wrapped around a DynInst is the "right" way to do it, but I can see where that also looks like a lot of mostly unnecessary overhead.

Korey Sewell June 12, 2011, 8:46 a.m. (June 12, 2011, 8:46 a.m.)

The hacky cache_unit.cc code will definitely be cleaned up in the final change. 

I was thinking that although the ProxyThreadContext<DynInst> seems like the right way to do this for DTB accesses, I think it may not work well for ITB accesses as it could be possible that the DynInst has not even been created yet. Typically, one might fetch the cache block and then create the DynInst (done in simple and o3) rather than vice versa (done in inorder).

I'm thinking the right thing to do is to make the _pc field in Request more than a debugging feature, and then add a "isSet" flag there along with an  assertion in "getPC()" to make sure that any reader of the PC will not get a uninitialized value. That way, you have one generalized way to access the TLB whether it be ITB or DTB access.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Diff: