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

["Ivan Shcherbakov" <ivan@sysprogs.com>]

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?