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

[Ivan Shcherbakov]

Hi Alex and Peter,

It would be great to hear your feedback on handling the WHPX stepping via an 
added argument to vm_prepare_start(). It is only called from 2 places, so the 
changes will be minimal. I this works for you, I will gladly send an updated 
patch.

I am also fully open to other suggestions. You obviously know the QEMU codebase 
much better, so I'm happy to refactor it so that it blends in well with the 
rest of the code.

Best,
Ivan

-----Original Message-----
From: Qemu-devel <qemu-devel-bounces+ivan=sysprogs.com@nongnu.org> On Behalf Of 
Ivan Shcherbakov
Sent: Thursday, February 24, 2022 7:54 AM
To: 'Alex Bennée' <alex.bennee@linaro.org>; 'Peter Maydell' 
<peter.maydell@linaro.org>
Cc: 'Paolo Bonzini' <pbonzini@redhat.com>; qemu-devel@nongnu.org; 
armbru@redhat.com; mst@redhat.com
Subject: RE: [PATCH 3/3] whpx: Added support for breakpoints and stepping

> I haven't looked at the rest of the patch -- but can you explain where 
> whpx is different from how other accelerators handle debug such that 
> it needs to know whether gdb is connected or not ?
This mainly comes from the way single-stepping is handled. WHPX needs to know 
whether you want to trap INT1 before starting the first VCPU. The current 
gdbstub implementation doesn’t make it easy. Assume the scenario:

1. You have 2 VCPUs. You run the first one and step the second one.
2. gdb_continue_partial() calls cpu_resume(0) 3. gdb_continue_partial() calls 
cpu_single_step(1).

WHPX needs to know whether to trap INT1 at step #2 (starting the first CPU), 
but it won't know it until step #3. So, the current logic simply checks if gdb 
is connected at all in step #2.

>Just the fact you have connected to the gdbserver shouldn't affect how you run 
>WHPX up until the point there are things you need to trap - i.e.
>handling installed breakpoints.

This can be addressed by adding a "bool stepping_expected" argument to 
vm_prepare_start(). It will be set to true if gdb_continue_partial() expects 
ANY thread to be stepped, and will be false otherwise. It will also require a 
new callback in AccelOpsClass (e.g. on_vm_starting(bool stepping_expected)) 
that will be called from vm_prepare_start(). The WHPX implementation will then 
check if any breakpoints are set, and if the last call to this function 
expected stepping, and use it to decide whether to trap INT1.

Let me know if this will work better.

Best,
Ivan