Page MenuHomeFreeBSD

bhyve: Fix vCPU single-stepping on VMX
ClosedPublic

Authored by bnovkov on May 3 2023, 3:24 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 1, 11:51 AM
Unknown Object (File)
Thu, Nov 28, 1:40 AM
Unknown Object (File)
Tue, Nov 26, 7:34 AM
Unknown Object (File)
Nov 5 2024, 2:40 PM
Unknown Object (File)
Oct 5 2024, 3:12 AM
Unknown Object (File)
Oct 2 2024, 9:17 AM
Unknown Object (File)
Oct 1 2024, 9:37 PM
Unknown Object (File)
Oct 1 2024, 5:42 AM

Details

Summary

This patch fixes virtual machine single stepping on VMX hosts.

Currently, when using bhyve's gdb stub, each attempt at single-stepping a vCPU lands in a timer interrupt. The current single-stepping mechanism uses the Monitor Trap Flag feature to cause VMEXIT after a single instruction is executed. Unfortunately, the SDM states that MTF causes VMEXITs for the next instruction that gets executed, which is often not what the person using the debugger expects. [1]

This patch adds a new VM capability that masks interrupts on a vCPU by blocking interrupt injection and modifies the gdb stub to use the newly added capability while single-stepping a vCPU.

[1] Intel SDM 25-14 Vol. 3C

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/amd64/include/vmm.h
500

Suggestion: It don't have to be used for single stepping only. So, just name it VM_CAP_MASK_HWINTR.

sys/amd64/vmm/intel/vmx.c
3646

Shouldn't we set retval to an error code if vmx_getreg fails?

After taking another look at this function, retval and error seem to be duplicates and we should remove one.

Thank you for pointing out the check kvm does before injecting interrupts! I dug around the source code but I somehow missed this.
I've updated the patch to check the VM_CAP_MASK_HWINTR capability before injecting interrupts, so the whole thing is now completely non-invasive to the guest.
As for the RFLAGS.TF mechanism, I personally wouldn't use it instead of MTF VMEXITs since we can't intercept POPF and PUSH on VMX, which means that stepping over them would probably crash the guest. Admittedly, this is a minor issue, but it wouldn't occur if we stick with MTF VMEXITs.

See also https://docs.kernel.org/virt/kvm/api.html?highlight=kvm_guestdbg_blockirq#kvm-set-guest-debug

I was thinking about making this behaviour configurable using the maintenance packet command, but decided to do it in a separate patch if necessary.
Do you think that this feature would be needed? If so, I'll commit it in a separate patch.

sys/amd64/include/vmm.h
500

Good point, renamed.

In D39949#910195, @bojan.novkovic_fer.hr wrote:

Thank you for pointing out the check kvm does before injecting interrupts! I dug around the source code but I somehow missed this.
I've updated the patch to check the VM_CAP_MASK_HWINTR capability before injecting interrupts, so the whole thing is now completely non-invasive to the guest.

So, your patch doesn't have the caveats anymore which are mentioned in your commit message?

As for the RFLAGS.TF mechanism, I personally wouldn't use it instead of MTF VMEXITs since we can't intercept POPF and PUSH on VMX, which means that stepping over them would probably crash the guest. Admittedly, this is a minor issue, but it wouldn't occur if we stick with MTF VMEXITs.

Hmm. How does qemu solve it on vmx? Nevertheless, it would be a follow up commit. It's no blocker for this commit.

See also https://docs.kernel.org/virt/kvm/api.html?highlight=kvm_guestdbg_blockirq#kvm-set-guest-debug

I was thinking about making this behaviour configurable using the maintenance packet command, but decided to do it in a separate patch if necessary.
Do you think that this feature would be needed? If so, I'll commit it in a separate patch.

Yes, it should be a separate patch but as long as no one is asking for this feature, we don't need it.

sys/amd64/vmm/intel/vmx.c
2653

stray whitespace

This revision is now accepted and ready to land.May 8 2023, 6:48 AM
jhb added a subscriber: jhb.

It would be good to update the commit log now to reflect the new status? This does seem like a fairly nice fix and much nicer than the other approaches I had considered.

bnovkov marked 2 inline comments as done.
bnovkov edited the summary of this revision. (Show Details)

Remove stray whitespace, change commit message to reflect new changes.

This revision now requires review to proceed.May 8 2023, 7:07 PM
In D39949#910921, @jhb wrote:

It would be good to update the commit log now to reflect the new status? This does seem like a fairly nice fix and much nicer than the other approaches I had considered.

Thank you for pointing out the check kvm does before injecting interrupts! I dug around the source code but I somehow missed this.
I've updated the patch to check the VM_CAP_MASK_HWINTR capability before injecting interrupts, so the whole thing is now completely non-invasive to the guest.

So, your patch doesn't have the caveats anymore which are mentioned in your commit message?

That is right, these caveats were tied to the RFLAGS.IF mechanism.
I've updated the commit message to reflect the new changes.

This revision was not accepted when it landed; it landed in state Needs Review.May 9 2023, 8:06 AM
This revision was automatically updated to reflect the committed changes.
gusev.vitaliy_gmail.com added inline comments.
sys/amd64/vmm/intel/vmx.c
3641

Trailing spaces/tabs are here.