HV: kata-runtime support for ACRN hypervisor

[vijaydhanraj]

Add ACRN as a supported hypervisor in kata-runtime.

Fixes: #1778
Signed-off-by: Vijay Dhanraj [email protected]

[amshinde]

Thanks @vijaydhanraj for the PR.
Do you think you can split the PR into separate commits, so that it is easier to review this PR.

[vijaydhanraj]

Thanks @vijaydhanraj for the PR.
Do you think you can split the PR into separate commits, so that it is easier to review this PR.

Sure, will split the patches and update.

[devimc]

@vijaydhanraj thanks for the pull request 😃

[devimc]

this file is auto-generated, can you remove it?

[vijaydhanraj]

@vijaydhanraj power is failing in travis, could you add the equivalent or dummy (if not supported or to be added in the future ) implementation like you did in acrn_amd64.go

@jcvenegas Thanks for point out!. Instead of adding dummy functions/ interfaces for each arch, should I move virtcontainers/acrn_amd64.go file to vendor/govmm/intel/acrn. Please suggest.

[jodh-intel]

Hi @vijaydhanraj - I think you are right - the acrn support should be added to https://github.com/intel/govmm and then the runtime can leverage acrn support via the vendored in version of govmm. But let's see what @markdryan thinks.

[vijaydhanraj]

Hi @markdryan any thoughts on adding virtcontainers/acrn_amd64.go file to vendor/govmm/intel/acrn to avoid adding dummy functions to for unsupported architecture?

[egernst]

That doesn't seem correct to me.

[vijaydhanraj]

Just spoke with @amshinde. Will remove acrn_amd64.go as there is nothing specific to amd64. Will add the code to acrn_arch_base.go.

[markdryan]

Are you still interested in adding acrn support to govmm? If so, I need a bit more context and an explanation of what exactly would be added and how. For example, does acrn use qmp or does it have a totally different protocol?

[vijaydhanraj]

Hi @markdryan, I don't think the support is needed. But I do use "acrnctl" (this another binary) for interacting with the ACRN-DM. It works using sockets underneath but this is abstracted from the user (application). The user(or app) just uses the command it exposes. For example to hot-plug container rootfs, the command will be acrnctl blkrescan vm_name slot,newpath. (similar to Firecracker use case)

[devimc]

would you mind raising an issue and mentioning it here?, that way we can track it

[vijaydhanraj]

Removed TODO and raised #1784.

[devimc]

Use WithField instead of Errno=%d

[vijaydhanraj]

done

[devimc]

raise an issue and mention it here

[vijaydhanraj]

Trimmed HV config passed to ACRN and removed TODO.

[devimc]

2019 ?

[vijaydhanraj]

done

[devimc]

issue 😄

[vijaydhanraj]

Raised #1785 to track the issue.

[devimc]

don't use format specifiers, use WithField or WithFields

[vijaydhanraj]

done

[devimc]

why? please add more details or raise an issue and mention it here

[vijaydhanraj]

Is getThreadIDs() implementation mandatory to support ACRN with kata. If not, I can leave it as it is and just return success.
Please suggest.

[vijaydhanraj]

This is not supported, so just returning success. Removed the TODO.

[egernst]

we may want to have a generic 'not supported' error returned? . existing implementation would work fine. getThreadIds is only used for applying appropriate cgroups on host, which isn't relevant here.

[vijaydhanraj]

Thanks, done

[devimc]

2019

[vijaydhanraj]

done

[devimc]

2019

[vijaydhanraj]

done

[jcvenegas]

@vijaydhanraj power is failing in travis, could you add the equivalent or dummy (if not supported or to be added in the future ) implementation like you did in acrn_amd64.go

[WeiZhang555]

We have another Hypervisor support now 😄

[bergwolf]

Why cgo? All you need are the two data structures and the ioctl number. You can easily define them in go.

[vijaydhanraj]

Hi @bergwolf, 1) without cgo is it still possible to ensure that the address of the struct will be 8-byte memory aligned? Not sure what is the replacement for attribute(aligned(8)). 2) I only need to initialize UUID to check if a VM can be created using ACRN but declaring acrn_create_vm as go struct results in make-check failing with unused parameter error for other variables in the struct. 3) Would like to retain the same name of the member variables as defined in the kernel, but declaring the struct as go, requires we remove "_" from member names.

[bergwolf]

@vijaydhanraj
1). The go specification requires that

2. For a variable x of struct type: unsafe.Alignof(x) is the largest of all the values unsafe.Alignof(x.f) for each field f of x, but at least 1.

So the acrn_create_vm struct is 8-byte aligned on 64 bit architectures as it has uint64_t in it.

2). you can use //nolint to disable it, see https://github.com/golangci/golangci-lint#nolint

[markdryan]

(updating my comment as it wasn't very helpful, updates in italics)

without cgo is it still possible to ensure that the address of the struct will be 8-byte memory aligned? Not sure what is the replacement for attribute(aligned(8)).

It also depends on where and how the struct variable is defined. From the go docs

The first word in a variable or in an allocated struct, array, or slice can be relied upon to be 64-bit aligned.

Apart from that there's currently no "safe" way to request an arbitrary alignment for a variable. However, as @bergwolf there is a way to request 8-byte alignment for structs, which is all you need.

[vijaydhanraj]

Thanks @bergwolf , @markdryan. Have removed the cgo and created go structs. Have also used nolint to suppress few variable names.

[amshinde]

Why is this empty?

[vijaydhanraj]

This is only available for ACRN. Since it is not the default hypervisor, set it to be empty.

[amshinde]

I am asking because, below you are checking if h.Path is empty and assigning it to defaultHypervisorCtlPath if its empty.

[vijaydhanraj]

Good catch. This should have been h.CtlPath instead of h.path.

[amshinde]

Is there a reason you want to provide the virtpath instead of PCIAddr here?

[vijaydhanraj]

This change corresponds to kata-containers/agent#574. In the runtime, since the virpath is known (as the container rootfs in not hot-plugged but a dummy one is created and later updated), it is better to provide this information to the agent directly, rather than providing PCIAddr and then agent finding out the virtpath.

[amshinde]

Fair enough.

[bergwolf]

Does ACRN hypervisor support following features (as they are listed in the config file)?

1. block device cache control
2. 9pfs
3. vsock
4. vfio passthrough
5. vm template

[vijaydhanraj]

ACRN currently doesn't support these options. I have removed them.

[bergwolf]

This would effectively invalidate kvm check when vmVMM is true. Please do not assume acrn is the only hypervisor we support inside a guest.

[vijaydhanraj]

I see 2 options here,
Option 1: If onVMM is true, first check if ACRN is supported. If that fails, try KVM.
Option 2: Open the configuration.toml and check the hypervisor currently being used. Based on the hypervisor, check if the VM container can be created.

Let me know which is preferred. If you have any other suggestions, please let me know.

[bergwolf]

Another options is to always check if kvm is usable. And if that fails, check if we are on vmm and if acrn is usable.

Since it becomes hypervisor specific, your option 2 might be better. @jodh-intel wdyt?

[vijaydhanraj]

Hi @jodh-intel any thoughts on the above approach?

[vijaydhanraj]

I see 2 options here,
Option 1: If onVMM is true, first check if ACRN is supported. If that fails, try KVM.
Option 2: Open the configuration.toml and check the hypervisor currently being used. Based on the hypervisor, check if the VM container can be created.

Let me know which is preferred. If you have any other suggestions, please let me know.

Hi @jodh-intel I was referring to this. I have implemented option#2 for now as it seemed generic. But please let me know if you have something else in mind.

[bergwolf]

In that case acrn should increase its BlockIndex when launching the guest, instead of hacking the index number on hotplug.

[vijaydhanraj]

globalIdx here is used to find the corresponding virtpath (/dev/vdX) starting with /dev/vda. Not sure how to get this information in ACRN which doesn't maintain the global mapping. This approach is very similar to the Firecracker approach. I believe this issue (#1061) should address this workaround as well. Let me know if I misunderstood.

[bergwolf]

@vijaydhanraj index is maintained at the sandbox level and there are helper functions for ACRN to use. You can call getAndSetSandboxBlockIndex() to bump it once and for all when launching the guest ensuring that the first index is taken by the vm rootfs.

[vijaydhanraj]

ok thanks @bergwolf . I am bumping the index using getAndSetSandboxBlockIndex when acrn appendImage() is called and have left the globalIdx unmodified.

[jodh-intel]

const? Or maybe retain as var if you add tests that need to change this.

[vijaydhanraj]

made this as a const.

[jodh-intel]

Could you create a var/const for the value here as it's now used multiple times?

[vijaydhanraj]

created const for all the values

[jodh-intel]

In fact, it might be worth creating variables for these duplicated strings too (although not strictly required for this PR since they are already duplicated in master).

[vijaydhanraj]

created const for all the values

[jodh-intel]

This line seems redundant as the following line is logging it in structured format?

[vijaydhanraj]

ah missed removing the earlier log. done.

[jodh-intel]

Nit: extraneous blank line.

[vijaydhanraj]

done.

[jodh-intel]

There is an extra line of output when you run make:

 $(CONFIG_QEMU) $(CONFIG_NEMU) $(CONFIG_FC) $(CONFIG_ACRN)     # <--- What is this? :)
     GENERATE data/kata-collect-data.sh
     GENERATE cli/config-generated.go
kata-runtime - version 1.8.0-alpha1 (commit addf7ec37319a15e7b6e7a52ad470d0dcdc421e0)

[vijaydhanraj]

oops, that was a debug print. Removed it.

[jodh-intel]

Hi @vijaydhanraj - thanks, but I think you may have forgotten to re-push?

[vijaydhanraj]

Hi @jodh-intel I was looking for your thoughts on (#1779) before pushing. But now have implemented option#2 and have pushed the changes. Please let me know what you think.

[jodh-intel]

? Do you mean the proxy PR? 😕

[caoruidong]

/test

[jodh-intel]

Hi @vijaydhanraj - I've picked over this a little more. I think we need @mcastelino and/or @amshinde to tal at the networking parts.

A general comment: this is a big code change. Can you add some unit tests to support it?

[jodh-intel]

Should this be nolint too?

[vijaydhanraj]

added nolint to suppress unused variables. uuid is used when creating VM. Please ref to lines 228-238 in the file.

[jodh-intel]

Could you add a comment explaining why this is needed?

[vijaydhanraj]

yes, have added comments.

[jodh-intel]

Please remove all commented out code and add back if necessary in a future PR.

[vijaydhanraj]

Retained the comments and removed the commented code.

[jodh-intel]

Shouldn't this be called createDummyVirtBlkDev (capital D and no i)? A comment explaining the point of this function would be useful imho.

[vijaydhanraj]

Good catch, this was a typo and I meticulously followed it everywhere :). Changed this to createDummyVirtioBlkDev

[jodh-intel]

Please can you retain the comment and the URL but remove the commented out code. We want to keep the code as clean as possible. Once that bug is fixed, re-introduce the code.

[vijaydhanraj]

yes, retained the comment and removed the commented code.

[jodh-intel]

Try to use one log call per issue. I think you can combine these two log calls by adding all the fields into a single call?

[vijaydhanraj]

We need both logs here. "launching acrn with" log gives the params with which acrn is launched. This will be handy to check how the VM was launched. The other log is in case of error and it doesn't log the params.

[jodh-intel]

Simplify to:

defer imageFile.Close()

... but I'm confused - why are you opening this file? If you want to check it exists, can't you just stat it?

[vijaydhanraj]

used stat() here.

[jodh-intel]

You've used this string already above so I'd consider creating a const for it "just in case" it ever needs to change.

[vijaydhanraj]

added const for "lpc". But this will not be changed.

[jodh-intel]

I don't understand what is happening here - can you remove the case ModelEnlightened commented out code?

[vijaydhanraj]

removed the dead code/comment.

[jodh-intel]

Do you need a default case here (returning an error`)?

[vijaydhanraj]

appendNetwork() just returns a Device. The error will be an empty device or no device being added to the devices array. But did add default case and log indicating no support.

[amshinde]

@vijaydhanraj Does ACRN support virtio-scsi and nvdimm?
If ACRN does not support 9p, then I suppose it works only with devicemapper?. If so we need to document this here.

[vijaydhanraj]

No it doesn't support virtio-scsi or nvdimm. Added a note in the toml file.

[amshinde]

I am asking because, below you are checking if h.Path is empty and assigning it to defaultHypervisorCtlPath if its empty.

[amshinde]

Can you add explanation for why this devices is needed?

[vijaydhanraj]

LPC device is nothing but UART. Will revisit and see if this can be removed.

[vijaydhanraj]

Hi @amshinde, if there will be no need for UART device, I can remove this. This is primarily used for guest OS serial console and IO(keyboard, mouse).
Does VM debug console require a serial device for its console?

[amshinde]

Can you add documentation here explaining what this function does and why we need a pool of dummy devices?

[vijaydhanraj]

done.

[amshinde]

Are macvtap devices not supported? If so, how come you are handling macvtap devices in AcrnNetdevParam func above?

[vijaydhanraj]

macvtap is supported. changed to true. Good catch!

[amshinde]

Do you support these in ACRN?

[vijaydhanraj]

ACRN only supports VethEndpoint (bridge +Tap)and MACVTAP. Have removed BridgedMacvlanEndpoint and IPVlanEndpoint.

[amshinde]

Fair enough.

[vijaydhanraj]

d some unit tests to support it?

yes will work on adding few unit tests.

[jodh-intel]

We now have https://github.com/kata-containers/tests/blob/master/Unit-Test-Advice.md to help wrt adding unit tests.

[vijaydhanraj]

We now have https://github.com/kata-containers/tests/blob/master/Unit-Test-Advice.md to help wrt adding unit tests.

Thanks @jodh-intel

[vijaydhanraj]

We now have https://github.com/kata-containers/tests/blob/master/Unit-Test-Advice.md to help wrt adding unit tests.

Thanks @jodh-intel

Hi @jodh-intel I have added the test cases for acrn. Please let me know if anything else is needed.

[jodh-intel]

Thanks @vijaydhanraj !

lgtm

[devimc]

/test

[GabyCT]

/test

[vijaydhanraj]

Hi @amshinde, @jodh-intel: Even with suggested change in using c.App.Metadata["runtimeConfig"] instead of directly using loadconfiguration, I see the firecracker and nemu tests are failing. Am I missing anything here?

Failure:
07:50:13 time="2019-07-09T14:50:13Z" level=error msg="/usr/share/defaults/kata-containers/configuration-qemu.toml: file /usr/bin/qemu-system-x86_64 does not exist" arch=amd64 name=kata-runtime pid=31316 source=runtime
07:50:13 /usr/share/defaults/kata-containers/configuration-qemu.toml: file /usr/bin/qemu-system-x86_64 does not exist
07:50:13 Failed at 44: sudo -E PATH=$PATH "$kata_runtime_path" kata-check
07:50:13 Failed at 41: "${cidir}/install_runtime.sh"

[jodh-intel]

@vijaydhanraj - I think you may be hitting #1779 (comment) now; the runtime will load configuration.toml and the default packaged version of that file specifies qemu as the hypervisor. Hence, we prolly need to update install_runtime.sh to switch configuration.toml to link to configuration-acrn.toml.

[vijaydhanraj]

@vijaydhanraj - I think you may be hitting #1779 (comment) now; the runtime will load configuration.toml and the default packaged version of that file specifies qemu as the hypervisor. Hence, we prolly need to update install_runtime.sh to switch configuration.toml to link to configuration-acrn.toml.

Hi @jodh-intel, should I update for acrn here https://github.com/kata-containers/tests/blob/master/.ci/install_runtime.sh#L54..L62?
Also, the error seems to happen at https://github.com/kata-containers/tests/blob/1e98659a8695521a337fa67306e6fc679d1fe4a6/.ci/install_runtime.sh#L44 before the hypervisor check.

[jodh-intel]

Not sure without digging into this. Any thoughts @chavafg?

[vijaydhanraj]

thanks @chavafg @amshinde @jodh-intel : Root caused the failures to the recent change in beforeSubcommands() function. When installing_runtime (qemu, firecracker and nemu), the script first calls kata-check and then loads the appropriate configuration.toml file. This was not an issue earlier as beforeSubCommands() would just return nil if the command was kata-check without calling LoadConfiguration() function. So the failure wasn't seen with firecracker or nemu. But now even for kata-check LoadConfiguration() is called by beforeSubComamnds() which works fine for qemu as it is default but fails for firecracker or nemu.

Fix:

diff --git a/.ci/install_runtime.sh b/.ci/install_runtime.sh
index 06968be..fe0c8cf 100755
--- a/.ci/install_runtime.sh
+++ b/.ci/install_runtime.sh
@@ -39,6 +39,21 @@ NEW_RUNTIME_CONFIG="${PKGDEFAULTSDIR}/configuration.toml"
 # Note: This will also install the config file.
 build_and_install "github.com/kata-containers/runtime" "" "true"
 
+if [ "$KATA_HYPERVISOR" = "firecracker" ]; then
+       echo "Enable firecracker configuration.toml"
+       sudo mv "${PKGDEFAULTSDIR}/configuration-fc.toml" "${PKGDEFAULTSDIR}/configuration.toml"
+fi
+
+if [ "$KATA_HYPERVISOR" = "nemu" ]; then
+       echo "Enable nemu configuration.toml"
+       sudo mv "${PKGDEFAULTSDIR}/configuration-nemu.toml" "${PKGDEFAULTSDIR}/configuration.toml"
+fi
+
+if [ "$KATA_HYPERVISOR" = "acrn" ]; then
+       echo "Enable acrn configuration.toml"
+       sudo mv "${PKGDEFAULTSDIR}/configuration-acrn.toml" "${PKGDEFAULTSDIR}/configuration.toml"
+fi
+
 # Check system supports running Kata Containers
 kata_runtime_path=$(command -v kata-runtime)
 sudo -E PATH=$PATH "$kata_runtime_path" kata-check
@@ -51,16 +66,6 @@ if [ -e "${NEW_RUNTIME_CONFIG}" ]; then
        runtime_config_path="${NEW_RUNTIME_CONFIG}"
 fi
 
-if [ "$KATA_HYPERVISOR" = "firecracker" ]; then
-       echo "Enable firecracker configuration.toml"
-       sudo mv "${PKGDEFAULTSDIR}/configuration-fc.toml" "${PKGDEFAULTSDIR}/configuration.toml"
-fi
-
-if [ "$KATA_HYPERVISOR" = "nemu" ]; then
-       echo "Enable nemu configuration.toml"
-       sudo mv "${PKGDEFAULTSDIR}/configuration-nemu.toml" "${PKGDEFAULTSDIR}/configuration.toml"
-fi
-
 if [ -z "${METRICS_CI}" ]; then
        echo "Enabling all debug options in file ${runtime_config_path}"
        sudo sed -i -e 's/^#\(enable_debug\).*=.*$/\1 = true/g' "${runtime_config_path}"

@chavafg: Can you please help with this change?

[chavafg]

Hi @vijaydhanraj, opened kata-containers/tests#1793 to address the nemu and fc issues.

[vijaydhanraj]

Hi @vijaydhanraj, opened kata-containers/tests#1793 to address the nemu and fc issues.

Thanks @chavafg!!

[chavafg]

/test

[amshinde]

@chavafg @GabyCT The fc job seems to not been triggered. Can you take a look here?

[chavafg]

@GabyCT recently changed the name of the job, so it is now shown as jenkins-ci-ubuntu-1804-firecracker and passed correctly.

[amshinde]

Thanks @chavafg.
@bergwolf @devimc Does this PR look good to you?

[WeiZhang555]

Ping @bergwolf @vijaydhanraj @amshinde @devimc @jodh-intel

[WeiZhang555]

Here you stored a.info, but before this you just Load and Store a.state, so which one do you really mean to save? a.info or a.state ?

[vijaydhanraj]

Hi @WeiZhang555 @amshinde @bergwolf , plan is to store both a.state and a.info. As a part a.state UUID related information will be stored. Since this is WIP, removed the code and just added comment and issue ID in its place. Once the issue is addressed through a new PR, this will not be the case.
Ref: #1785

[WeiZhang555]

I think you must save and load same information always, if you save/load a.info here and save/load a.state there, it will bring chaos.

If you plan to store both a.state and a.info, then you should merge them and save them together.

@vijaydhanraj

[WeiZhang555]

Then you stored a.info again. I guess a.info is the real data you want to save?