Differential D3880

x86/xen: Consolidate xen-os.h in a single place
ClosedPublic
Actions

Authored by julien.grall_citrix.com on Oct 13 2015, 6:02 PM.

Details

Reviewers

royger

Summary

amd64 and i386 platform code contain very similar xen/xen-os.h

The only differences are:

Functions/variables/types which were unused in i386/xen/xen-os.h:
- xen_xchg
- __xchg_dummy
- __xg
- __xchg
- atomic_t
- atomic_inc
- rdtscll

The functions/variables/types unused in xen-os.h can be dropped and there
is no more differences betwen amd64 and i386.

The new header is placed in x86/include/xen and each platform will have
dummy headers include x86/xen/*.h. This is to be able to include
machine/xen/*.h in the PV drivers.

Test Plan

Only build tested.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 772
Build 772: arc lint + arc unit

Event Timeline

julien.grall_citrix.com updated this revision to Diff 9354.Oct 13 2015, 6:02 PM

julien.grall_citrix.com retitled this revision from to x86/xen: Consolidate the architecture specific headers in a single place.

julien.grall_citrix.com edited the test plan for this revision. (Show Details)

julien.grall_citrix.com added a reviewer: royger.

julien.grall_citrix.com updated this object.

Herald added a subscriber: imp. · View Herald TranscriptOct 13 2015, 6:02 PM

I uploaded the wrong version. The major change is in synch_cmpxgh where
the !amd64__ part was missing.

LGTM, just a couple of comments.

sys/amd64/include/xen/synch_bitops.h
4 ↗	(On Diff #9355)	Although there's no code in this file it probably needs a proper header license/copyright anyway.
sys/x86/include/xen/synch_bitops.h
5 ↗	(On Diff #9355)	Hm, I would really like to get a proper BSD license on this file,the following statement doesn't clarify under what license this file is in. Would it be possible to replace the code in this file with code already existing in <machine>/include/atomic.h? Or even better, unconditionally define SMP and include atomic.h, and rewrite all the synch_* ops based on the native FreeBSD atomic ops. I can commit this and fix it up later (but it will have to wait until next week).
sys/x86/include/xen/xen-os.h
31	This file could also do with some cleaning. I'm quite sure we don't need to define CONFIG_X86_PAE, and then the bitops defined below probably have native FreeBSD equivalents so we don't need to open code them. I will take a look next week.
94	Stray newline?

julien.grall_citrix.com added inline comments.Oct 15 2015, 10:23 AM

sys/amd64/include/xen/synch_bitops.h
4 ↗	(On Diff #9355)	I guess I should retain the previous copyright here?
sys/x86/include/xen/synch_bitops.h
5 ↗	(On Diff #9355)	I think unconditionally define SMP and include atomic.h would be tricky. Because we would end up to use SMP-atomic on any file include synch-ops.h Furthermore, we are screwed if machine/atomic.h is included before synch_ops.h .
sys/x86/include/xen/xen-os.h
31	I've got a branch which remove a lot of unused code [1] . I didn't merge these clean up in this review to keep it simple and avoid mixing code movement with removal. [1] http://xenbits.xen.org/gitweb/?p=people/julieng/freebsd.git;a=shortlog;h=refs/heads/dev-xen-upstream
94	I've copied this header from i386/include/xen/xen-os.h which contain a stray line compare to amd64/include/xen/xen-os.h. Although the review tools thinks I copied from amd64/include/xen/xen-os.h hence the "stray newline". Anyway, I will drop it in the next revision.

LGTM, the only comment left is about licensing and whether we can get rid of the Linus copyright in synch_bitops.h. I know this is not your fault, I can take a stab at fixing it next week probably.

sys/amd64/include/xen/synch_bitops.h
4 ↗	(On Diff #9355)	No, the original copyright doesn't make sense anymore, because the code has moved. By looking at other dummy files in amd64/include most of them then to use: /- This file is in the public domain. / /* $FreeBSD$ */ Note the FreeBSD keyword.
sys/x86/include/xen/synch_bitops.h
5 ↗	(On Diff #9355)	My bad, this was a poor suggestion. So I guess we are back to whether existing code in this file can be replaced with equivalent code from FreeBSD's atomic.h (copy & paste would be fine). We should certainly get rid of the Linus copyright and put this file under a proper BSD license.
sys/x86/include/xen/xen-os.h
31	Ack, no problem.
94	Phabricator can sometimes be a pain in the ass with this, sorry. If you upload a new version and can get rid of the extra newline that would be fine.

I don't think we can get a rid of the linus copyright without re-writing the code because it has been copied from the Linux headers (arch/x86/include/asm/sync_bitops.h).

julien.grall_citrix.com retitled this revision from x86/xen: Consolidate the architecture specific headers in a single place to x86/xen: Consolidate xen-os.h in a single place.Oct 15 2015, 3:01 PM

julien.grall_citrix.com updated this object.

julien.grall_citrix.com edited edge metadata.

Changes in this version:

Only consolidate xen-os.h
Stray lines and whitespaces removed

julien.grall_citrix.com added a child revision: D3921: xen: Code cleanup and small bug fixes.Oct 16 2015, 5:52 PM

Phabricator seems to have mangled the diff, I get the following when trying to apply it:

$ stg import D3880.diff
Checking for changes in the working directory ... done
Importing patch "D3880.diff" ... error: amd64/include/xen/xen-os.h: does not exist in index
error: i386/include/xen/xen-os.h: does not exist in index
error: x86/include/xen/xen-os.h: does not exist in index
stg import: Diff does not apply cleanly

Can you upload a plain diff generated with format-patch somewhere?

I've pushed a branch with this review on top of master:
http://xenbits.xen.org/gitweb/?p=people/julieng/freebsd.git;a=shortlog;h=refs/heads/arcpatch-D3880

royger accepted this revision.Oct 20 2015, 1:12 PM

royger edited edge metadata.

This revision is now accepted and ready to land.Oct 20 2015, 1:12 PM

royger closed this revision.Oct 21 2015, 10:05 AM