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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| usr.sbin/bhyve/amd64/vmexit.c | ||
|---|---|---|
| 520 | Can we reuse the existing VM_EXITCODE_DEBUG? | |
| usr.sbin/bhyve/gdb.c | ||
| 763 | ||
| 776 | Why not checking VM_CAP_MASK_HWINTR? | |
| 806 | Seems unrelated. So, it should be fixed in a seperate commit. This has some benefits:
All in all, it makes the review and bug hunting (imagine a bisect ends at this commit) easier and faster | |
| 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. | |
Comment Actions
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 | ||
| 759 | ||
| 765 | ||
| 772 | 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. | |
| 776 | ||
| 776 | 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. | |
| 779 | ||
| usr.sbin/bhyve/gdb.c | ||
|---|---|---|
| 763 | 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. | |