Changes for successful build on aarch64 #759

dianpopa · 2018-12-10T17:39:25Z

Addressed Issue

Description of changes

Necessary changes for successful build on a A-72 Cortex (aarch64).

added dummy code for non x86_64 architecture inside dumbo and fc_util. Opened issue for adding implementation Get the processor’s time-stamp counter in aarch64 #758
x86_64 crate was replaced by the arch crate which was written in such way that the architectural specific code is conditionally compiled
syscalls that were used for seccomp filtering are architecture dependent. Added new module for conditionally compiling them.
cpuid crate is compiled only on x86_64 platforms
added workaround for being able to statically link against musl on aarch64 platforms. See rust issue.

Does firecracker now compile on aarch64?

Yes

cargo build --target aarch64-unknown-linux-musl
./target/aarch64-unknown-linux-musl/debug/firecracker

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

alexandruag

Also, I may have done something wrong, but I got some warnings when I tried to build the sources on an ARM instance. Maybe you can have a look if this also happens to you by running cargo clean first and then building.

alexandruag · 2018-12-19T04:30:03Z

arch/Cargo.toml

 version = "0.1.0"
 authors = ["The Chromium OS Authors"]

 [dependencies]
 byteorder = "=1.2.1"
 libc = ">=0.2.39"

-kvm_gen = { path = "../kvm_gen" }
+arch_gen = { path = "../arch_gen" }


nit: In the commit message you still refer to this crate (I think) as arch_sys.

Indeed. Adjusted commit message.

alexandruag · 2018-12-19T13:48:59Z

.cargo/config

@@ -1,2 +1,6 @@
 [build]
 target = "x86_64-unknown-linux-musl"
+
+[target.aarch64-unknown-linux-musl]


If the config file supports comments, it would be nice to add a short explanation/link regarding these particular configuration options and why the workaround is needed. We could even write it as an issue of our own, because we'll probably forget the workaround is in place here.

I am already referencing the original issue in the conversation of this PR and in a separate issue of our own: #756.
I also added a comment where I explain why the workaround is needed. let me know if you think something else is needed.

alexandruag · 2018-12-24T18:20:04Z

api_server/Cargo.toml

@@ -26,7 +26,7 @@ kernel = { path = "../kernel" }
 memory_model = { path = "../memory_model" }
 net_util = { path = "../net_util" }
 rate_limiter = { path = "../rate_limiter" }
-x86_64 = { path = "../x86_64" }
+arch = { path = "../arch" }


This would look nice on top of the local dependencies list, because otherwise the alphabetical order is broken. I think there is at least one more Cargo.toml where this happens.

alexandruag · 2018-12-24T18:28:35Z

arch_gen/src/x86/mod.rs

+#[allow(non_upper_case_globals)]
+#[allow(non_camel_case_types)]
+#[allow(non_snake_case)]
+pub mod bootparam;


If this isn't fully auto generated, it would look nice to have the module declarations first, and then the impls.

alexandruag · 2018-12-24T18:32:51Z

arch/src/lib.rs

+    ZeroPageSetup,
+    #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
+    /// X86_64 specific error triggered during system configuration.
+    X86_64Setup(x86_64::Error),


Alphabetical order ruined again.

alexandruag · 2018-12-24T20:04:57Z

dumbo/src/tcp/connection.rs

@@ -1651,4 +1656,11 @@ pub(crate) mod tests {
        // and we don't wait for our FIN to be ACKed.
        assert!(c.is_done());
    }
+
+    #[test]
+    fn test_xor_rng_u32() {


This test is not really useful, because 1000 such comparisons don't necessarily have be true. I think we should drop this altogether, unless we're testing some meaningful statistical property of xor_rng_u32.

I am failing to understand why the rdtsc (i.e number of clock cycles since reset) would return same thing upon different invocations. Can you please clarify?

rdtsc will indeed return different values for different invocations, but the comment refers to the xor_rng32 function itself. What I'm trying to say here is:

I'm not sure if all those bit operations from xor_rng32 will always lead to different results for different inputs (via rdtsc). Maybe we could find an answer just by doing a bit of analysis on that particular formula, but

Even if the previous point is false, a rng function in general makes no guarantee about generating different outputs for successive invocations, so the test is a bit semantically incongruent from that perspective also :D

alexandruag · 2018-12-24T20:13:38Z

arch/src/lib.rs

+#[cfg(target_arch = "x86_64")]
+mod x86_64;
+
+#[cfg(not(target_arch = "x86_64"))]


Why not have

#[cfg(target_arch = "aarch64")] mod aarch64;

instead of a generic template?

My intention was to firstly add a template that in a later PR I turn into aarch64 code. I removed the template all along to not be confusing.

alexandruag · 2018-12-24T20:14:08Z

arch/src/lib.rs

+    interrupts, regs, CMDLINE_START, COMMAND_LINE_SIZE, ZERO_PAGE_START,
+};
+
+#[cfg(not(target_arch = "x86_64"))]


The template vs aarch64 argument also applies here.

See above comment. aarch64 will come in a later PR. Removed template all along.

alexandruag · 2018-12-24T20:16:49Z

arch/src/template/mod.rs

@@ -0,0 +1,30 @@
+use memory_model::{GuestAddress, GuestMemory};


As mentioned before, I think this should be turned into a stub for aarch64, instead of a generic template. If it will eventually turn into aarch64 we might as well do it now. Otherwise, I don't really like something like this lingering in the repository. If its purpose was to document the required functionality that ppl must implement to support new architectures, we could move all this information in an actual doc somewhere.

See grand-previous comment.

alexandruag · 2018-12-24T20:34:13Z

vmm/src/vstate.rs

+        Ok(())
+    }
+
+    #[cfg(not(target_arch = "x86_64"))]


I think we should change this to #[cfg(target_arch = "aarch64")].

Removed this. In a later PR this will be aarch64 labeled.

andreeaflorescu

I will do another round of reviews cause I didn't got the chance to review it all.

andreeaflorescu · 2019-01-07T10:12:43Z

api_server/Cargo.toml

@@ -26,7 +26,7 @@ kernel = { path = "../kernel" }
 memory_model = { path = "../memory_model" }
 net_util = { path = "../net_util" }
 rate_limiter = { path = "../rate_limiter" }
-x86_64 = { path = "../x86_64" }
+arch = { path = "../arch" }


Is this a temporary state of affairs or is this the final solution for adding support for more architectures?

I am asking this because I always found the x86_64 crate to be out of place and renaming it to arch is not making the situation better. I would rather have it split in crates that have a better meaning. Memory related modules should be placed in the memory crate, bootparams should probably be in kernel (or better yet just have the HIMEM variable there since this is the only thing that we use from the generated file) and so on.

If this is just a temporary solution to kick start the arm support it is totally fine, but if we choose to go down this path with the design it would be interesting to know what is the reasoning in still grouping these under a single crate.

@andreeaflorescu , let us brainstorm to find another name for this crate. I agree that arch is not the best one.

andreeaflorescu · 2019-01-07T10:21:06Z

arch_gen/src/lib.rs

+extern crate memory_model;
+
+#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
+pub mod x86;


In this case we shouldn't advertise the target_arch to both x86 and x86_64. But anyway, I don't think it makes a big difference here because the Linux headers are the same for both x86 and x86_64 so the autogenerated code will look the same for both architectures.

dianpopa · 2019-01-08T17:49:02Z

@alexandruag As far as the warnings are related-> there is some code which is currently used only in x86 specific functions but which is not x86 specific functionality (the best example is the ioctl_with_mut_ptr from kvm/src/lib.rs). This kind of code is the one that triggers warnings right now. I think at this point it is more confusing adding a label to code that is not x86 specific than having warnings on aaarch64 build (where we do not yet have functionality).

acatangiu · 2019-01-16T11:16:54Z

vmm/src/vstate.rs

    /// Cannot set the memory regions.
    SetUserMemoryRegion(sys_util::Error),
+    #[cfg(target_arch = "x86_64")]


When we add multiple archs we might want to order errors like these by arch rather than just alphabetically.

MSRS setup is available for x86 and x86_64. You should add both here and to all the other errors as well.

andreeaflorescu · 2019-01-16T11:48:21Z

Cargo.lock

@@ -15,6 +15,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 name = "api_server"
 version = "0.1.0"
 dependencies = [
+ "arch 0.1.0",


As we also discussed face-to-face I don't think that arch is a good crate name because it is too generic and can be misleading for people reading the code. When I looked at this code the first time I thought that you want to write all architecture specific code in one arch crate.
The arch crate seems to offer functionality to configure the system before/after boot and should have a name that better suggests what it is doing. I honestly don't have a better suggestion now, but we should really think about changing this name in the near future.

Unless someone can contribute a better name, I think arch is the right way to go. This is a standard naming convention in many projects.

IMHO renaming would only be necessary if we were to make this crate public. But in the context of an internal crate containing various arch-specific code, arch is a perfectly valid name.

I really don't like that name because it doesn't say anything about the content of the crate. It is just telling me that the code there might work on many architectures or be specific to some architectures. I am not saying we should change it now, but just to be open about changing it and maybe refactor the contents of it at a later time.

arch-specific-misc-code 😆

andreeaflorescu · 2019-01-16T11:53:59Z

arch_gen/Cargo.toml

+authors = ["Amazon firecracker team <firecracker-devel@amazon.com>"]
+
+[dependencies]
+memory_model = { path = "../memory_model" }


It is rather strange to have an auto-generated crate having a dependency on memory-model. Can we have the dependent code in arch instead?

You are right. I will take a look and see where that comes from.

andreeaflorescu · 2019-01-16T11:57:23Z

arch/src/x86_64/mod.rs

 }
-pub type Result<T> = result::Result<T, Error>;
+
+// Magic addresses used to lay out x86_64 VMs.


Why were these moved from the layout.rs file?

I noticed that some of the constants here are made public even though they do not need to be (they are hardcoded values that are used only in this crate. For example MPTABLE_START is used only once in mptable.rs and was made public or the EBDA_START which was only used in mod.rs). Variables FIRST_ADDR_PAST_32BITS and MEM_32BIT_GAP_SIZE were defined in mod.rs not in layout so there was also some inconsistency. To make things clear moved I left in the public module layout the variables that are externally used for setting up things and moved the variables that are private to this module in their specific file. I also turned the comments for each variable in layout.rs to doc comments since they need to be public.
Take a look.

andreeaflorescu · 2019-01-16T12:08:11Z

arch/src/lib.rs

@@ -14,23 +14,24 @@ use std::result;

 #[derive(Debug, PartialEq)]
 pub enum Error {
+    #[cfg(target_arch = "x86_64")]


Is this code only x86_64 specific? I am expecting the setup to be sort of the same for both x86 and x86_64 so I don't think we should be so restrictive with errors. I suspect that changes would be minor between the 2 which means that the 2 architecture (x86 & x86_64) can share one module and have maybe just few lines specific to each architecture.

andreeaflorescu · 2019-01-16T12:12:25Z

vmm/src/vstate.rs

    /// Cannot set the memory regions.
    SetUserMemoryRegion(sys_util::Error),
+    #[cfg(target_arch = "x86_64")]


MSRS setup is available for x86 and x86_64. You should add both here and to all the other errors as well.

andreeaflorescu · 2019-01-16T12:14:17Z

vmm/Cargo.toml

@@ -28,6 +27,9 @@ rate_limiter = { path = "../rate_limiter" }
 seccomp = { path = "../seccomp" }
 sys_util = { path = "../sys_util" }

+[target.'cfg(target_arch = "x86_64")'.dependencies]


and target_arch = "x86"

All method labeled in this commit as x86_64 should be x86_64 and x86 as per KVM API documentation.

Firecracker does not run on x86 out of the box. We have x86_64 specific code inside our implementation (dumbo crate).
I do not understand why you are invoking the KVM API documentation for the vmm crate toml file. I did respect by hard the KVM API but for the kvm crate where we are implementing wrappers on the kvm api. Take a look at ioctl_defs.h and functions calling those ioctl (kvm/src/lib.rs).
By labeling this crate as x86 we are also implying that it builds on x86, right? vmm depends on the devices crate which in turn depends on dumbo (dumbo
does not build on x86, take a look at xor_rng_u32).

Since Firecracker isn't supposed to support x86, I think we should not have cfg(target_arch = "x86") anywhere in the code.

In the case of a crate that we can move to crates.io, it makes sense to also do x86 as long as the crate is fully functional on x86 as well.

andreeaflorescu · 2019-01-16T12:16:05Z

vmm/src/lib.rs

@@ -298,6 +298,7 @@ impl KvmContext {
        check_cap(&kvm, Cap::Ioeventfd)?;
        check_cap(&kvm, Cap::Irqfd)?;
        // check_cap(&kvm, Cap::ImmediateExit)?;
+        #[cfg(target_arch = "x86_64")]


& x86 here as well.

Also, * move the autogenerated code in a different crate named `arch_gen` * x86_64 code code inside `arch` and `arch_gen` is labeled as such Signed-off-by: Diana Popa <dpopa@amazon.com>

Signed-off-by: Diana Popa <dpopa@amazon.com>

On other architectures different than x86_64, the function will return a dummy value. Signed-off-by: Diana Popa <dpopa@amazon.com>

On other architectures, make this function return a dummy value. Also, added unit test for the x86_64 scenarios. Signed-off-by: Diana Popa <dpopa@amazon.com>

- Refactored crate to support addition of non-x86_64 architectures - Labeled code accordingly - non-x86_64 code was moved outside x86_64 folder - Adjusted vmm related code - Added aarch64 dummy template Signed-off-by: Diana Popa <dpopa@amazon.com>

libc syscalls are dependent on the architecture. Thus, "default_syscalls" is now a module that conditionally compiles allowed syscalls depending on target architecture. Signed-off-by: Diana Popa <dpopa@amazon.com>

* labeled x86_64 specific code * conditonal compilation for cpuid Signed-off-by: Diana Popa <dpopa@amazon.com>

for aarch64 musl target. Signed-off-by: Diana Popa <dpopa@amazon.com>

Signed-off-by: Diana Popa <dpopa@amazon.com>

dianpopa added the Feature: ARM Support label Dec 10, 2018

dianpopa self-assigned this Dec 10, 2018

dianpopa force-pushed the i556 branch 2 times, most recently from 60510b0 to a5ba4b9 Compare December 11, 2018 11:51

alexandruag reviewed Dec 24, 2018

View reviewed changes

andreeaflorescu reviewed Jan 7, 2019

View reviewed changes

dianpopa force-pushed the i556 branch 3 times, most recently from 80ef66a to 10cb637 Compare January 8, 2019 17:41

acatangiu previously approved these changes Jan 16, 2019

View reviewed changes

andreeaflorescu suggested changes Jan 16, 2019

View reviewed changes

dianpopa dismissed acatangiu’s stale review via 53a375e January 17, 2019 15:18

dianpopa force-pushed the i556 branch from 10cb637 to 53a375e Compare January 17, 2019 15:18

dianpopa added 9 commits January 17, 2019 17:18

ARM: rename x86_64 crate to arch

bde4de8

Also, * move the autogenerated code in a different crate named `arch_gen` * x86_64 code code inside `arch` and `arch_gen` is labeled as such Signed-off-by: Diana Popa <dpopa@amazon.com>

kvm: label x86 and x86_64 specific code

327de58

Signed-off-by: Diana Popa <dpopa@amazon.com>

fc_util: correctly label x86_64 specific func

536bad1

On other architectures different than x86_64, the function will return a dummy value. Signed-off-by: Diana Popa <dpopa@amazon.com>

dumbo: correctly label x86_64 specific function

a54496b

On other architectures, make this function return a dummy value. Also, added unit test for the x86_64 scenarios. Signed-off-by: Diana Popa <dpopa@amazon.com>

arch: prepare crate for aarch64 code

5e6575c

- Refactored crate to support addition of non-x86_64 architectures - Labeled code accordingly - non-x86_64 code was moved outside x86_64 folder - Adjusted vmm related code - Added aarch64 dummy template Signed-off-by: Diana Popa <dpopa@amazon.com>

vmm: label allowed syscalls as per architecture

7e94f7f

libc syscalls are dependent on the architecture. Thus, "default_syscalls" is now a module that conditionally compiles allowed syscalls depending on target architecture. Signed-off-by: Diana Popa <dpopa@amazon.com>

vmm: additional x86_64 necessary labeling

21ce2ab

* labeled x86_64 specific code * conditonal compilation for cpuid Signed-off-by: Diana Popa <dpopa@amazon.com>

cargo: workaround for successful compilation...

bd3abb8

for aarch64 musl target. Signed-off-by: Diana Popa <dpopa@amazon.com>

vmm: no tss cap check on non x86_64 platforms

06775db

Signed-off-by: Diana Popa <dpopa@amazon.com>

dianpopa force-pushed the i556 branch from 53a375e to 06775db Compare January 17, 2019 15:19

andreeaflorescu approved these changes Jan 18, 2019

View reviewed changes

acatangiu approved these changes Jan 18, 2019

View reviewed changes

Merge branch 'master' into i556

278ddb6

acatangiu merged commit 472e5d2 into firecracker-microvm:master Jan 18, 2019

raduweiss modified the milestone: ARM Support Feb 15, 2019

dianpopa mentioned this pull request Mar 15, 2019

Refactor x86_64 crate #516

Closed

dianpopa deleted the i556 branch September 24, 2019 15:21

dianpopa removed the Feature: CPU Support: ARM label Oct 21, 2020

		@@ -0,0 +1,30 @@
		use memory_model::{GuestAddress, GuestMemory};

Changes for successful build on aarch64 #759

Changes for successful build on aarch64 #759

Conversation

dianpopa commented Dec 10, 2018 • edited Loading

Addressed Issue

Description of changes

Does firecracker now compile on aarch64?

alexandruag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreeaflorescu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianpopa commented Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianpopa commented Dec 10, 2018 •

edited

Loading

dianpopa commented Jan 8, 2019 •

edited

Loading