paravirt: Generalize parts of the XEN timer code into pvclock
ClosedPublic
Actions

Authored by bryanv on Jan 3 2015, 11:51 PM.

Details

Reviewers

royger
rstone

Commits

rS278183: Generalized parts of the XEN timer code into a generic pvclock

Summary

This will later be used to support KVM clock. I'm not sure if there is a good way to sort out generic upstream Xen headers to include pvclock.h, so at the moment, casts are used to keep the code compiling.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

bryanv updated this revision to Diff 2982.Jan 3 2015, 11:51 PM

bryanv retitled this revision from to Generalized parts of the XEN timer code into pvclock.

bryanv updated this object.

bryanv edited the test plan for this revision. (Show Details)

bryanv added a reviewer: royger.

bryanv set the repository for this revision to rS FreeBSD src repository - subversion.

bryanv set the repository for this revision to rS FreeBSD src repository - subversion.Jan 4 2015, 5:50 AM

bryanv retitled this revision from Generalized parts of the XEN timer code into pvclock to paravirt: Generalize parts of the XEN timer code into pvclock.Jan 4 2015, 5:58 AM

Thanks for doing this, the change looks fine, but could you wait until Friday so I can test it on Xen before committing it?

sys/x86/include/pvclock.h
50 ↗	(On Diff #2982)	You don't need the __packed here or above, the layout of those structures already make them packed.
sys/x86/x86/pvclock.c
130 ↗	(On Diff #2982)	I think this change (from the previous code found in the Xen timer) fixes the theoretical issue described here: https://lists.freebsd.org/pipermail/freebsd-virtualization/2014-January/001926.html

emaste added a subscriber: emaste.Jan 5 2015, 7:44 PM

In D1429#6, @royger wrote:

Thanks for doing this, the change looks fine, but could you wait until Friday so I can test it on Xen before committing it?

Absolutely. Thanks for reviewing.

I've successfully tested this on a Xen PVHVM guest while performing migrations and it works fine, also tested on Dom0 with the same result.

sys/x86/x86/pvclock.c
153 ↗	(On Diff #2982)	This doesn't need to be a volatile AFAICT.

This revision is now accepted and ready to land.Jan 9 2015, 11:02 AM

bryanv added inline comments.Jan 22 2015, 1:13 AM

sys/x86/x86/pvclock.c
130 ↗	(On Diff #2982)	Cool. Bonus fix :)
153 ↗	(On Diff #2982)	Removed.

rstone added a reviewer: rstone.Feb 2 2015, 3:36 PM

rstone added a subscriber: rstone.

rstone added inline comments.

sys/x86/x86/pvclock.c
121 ↗	(On Diff #2982)	Doesn't this need to happen underneath a critical_enter()? If we switch to a different (virtual) CPU in the loop we could potentially see data for one (physical) CPU but read the TSC from a different CPU.
130 ↗	(On Diff #2982)	I don't see how this code prevents reading from ti->version twice in the condition?

rstone added inline comments.Feb 2 2015, 3:46 PM

sys/x86/x86/pvclock.c
121 ↗	(On Diff #2982)	scratch that, the caller already does the critical_enter() (and it has to be the one to do it). I think that a CRITICAL_ASSERT(); would be a good idea where to document this important prerequisite.