Page MenuHomeFreeBSD

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

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

Details

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
Lint
Lint Skipped
Unit
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.
bryanv set the repository for this revision to rS FreeBSD src repository.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
royger edited edge metadata.Jan 5 2015, 7:19 PM

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
bryanv added a comment.Jan 6 2015, 2:33 AM
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.

royger accepted this revision.Jan 9 2015, 11:02 AM
royger edited edge metadata.

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

bryanv closed this revision.Feb 4 2015, 8:27 AM
bryanv updated this revision to Diff 3627.

Closed by commit rS278183 (authored by @bryanv).