Add support for KVM_ENABLE_CAP ioctl #49

sboeuf · 2019-06-10T17:04:30Z

By adding the support for the ioctl KVM_ENABLE_CAP, this commit will
allow consumers of the kvm-ioctls crate to enable any KVM capability
they would be missing from their VMM implementation.

One example of capability that can be interesting to enable is the
newly added KVM_CAP_SPLIT_IRQCHIP, that allows the consumer to prevent
KVM from creating a virtual IOAPIC and PIC. This capability can let
a VMM provide its own userspace implementation of those components for
security purposes.

Signed-off-by: Sebastien Boeuf [email protected]

src/ioctls/vm.rs

andreeaflorescu · 2019-06-11T08:25:47Z

src/ioctls/vm.rs

+    /// ```rust
+    /// # extern crate kvm_ioctls;
+    /// extern crate kvm_bindings;
+    /// #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]


I am not a big fan of having x86_64 specific examples when the ioctl is available on other platforms as well. I would suggest to use KVM_CAP_ARM_USER_IRQ on aarch64 and KVM_CAP_SPLIT_IRQCHIP on x86_64. The only code that should be specific to the platform should be cap.cap = ... and cap.args[0] = 24. Also add a comment about what is 24.

Yes you're right, it's actually good to show an example with two different architectures.

Well let me take that back... The cap KVM_CAP_ARM_USER_IRQ that you suggested is only meant to be checked through check_extension(), but it cannot be set from the VMM.
And based on the KVM bindings 4.20, there is no arm/aarch64 capability that we could take as an example.
I reworked the structure of the documentation test though, let me know if it's okay with you.

Yeah, I didn't look in depth. Is there really no other cap that we can enable for aarch64?

Unless I'm reading KVM doc the wrong way (could be possible :p), I don't think there are some ARM caps that we can simply try to enable.
The only one could be KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 but it's not part of our bindings because it's been introduced recently on the kernel.

KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 is only available starting with 5.0 so it will fail anyway since we don't have that capability on the host kernel.

src/ioctls/vm.rs

alxiord · 2019-06-12T08:49:00Z

src/cap.rs

@@ -23,6 +23,8 @@ use kvm_bindings::*;
 #[allow(missing_docs)]
 pub enum Cap {
    Irqchip = KVM_CAP_IRQCHIP,
+    #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
+    SplitIrqchip = KVM_CAP_SPLIT_IRQCHIP,


The capabilities are sorted by value. Please move this lower (should be between KVM_CAP_S390_USER_SIGP and KVM_CAP_IMMEDIATE_EXIT).

alxiord · 2019-06-12T08:49:44Z

src/ioctls/vm.rs

+    ///     cap.args[0] = 24;
+    ///     vm.enable_cap(&cap).unwrap();
+    /// }
+    ///


nit: remove blank line

alxiord · 2019-06-12T08:53:48Z

src/ioctls/vm.rs

+    /// # Example
+    ///
+    /// ```rust
+    /// # extern crate kvm_ioctls;


Please remove his line (and the next one prefixed by #). They don't show up in the generated doc and the doctest passes without them.

Fixed but those lines (prefixed by #) where there in every other doctest.

I can't remove it, the CI complains:

---- src/ioctls/vm.rs - ioctls::vm::VmFd::enable_cap (line 627) stdout ---- error[E0433]: failed to resolve: use of undeclared type or module `Kvm` --> src/ioctls/vm.rs:632:11 | 7 | let kvm = Kvm::new().unwrap(); | ^^^ use of undeclared type or module `Kvm`

alxiord · 2019-06-12T08:58:34Z

src/ioctls/vm.rs

+    ///
+    /// ```
+    ///
+    pub fn enable_cap(&self, cap: &kvm_enable_cap) -> Result<()> {


What architectures is this available on? The documentation only mentions mips, ppc, s390, I found patches for x86, but what about aarch64?

The documentation states that all architectures support this ioctl:

4.37 KVM_ENABLE_CAP Capability: KVM_CAP_ENABLE_CAP Architectures: mips, ppc, s390 Type: vcpu ioctl Parameters: struct kvm_enable_cap (in) Returns: 0 on success; -1 on error Capability: KVM_CAP_ENABLE_CAP_VM Architectures: all Type: vcpu ioctl Parameters: struct kvm_enable_cap (in) Returns: 0 on success; -1 on error +Not all extensions are enabled by default. Using this ioctl the application can enable an extension, making it available to the guest. On systems that do not support this ioctl, it always fails. On systems that do support it, it only works for extensions that are supported for enablement. To check if a capability can be enabled, the KVM_CHECK_EXTENSION ioctl should be used. struct kvm_enable_cap { /* in */ __u32 cap; The capability that is supposed to get enabled. __u32 flags; A bitfield indicating future enhancements. Has to be 0 for now. __u64 args[4]; Arguments for enabling a feature. If a feature needs initial values to function properly, this is the place to put them. __u8 pad[64]; }; The vcpu ioctl should be used for vcpu-specific capabilities, the vm ioctl for vm-wide capabilities.

Oh my bad! Talking with @andreeaflorescu I realized this snippet of the doc is not part of 4.14 or 4.20, it's only been introduced later in 5.x.
So I suggest that for now I can remove the arm/aarch64 target.

Sounds good to me.

Ok I've updated the PR!

sboeuf · 2019-06-12T15:03:03Z

@aghecenco comments addressed PTAL

By adding the support for the ioctl KVM_ENABLE_CAP, this commit will allow consumers of the kvm-ioctls crate to enable any KVM capability they would be missing from their VMM implementation. One example of capability that can be interesting to enable is the newly added KVM_CAP_SPLIT_IRQCHIP, that allows the consumer to prevent KVM from creating a virtual IOAPIC and PIC. This capability can let a VMM provide its own userspace implementation of those components for security purposes. Unfortunately, we couldn't test enabling a capability for arm/aarch64 since there is no capability available for these architectures. Signed-off-by: Sebastien Boeuf <[email protected]>

sboeuf · 2019-06-12T16:44:05Z

@andreeaflorescu @sameo @acatangiu @aghecenco please take a look.
The PR is ready, and @andreeaflorescu approved it, but the approval went away because I had to rebase it on master and repush...

andreeaflorescu · 2019-06-12T16:50:48Z

@sboeuf you don't need to do push forces for updating to master. You can use the update branch button. The merge commit is not kept when merging the PR.

sboeuf · 2019-06-12T16:56:52Z

@andreeaflorescu oh okay good to know!

sboeuf requested a review from andreeaflorescu June 10, 2019 17:04

sboeuf force-pushed the enable_cap branch 4 times, most recently from 81dc6b7 to d209557 Compare June 10, 2019 17:35

andreeaflorescu reviewed Jun 11, 2019

View reviewed changes

sboeuf force-pushed the enable_cap branch 7 times, most recently from 22e1d2e to cdac6a8 Compare June 12, 2019 08:48

sboeuf requested review from acatangiu, alxiord and sameo as code owners June 12, 2019 08:48

alxiord reviewed Jun 12, 2019

View reviewed changes

sboeuf force-pushed the enable_cap branch from cdac6a8 to df8f7ea Compare June 12, 2019 15:02

sboeuf force-pushed the enable_cap branch from df8f7ea to 2bd0a61 Compare June 12, 2019 15:55

andreeaflorescu previously approved these changes Jun 12, 2019

View reviewed changes

sboeuf dismissed andreeaflorescu’s stale review via c620b6a June 12, 2019 16:08

sboeuf force-pushed the enable_cap branch from 2bd0a61 to c620b6a Compare June 12, 2019 16:08

sameo approved these changes Jun 12, 2019

View reviewed changes

andreeaflorescu approved these changes Jun 12, 2019

View reviewed changes

sameo merged commit d48fdaa into rust-vmm:master Jun 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for KVM_ENABLE_CAP ioctl #49

Add support for KVM_ENABLE_CAP ioctl #49

sboeuf commented Jun 10, 2019

andreeaflorescu Jun 11, 2019

sboeuf Jun 11, 2019

sboeuf Jun 11, 2019

andreeaflorescu Jun 12, 2019

sboeuf Jun 12, 2019

andreeaflorescu Jun 12, 2019

alxiord Jun 12, 2019

sboeuf Jun 12, 2019

alxiord Jun 12, 2019

sboeuf Jun 12, 2019

alxiord Jun 12, 2019

sboeuf Jun 12, 2019

sboeuf Jun 12, 2019 •

edited

Loading

alxiord Jun 12, 2019

sboeuf Jun 12, 2019

sboeuf Jun 12, 2019

andreeaflorescu Jun 12, 2019

sboeuf Jun 12, 2019

sboeuf commented Jun 12, 2019

sboeuf commented Jun 12, 2019

andreeaflorescu commented Jun 12, 2019

sboeuf commented Jun 12, 2019

Add support for KVM_ENABLE_CAP ioctl #49

Add support for KVM_ENABLE_CAP ioctl #49

Conversation

sboeuf commented Jun 10, 2019

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

sboeuf Jun 12, 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

sboeuf commented Jun 12, 2019

sboeuf commented Jun 12, 2019

andreeaflorescu commented Jun 12, 2019

sboeuf commented Jun 12, 2019

sboeuf Jun 12, 2019 •

edited

Loading