Re: [PATCH 3/3] whpx: Added support for breakpoints and stepping

[Paolo Bonzini <pbonzini@redhat.com>]

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 <ivan@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