Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dummy PR for proxy bindings #3

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 21 additions & 12 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/arch/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ authors = ["The Chromium OS Authors"]
edition = "2018"

[dependencies]
kvm-bindings = { git = "https://github.com/firecracker-microvm/kvm-bindings", tag = "v0.2.0-2", features = ["fam-wrappers"] }
kvm-ioctls = { git = "https://github.com/firecracker-microvm/kvm-ioctls", tag = "v0.5.0-2" }
kvm-bindings = ">=0.3.0"
kvm-ioctls = ">=0.5.0"
libc = ">=0.2.39"
vm-memory = { version = ">=0.2.2", features = ["backend-mmap"] }

Expand Down
4 changes: 2 additions & 2 deletions src/cpuid/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ authors = ["Amazon Firecracker team <[email protected]>"]
edition = "2018"

[dependencies]
kvm-bindings = { git = "https://github.com/firecracker-microvm/kvm-bindings", tag = "v0.2.0-2", features = ["fam-wrappers"] }
kvm-ioctls = { git = "https://github.com/firecracker-microvm/kvm-ioctls", tag = "v0.5.0-2" }
kvm-bindings = ">=0.3.0"
kvm-ioctls = ">=0.5.0"

utils = { path = "../utils"}
5 changes: 5 additions & 0 deletions src/kvm_bindings_versionize/.cargo/config
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# This workaround is needed because the linker is unable to find __addtf3,
# __multf3 and __subtf3.
# Related issue: https://github.com/rust-lang/compiler-builtins/issues/201
[target.aarch64-unknown-linux-musl]
rustflags = [ "-C", "target-feature=+crt-static", "-C", "link-arg=-lgcc"]
16 changes: 16 additions & 0 deletions src/kvm_bindings_versionize/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
[package]
name = "kvm-bindings-versionize"
version = "0.3.0"
authors = ["Amazon firecracker team <[email protected]>"]
description = "Rust FFI bindings to KVM generated using bindgen."
repository = "https://github.com/rust-vmm/kvm-bindings"
readme = "README.md"
keywords = ["kvm"]
license = "Apache-2.0"

[dependencies]
kvm-bindings = ">=0.3.0"
vmm-sys-util = { version = ">=0.2.0" }

versionize = { version = ">=0.1.1" }
versionize_derive = { version = ">=0.1.1" }
84 changes: 84 additions & 0 deletions src/kvm_bindings_versionize/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

#![allow(non_upper_case_globals)]
#![allow(non_camel_case_types)]
#![allow(non_snake_case)]

#[macro_use]
extern crate vmm_sys_util;

extern crate kvm_bindings;
extern crate versionize;
extern crate versionize_derive;

#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
mod x86;

#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
pub use self::x86::*;

use std::mem::{align_of, size_of};
use std::ptr;

use vmm_sys_util::fam::{FamStruct, FamStructWrapper};

fn convert_bitwise<T, U>(t: &T) -> U {
assert_eq!(align_of::<T>(), align_of::<U>());
assert_eq!(size_of::<T>(), size_of::<U>());
// Safe because `src` is aligned, the two types have the same size and alignment,
// and they are both plain old data structs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to guarantee this? Maybe force them through a dummy FFI or something similar to make sure we don't accidentally break this assumption?

My worry is that the function safety is based on only using it for POD or repr(C) data, but that isn't in any way enforced, and we could accidentally misuse it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assumption must hold for kvm bindings, and the unit tests and sanity checks above verify everything except the presence of #[repr(C)], which is where the dummy FFI might come in handy for extra certainty.

unsafe { ptr::read(t as *const T as *const U) }
}

// This requires `upstream` to be a valid module path/alias in the scope that's invoking the
// macro (i.e. `use kvm_bindings as upstream;`). It also requires the `convert_bitwise`
// function above to be in scope.
#[macro_export]
macro_rules! impl_conversions {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool how conversion is ultimately only casting and thus no/low overhead 👍

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is not just casting, but rather equivalent to a memcpy. My initial measurements didn't detect significant overhead, so maybe it's the compiler that in the end applies some more simplifications.

($T:ident) => {
impl From<upstream::$T> for $T {
fn from(other: upstream::$T) -> Self {
convert_bitwise(&other)
}
}

impl From<$T> for upstream::$T {
fn from(other: $T) -> Self {
convert_bitwise(&other)
}
}

impl From<&upstream::$T> for $T {
fn from(other: &upstream::$T) -> Self {
convert_bitwise(other)
}
}

impl From<&$T> for upstream::$T {
fn from(other: &$T) -> Self {
convert_bitwise(other)
}
}
};
}

/// Helper function for FamStructWrapper conversion. Can/should be replaced by implementing
/// `From` for `FamStructWrapper` in `vmm-sys-util`.
pub fn convert_fam_struct_wrapper<'a, T, U>(src: &'a FamStructWrapper<T>) -> FamStructWrapper<U>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is reallocation with deep copy, is there any way to do some trickery here as well?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extent of the copying is comparable to the conversion above, but it can be made to look simpler :D

where
T: Default + FamStruct + 'static,
U: Default + FamStruct + From<&'a T> + 'static,
T::Entry: 'static,
U::Entry: From<&'a T::Entry> + 'static,
{
// The `FamStructWrapper.len()` method is private for some reason.
let mut dst = FamStructWrapper::new(src.as_slice().len());
*dst.as_mut_fam_struct() = src.as_fam_struct_ref().into();

for (index, entry) in src.as_slice().iter().enumerate() {
dst.as_mut_slice()[index] = entry.into();
}

dst
}
Loading