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

[Alex Bennée]

"Ivan Shcherbakov" <ivan@sysprogs.com> writes:

> 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.

Is the limitation that whpx_set_exception_exit_bitmap needs to be set
before any vCPU can be run or that it cannot be set if any vCPUs are
currently running?

If it is the later wouldn't adding a hook into the vm_change_state_head
callbacks be enough?

> 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


-- 
Alex Bennée