Re: [Qemu-devel] [PATCH v3 35/45] windbg: debug exception subscribing

[Ladi Prosek <lprosek@redhat.com>]

On Wed, Dec 6, 2017 at 8:29 AM, Mihail Abakumov
<mikhail.abakumov@ispras.ru> wrote:
> Ladi Prosek писал 2017-11-29 10:13:
>
>> On Tue, Nov 21, 2017 at 3:10 PM, Mihail Abakumov
>> <mikhail.abakumov@ispras.ru> wrote:
>>>
>>> Added handler registration of gdb debug exception. Its exception also can
>>> be used for windbg.
>>>
>>> Signed-off-by: Mihail Abakumov <mikhail.abakumov@ispras.ru>
>>> Signed-off-by: Pavel Dovgalyuk <dovgaluk@ispras.ru>
>>> Signed-off-by: Dmitriy Koltunov <koltunov@ispras.ru>
>>> ---
>>>  cpus.c                  |   18 +++++++++++++++++-
>>>  gdbstub.c               |    4 ++++
>>>  include/sysemu/sysemu.h |    2 ++
>>>  windbgstub.c            |   16 ++++++++++++----
>>>  4 files changed, 35 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index 9bed61eefc..212553b7e3 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -77,6 +77,8 @@ int64_t max_advance;
>>>  static QEMUTimer *throttle_timer;
>>>  static unsigned int throttle_percentage;
>>>
>>> +static void (*excp_debug_handler)(CPUState *cpu);
>>> +
>>>  #define CPU_THROTTLE_PCT_MIN 1
>>>  #define CPU_THROTTLE_PCT_MAX 99
>>>  #define CPU_THROTTLE_TIMESLICE_NS 10000000
>>> @@ -960,9 +962,23 @@ static bool cpu_can_run(CPUState *cpu)
>>>      return true;
>>>  }
>>>
>>> +bool register_excp_debug_handler(void (*handler)(CPUState *cpu))
>>> +{
>>> +    if (excp_debug_handler == NULL) {
>>> +        excp_debug_handler = handler;
>>> +        return true;
>>> +    } else {
>>> +        error_report("ERROR: Something debugger already using");
>>
>>
>> So I take it that -windbg and -gdb cannot be used at the same time.
>> Should it be handled in a more explicit way with a more user-friendly
>> error?
>>
>> Right now I get this error, which is more like an implementation
>> detail (could the debug handler be refactored into a multicast event
>> in the future?) and does not even make sense as a sentence.
>
>
> Yes. I have added a more user-friendly error. "Something debugger is
> already in use. '-gdb' and '-windbg' cannot be used at the same time".
> Or what do you mean under the explicit way? How to do it better?

Yes, "'-gdb' and '-windbg' cannot be used at the same time" would be
nicer. By being more explicit I mean issuing the error from the code
that parses the options.

In other words, I can imagine an implementation of
register_excp_debug_handler() that allows registering multiple
handlers. But running windbg and gdb at the same would probably still
not be a good idea.

Maybe I'm nit-picking and it's ok for the error to stay here.
Wondering what other reviewers think.

>>
>>> +        return false;
>>> +    }
>>> +}
>>> +
>>>  static void cpu_handle_guest_debug(CPUState *cpu)
>>>  {
>>> -    gdb_set_stop_cpu(cpu);
>>> +    if (excp_debug_handler != NULL) {
>>> +        excp_debug_handler(cpu);
>>> +    }
>>> +
>>>      qemu_system_debug_request();
>>>      cpu->stopped = true;
>>>  }
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index 2a94030d3b..8c76f54117 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -2006,6 +2006,10 @@ int gdbserver_start(const char *device)
>>>      s->mon_chr = mon_chr;
>>>      s->current_syscall_cb = NULL;
>>>
>>> +    if (!register_excp_debug_handler(gdb_set_stop_cpu)) {
>>> +        exit(1);
>>> +    }
>>> +
>>>      return 0;
>>>  }
>>>
>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>>> index b21369672a..34588c99b4 100644
>>> --- a/include/sysemu/sysemu.h
>>> +++ b/include/sysemu/sysemu.h
>>> @@ -193,6 +193,8 @@ QemuOpts *qemu_get_machine_opts(void);
>>>
>>>  bool defaults_enabled(void);
>>>
>>> +bool register_excp_debug_handler(void (*handler)(CPUState *cpu));
>>> +
>>>  extern QemuOptsList qemu_legacy_drive_opts;
>>>  extern QemuOptsList qemu_common_drive_opts;
>>>  extern QemuOptsList qemu_drive_opts;
>>> diff --git a/windbgstub.c b/windbgstub.c
>>> index 489abe6d6c..b33f412659 100755
>>> --- a/windbgstub.c
>>> +++ b/windbgstub.c
>>> @@ -115,16 +115,20 @@ static void windbg_send_control_packet(uint16_t
>>> type)
>>>      windbg_state->ctrl_packet_id ^= 1;
>>>  }
>>>
>>> -static void windbg_vm_stop(void)
>>> +static void windbg_bp_handler(CPUState *cpu)
>>>  {
>>> -    CPUState *cpu = qemu_get_cpu(0);
>>> -    vm_stop(RUN_STATE_PAUSED);
>>> -
>>>      SizedBuf buf = kd_gen_exception_sc(cpu);
>>>      windbg_send_data_packet(buf.data, buf.size,
>>> PACKET_TYPE_KD_STATE_CHANGE64);
>>>      SBUF_FREE(buf);
>>>  }
>>>
>>> +static void windbg_vm_stop(void)
>>> +{
>>> +    CPUState *cpu = qemu_get_cpu(0);
>>> +    vm_stop(RUN_STATE_PAUSED);
>>> +    windbg_bp_handler(cpu);
>>> +}
>>> +
>>>  static void windbg_process_manipulate_packet(ParsingContext *ctx)
>>>  {
>>>      CPUState *cpu;
>>> @@ -432,6 +436,10 @@ int windbg_server_start(const char *device)
>>>
>>>      qemu_register_reset(windbg_handle_reset, NULL);
>>>
>>> +    if (!register_excp_debug_handler(windbg_bp_handler)) {
>>> +        exit(1);
>>> +    }
>>> +
>>>      atexit(windbg_exit);
>>>      return 0;
>>>  }
>>>
>