First pass at reducing the diff between bhyve and hyperkit #73

rn · 2016-10-23T18:58:39Z

xhyve/hyperkit was derived from bhyve and shares more common code with it than there are differences. This is a first pass at trying to reduce the difference and mostly does two things:

whitespace cleanups. Bring hyperkit code more inline with bhyve formatting
clean up casts introduced in xhyve. While the casts are necessary to compile with clang, xhyve used a lot of unnecessary parentheses and whitespaces. Cleaning them up increases the chance of upstreaming them to bhyve.

/cc @avsm @luigirizzo

Remove mirage-block-unix pin

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

Use the initrd_addr_max value from the kernel header if the kernel version is 2.03 or newer. Use hardcoded(0x37ffffff) value if older. Compare the initrd_max and memory.size, and re-assign if initrd_max is larger. Add ALIGNDOWN for ramdisk_start, uses prevent to go past the address limit. Based on qemu/hw/i386/pc.c, Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

kexec: fix wrong calculation method of ramdisk_start

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

Also adjust the make variables and their names Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

[RFC] restructure the layout of the source code

Various bits of code (e.g. kernel and initrd loading) rely on memory.base and memory.size referring to lowmem only, with highmem being handled separately, in order to avoid locating things into the MMIO region between the two. Rename the variable to make this more obvious and to forestall any ideas about "fixing" memory.size. Signed-off-by: Ian Campbell <ian.campbell@docker.com>

kexec: Rename "memory" to "lowmem".

… tracker We get a small but steady stream of Docker for Mac bug reports filled against Hyperkit, many of which are not actually Hyperkit issues. Try and direct users to file the bugs downstream by default so that they can be more effectively triaged. Signed-off-by: Ian Campbell <ian.campbell@docker.com>

ijc · 2016-10-24T09:10:41Z

src/lib/acpitbl.c

 	/* fixup table */
-	acpitbl_write32(rdsp, 0x10, ((uint32_t) (XHYVE_ACPI_BASE + RSDT_OFFSET)));
-	acpitbl_write64(rdsp, 0x18, ((uint64_t) (XHYVE_ACPI_BASE + XSDT_OFFSET)));
+	acpitbl_write32(rdsp, 0x10, (uint32_t)XHYVE_ACPI_BASE + RSDT_OFFSET);


This ones a bit subtle, depending on how the constants are defined. If RSDT_OFFSET were 0xXXXXUL (which wouldn't be unreasonable) then I think the uint32_t + unsigned long would potentially be promoted to an unsigned long, despite the first operand having been cast to uint32_t, which wasn't quite as intended.

We could write it as (unt32_t)(FOO + BAR) (i.e. just fix the whitespace not the braces) but perhaps we would be better off letting the implicit conversion (due to the argument to acpitbl_write32 taking a uint32_t) DTRT? That might mean changing the constants to be unsigned, but arguably that would be correct in its own right?

BTW, I would be fine with a "lets apply as is now and reconsider later" answer here...

I'll fix it. was a bit too eager removing parentheses

ijc · 2016-10-24T09:24:22Z

src/lib/block_if.c

+			else
+				br->br_resid = 0;
+		} else
+#else


Shall we just delete this? We can always get it back from git if we need it..

was contemplating this as well. Yeah, I'll remove it

ijc · 2016-10-24T09:31:53Z

src/lib/pci_emul.c


 	if ((size & (size - 1)) != 0)
-		size = 1UL << flsl((long) size); /* round up to a power of 2 */
+		size = 1UL << flsl((long)size); /* round up to a power of 2 */


This is an existing problem, but isn't this chopping the top bit off (size is a uint64_t, long is a signed probably 64-bit value). Maybe we want the long long version? Obviously size > 2^63 is not something we should worry about in practice, except I'm not 100% convinced there isn't some UD here.

I think the signed-ness of the variable should not matter for flsl as it would just look at the bits. long is indeed 64bit so this should work...though i'm going to change this to long long which is a bit more correct, I think, at least on 32bit systems

ijc · 2016-10-24T09:33:46Z

src/lib/pci_virtio_net_tap.c

 #define VTNET_MAXSEGS 32

+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wunused-macros"


I think we should just add this to CFLAGS

ijc · 2016-10-24T09:37:51Z

LGTM, I made some comments which were mostly asides rather than issues with the changes. Only one worth thinking about is turning off Wunused-macros globally via CFLAGS rather than file by file.

While the casts are necessary to compile with clang

I didn't spot any casts which didn't look like good practice (as opposed to just placating the compiler), but isn't FreeBSD (and hence bhyve) compiling with clang by default these days too? Is it just that Apple's version of clang is defaulting to more warnings, or is it that Hyperkit's config.mk is turning on lots of extra stuff? Just curious why these casts haven't already been found necessary in bhyve.

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

Also re-arrange CFLAGS_WARN so that disabling warnings is at the end and in alphabetical order. Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

Remove excessive parentheses. Perform pointer arithmetic on (char *) instead of casting to a uintptr_t and back to pointer. While at it, also do some minor coding style clean up. Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

- removed disabled code (and added a comment) - remove excessive parenthesis in conditionals and casts - fix indentation Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

While at it also fix up some whitespaces to redice diff with bhyve. Note: bhyve's version of inout.c is very untidy wrt to whitespaces. this patch does not copy this untidiness. Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

While at it, do some minor whitespace cleanup to reduce the diff with bhyve and replace a flsl() with a slightly more correct flsll (this shouldn't matter on 64bit systems). Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

While at it, adjust some whitespaces to reduce the diff to bhyve Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

While the network backends are quite different, they still share a lot of common code. Unify it as much as possible. While at it, remove some superfluous parentheses from casts and asserts. Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

While at it, also tidy up a few casts. Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

GordonTheTurtle · 2016-10-24T16:44:12Z

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "cleanup" git@github.com:rneugeba/hyperkit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~28
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

avsm · 2016-10-24T16:52:07Z

src/lib/block_if.c

 };

 struct blockif_ctxt {
-	int bc_magic;


do we know why an assert is appropriate here instead of a dynamic check? All the lseeks appear to assume success.

Did GH misplace your comment? there seems to be no assert in the context.

It looks like everything got very confused after a force push...

avsm · 2016-10-24T16:53:40Z

src/lib/block_if.c

would just prefer to keep this codebase clean without the "bhyve removal" comments -- its pretty obvious from the perror that alternative implementations could exist.

I added the comment, because in a previous version i had #if 0 and the original version had the bhyve code commented out. Both of which make it blindingly obvious that something from the original source was hacked to make it work on OSX. I'm for removing dead code, but do prefer to leave a breadcrumb indicating that something may have changed due to a hack, just in case someone is debugging this code in a few months time.

Just brainstorming some alternatives...

An #ifdef BHYVE or #ifdef __FreeBSD__ might be nicer than the big block of commented code (which == a big hunk of diff vs upstream) and not as objectionable as the breadcrumb comment?

In some cases perhaps a #define FEATURE_BLAH 0 would allow some of the if (bh->something) blocks to turn into if(FEATURE_BLAH && bh->something) and clang could still do dead code elimination.

I think I like that, ie either __FreeBSD__ or feature (if appropriate). Will update again...though it looks like GH was royally confused with my force push which also updated the author and committer. Might have to open a new PR

Yeah, #if __FreeBSD__ sgtm

grehan-freebsd · 2016-10-24T18:09:22Z

Just curious why these casts haven't already been found necessary in bhyve

usr.sbin/bhyve is built with WARNS=2 in the Makefile - this may not be as strict as the default with clang on OSX.

More than happy to take changes which would allow a stricter WARNS value to be used on FreeBSD.

rn · 2016-10-24T19:39:11Z

@grehan-freebsd hyperkit is compiled with -Weverything and then some exceptions added afterwards. Some/most of the casts seem like good practise (like not assuming a void * points to something of size 1 byte, unsigned vs signed arithmetic, assignments to smaller data types etc)

I currently don't have a FreeBSD dev environment set up, but we are definitely interested in reducing the diff, especially for the device emulation portions. It might take a few more passes before we get there, re @luigirizzo email to emulation@freebsd.org

luigirizzo · 2016-10-25T09:29:11Z

Peter, Rolf: I am in favour of strict compiler warnings, the only one I would remove in CFLAGS is -Wpadded because it seems to make life miserable: it complains both when a struct is declared __packed but no padding would be inserted anyways, and when it is not __packed but padding is inserted. The whole point of using __packed is tell the compiler which structs should be packed and which should not, then leave us alone.

luigirizzo · 2016-10-25T09:35:38Z

Regarding diff reduction with bhyve, given that the task is much larger than I expected, I would approach it in an opportunistic way, i.e. as we go through a subsystem (e.g. for merging a new device emulation or fixing a feature) also take the chance to revert the gratuitous changes applied in the port (whitespace, file renaming, changes in function prototypes). Likewise for importing changes into FreeBSD.

ijc · 2016-10-25T10:41:08Z

@luigirizzo I couldn't agree more regarding -Wpadded!

thebsdbox · 2016-10-25T11:38:55Z

Given most of the bhyve code is focussed on contexts of a number of VMs, would you look at reinstating the code to act in the following manner (although always referencing the same context as hyperkit doesn't run multiple VMs the same way bhyve does). This would make it much simpler to import the additional pci devices that have been added into bhyve e.g.

E1000
VNC driver
- USB device emul (needed by vnc for mouse/kb) <- this being the worst to rely on numerous vm contexts.

(just a thought)

rn · 2016-10-25T12:59:27Z

I haven't looked at the context structure in FreeBSD in too much detail yet, but re-instating it would maybe make sense. However, there are q fair few number of lower hanging fruits, which I would pick first

rn · 2016-11-02T20:12:14Z

closing this pull request. GH got confused with rewriting the git authot. Will open an updated one shortly.

rn and others added 15 commits October 11, 2016 20:48

Merge pull request moby#64 from djs55/remove-mirage-block-unix-pin

51d7e6a

Remove mirage-block-unix pin

kexec: fix typo for initrd_addr_max

cefa6ce

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>

Merge pull request moby#66 from zchee/kexec-ramdisk_start

ef5a500

kexec: fix wrong calculation method of ramdisk_start

Add .opam to the .gitignore in case one is using a local opam repo

2f8339e

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

src: rename xhyve.c to hyperkit.c

b69717c

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

src: move the bulk of the source code into ./src/lib

3c70454

Also adjust the make variables and their names Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

src: move include files into ./src

226c87c

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

test: move exp file into the test sub-directory

1876075

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

lib: remove unused files

bb4d23c

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

build: re-arrange source code files in alphabetical order

1bb868f

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

Merge pull request moby#65 from rneugeba/reorg

91630ed

[RFC] restructure the layout of the source code

Merge pull request moby#68 from ijc25/master

54b81a6

kexec: Rename "memory" to "lowmem".

GordonTheTurtle added the status/0-triage label Oct 23, 2016

ijc reviewed Oct 24, 2016

View reviewed changes

rn added 9 commits October 24, 2016 13:51

docs: Add coding style *guidelines*

3158ca2

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

build: disable warning on un-used macros

a029bc6

Also re-arrange CFLAGS_WARN so that disabling warnings is at the end and in alphabetical order. Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

acpitbl: clean up casts

becdebc

Remove excessive parentheses. Perform pointer arithmetic on (char *) instead of casting to a uintptr_t and back to pointer. While at it, also do some minor coding style clean up. Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

blockif: reduce diff to bhyve and cleanup

af8d2ac

- removed disabled code (and added a comment) - remove excessive parenthesis in conditionals and casts - fix indentation Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

inout: remove superfluous parentheses from casts

7a621b2

While at it also fix up some whitespaces to redice diff with bhyve. Note: bhyve's version of inout.c is very untidy wrt to whitespaces. this patch does not copy this untidiness. Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

pci_emul: remove superfluous parentheses

6ce601a

While at it, do some minor whitespace cleanup to reduce the diff with bhyve and replace a flsl() with a slightly more correct flsll (this shouldn't matter on 64bit systems). Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

pci_irq: remove superfluous parentheses

574bb13

While at it, adjust some whitespaces to reduce the diff to bhyve Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

pci_lpc: remove superfluous parentheses from casts

ee2f71e

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

pci_uart: remove superfluous parentheses from casts

74db4cc

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

rn added 4 commits October 24, 2016 17:28

pci_virtio_block: reduce diff to bhyve

62c66b5

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

pci_virtio_net: reduce diff to bhyve

99c6e20

While the network backends are quite different, they still share a lot of common code. Unify it as much as possible. While at it, remove some superfluous parentheses from casts and asserts. Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

pci_virtio_rnd: Mostly whitespace changes to reduce diff to bhyve

a2c92c0

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

smbiostbl: whitespace cleanup to reduce diff to bhyve

d93a4e3

While at it, also tidy up a few casts. Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

rn force-pushed the cleanup branch from 83729b2 to d93a4e3 Compare October 24, 2016 16:44

GordonTheTurtle added the dco/no label Oct 24, 2016

avsm reviewed Oct 24, 2016

View reviewed changes

rn closed this Nov 2, 2016

ijc mentioned this pull request Nov 3, 2016

First pass at reducing the diff between bhyve and hyperkit #75

Merged

First pass at reducing the diff between bhyve and hyperkit #73

First pass at reducing the diff between bhyve and hyperkit #73

Uh oh!

Conversation

rn commented Oct 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijc commented Oct 24, 2016

Uh oh!

GordonTheTurtle commented Oct 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grehan-freebsd commented Oct 24, 2016

Uh oh!

rn commented Oct 24, 2016

Uh oh!

luigirizzo commented Oct 25, 2016

Uh oh!

luigirizzo commented Oct 25, 2016

Uh oh!

ijc commented Oct 25, 2016

Uh oh!

thebsdbox commented Oct 25, 2016

Uh oh!

rn commented Oct 25, 2016

Uh oh!

rn commented Nov 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants