Big refactor

[jounathaen]

This is a big refactor around the former VirtualCPU and UhyveCPU traits.

Fixes #382

• A VM is now centered around a struct UhyveVM, which is generic over VirtualCPU implementations that provide the platform specific virtualization code.
• Reduced duplicate code between linux and macos.
• macOS now also uses MmapMemory.
• MmapMemory provides more abstractions.
• Put more architecture specific code to the respective arch modules.
  • Pagetable Initialization.
  • Virtual address resolution.
  • Tests for the above mentioned.
• Use GuestVirtAddr and GuestPhysAddr instead of usize/u64 where applicable.

Note, the aarch64 code was not touched and is probably completely broken after this PR. But I think it already was before.

[codecov]

Codecov Report

Attention: Patch coverage is 81.08808% with 146 lines in your changes are missing coverage. Please review.

Project coverage is 68.19%. Comparing base (63cd2fc) to head (3768b34).

Files	Patch %	Lines
src/hypercall.rs	67.83%	46 Missing ⚠️
src/linux/x86_64/kvm_cpu.rs	70.27%	44 Missing ⚠️
src/vm.rs	87.71%	14 Missing ⚠️
src/mem.rs	89.09%	12 Missing ⚠️
src/virtio.rs	0.00%	12 Missing ⚠️
src/bin/uhyve.rs	0.00%	6 Missing ⚠️
uhyve-interface/src/parameters.rs	73.68%	5 Missing ⚠️
src/linux/mod.rs	71.42%	4 Missing ⚠️
src/arch/x86_64/mod.rs	98.83%	2 Missing ⚠️
src/paging.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #527      +/-   ##
==========================================
+ Coverage   65.97%   68.19%   +2.22%     
==========================================
  Files          17       20       +3     
  Lines        2110     2305     +195     
==========================================
+ Hits         1392     1572     +180     
- Misses        718      733      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jounathaen]

I think, the reason this fails is the hello_c kernel. It tries to read from the location 0x0000000100000010, which is far from what the default memory size in uhyve is.
I guess this was an error before, but didn't lead to a crash before.

[mkroening]

Thanks, this looks good overall. :)

I have a few comments though, it's a big PR after all. :D

[mkroening]

I think, we should not force users to specify a Vcpu implementation, since that makes code unnecessarily verbose. Essentially, we should move these cfgs into the library somehow, making default usage easier.

I think a default type would be easiest. Something like:

#[cfg(target_os = "linux")]
pub type Vcpu = KvmCpu;
#[cfg(target_os = "macos")]
pub type Vcpu = XhyveCpu;

pub struct UhyveVm<T = Vcpu> {}

That would avoid exploding configurations into users of the library when there is only one option per target anyway. This also allows explicitly setting the type parameter, should that change in the future, though.

[jounathaen]

Interestingly, this is not necessarily always required. See src/bin/uhyve.rs. As there is only one implementation for VirtualCpuType present for each architecture, Rust apparently picks that one.

I'm not quite sure if the verbosity is really a problem. As for our use cases, the virtualization backend does matter, it might make sense if the user should be forced to think about it. I'm still undecided, though.

[jounathaen]

The benchmarks you picked here are probably a pretty good example of where it should be kept explicit. If one day the default changes, you wouldn't want a surprising change in the benchmark performance just because it uses a different backend.

[mkroening]

The reason explicit types are only required here is that run is only implemented for the concrete type:

impl<VCpuType: VirtualCPU> UhyveVm<VCpuType> {
	pub fn new();
}

impl UhyveVm<KvmCpu> {
	pub fn run();
}

So whenever UhyveVm::<T>::run() is called, T is coerced to KvmCpu (or whatever the concrete type is), since on that platform no other implementor of VirtualCPU exists.

Consistency could be achieved either

• by making the signature of UhyveVm::<T>::run() independent of KvmCpu, forcing the user to explicitly supply T every time
• or by providing the type hint for UhyveVm.

I strongly support the latter, since I see the added verbosity as pure boilerplate code.

Regarding benchmarks: Changes in benchmark performance should not be expected to be reflected in public API types. Changes in performance can happen through any changes in the library. This should be first and foremost the responsibility of the changelog.

[jounathaen]

For me, this still boils down to a personal preference. I completely see why you favor this, but I, personally, don't like to be implicit on such an important design aspect here.

Consistency could be achieved either

• by making the signature of UhyveVm::::run() independent of KvmCpu, forcing the user to explicitly supply T every time

I thought about this before, and this would be a nice approach. The only problem here is run with a debugger attached, which is on first sight not that trivial to make it part of the VirtualCPU trait.

I'm all open to improving that part, but I'd suggest doing that, once other debugger interfaces (e.g., whatever MacOS uses) get implemented.

Regarding benchmarks: Changes in benchmark performance should not be expected to be reflected in public API types. Changes in performance can happen through any changes in the library. This should be first and foremost the responsibility of the changelog.

I totally agree when talking about smaller changes in performance due to implementation changes. But a change in the virtualization backend just because the default has changed can easily change the performance by orders of magnitude. This is not just an implementation detail for a library user, but a relevant choice when using UhyveVM.

[mkroening]

Box<T, A = GlobalAlloc>, HashMap<K, V, S = RandomState> and LazyCell<T, F = fn() -> T> all provide sensible defaults to avoid repeating yourself. Unnecessary cfg's should really be avoided. Something like having a cross-platform AccelVcpu and EmulatedVcpu or similar in the future and a default would at least get rid of that.

I really cannot see how this would be an improvement for anyone. I can only see this making everyone's life more difficult (maintainers and users of the library). While I am strongly against this, the behavior and current use should at the very least be consistent and not change on whether rustc can infer T through run.

[jounathaen]

This is resolved now, right?

[mkroening]

Yaaay! 🥳