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

Add kvm guest memfd related capabilities #288

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
- [[#273](https://github.com/rust-vmm/kvm-ioctls/pull/273)]: `DeviceFd::get_device_attr` is now
marked as unsafe.
- [[#277](https://github.com/rust-vmm/kvm-ioctls/pull/277)]: Updated kvm-bindings to 0.9.1.
- [[#288](https://github.com/rust-vmm/kvm-ioctls/pull/288)]: Add kvm guest memfd related
capabilities.

## v0.18.0

Expand Down
3 changes: 3 additions & 0 deletions src/cap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,4 +171,7 @@ pub enum Cap {
ExitHypercall = KVM_CAP_EXIT_HYPERCALL,
#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
MemoryFaultInfo = KVM_CAP_MEMORY_FAULT_INFO,
UserMemory2 = KVM_CAP_USER_MEMORY2,
GuestMemfd = KVM_CAP_GUEST_MEMFD,
MemoryAttributes = KVM_CAP_MEMORY_ATTRIBUTES,
}
63 changes: 59 additions & 4 deletions src/ioctls/vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1390,10 +1390,51 @@ impl VmFd {
/// Wrapper over `KVM_CHECK_EXTENSION`.
///
/// Returns 0 if the capability is not available and a positive integer otherwise.
fn check_extension_int(&self, c: Cap) -> i32 {
// SAFETY: Safe because we know that our file is a VM fd and that the extension is one of
// the ones defined by kernel.
unsafe { ioctl_with_val(self, KVM_CHECK_EXTENSION(), c as c_ulong) }
/// See the documentation for `KVM_CHECK_EXTENSION`.
///
/// # Arguments
///
/// * `c` - KVM capability to check.
///
/// # Example
///
/// ```
/// extern crate kvm_bindings;
///
/// # use kvm_bindings::{KVM_MEMORY_ATTRIBUTE_PRIVATE};
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for this import, since the final version of the code is checking the MaxVcpus capability

/// # use kvm_ioctls::Kvm;
/// use kvm_ioctls::Cap;
///
/// let kvm = Kvm::new().unwrap();
/// assert!(kvm.check_extension_int(Cap::MaxVcpus) > 0);
/// ```
pub fn check_extension_int(&self, c: Cap) -> i32 {
self.check_extension_raw(c as c_ulong)
}

/// Wrapper over `KVM_CHECK_EXTENSION`.
///
/// Returns 0 if the capability is not available and a positive integer otherwise.
/// See the documentation for `KVM_CHECK_EXTENSION`.
///
/// # Arguments
///
/// * `c` - KVM capability to check in a form of a raw integer.
///
/// # Example
///
/// ```
/// # use kvm_ioctls::Kvm;
/// # use std::os::raw::c_ulong;
/// use kvm_ioctls::Cap;
///
/// let kvm = Kvm::new().unwrap();
/// assert!(kvm.check_extension_raw(Cap::MaxVcpus as c_ulong) > 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the wrong function to call here, as it does the system-wide extension check, instead of the VM-level extension check. If you fix this, you can also remove the unittest you added, because they're the same as these doctests :)

/// ```
pub fn check_extension_raw(&self, c: c_ulong) -> i32 {
// SAFETY: Safe because we know that our file is a KVM fd.
// If `c` is not a known kernel extension, kernel will return 0.
unsafe { ioctl_with_val(self, KVM_CHECK_EXTENSION(), c) }
}

/// Checks if a particular `Cap` is available.
Expand Down Expand Up @@ -2513,6 +2554,20 @@ mod tests {
assert!(vm.check_extension(Cap::MpState));
}

#[test]
fn test_check_extension_int() {
let kvm = Kvm::new().unwrap();
let vm = kvm.create_vm().unwrap();
assert!(vm.check_extension_int(Cap::MpState) > 0);
}
Comment on lines +2557 to +2562
Copy link
Contributor

Choose a reason for hiding this comment

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

Since test_check_extension_raw exists, in my view test_check_extension_int is not necessary, it's just a wrapper on check_extension_raw. This test is just duplicating existing code :)


#[test]
fn test_check_extension_raw() {
let kvm = Kvm::new().unwrap();
let vm = kvm.create_vm().unwrap();
assert!(vm.check_extension_raw(Cap::MpState as c_ulong) > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this is literally a check_extension test which gets expanded, if we intend to test check_extension_raw positively, we can do something like assert_eq!(vm.check_extension_raw(Cap::MaxVcpus), 1024); here to state that we want an int which its value matters.

Or else we just do negative test here :)

Copy link
Contributor

Choose a reason for hiding this comment

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

1024 here is for RISC-V, as of the other two archs, we will need to set its value accordingly

}

#[test]
#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
#[cfg_attr(not(has_sev), ignore)]
Expand Down