Page MenuHomeFreeBSD

bhyve: refactor gdbstub to enable single-stepping on AMD CPUs
ClosedPublic

Authored by bnovkov on Oct 19 2023, 4:01 PM.
Tags
Referenced Files
Unknown Object (File)
Fri, Feb 16, 12:03 AM
Unknown Object (File)
Fri, Feb 16, 12:03 AM
Unknown Object (File)
Fri, Feb 16, 12:02 AM
Unknown Object (File)
Jan 25 2024, 8:30 AM
Unknown Object (File)
Jan 10 2024, 8:41 PM
Unknown Object (File)
Jan 5 2024, 10:33 PM
Unknown Object (File)
Dec 25 2023, 5:23 PM
Unknown Object (File)
Dec 23 2023, 1:47 AM

Details

Summary

This patch refactors the existing Intel-specific single-stepping mechanism in bhyve's GDB stub to work with both AMD and Intel CPUs.

This work was sponsored by the GSoC '22 program.

Diff Detail

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

Event Timeline

usr.sbin/bhyve/amd64/vmexit.c
520

Can we reuse the existing VM_EXITCODE_DEBUG?

usr.sbin/bhyve/gdb.c
762
775

Why not checking VM_CAP_MASK_HWINTR?

803

Seems unrelated. So, it should be fixed in a seperate commit. This has some benefits:

  1. We can merge such small fixes earlier
  2. The diff gets smaller
  3. The diff is more readable because it doesn't contain any noise

All in all, it makes the review and bug hunting (imagine a bisect ends at this commit) easier and faster

bnovkov added inline comments.
usr.sbin/bhyve/amd64/vmexit.c
520

As far as I can tell, that exitcode is used to suspend a vCPU. I didn't want to tie debug exception VMEXITs with this because I perceive them as two separate things + I didn't want to risk inadvertently breaking something that depends on VM_EXITCODE_DEBUG.

bnovkov added inline comments.
usr.sbin/bhyve/gdb.c
803

I've split this out into a separate revision (D42406).

This generally looks fine to me modulo some nits. I think we should probably remove checking for the VM_CAP_MASK_HWINTR capability though.

usr.sbin/bhyve/amd64/vmexit.c
520

Yeah, this is for an intercepted DB# exception similar VM_EXITCODE_BPT for BP#. VM_EXITCODE_DEBUG is for when the userland hypervisor requests a vCPU to suspend.

usr.sbin/bhyve/gdb.c
758
764
771

We should probably refactor this in a followup commit to check these capabilities once during gdb_init on the BSP (they aren't really per-vCPU but are per-VM caps) and then cache the results with some sort of enum { NONE, MTRAP, RFLAGS_TF } stepping_support global. That could then be used when doing a step to only set the relevant capability as well.

775
775

VM_CAP_MASK_HWINTR is not required, it just is a nice to have, so I don't think we need to require it here, we just need to require one of MTRAP or RFLAGS_TF.

778
bnovkov marked an inline comment as done.

Address @jhb 's comments.

jhb added inline comments.
usr.sbin/bhyve/gdb.c
762

Ah, missed this earlier (but can just fix during commit). This one is not fatal and we should just ignore an error setting this capability.

This revision is now accepted and ready to land.Dec 12 2023, 10:40 PM