Enable usage of sync_regs #190

ctfhacker · 2022-01-15T02:31:29Z

Enable the use of the sync_regs API from KVM to allow bulk getting and setting of general purpose registers, system registers, and vcpu events, reducing the number of ioctls needed.

andreeaflorescu

@ctfhacker thanks for your contribution. It looks like the tests are failing on arm, if this is only available on x86_64, could you mark the code as such so that the tests pass on arm as well? Can you also improve your commit message by:

Writing in the commit message what is the sync_regs functionality useful for
Sign your commit by running git commit -s --amend --no-edit

ctfhacker · 2022-01-26T20:20:49Z

Perfect! Let me know how this looks now with the checks passing.

andreeaflorescu

This was a really nice PR to review, thanks for your contribution. I really appreciate that you took the time to write documentation, examples, and tests! Also, really nice how you structured the reg parameter such that invalid values cannot be passed 👍

I have just a few small comments, otherwise LGTM.

andreeaflorescu · 2022-01-27T07:32:43Z

src/ioctls/vcpu.rs

+    #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
+    pub fn sync_regs(&self) -> kvm_sync_regs {
+        let kvm_run: &mut kvm_run = self.kvm_run_ptr.as_mut_ref();
+        unsafe { kvm_run.s.regs }


Unsafe code needs to have a comment about why is it safe. You can take a look at some examples here:

https://github.com/rust-vmm/kvm-ioctls/blob/97ff779b6ea96ae451308149b0ae38f13b271c19/src/ioctls/vcpu.rs#L1347

https://github.com/rust-vmm/kvm-ioctls/blob/97ff779b6ea96ae451308149b0ae38f13b271c19/src/ioctls/vcpu.rs#L1223

The unsafeness in this case comes from accessing the union field, so we need to write about why is it safe to access.

From the perspective of an arbitrary vcpu, I'm not sure we have the safety guarantee that the kvm_run pointer was mmap'ed with enough space for the sync_regs itself.

I was looking at adding a sanity check that the KvmRunWrapper.mmap_size is large enough to fill the kvm_run structure, but wasn't quite sure if that was the best way.

Or, could we make the both sync_regs and sync_regs_mut themselves unsafe, since we can't guarantee that the pointer from kvm_run_ptr is large enough for the entire kvm_run structure?

Marking them as unsafe is a good path forward if we cannot ensure that using it is safe.

What about the mmap_size check didn't work? It looks like the next best thing if 'has space for sync_regs' can't be built in to the type. Returning an Result from sync_regs_mut might be awkward but maybe better than the unsafe.

Ah! KvmRunWrapper does keep the mmap_size around in the structure which we can check against. For some reason, I thought KvmRunWrapper could be created from an arbitrary pointer.

I'll see about adding that check in.

My thinking was because anyone can create a KvmRunWrapper, then calling sync_regs which relies on that kvm_run_ptr can't be trusted (or at least isn't safe in all cases). I think adding a check against the kvm_run_ptr.mmap_size should suffice. I don't see a problem adding a Result for that either. Only question would be looking into if the kvm_run parameters have changed in versions or how to get the correct offset/length for any given bindings.

Having mmap_size be correct is guaranteed by KvmRunWrapper. Getting that bit of unsafe code right is already critical to the safety of the system and allows other parts to rely on the type being correct if it exists.

Thanks for that explanation. I saw that KvmRunWrapper is exposed by the library, enabling the user to specify whatever mmap_size they wished. With that though, the user can't use this smaller KvmRunWrapper to construct an invalid Vcpu, so everything looks fine.

Thank you for spelling that out for me. Much appreciated! I'll remove those unsafes.

andreeaflorescu · 2022-01-27T07:43:12Z

src/ioctls/vcpu.rs

+        vcpu.set_sync_valid_reg(SyncReg::Register);
+        assert_eq!(
+            vcpu.kvm_run_ptr.as_mut_ref().kvm_valid_regs,
+            SyncReg::Register as u64
+        );
+        vcpu.clear_sync_valid_reg(SyncReg::Register);
+        assert_eq!(vcpu.kvm_run_ptr.as_mut_ref().kvm_valid_regs, 0);
+
+        vcpu.set_sync_valid_reg(SyncReg::SystemRegister);
+        assert_eq!(
+            vcpu.kvm_run_ptr.as_mut_ref().kvm_valid_regs,
+            SyncReg::SystemRegister as u64
+        );
+        vcpu.clear_sync_valid_reg(SyncReg::SystemRegister);
+        assert_eq!(vcpu.kvm_run_ptr.as_mut_ref().kvm_valid_regs, 0);
+
+        vcpu.set_sync_valid_reg(SyncReg::VcpuEvents);
+        assert_eq!(
+            vcpu.kvm_run_ptr.as_mut_ref().kvm_valid_regs,
+            SyncReg::VcpuEvents as u64
+        );
+        vcpu.clear_sync_valid_reg(SyncReg::VcpuEvents);
+        assert_eq!(vcpu.kvm_run_ptr.as_mut_ref().kvm_valid_regs, 0);


This code block and the ones below with sync_dirty_reg can each be reduced to the following (without loosing too much of the code readability):

let sync_regs = [SyncReg::Register, SyncReg::SystemRegister, SyncReg::VcpuEvents]; for reg in sync_regs { vcpu.set_sync_valid_reg(reg); assert_eq!( vcpu.kvm_run_ptr.as_mut_ref().kvm_valid_regs, SyncReg::Register as u64 ); vcpu.clear_sync_valid_reg(reg); assert_eq!(vcpu.kvm_run_ptr.as_mut_ref().kvm_valid_regs, 0); }

Would you be okay with this change to remove some of the code duplication?

For sure. That makes total sense to me.

andreeaflorescu · 2022-01-28T08:53:56Z

You'll need to do a borrow on the array used in tests to fix the problems reported by the CI. Once that is done, I think we can merge it.

ctfhacker · 2022-01-28T18:34:39Z

The coverage score seemed to drop from 86.1 -> 85.4. Should I update that? I think all the new code has unit tests for them.

andreeaflorescu · 2022-01-31T08:17:45Z

The coverage score seemed to drop from 86.1 -> 85.4. Should I update that? I think all the new code has unit tests for them.

Yes, you should just update the coverage score. The coverage is a bit too sensitive. We will work on addressing that at the project level.

ctfhacker · 2022-02-02T19:56:15Z

Roger that! Just updated and pushed that coverage change. Apologies if commit --amend with push --force wasn't the correct way of getting this PR ready to go.

lauralt · 2022-02-09T10:08:32Z

src/ioctls/vcpu.rs

+        // hlt is the only expected return from guest execution
+        assert!(matches!(vcpu.run().expect("run failed"), VcpuExit::Hlt));


When running this test on my laptop, this is failing with:
thread 'ioctls::vcpu::tests::test_sync_regs_with_run' panicked at 'assertion failed: matches!(vcpu.run().expect(\"run failed\"), VcpuExit :: Hlt)', src/ioctls/vcpu.rs:2648:9
Is there something else (a capability, etc) we need to check before doing this assert?

Ah, there is the KVM_CAP_SYNC_REGS capability, containing which sync_regs are available. We can add the assert in the unit tests with something like

let sync_regs_cap = unsafe { unsafe { ioctl_with_val(&vm, KVM_CHECK_EXTENSION(), Cap::SyncRegs as c_ulong) } }; assert_eq!(sync_regs_cap, 7);

or opening up check_extension_int to pub rather than private.

Either way, I think we'll also need to add this to the example code to make sure the user knows to check the capability.

Any thoughts on opening check_extension_int to pub? I've never seen it, but supposedly that capability could come back with fewer than 3 bits set, signifying that one of the three sync_regs isn't in use. Opening up check_extension_int would give the user the ability to look specifically which bits are not set rather than an all or nothing, but I'm not sure opening that up function to this one use case is worthwhile.

I've pushed this test case up to see if your capability isn't 7 to see if that was the problem.

Yes, that was the problem. We have the check_extension method that is public and can be used for that, i.e.:

if kvm.check_extension(Cap::SyncRegs) { ... }

And yes we should also update the documentation examples. Thanks!

lauralt

Besides the comment about the failing test, looks good to me!

The coverage file might need to be updated again :(.

src/ioctls/vcpu.rs

lauralt · 2022-02-09T12:26:25Z

src/ioctls/vcpu.rs

+        vcpu.set_sync_dirty_reg(SyncReg::VcpuEvents);
+
+        // Ensure sync reg bits are available for use
+        assert!(kvm.check_extension(Cap::SyncRegs));


This way the test is still going to fail on some machines. I propose we run this test only if we have the capability, similar to how it is done here:

if kvm.check_extension(Cap::SyncRegs) { let vm = kvm.create_vm().unwrap(); // This example is based on https://lwn.net/Articles/658511/ ... assert_eq!(sync_regs.regs.rax, 0x8001); }

I just noticed there are a few places where we are going with the assert approach, but we should probably update them.

lauralt · 2022-02-09T12:46:09Z

src/ioctls/vcpu.rs

+        let vm = kvm.create_vm().unwrap();
+        let mut vcpu = vm.create_vcpu(0).unwrap();
+
+        if kvm.check_extension(Cap::SyncRegs) {


Setting the sync registers works on every machine, so I think we can run the current test without checking for the extension. My proposal was to check for it only for the other test where we call run as well. Same thing applies for the documentation tests. What do you think?

Ah, yeah that makes sense. I agree with that. It isn't technically going to be a problem until run is called, rather than hinting ahead of time.

KVM provides the ability to synchronize general purpose registers, system registers, and vcpu events without the need of an ioctl. This is worthwhile for VMs that exit frequently to avoid calling `get_regs`, `get_sregs`, or `get_vcpu_events` each exit or `set_regs`, `set_sregs`, or `set_vcpu_events` on each entry. Signed-off-by: Cory Duplantis <cld251@gmail.com>

lauralt

Thanks!

ctfhacker requested review from acatangiu, alxiord, andreeaflorescu, lauralt and sameo as code owners January 15, 2022 02:31

andreeaflorescu reviewed Jan 26, 2022

View reviewed changes

ctfhacker force-pushed the sync_regs branch 4 times, most recently from 2ca05d6 to db82986 Compare January 26, 2022 19:33

andreeaflorescu reviewed Jan 27, 2022

View reviewed changes

ctfhacker force-pushed the sync_regs branch 5 times, most recently from 8092c2f to 7d6f26d Compare January 27, 2022 22:36

andreeaflorescu previously approved these changes Jan 28, 2022

View reviewed changes

ctfhacker dismissed andreeaflorescu’s stale review via 4bc6aae January 28, 2022 15:02

ctfhacker force-pushed the sync_regs branch from 7d6f26d to 4bc6aae Compare January 28, 2022 15:02

ctfhacker force-pushed the sync_regs branch 2 times, most recently from 33129ba to 4993392 Compare February 2, 2022 17:27

andreeaflorescu previously approved these changes Feb 5, 2022

View reviewed changes

ctfhacker dismissed andreeaflorescu’s stale review via fe5a833 February 6, 2022 21:02

ctfhacker force-pushed the sync_regs branch 2 times, most recently from fe5a833 to 9137a2a Compare February 6, 2022 21:04

ctfhacker force-pushed the sync_regs branch 3 times, most recently from 44585ac to 03276fa Compare February 6, 2022 21:33

lauralt reviewed Feb 9, 2022

View reviewed changes

src/ioctls/vcpu.rs Outdated Show resolved Hide resolved

ctfhacker force-pushed the sync_regs branch 5 times, most recently from bd87348 to dbc1f95 Compare February 9, 2022 12:18

lauralt reviewed Feb 9, 2022

View reviewed changes

ctfhacker force-pushed the sync_regs branch from dbc1f95 to bc972b6 Compare February 9, 2022 12:33

lauralt reviewed Feb 9, 2022

View reviewed changes

ctfhacker force-pushed the sync_regs branch from bc972b6 to 824cac5 Compare February 9, 2022 12:55

lauralt approved these changes Feb 9, 2022

View reviewed changes

andreeaflorescu approved these changes Feb 9, 2022

View reviewed changes

andreeaflorescu merged commit b5b9b75 into rust-vmm:main Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable usage of sync_regs #190

Enable usage of sync_regs #190

ctfhacker commented Jan 15, 2022

andreeaflorescu left a comment

ctfhacker commented Jan 26, 2022

andreeaflorescu left a comment

andreeaflorescu Jan 27, 2022

ctfhacker Jan 27, 2022

andreeaflorescu Jan 27, 2022

dgreid Feb 6, 2022

ctfhacker Feb 6, 2022

dgreid Feb 6, 2022

ctfhacker Feb 6, 2022

andreeaflorescu Jan 27, 2022

ctfhacker Jan 27, 2022

andreeaflorescu commented Jan 28, 2022

ctfhacker commented Jan 28, 2022

andreeaflorescu commented Jan 31, 2022

ctfhacker commented Feb 2, 2022

lauralt Feb 9, 2022

ctfhacker Feb 9, 2022

lauralt Feb 9, 2022

lauralt left a comment

lauralt Feb 9, 2022

lauralt Feb 9, 2022

ctfhacker Feb 9, 2022

lauralt left a comment

		// hlt is the only expected return from guest execution
		assert!(matches!(vcpu.run().expect("run failed"), VcpuExit::Hlt));

Enable usage of sync_regs #190

Enable usage of sync_regs #190

Conversation

ctfhacker commented Jan 15, 2022

andreeaflorescu left a comment

Choose a reason for hiding this comment

ctfhacker commented Jan 26, 2022

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

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 commented Jan 28, 2022

ctfhacker commented Jan 28, 2022

andreeaflorescu commented Jan 31, 2022

ctfhacker commented Feb 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauralt 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

lauralt left a comment

Choose a reason for hiding this comment