Code Review - Updates to vmrunkernel from vmm-akaros

[Michael Taufen]

The following changes since commit ca3efb4056d3c531983821c023ecb7ca6796f6c2:

  mlx4: Enable QP destruction (2016-02-10 18:26:46 -0500)

are available in the git repository at:

  g...@github.com:mtaufen/akaros.git updates-from-vmm-akaros

for you to fetch changes up to 15c0bfc76465921f0f88d163d99a683fde1302a1:

  Updates from vmm-akaros (2016-02-10 16:31:12 -0800)

----------------------------------------------------------------

Michael Taufen (1):

      Updates from vmm-akaros

 kern/arch/x86/trap.c          |  12 ++--

 kern/arch/x86/vmm/intel/vmx.c |   2 +-

 tests/vmm/vmrunkernel.c       | 247 ++++++++++++++++++++++++++++++++++++++++++++++++-----------------------

 3 files changed, 175 insertions(+), 86 deletions(-)

[Barret Rhoden]

Hi -

Minor comments below. Btw, do I need to be using the latest linux
kernel from g...@github.com:rminnich/linux.git for this?


On 2016-02-11 at 00:36 "'Michael Taufen' via Akaros"

<aka...@googlegroups.com> wrote:
> The following changes since commit
> ca3efb4056d3c531983821c023ecb7ca6796f6c2:
>
> mlx4: Enable QP destruction (2016-02-10 18:26:46 -0500)
>
> are available in the git repository at:
>
> g...@github.com:mtaufen/akaros.git updates-from-vmm-akaros

> From 15c0bfc76465921f0f88d163d99a683fde1302a1 Mon Sep 17 00:00:00 2001
> From: Michael Taufen <mta...@gmail.com>
> Date: Wed, 10 Feb 2016 09:37:58 -0800
> Subject: Updates from vmm-akaros

> diff --git a/kern/arch/x86/vmm/intel/vmx.c b/kern/arch/x86/vmm/intel/vmx.c

> @@ -1,4 +1,4 @@
> -//#define DEBUG
> +#define DEBUG

Looks like this debugging bit slipped into this commit.


> diff --git a/tests/vmm/vmrunkernel.c b/tests/vmm/vmrunkernel.c

> -/* Kind of sad what a total clusterf the pc world is. By 1999, you could just scan the hardware
> - * and work it out. But 2005, that was no longer possible. How sad.
> +/* Kind of sad what a total clusterf the pc world is. By 1999, you could just scan the hardware
> + * and work it out. But 2005, that was no longer possible. How sad.
> * so we have to fake acpi to make it all work. !@#$!@#$#.
> * This will be copied to memory at 0xe0000, so the kernel can find it.
> */

Checkpatch complained about this - line over 80 chars, with no need for
it. I know most of this code is going to move around or be removed, so
it'd be nice to fix it eventually.

> +
> + /* Allocate memory for, and zero the bootparams
> + * page before writing to it, or Linux thinks
> + * we're talking crazy.
> + */
> + a += 4096;
> + bp = a;
> + memset(bp, 0, 4096);
> +
> + /* Set the kernel command line parameters */
> + a += 4096;
> + cmdline = a;
> + a += 4096;

For a future commit, we're going to need to sort out a better way to do
allocations for the guest physical memory, so that we don't need to have
everything in one giant function.

> + bp->hdr.cmd_line_ptr = (uintptr_t) cmdline;
> + sprintf(cmdline, "earlyprintk=vmcall,keep"
> + " console=hvc0"
> + " virtio_mmio.device=1M@0x100000000:32"
> + " nosmp"
> + " maxcpus=1"
> + " acpi.debug_layer=0x2"
> + " acpi.debug_level=0xffffffff"
> + " apic=debug"
> + " noexec=off"
> + " nohlt"
> + " init=/bin/sh"
> + " lapic=notscdeadline"
> + " lapictimerfreq=1000"
> + " pit=none");
> +
> +
> + /* Put the e820 memory region information in the boot_params */
> + bp->e820_entries = 3;
> + int e820i = 0;
> + bp->e820_map[e820i].addr = 0;
> + bp->e820_map[e820i].size = 16*1048576;
> + bp->e820_map[e820i++].type = E820_RESERVED;
> +
> + bp->e820_map[e820i].addr = 16*1048576;

Minor formatting thing: please put spaces around operators. Checkpatch
doesn't seem to catch these for some reason.

> + bp->e820_map[e820i].size = 128*1048576;
> + bp->e820_map[e820i++].type = E820_RAM;
> +
> + //bp->e820_map[2].addr = 4096*1048576ULL;
> + //bp->e820_map[2].size = 2*1048576;
> + //bp->e820_map[2].type = E820_RAM;

Do we need this commented out stuff, or was it just for debugging?

> + bp->e820_map[e820i].addr = 0xf0000000;
> + bp->e820_map[e820i].size = 0x10000000;
> + bp->e820_map[e820i++].type = E820_RESERVED;
> +

[Michael Taufen]

> Looks like this debugging bit slipped into this commit. 

Got it.

> Checkpatch complained about this - line over 80 chars, with no need for 
> it.  I know most of this code is going to move around or be removed, so 
> it'd be nice to fix it eventually. 

I'll have to talk to Ron to see exactly what he was getting at with this comment.

For now, I just shortened the line so Checkpatch won't complain.

> For a future commit, we're going to need to sort out a better way to do
> allocations for the guest physical memory, so that we don't need to have 
> everything in one giant function. 

Agreed, this is on my todo list. I'll probably look at refactoring vmrunkernel.c after I finish

the floating point stuff. The most trivial upgrade in this case would be to just hide the 

bump-allocated pointer behind an allocation function.

New pull request:

The following changes since commit ca3efb4056d3c531983821c023ecb7ca6796f6c2:

  mlx4: Enable QP destruction (2016-02-10 18:26:46 -0500)

are available in the git repository at:

  g...@github.com:mtaufen/akaros.git updates-from-vmm-akaros

for you to fetch changes up to 6caa97691baa761a1f38c8826d79b0e28d590985:

  Updates from vmm-akaros (2016-02-13 16:11:09 -0800)

----------------------------------------------------------------

Michael Taufen (1):

      Updates from vmm-akaros

 kern/arch/x86/trap.c    |  12 ++--

 tests/vmm/vmrunkernel.c | 238 +++++++++++++++++++++++++++++++++++++++++++++------------------------

 2 files changed, 162 insertions(+), 88 deletions(-)

[Michael Taufen]

And yes, you will want the latest kernel from the last_known_good branch of rminnich/linux.

[Michael Taufen]

Oops, one more thing. Added linux_bootparam.h file.

The following changes since commit ca3efb4056d3c531983821c023ecb7ca6796f6c2:

  mlx4: Enable QP destruction (2016-02-10 18:26:46 -0500)

are available in the git repository at:

  g...@github.com:mtaufen/akaros.git updates-from-vmm-akaros

for you to fetch changes up to 94877c39adc7ad64d43af4772edc40bf367fc6ab:

  Updates from vmm-akaros (2016-02-16 10:21:39 -0800)

----------------------------------------------------------------

Michael Taufen (1):

      Updates from vmm-akaros

 kern/arch/x86/trap.c               |  12 +--

 tests/vmm/vmrunkernel.c            | 238 ++++++++++++++++++++++++++++++++++++++--------------------

 user/vmm/include/linux_bootparam.h | 200 ++++++++++++++++++++++++++++++++++++++++++++++++

 3 files changed, 362 insertions(+), 88 deletions(-)

 create mode 100644 user/vmm/include/linux_bootparam.h

[Barret Rhoden]

On 2016-02-16 at 10:28 Michael Taufen <mta...@gmail.com> wrote:
> Oops, one more thing. Added linux_bootparam.h file.

Can you mention where you got it from (e.g. Linux 4.1 or a commit SHA),
and put an appropriate copyright header at the top? I know it's a bit
hacky, but we want to be explicit whenever we borrow code from other
projects.

[mta...@google.com]

Oops, one more thing, added linux_bootparam.h file. New official pull request:

The following changes since commit ca3efb4056d3c531983821c023ecb7ca6796f6c2:

mlx4: Enable QP destruction (2016-02-10 18:26:46 -0500)

are available in the git repository at:

g...@github.com:mtaufen/akaros.git updates-from-vmm-akaros

for you to fetch changes up to 94877c39adc7ad64d43af4772edc40bf367fc6ab:

Updates from vmm-akaros (2016-02-16 10:21:39 -0800)

----------------------------------------------------------------
Michael Taufen (1):
Updates from vmm-akaros

kern/arch/x86/trap.c | 12 +--
tests/vmm/vmrunkernel.c | 238 ++++++++++++++++++++++++++++++++++++++--------------------
user/vmm/include/linux_bootparam.h | 200 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 362 insertions(+), 88 deletions(-)
create mode 100644 user/vmm/include/linux_bootparam.h

[Michael Taufen]

Ok, will do.

--
You received this message because you are subscribed to a topic in the Google Groups "Akaros" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/akaros/lMNCvEThTXM/unsubscribe.
To unsubscribe from this group and all its topics, send an email to akaros+un...@googlegroups.com.
To post to this group, send email to aka...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[mta...@google.com]

Added copyright info and source files:

The following changes since commit ca3efb4056d3c531983821c023ecb7ca6796f6c2:

mlx4: Enable QP destruction (2016-02-10 18:26:46 -0500)

are available in the git repository at:

g...@github.com:mtaufen/akaros.git updates-from-vmm-akaros

for you to fetch changes up to ca0a17789dc07bd50a9091c7309d75692d56057a:

Updates from vmm-akaros (2016-02-16 13:41:55 -0800)

----------------------------------------------------------------
Michael Taufen (1):
Updates from vmm-akaros

kern/arch/x86/trap.c | 12 +--
tests/vmm/vmrunkernel.c | 238 ++++++++++++++++++++++++++++++++++++++--------------------

user/vmm/include/linux_bootparam.h | 215 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 377 insertions(+), 88 deletions(-)
create mode 100644 user/vmm/include/linux_bootparam.h

[Michael Taufen]

Hmm, I actually brought this file over from vmm-akaros/include/bootparam.h. On closer inspection it looks like a mash-up between a couple of files from Linux (e820.h and bootparam.h) and an additional do_not_call_bootparam_asserts function (I'll ask Ron about that one, not exactly sure what it's for yet). I'll add a copyright notice from Linux and see what Ron says about that fn.

[Michael Taufen]

Added copyright info:

The following changes since commit ca3efb4056d3c531983821c023ecb7ca6796f6c2:

  mlx4: Enable QP destruction (2016-02-10 18:26:46 -0500)

are available in the git repository at:

  g...@github.com:mtaufen/akaros.git updates-from-vmm-akaros

for you to fetch changes up to ca0a17789dc07bd50a9091c7309d75692d56057a:

  Updates from vmm-akaros (2016-02-16 13:41:55 -0800)

----------------------------------------------------------------

Michael Taufen (1):

      Updates from vmm-akaros

 kern/arch/x86/trap.c               |  12 +--

 tests/vmm/vmrunkernel.c            | 238 ++++++++++++++++++++++++++++++++++++++--------------------

 user/vmm/include/linux_bootparam.h | 215 ++++++++++++++++++++++++++++++++++++++++++++++++++++

 3 files changed, 377 insertions(+), 88 deletions(-)

 create mode 100644 user/vmm/include/linux_bootparam.h

[Michael Taufen]

Ah crud I used my google.com account again. I guess it's defaulting back to my google.com account. Doesn't help that I pretty much have the same profile picture for both :P.

[Barret Rhoden]

Thanks!

Merged to master at 23616f789424..b94d8034d685 (from, to]

You can see the entire diff with 'git diff' or at
https://github.com/brho/akaros/compare/23616f789424...b94d8034d685

Barret

On 2016-02-16 at 13:46 Michael Taufen <mta...@gmail.com> wrote:
> Added copyright info:
>
> The following changes since commit
> ca3efb4056d3c531983821c023ecb7ca6796f6c2:
>
> mlx4: Enable QP destruction (2016-02-10 18:26:46 -0500)
>
> are available in the git repository at:
>
> g...@github.com:mtaufen/akaros.git updates-from-vmm-akaros
>
> for you to fetch changes up to
> ca0a17789dc07bd50a9091c7309d75692d56057a:
>
> Updates from vmm-akaros (2016-02-16 13:41:55 -0800)
>
> ----------------------------------------------------------------
> Michael Taufen (1):
> Updates from vmm-akaros
>
> kern/arch/x86/trap.c | 12 +--
> tests/vmm/vmrunkernel.c | 238
> ++++++++++++++++++++++++++++++++++++++--------------------
> user/vmm/include/linux_bootparam.h | 215
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 377 insertions(+), 88 deletions(-)
> create mode 100644 user/vmm/include/linux_bootparam.h
>
>
> On Tuesday, February 16, 2016 at 10:44:42 AM UTC-8, Barret Rhoden
> wrote:
> >
> > On 2016-02-16 at 10:28 Michael Taufen <mta...@gmail.com

[Michael Taufen]

Yay!