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

[julien.grall_citrix.com]

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.

[julien.grall_citrix.com]

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

[royger]

LGTM, just a couple of comments.

sys/amd64/include/xen/synch_bitops.h

4

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

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]

sys/amd64/include/xen/synch_bitops.h

4

I guess I should retain the previous copyright here?

sys/x86/include/xen/synch_bitops.h

5

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.

[royger]

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

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

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.

[julien.grall_citrix.com]

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]

Changes in this version:

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

[royger]

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?

[julien.grall_citrix.com]

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