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

[Ivan Shcherbakov]

Hi Alex,

Is there anything I could do to get the WHPX debugging support accepted into 
QEMU? Would the proposed callback AccelOpsClass work for you, or would you 
prefer another approach?

Best,
Ivan

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

Hi Alex,

Thanks for getting back to me. It is definitely the latter case (i.e. it is 
possible to change it while the target is stopped at a breakpoint before 
resuming any VCPUs).
vm_state_notify() does look like it's intended for this type of notifications, 
but unfortunately, it doesn't make a distinction between stepping and running 
normally.
Below is the relevant code from gdbstub.c:

>static int gdb_continue_partial(char *newstates) {
>    int flag = 0;
>
>    /* Various corner cases omitted for brevity  */
>        if (vm_prepare_start()) {
>            return 0;
>        }
>    CPU_FOREACH(cpu) {
>        switch (newstates[cpu->cpu_index]) {
>        case 's':
>            trace_gdbstub_op_stepping(cpu->cpu_index);
>            cpu_single_step(cpu, gdbserver_state.sstep_flags);
>            cpu_resume(cpu);
>            flag = 1;
>            break;
>        case 'c':
>            trace_gdbstub_op_continue_cpu(cpu->cpu_index);
>            cpu_resume(cpu);
>            flag = 1;
>            break;
>        default:
>            res = -1;
>            break;
>        }
>    }
>}

Also:

>int vm_prepare_start(void)
>{
>    runstate_set(RUN_STATE_RUNNING);
>    vm_state_notify(1, RUN_STATE_RUNNING);
>    return 0;
>}

and:

>void vm_state_notify(bool running, RunState state);

So, currently, vm_prepare_start() has no way of distinguishing between 
single-stepping and normal running unless gdb_continue_partial() scans the 
'newstates' array before calling it, and passes some extra argument to 
vm_prepare_start(), indicating whether a step request was present anywhere in 
the array.

I couldn't find any existing run state that would match single-stepping, and 
adding another run state looks like a very non-trivial change that can easily 
backfire. Adding another argument to vm_state_notify() could be easier, but I 
am still afraid it could break some other part of QEMU, so I thought adding a 
new member to AccelOpsClass would be a safer bet. But again, if you think 
another way to do it is better, I am very open to it.

Best regards,
Ivan

-----Original Message-----
From: Alex Bennée <alex.bennee@linaro.org>
Sent: Monday, February 28, 2022 2:28 AM
To: Ivan Shcherbakov <ivan@sysprogs.com>
Cc: 'Peter Maydell' <peter.maydell@linaro.org>; '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

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?