Maxim Levitsky <mlevitsk@xxxxxxxxxx> writes:
> On Wed, 2021-10-13 at 16:50 +0100, Alex Bennée wrote:
>> Maxim Levitsky <mlevitsk@xxxxxxxxxx> writes:
>>
>> > Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
>> > ---
>> > accel/kvm/kvm-all.c | 25 ++++++++++++++++++
>> > gdbstub.c | 60 ++++++++++++++++++++++++++++++++++++--------
>> > include/sysemu/kvm.h | 13 ++++++++++
>> > 3 files changed, 88 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> > index 6b187e9c96..e141260796 100644
>> > --- a/accel/kvm/kvm-all.c
>> > +++ b/accel/kvm/kvm-all.c
>> > @@ -169,6 +169,8 @@ bool kvm_vm_attributes_allowed;
>> > bool kvm_direct_msi_allowed;
>> > bool kvm_ioeventfd_any_length_allowed;
>> > bool kvm_msi_use_devid;
>> > +bool kvm_has_guest_debug;
>> > +int kvm_sstep_flags;
>> > static bool kvm_immediate_exit;
>> > static hwaddr kvm_max_slot_size = ~0;
>> >
>> > @@ -2559,6 +2561,25 @@ static int kvm_init(MachineState *ms)
>> > kvm_sregs2 =
>> > (kvm_check_extension(s, KVM_CAP_SREGS2) > 0);
>> >
>> > + kvm_has_guest_debug =
>> > + (kvm_check_extension(s, KVM_CAP_SET_GUEST_DEBUG) > 0);
>> > +
>> > + kvm_sstep_flags = 0;
>> > +
>> > + if (kvm_has_guest_debug) {
>> > + /* Assume that single stepping is supported */
>> > + kvm_sstep_flags = SSTEP_ENABLE;
>> > +
>> > + int guest_debug_flags =
>> > + kvm_check_extension(s, KVM_CAP_SET_GUEST_DEBUG2);
>> > +
>> > + if (guest_debug_flags > 0) {
>> > + if (guest_debug_flags & KVM_GUESTDBG_BLOCKIRQ) {
>> > + kvm_sstep_flags |= SSTEP_NOIRQ;
>> > + }
>> > + }
>> > + }
>> > +
>> > kvm_state = s;
>> >
>> > ret = kvm_arch_init(ms, s);
>> > @@ -3188,6 +3209,10 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
>> >
>> > if (cpu->singlestep_enabled) {
>> > data.dbg.control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
>> > +
>> > + if (cpu->singlestep_enabled & SSTEP_NOIRQ) {
>> > + data.dbg.control |= KVM_GUESTDBG_BLOCKIRQ;
>> > + }
>> > }
>> > kvm_arch_update_guest_debug(cpu, &data.dbg);
>> >
>> > diff --git a/gdbstub.c b/gdbstub.c
>> > index 5d8e6ae3cd..48bb803bae 100644
>> > --- a/gdbstub.c
>> > +++ b/gdbstub.c
>> > @@ -368,12 +368,11 @@ typedef struct GDBState {
>> > gdb_syscall_complete_cb current_syscall_cb;
>> > GString *str_buf;
>> > GByteArray *mem_buf;
>> > + int sstep_flags;
>> > + int supported_sstep_flags;
>> > } GDBState;
>> >
>> > -/* By default use no IRQs and no timers while single stepping so as to
>> > - * make single stepping like an ICE HW step.
>> > - */
>> > -static int sstep_flags = SSTEP_ENABLE|SSTEP_NOIRQ|SSTEP_NOTIMER;
>> > +static GDBState gdbserver_state;
>> >
>> > /* Retrieves flags for single step mode. */
>> > static int get_sstep_flags(void)
>> > @@ -385,11 +384,10 @@ static int get_sstep_flags(void)
>> > if (replay_mode != REPLAY_MODE_NONE) {
>> > return SSTEP_ENABLE;
>> > } else {
>> > - return sstep_flags;
>> > + return gdbserver_state.sstep_flags;
>> > }
>> > }
>> >
>> > -static GDBState gdbserver_state;
>> >
>> > static void init_gdbserver_state(void)
>> > {
>> > @@ -399,6 +397,23 @@ static void init_gdbserver_state(void)
>> > gdbserver_state.str_buf = g_string_new(NULL);
>> > gdbserver_state.mem_buf = g_byte_array_sized_new(MAX_PACKET_LENGTH);
>> > gdbserver_state.last_packet = g_byte_array_sized_new(MAX_PACKET_LENGTH + 4);
>> > +
>> > +
>> > + if (kvm_enabled()) {
>> > + gdbserver_state.supported_sstep_flags = kvm_get_supported_sstep_flags();
>> > + } else {
>> > + gdbserver_state.supported_sstep_flags =
>> > + SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
>> > + }
>>
>> This fails to build:
>>
>> o -c ../../gdbstub.c
>> ../../gdbstub.c: In function ‘init_gdbserver_state’:
>> ../../gdbstub.c:403:49: error: implicit declaration of function ‘kvm_get_supported_sstep_flags’ [-Werror=implicit-function-declaration]
>> 403 | gdbserver_state.supported_sstep_flags = kvm_get_supported_sstep_flags();
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../../gdbstub.c:403:49: error: nested extern declaration of ‘kvm_get_supported_sstep_flags’ [-Werror=nested-externs]
>> ../../gdbstub.c: In function ‘gdbserver_start’:
>> ../../gdbstub.c:3531:27: error: implicit declaration of function
>> ‘kvm_supports_guest_debug’; did you mean ‘kvm_update_guest_debug’?
>> [-Werror=implicit-function-declaration]
>> 3531 | if (kvm_enabled() && !kvm_supports_guest_debug()) {
>> | ^~~~~~~~~~~~~~~~~~~~~~~~
>> | kvm_update_guest_debug
>> ../../gdbstub.c:3531:27: error: nested extern declaration of ‘kvm_supports_guest_debug’ [-Werror=nested-externs]
>> cc1: all warnings being treated as errors
>>
>> In fact looking back I can see I mentioned this last time:
>>
>> Subject: Re: [PATCH 2/2] gdbstub: implement NOIRQ support for single step on
>> KVM, when kvm's KVM_GUESTDBG_BLOCKIRQ debug flag is supported.
>> Date: Mon, 19 Apr 2021 17:29:25 +0100
>> In-reply-to: <20210401144152.1031282-3-mlevitsk@xxxxxxxxxx>
>> Message-ID: <871rb69qqk.fsf@xxxxxxxxxx>
>>
>> Please in future could you include a revision number for your spin and
>> mention bellow the --- what changes have been made since the last
>> posting.
>
> You mean it fails to build without KVM? I swear I tested build with TTG only after you mentioned this
> (or as it seems I only tried to).
>
> Could you give me the ./configure parameters you used?
That's with the standard parameters on an x86 host (which will disable
KVM and be TCG only for ARM). I suspect you could get the same results
with --disable-kvm on a native ARM system but I haven't tested that.
>
> Sorry for this!
> Best regards,
> Maxim Levitsky
>
>>
>>
--
Alex Bennée
