dev: Prevent MC146818 timer RTC events firing before startup

Information
Submitter:	Cagdas Dirik
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	abardsley, ahansson, ali

Description

This change includes edits to MC146818 timer to prevent RTC events firing before startup to comply with SimObject initialization call sequence.

Testing Done

Issue Summary

Description	From	Last Updated	Status
no need for virtual, perhaps also choose a different name to avoid confusion, schedule()?	Andreas Hansson	Dec. 3, 2014, 9:06 a.m.	Resolved
can also do it in the initialisation list	Andreas Hansson	Dec. 3, 2014, 9:11 a.m.	Resolved
no need for virtual again, I would suggest to name it schedule() or similar	Andreas Hansson	Dec. 3, 2014, 9:06 a.m.	Resolved
An alternative to have methods on the objects would be to simply call schedule(event, curTick() + event.getOffset()); or similar	Andreas Hansson	Dec. 3, 2014, 9:14 a.m.	Resolved

Description:

~		This change includes edits MC146818 timer to prevent RTC events firing before startup to comply with SimObject initialization call sequence.
	~	This change includes edits to MC146818 timer to prevent RTC events firing before startup to comply with SimObject initialization call sequence.

Andreas Hansson Nov. 29, 2014, 2:19 a.m. (Nov. 29, 2014, 2:19 a.m.)

One more thought, do you need to serialise and unserialise the offset? If not, why not?

Cagdas Dirik Dec. 4, 2014, 10:26 a.m. (Dec. 4, 2014, 10:26 a.m.)

Offset for mc146818 events are already serialized, but they were posted immediately during unserialize. This change postpones event scheduling.

src/dev/mc146818.hh (Diff revision 1)

no need for virtual, perhaps also choose a different name to avoid confusion, schedule()?

Show all issues

src/dev/mc146818.hh (Diff revision 1)

can also do it in the initialisation list

Show all issues

src/dev/mc146818.hh (Diff revision 1)

no need for virtual

again, I would suggest to name it schedule() or similar

Show all issues

src/dev/mc146818.cc (Diff revision 1)

An alternative to have methods on the objects would be to simply call schedule(event, curTick() + event.getOffset()); or similar

Show all issues

Overall it looks great.

The summary should start with a keyword and be < 65 char. Have a look at http://www.gem5.org/Commit_Access. The list is fairly well hidden. In your case I would suggest dev.

Also, the check you do in simulate.py should probably be a patch on its own.

Summary:

-	Prevent MC146818 timer RTC events firing before startup
+	dev: Prevent MC146818 timer RTC events firing before startup

Diff:

Revision 2 (+27 -8)

Show changes

	src/dev/mc146818.hh
	src/dev/mc146818.cc
	src/dev/x86/cmos.hh
	src/dev/x86/cmos.cc

Diff:

Revision 3 (+27 -8)

Show changes

	src/dev/mc146818.hh
	src/dev/mc146818.cc
	src/dev/x86/cmos.hh
	src/dev/x86/cmos.cc

Diff:

Revision 4 (+27 -8)

Show changes

	src/dev/mc146818.hh
	src/dev/mc146818.cc
	src/dev/x86/cmos.hh
	src/dev/x86/cmos.cc

Diff:

Revision 5 (+27 -8)

Show changes

	src/dev/mc146818.hh
	src/dev/mc146818.cc
	src/dev/x86/cmos.hh
	src/dev/x86/cmos.cc

Diff:

Revision 6 (+27 -8)

Show changes

	src/dev/mc146818.hh
	src/dev/mc146818.cc
	src/dev/x86/cmos.hh
	src/dev/x86/cmos.cc

Looks good. What about the regressions?

Cagdas Dirik Dec. 5, 2014, 8:47 a.m. (Dec. 5, 2014, 8:47 a.m.)

I have run hello world in se mode for both X86 and ARM. I also have a small program that does int array manipulation, and I run it in se mode for both X86 and ARM. I typically run timing and detailed cpu with simple memory. I have also run this same small program in FS mode on X86.

If there are more regression tests, could you please let me know? Since I am new here, I am figuring out as I go.

Andreas Hansson Dec. 11, 2014, 2:49 a.m. (Dec. 11, 2014, 2:49 a.m.)

It seems this changeset causes the ALPHA tsunami-minor/o3 regressions to fail (for example build/ALPHA/tests/opt/long/fs/10.linux-boot/alpha/linux/tsunami-minor). I have not yet narrowed down the root cause, so perhaps you could have a look Cagdas?

Andreas Hansson Dec. 11, 2014, 3:37 a.m. (Dec. 11, 2014, 3:37 a.m.)

dev/alpha/tsunami_io needs to override startup and call rtc.startup()

it's probably worth also removing the inclusion of mc146818.hh in dev/arm/rtc_pl031.cc

Cagdas Dirik Dec. 11, 2014, 11:29 a.m. (Dec. 11, 2014, 11:29 a.m.)

Sure, I will take a look what other changes are missing. I should have dug deeper to see who else has an MC146818 timer or at least including the header file. Thank you for catching the issue.

Change Summary:

This update is to fix regression failures for alpha and mips.

Diff:

Revision 7 (+38 -9)

Show changes

	src/dev/mc146818.hh
	src/dev/mc146818.cc
	src/dev/alpha/tsunami_io.hh
	src/dev/alpha/tsunami_io.cc
	src/dev/arm/rtc_pl031.cc
	src/dev/x86/cmos.hh
	src/dev/x86/cmos.cc

Change Summary:

Added changes for MIPS.

Diff:

Revision 8 (+49 -9)

Show changes

	src/dev/mc146818.hh
	src/dev/mc146818.cc
	src/dev/alpha/tsunami_io.hh
	src/dev/alpha/tsunami_io.cc
	src/dev/arm/rtc_pl031.cc
	src/dev/mips/malta_io.hh
	src/dev/mips/malta_io.cc
	src/dev/x86/cmos.hh
	1 more

Ship It!

Andreas Hansson Dec. 30, 2014, 3:25 a.m. (Dec. 30, 2014, 3:25 a.m.)
```
One more for the pile Nilay :-)
```

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Summary:

Diff:

Diff:

Diff:

Diff:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)