bhyve: Fix vCPU single-stepping on VMX

[bnovkov]

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

[corvink]

The patch looks good but I would try to follow kvm.

kvm uses RFLAGS.TF for single stepping (https://elixir.bootlin.com/linux/latest/source/arch/x86/kvm/x86.c#L12834) and blocks interrupt injection (https://elixir.bootlin.com/linux/latest/source/arch/x86/kvm/x86.c#L10082, https://qemu-project.gitlab.io/qemu/system/gdb.html#changing-single-stepping-behaviour) while single stepping.

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

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

3636

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.

[bnovkov]

Address @corvink 's comments.

[bnovkov]

In D39949#909484, @corvink wrote:

kvm uses RFLAGS.TF for single stepping (https://elixir.bootlin.com/linux/latest/source/arch/x86/kvm/x86.c#L12834) and blocks interrupt injection (https://elixir.bootlin.com/linux/latest/source/arch/x86/kvm/x86.c#L10082, https://qemu-project.gitlab.io/qemu/system/gdb.html#changing-single-stepping-behaviour) while single stepping.

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.

[corvink]

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

In D39949#909484, @corvink wrote:

kvm uses RFLAGS.TF for single stepping (https://elixir.bootlin.com/linux/latest/source/arch/x86/kvm/x86.c#L12834) and blocks interrupt injection (https://elixir.bootlin.com/linux/latest/source/arch/x86/kvm/x86.c#L10082, https://qemu-project.gitlab.io/qemu/system/gdb.html#changing-single-stepping-behaviour) while single stepping.

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

2643

stray whitespace

[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]

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

[bnovkov]

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.

In D39949#910672, @corvink 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?

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

[gusev.vitaliy_gmail.com]

sys/amd64/vmm/intel/vmx.c

3631

Trailing spaces/tabs are here.