O3: Fixes fetch deadlock when the interrupt clears before CPU handles it.

Information
Submitter:	Ali Saidi
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	ali, gblack, nate, stever

Description

O3: Fixes fetch deadlock when the interrupt clears before CPU handles it.

When this condition occurs the cpu should restart the fetch stage to fetch from
the original execution path. Fault handling in the commit stage is cleaned up a
little bit so the control flow is simplier. Finally, if an instruction is being
used to carry a fault it isn't executed, so the fault propigates appropriately.

Testing Done

Could you please explain the reason for this change? I'm not familiar enough with ARM's interrupt architecture to know what the commit message is saying. My guess is that the interrupt controller initially thought it was going to signal an interrupt but then later changed it's mind. If that's the case, then really shouldn't we change the interrupt controller so it doesn't change its mind? I don't that that's possible, but I can't think of why it wouldn't be.

Ali Saidi Dec. 9, 2010, 1:39 a.m. (Dec. 9, 2010, 1:39 a.m.)

No, it's the case that a device signaled it wanted to interrupt and then that interrupt was cleared before the O3 cpu redirected to that then fetch locks up. For example if a device sends an interrupt at tick N, and tick N+1 a ISR read hit the device and clears the interrupt, and at N+10 then O3 cpu actually gets around to re-directing. Without this change fetch comes to a screeching halt. Part of it is the issue that we signal an interrupt pending, separately from producing the fault, but this isn't an ARM only issue. Even if you find a different bandaid, without in the O3 cpu has a bug, it's only a question of it you hit it or not.

src/cpu/o3/iew_impl.hh (Diff revision 1)

Shouldn't the instruction be a nop in this case and do nothing when executed?

src/cpu/o3/commit_impl.hh (Diff revision 1)

I think this code is evolving toward unnecessary complexity.  It looks to me like there are two separate things going on:
1. calling cpu->checkInterrupts() to see if there are pending interrupts, and if so, initiating the pipeline flush and storing the interrupt code in the 'interrupt' variable
2. if the 'interrupt' variable is set and the pipeline is flushed, handling the interrupt
Having both of these things combined in a single function that has to get called if either one of these things needs to be done is confusing.  Is there even any need to store the interrupt fault code between 1 and 2?  Seems like we just need a bool to remember that we passed 1 and are waiting for 2, and a few tests to see if either we arrived at 2 or the interrupt got cleared so we should roll back to a pre-1 state.

src/cpu/o3/fetch_impl.hh (Diff revision 1)

I'm confused... in fetchCacheLine() we check (interruptPending && !(pc & 0x3)) and isSwitchedOut() and return false.... so what are the cases where fetchCacheLine returns true that need to be fixed up here?  And aren't those cases that should be fixed up by making fetchCacheLine() return false rather than adding all these special checks here?

src/cpu/o3/fetch_impl.hh (Diff revision 1)

Why not combine this info into a single DPRINTF?

src/cpu/o3/commit_impl.hh (Diff revision 1)

I split this into two functions, but boolean is if interrupt == NoFault or not. The flow control is much better and after it passes regressions I'll post it.

src/cpu/o3/fetch_impl.hh (Diff revision 1)

Moved the check into fetchCacheLine. No ugly macros, but there still is an alpha specific bit

src/cpu/o3/fetch_impl.hh (Diff revision 1)

merged

src/cpu/o3/iew_impl.hh (Diff revision 1)

yup

Summary:

-	O3: Fixes fetch deadlock when the interrupt master clears single before CPU handles it.
+	O3: Fixes fetch deadlock when the interrupt clears before CPU handles it.

Description:

~		O3: Fixes fetch deadlock when the interrupt master clears single before CPU handles it.
~		Then the cpu should restart fetch stage to fetch from the original execution
~		path.
	~	O3: Fixes fetch deadlock when the interrupt clears before CPU handles it.
	~
	~	When this condition occurs the cpu should restart the fetch stage to fetch from
	+	the original execution path. Fault handling in the commit stage is cleaned up a
	+	little bit so the control flow is simplier. Finally, if an instruction is being
	+	used to carry a fault it isn't executed, so the fault propigates appropriately.

Diff:

Revision 2 (+92 -47)

Show changes

	src/arch/arm/interrupts.hh
	src/cpu/o3/commit.hh
	src/cpu/o3/commit_impl.hh
	src/cpu/o3/fetch.hh
	src/cpu/o3/fetch_impl.hh
	src/cpu/o3/iew_impl.hh

I still don't totally follow why this is necessary, although I believe it probably is. One sticky problem you can run into with x86 (and probably most ISAs) is if you enable interrupts and then immediately disable them again, expecting that that will let any pent up interrupts get handled. Is it possible the CPU might attempt to process one but then lose it's opportunity in that case? I know in some real world case (I don't remember the specifics) it can hang an OS which expects an interrupt to get through as described and move things along. If that doesn't matter here then never mind, but it's a potential problem it would be best to fix if possible.

Gabe Black Jan. 16, 2011, 5:01 p.m. (Jan. 16, 2011, 5:01 p.m.)

I think part of the problem here is that there's magical instantaneous communication about interrupts in most ISAs. Is that correct? If so, it would likely help for other ISAs to pass interrupts through the memory system like x86 does.

src/arch/arm/interrupts.hh (Diff revision 2)

This should probably go into it's own change and just get committed right away since it's just fixing a typo basically. It doesn't have anything to do with the O3 changes.

src/cpu/o3/commit.hh (Diff revision 2)

I think this is spelled propagate. This should be fixed in the commit message too, and wherever else it might be.

src/cpu/o3/commit_impl.hh (Diff revision 2)

This line is too long now.

Much better... thanks for splitting handleInterrupt(), this is much clearer and cleaner.  Just a few little things left.  I'll take your word on the merged DPRINTF since that didn't seem to make it into the revised diff.

src/cpu/o3/commit.hh (Diff revision 2)

Yea, it's definitely "propagate"

src/cpu/o3/commit_impl.hh (Diff revision 2)

I'm confused by the bare 'interrupt' term here... shouldn't it be 'interrupt != NoFault' or something like that?

src/cpu/o3/fetch.hh (Diff revision 2)

This is a big improvement... seems like it would be even better if we could create a new ISA-dependent function like suppressInterrupts() that returns ((pc & 0x3) != 0) for the Alpha case and false for everyone else, then get rid of this function and change the original line to
  (interruptPending && !suppressInterrupts(pc))
which addresses my other concern, that checkInterrupt() is kind of unspecific  since we already have a function called checkInterrupts().

Ali Saidi Jan. 17, 2011, 4:44 a.m. (Jan. 17, 2011, 4:44 a.m.)

I thought about creating one, but I couldn't decide what parameters should be passed to it. Until we have another ISA that needs similar functionality (and I'm not sure we will, I really don't know how to define a function to handle the case, so I don't think there is a point in doing it.

Gabe Black Jan. 17, 2011, 6 a.m. (Jan. 17, 2011, 6 a.m.)

I'm not a big fan of that sort of function either, primarily since it's really just moving the code around rather than factoring out it's ISA dependent-ness. I think the assumption that there's a PAL mode bit in the PC is one of the last widespread vestiges of the Alpha only days. It would be great to get rid of it, but in my experience that's been tricky to actually do.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Summary:

Description:

Diff: