Skip to content
This repository was archived by the owner on Jan 28, 2023. It is now read-only.

Add guest debugging support #81

Merged
merged 3 commits into from
Sep 17, 2018
Merged

Add guest debugging support #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a92ea5
Add guest debugging support
AlexAltea Jul 22, 2018
e37664c
Undo unnecessary API version bump
raphaelning Sep 17, 2018
8bbc47d
Remove API version comment
raphaelning Sep 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ itself as well as the host environment.
#define HAX_CAP_64BIT_RAMBLOCK (1 << 3)
#define HAX_CAP_64BIT_SETRAM (1 << 4)
#define HAX_CAP_TUNNEL_PAGE (1 << 5)
#define HAX_CAP_DEBUG (1 << 7)
```
* (Output) `wstatus`: The first set of capability flags reported to the
caller. The following bits may be set, while others are reserved:
Expand Down
4 changes: 2 additions & 2 deletions core/cpu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,12 +347,12 @@ vmx_result_t cpu_vmx_run(struct vcpu_t *vcpu, struct hax_tunnel *htun)
/* Must ensure the IRQ is disabled before setting CR2 */
set_cr2(vcpu->state->_cr2);

load_guest_msr(vcpu);
vcpu_load_guest_state(vcpu);

result = asm_vmxrun(vcpu->state, vcpu->launched);

vcpu->is_running = 0;
save_guest_msr(vcpu);
vcpu_save_guest_state(vcpu);
vcpu_load_host_state(vcpu);

#ifdef DEBUG_HOST_STATE
Expand Down
1 change: 1 addition & 0 deletions core/hax.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ int hax_get_capability(void *buf, int bufLeng, int *outLength)
#endif
cap->winfo |= HAX_CAP_TUNNEL_PAGE;
cap->winfo |= HAX_CAP_RAM_PROTECTION;
cap->winfo |= HAX_CAP_DEBUG;
if (cpu_data->vmx_info._ept_cap) {
cap->winfo |= HAX_CAP_EPT;
}
Expand Down
6 changes: 6 additions & 0 deletions core/include/cpu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ struct hstate {
uint64 fake_gs;
system_desc_t host_gdtr;
system_desc_t host_idtr;
// Debug registers
uint64 dr0;
uint64 dr1;
uint64 dr2;
uint64 dr3;
uint64 dr6;
};

struct hstate_compare {
Expand Down
1 change: 1 addition & 0 deletions core/include/hax_core_interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ int vcpu_put_fpu(struct vcpu_t *vcpu, struct fx_layout *fl);
int vcpu_get_fpu(struct vcpu_t *vcpu, struct fx_layout *fl);
int vcpu_set_regs(struct vcpu_t *vcpu, struct vcpu_state_t *vs);
int vcpu_get_regs(struct vcpu_t *vcpu, struct vcpu_state_t *vs);
void vcpu_debug(struct vcpu_t *vcpu, struct hax_debug_t *debug);

void * get_vcpu_host(struct vcpu_t *vcpu);
int set_vcpu_host(struct vcpu_t *vcpu, void *vcpu_host);
Expand Down
8 changes: 6 additions & 2 deletions core/include/vcpu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,9 @@ struct vcpu_t {
uint64 cr_pat;
uint64 cpuid_features_flag_mask;

/* Debugging */
uint32 debug_control;

/* Interrupt stuff */
uint32 intr_pending[8];
uint32 nr_pending_intrs;
Expand All @@ -229,10 +232,10 @@ struct vcpu_t {

struct vcpu_t *vcpu_create(struct vm_t *vm, void *vm_host, int vcpu_id);
int vcpu_execute(struct vcpu_t *vcpu);
void vcpu_load_guest_state(struct vcpu_t *vcpu);
void vcpu_save_guest_state(struct vcpu_t *vcpu);
void vcpu_load_host_state(struct vcpu_t *vcpu);
void vcpu_save_host_state(struct vcpu_t *vcpu);
void load_guest_msr(struct vcpu_t *vcpu);
void save_guest_msr(struct vcpu_t *vcpu);

int vtlb_active(struct vcpu_t *vcpu);
int vcpu_vmexit_handler(struct vcpu_t *vcpu, exit_reason_t exit_reason,
Expand All @@ -248,6 +251,7 @@ int vcpu_get_fpu(struct vcpu_t *vcpu, struct fx_layout *fl);
int vcpu_put_fpu(struct vcpu_t *vcpu, struct fx_layout *fl);
int vcpu_get_msr(struct vcpu_t *vcpu, uint64 entry, uint64 *val);
int vcpu_put_msr(struct vcpu_t *vcpu, uint64 entry, uint64 val);
void vcpu_debug(struct vcpu_t *vcpu, struct hax_debug_t *debug);

/* The declaration for OS wrapper code */
int hax_vcpu_destroy_host(struct vcpu_t *cvcpu, void *vcpu_host);
Expand Down
3 changes: 2 additions & 1 deletion core/include/vm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ enum exit_status {
HAX_EXIT_STATECHANGE,
HAX_EXIT_PAUSED,
HAX_EXIT_FAST_MMIO,
HAX_EXIT_PAGEFAULT
HAX_EXIT_PAGEFAULT,
HAX_EXIT_DEBUG
};

enum run_flag {
Expand Down
104 changes: 99 additions & 5 deletions core/vcpu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,17 +1154,35 @@ void vcpu_save_host_state(struct vcpu_t *vcpu)
save_host_msr(vcpu);

hstate->hcr2 = get_cr2();
hstate->dr0 = get_dr0();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the time, the vCPU is not in debugging mode (!(vcpu->debug_control & HAX_DEBUG_ENABLE)), so we don't need to save/load debug registers. At least, we can ignore them if no HW BP is enabled. And if we want to be more clever, we can save DR{i} (i = 0, 1, 2, 3) only if HW BP i is enabled. I think this could be an important optimization, because this function is run before every VM entry, so even a few extra instructions could end up having a non-negligible performance penalty.

Copy link
Contributor Author

@AlexAltea AlexAltea Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the time, the vCPU is not in debugging mode [...], so we don't need to save/load debug registers

I'm curious about this: What happens after a VM-exit event if the guest has modified dr{0,1,2,3,6}? If "DR exiting" is disabled, this would have side-effects on the host, right?

So saving/loading drN registers should occur if any of these events occur:

  1. "DR exiting" is disabled.
  2. "HAX_DEBUG_USE_HW_BP" is enabled.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's indeed a very good point. Should we always enable "DR exiting" though, because:

  1. If the debugger runs on the host, we can prevent the guest from tampering with DRs.
  2. If the debugger runs on the guest, we can keep track of the "dirty" DRs we should save/restore. This is more efficient than having "DR exiting" disabled and saving/restore all the DRs at every guest/host context switch.

If you agree with this approach, could you implement it in this PR (maybe as a separate commit)?

hstate->dr1 = get_dr1();
hstate->dr2 = get_dr2();
hstate->dr3 = get_dr3();
hstate->dr6 = get_dr6();
vcpu_enter_fpu_state(vcpu);
// CR0 should be written after host fpu state is saved
vmwrite(vcpu, HOST_CR0, get_cr0());
}

static void vcpu_update_exception_bitmap(struct vcpu_t *vcpu)
{
uint32 exc_bitmap;

exc_bitmap = (1u << VECTOR_MC);
if (vcpu->debug_control & (HAX_DEBUG_USE_HW_BP | HAX_DEBUG_STEP)) {
exc_bitmap |= (1u << VECTOR_DB);
}
if (vcpu->debug_control & HAX_DEBUG_USE_SW_BP) {
exc_bitmap |= (1u << VECTOR_BP);
}
vmwrite(vcpu, VMX_EXCEPTION_BITMAP, exc_bitmap);
}

static void fill_common_vmcs(struct vcpu_t *vcpu)
{
uint32 pin_ctls;
uint32 pcpu_ctls;
uint32 scpu_ctls;
uint32 exc_bitmap;
uint32 exit_ctls = 0;
uint32 entry_ctls;
uint32 vmcs_err = 0;
Expand Down Expand Up @@ -1198,8 +1216,6 @@ static void fill_common_vmcs(struct vcpu_t *vcpu)
}
}

exc_bitmap = 1u << VECTOR_MC;

#ifdef __x86_64__
exit_ctls = EXIT_CONTROL_HOST_ADDR_SPACE_SIZE | EXIT_CONTROL_LOAD_EFER |
EXIT_CONTROL_SAVE_DEBUG_CONTROLS;
Expand Down Expand Up @@ -1272,7 +1288,7 @@ static void fill_common_vmcs(struct vcpu_t *vcpu)
WRITE_CONTROLS(vcpu, VMX_SECONDARY_PROCESSOR_CONTROLS, scpu_ctls);
}

vmwrite(vcpu, VMX_EXCEPTION_BITMAP, exc_bitmap);
vcpu_update_exception_bitmap(vcpu);

WRITE_CONTROLS(vcpu, VMX_EXIT_CONTROLS, exit_ctls);

Expand Down Expand Up @@ -1368,10 +1384,39 @@ void vcpu_load_host_state(struct vcpu_t *vcpu)

load_host_msr(vcpu);
set_cr2(hstate->hcr2);
set_dr0(hstate->dr0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make these operations conditional, so as to minimize the performance penalty.

set_dr1(hstate->dr1);
set_dr2(hstate->dr2);
set_dr3(hstate->dr3);
set_dr6(hstate->dr6);

vcpu_exit_fpu_state(vcpu);
}

void vcpu_save_guest_state(struct vcpu_t *vcpu)
{
struct vcpu_state_t *state = vcpu->state;

save_guest_msr(vcpu);
state->_dr0 = get_dr0();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make these operations conditional, so as to minimize the performance penalty.

state->_dr1 = get_dr1();
state->_dr2 = get_dr2();
state->_dr3 = get_dr3();
state->_dr6 = get_dr6();
}

void vcpu_load_guest_state(struct vcpu_t *vcpu)
{
struct vcpu_state_t *state = vcpu->state;

load_guest_msr(vcpu);
set_dr0(state->_dr0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make these operations conditional, so as to minimize the performance penalty.

set_dr1(state->_dr1);
set_dr2(state->_dr2);
set_dr3(state->_dr3);
set_dr6(state->_dr6);
}

/*
* Copies bits 0, 1, 2, ..., (|size| * 8 - 1) of |src| to the same positions
* in the 64-bit buffer pointed to by |pdst|, and clears bits (|size| * 8)
Expand Down Expand Up @@ -2213,6 +2258,20 @@ static int exit_exc_nmi(struct vcpu_t *vcpu, struct hax_tunnel *htun)
dump_vmcs(vcpu);
break;
}
case VECTOR_DB: {
htun->_exit_status = HAX_EXIT_DEBUG;
htun->debug.rip = vcpu->state->_rip;
htun->debug.dr6 = vmx(vcpu, exit_qualification).raw;
htun->debug.dr7 = vmread(vcpu, GUEST_DR7);
return HAX_EXIT;
}
case VECTOR_BP: {
htun->_exit_status = HAX_EXIT_DEBUG;
htun->debug.rip = vcpu->state->_rip;
htun->debug.dr6 = 0;
htun->debug.dr7 = 0;
return HAX_EXIT;
}
}

if (exit_intr_info.vector == VECTOR_PF) {
Expand Down Expand Up @@ -2797,6 +2856,10 @@ static int exit_cr_access(struct vcpu_t *vcpu, struct hax_tunnel *htun)

static int exit_dr_access(struct vcpu_t *vcpu, struct hax_tunnel *htun)
{
// TODO: DR-exiting flag is disabled so this function is never executed.
// We should conditionally enable "DR exiting" whenever
// HAX_DEBUG_USE_HW_BP is enabled, to prevent the guest from
// tampering with debug drN registers.
uint64 *dr;
struct vcpu_state_t *state = vcpu->state;

Expand Down Expand Up @@ -2856,7 +2919,7 @@ static int exit_dr_access(struct vcpu_t *vcpu, struct hax_tunnel *htun)
if (vmx(vcpu, exit_qualification.dr.direction)) {
// MOV DR -> GPR
state->_regs[vmx(vcpu, exit_qualification).dr.gpr] = *dr;
} else {
} else if (!(vcpu->debug_control & HAX_DEBUG_USE_HW_BP)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MOV DR doesn't cause an VM exit by default, and I don't see DR_EXITING referenced anywhere, so probably this function is never called? In theory, we should enable DR_EXITING when HAX_DEBUG_USE_HW_BP is set, so as to prevent the guest from tampering with the DRs, but as you said that's a corner case. Could you add a TODO comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, that makes sense. I'll add a TODO comment for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment looks good. As I commented earlier, we should consider enabling DR_EXITING unconditionally.

// MOV DR <- GPR
*dr = state->_regs[vmx(vcpu, exit_qualification).dr.gpr];
}
Expand Down Expand Up @@ -3799,6 +3862,37 @@ int vcpu_set_msr(struct vcpu_t *vcpu, uint64 entry, uint64 val)
return handle_msr_write(vcpu, entry, val);
}

void vcpu_debug(struct vcpu_t *vcpu, struct hax_debug_t *debug)
{
if (debug->control & HAX_DEBUG_ENABLE) {
vcpu->debug_control = debug->control;
// Hardware breakpoints
if (debug->control & HAX_DEBUG_USE_HW_BP) {
vcpu->state->_dr0 = debug->dr[0];
vcpu->state->_dr1 = debug->dr[1];
vcpu->state->_dr2 = debug->dr[2];
vcpu->state->_dr3 = debug->dr[3];
vmwrite(vcpu, GUEST_DR7, debug->dr[7]);
} else {
vmwrite(vcpu, GUEST_DR7, 0);
}
// Single-stepping
if (debug->control & HAX_DEBUG_STEP) {
vmwrite(vcpu, GUEST_RFLAGS,
vmread(vcpu, GUEST_RFLAGS) | EFLAGS_TF);
} else {
vmwrite(vcpu, GUEST_RFLAGS,
vmread(vcpu, GUEST_RFLAGS) & ~EFLAGS_TF);
}
} else {
vcpu->debug_control = 0;
vmwrite(vcpu, GUEST_DR7, 0);
vmwrite(vcpu, GUEST_RFLAGS,
vmread(vcpu, GUEST_RFLAGS) & ~EFLAGS_TF);
}
vcpu_update_exception_bitmap(vcpu);
};

static void vcpu_dump(struct vcpu_t *vcpu, uint32 mask, const char *caption)
{
vcpu_vmread_all(vcpu);
Expand Down
6 changes: 6 additions & 0 deletions darwin/hax_driver/com_intel_hax/com_intel_hax_ui.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,12 @@ static int hax_vcpu_ioctl(dev_t dev, ulong cmd, caddr_t data, int flag,
vcpu_interrupt(mvcpu2cvcpu(vcpu), vector);
break;
}
case HAX_IOCTL_VCPU_DEBUG: {
struct hax_debug_t *hax_debug;
hax_debug = (struct hax_debug_t *)data;
vcpu_debug(cvcpu, hax_debug);
break;
}
default: {
int pid;
char task_name[17];
Expand Down
2 changes: 2 additions & 0 deletions include/darwin/hax_interface_mac.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@
/* API 2.0 */
#define HAX_VM_IOCTL_NOTIFY_QEMU_VERSION _IOW(0, 0x84, struct hax_qemu_version)

#define HAX_IOCTL_VCPU_DEBUG _IOW(0, 0xc9, struct hax_debug_t)

#define HAX_KERNEL64_CS 0x80
#define HAX_KERNEL32_CS 0x08
#ifdef __i386__
Expand Down
17 changes: 17 additions & 0 deletions include/hax_interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ struct hax_tunnel {
struct {
paddr_t dummy;
} state;
struct {
uint64_t rip;
uint64_t dr6;
uint64_t dr7;
} debug;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I wonder if we should add a reserved field. Are there any debugging features we could support in the future that might require passing additional data to QEMU? Problem is we have reached the size of the union (24 bytes), but we could also use a second tunnel (vcpu->io_buf) like the fast MMIO interface does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any debugging features we could support in the future that might require passing additional data to QEMU?

I don't think so. On the SDM I've seen other nice features that could benefit debuggers (e.g. tracing), but they can have their own ioctl since they are too wildly different from breakpoints/stepping and considerably more complex.

So I don't think we will ever need more than these 24 bytes.

};
uint64_t apic_base;
} PACKED;
Expand Down Expand Up @@ -182,6 +187,7 @@ struct hax_module_version {
#define HAX_CAP_64BIT_SETRAM (1 << 4)
#define HAX_CAP_TUNNEL_PAGE (1 << 5)
#define HAX_CAP_RAM_PROTECTION (1 << 6)
#define HAX_CAP_DEBUG (1 << 7)

struct hax_capabilityinfo {
/*
Expand Down Expand Up @@ -269,4 +275,15 @@ struct hax_qemu_version {
uint32_t least_version;
} PACKED;

#define HAX_DEBUG_ENABLE (1 << 0)
#define HAX_DEBUG_STEP (1 << 1)
#define HAX_DEBUG_USE_SW_BP (1 << 2)
#define HAX_DEBUG_USE_HW_BP (1 << 3)

struct hax_debug_t {
uint32_t control;
uint32_t reserved;
uint64_t dr[8];
} PACKED;

#endif // HAX_INTERFACE_H_
13 changes: 12 additions & 1 deletion windows/hax_entry.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@

#include "hax_win.h"

// vcpu.h
int vcpu_takeoff(struct vcpu_t *vcpu);
void vcpu_debug(struct vcpu_t *vcpu, struct hax_debug_t *debug);

#define NT_DEVICE_NAME L"\\Device\\HAX"
#define DOS_DEVICE_NAME L"\\DosDevices\\HAX"

Expand Down Expand Up @@ -244,7 +248,6 @@ NTSTATUS hax_get_versioninfo(void *buf, int buflength, int *ret_bl)
return STATUS_SUCCESS;
}

int vcpu_takeoff(struct vcpu_t *vcpu);
NTSTATUS HaxVcpuControl(PDEVICE_OBJECT DeviceObject,
struct hax_vcpu_windows *ext, PIRP Irp)
{
Expand Down Expand Up @@ -419,6 +422,14 @@ NTSTATUS HaxVcpuControl(PDEVICE_OBJECT DeviceObject,
vcpu_takeoff(cvcpu);
break;
}
case HAX_IOCTL_VCPU_DEBUG: {
if (inBufLength < sizeof(struct hax_debug_t)) {
ret = STATUS_INVALID_PARAMETER;
goto done;
}
vcpu_debug(cvcpu, (struct hax_debug_t*)inBuf);
break;
}
default:
hax_error("Unknow vcpu ioctl %lx\n",
irpSp->Parameters.DeviceIoControl.IoControlCode);
Expand Down
3 changes: 3 additions & 0 deletions windows/hax_entry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,4 +163,7 @@ extern PDRIVER_OBJECT HaxDriverObject;
#define HAX_VM_IOCTL_NOTIFY_QEMU_VERSION \
CTL_CODE(HAX_DEVICE_TYPE, 0x910, METHOD_BUFFERED, FILE_ANY_ACCESS)

#define HAX_IOCTL_VCPU_DEBUG \
CTL_CODE(HAX_DEVICE_TYPE, 0x916, METHOD_BUFFERED, FILE_ANY_ACCESS)

#endif // HAX_WINDOWS_HAX_ENTRY_H_