On 2/23/22 06:25, Ivan Shcherbakov wrote:
This adds support for breakpoints and stepping when debugging
WHPX-accelerated guests with gdb.
It enables reliable debugging of the Linux kernel in both single-CPU and SMP
modes.
Signed-off-by: Ivan Shcherbakov <i...@sysprogs.com>
Hi,
in general this patch is really good work, thanks for contributing it!
Just a couple notes:
+enum whpx_step_mode {
+ whpx_step_none = 0,
+ /* Halt other VCPUs */
+ whpx_step_exclusive,
+};
Please use
typedef enum WhpxStepMode {
WHPX_STEP_NONE,
WHPX_STEP_EXCLUSIVE,
} WhpxStepMode;
and likewise for WhpxBreakpointState. (In the case of WhpxStepMode I
would also consider simply a "bool exclusive" in whpx_cpu_run).
struct whpx_vcpu {
WHV_EMULATOR_HANDLE emulator;
bool window_registered;
@@ -156,7 +163,6 @@ struct whpx_vcpu {
uint64_t tpr;
uint64_t apic_base;
bool interruption_pending;
-
Please leave the empty line.
+ if (set) {
+ /* Raise WHvX64ExceptionTypeDebugTrapOrFault after each instruction
*/
+ reg_value.Reg64 |= TF_MASK;
+ } else {
+ reg_value.Reg64 &= ~TF_MASK;
+ }
Out of curiosity, does the guest see TF=1 if it single steps through a
PUSHF (and then break horribly on POPF :))?
+/*
+ * Linux uses int3 (0xCC) during startup (see int3_selftest()) and for
+ * debugging user-mode applications. Since the WHPX API does not offer
+ * an easy way to pass the intercepted exception back to the guest, we
+ * resort to using INT1 instead, and let the guest always handle INT3.
+ */
+static const uint8_t whpx_breakpoint_instruction = 0xF1;
Makes sense.
+ breakpoints->original_addresses =
+ g_renew(vaddr, breakpoints->original_addresses,
cpu_breakpoint_count);
+
+ breakpoints->original_address_count = cpu_breakpoint_count;
+
+ int max_breakpoints = cpu_breakpoint_count +
+ (breakpoints->breakpoints ? breakpoints->breakpoints->used : 0);
+
+ struct whpx_breakpoint_collection *new_breakpoints =
+ (struct whpx_breakpoint_collection *)g_malloc0(
+ sizeof(struct whpx_breakpoint_collection) +
+ max_breakpoints * sizeof(struct whpx_breakpoint));
+ new_breakpoints->allocated = max_breakpoints;
Why separate the original addresses in a different array (and why the
different logic, with used/allocated for one array and an exact size for
the other)
+ enum whpx_breakpoint_state state = breakpoints->data[i].state;
Same comment on coding style applies to this enum.
I would have done most changes for you, but I didn't really understand
the breakpoints vs breakpoint collection part, so I would like your
input on that.
I have queued the first two patches already.
Thanks!
Paolo
