Re: [Qemu-devel] [PATCH v2 03/28] semihosting: implement a semihosting console

[Alex Bennée]

Peter Maydell <address@hidden> writes:

> On Fri, 24 May 2019 at 11:46, Alex Bennée <address@hidden> wrote:
>>
>>
>> Peter Maydell <address@hidden> writes:
>>
>> > On Thu, 23 May 2019 at 11:39, Alex Bennée <address@hidden> wrote:
>> > I'm not sure about the "no callback" part of this API. The operation
>> > here is genuinely asynchronous and providing no mechanism for the
>> > caller to be able to say "ok, now wait til it completes" doesn't
>> > seem like a good plan.
>>
>> Well the caller doesn't really get a choice. At least in system mode
>> gdbstub does a vm_stop(RUN_STATE_DEBUG) and brings everything to a halt
>> anyway. All we've removed is the ability to tack on a callback (which
>> can get run in all sorts of contexts) when we restart.
>
> gdb_do_syscall() is asynchronous -- it arranges for the syscall
> to happen, but makes no guarantee about when it will finish.
> At the moment the gdb_do_syscall() API allows the caller
> to cope with this asynchronousness, because when the callback
> is called the syscall has definitely finished. As it happens
> the callers are buggy in that they don't actually do the
> sync that they need to do, but we could fix that bug (ie post
> a semaphore in the callback function, and wait on it after
> the gdb_do_syscall() call). The API for this new function
> doesn't allow us to do that.

So for the ARM semi side the console writes are treated as always
successful so the return value is correct (it doesn't matter for writec)
and no syncing to the guest is required. However I ran into a problem
because in gdbstub we have:

    /* Is there a GDB syscall waiting to be sent?  */
    if (s->current_syscall_cb) {
        put_packet(s, s->syscall_buf);
        return;
    }

which means it can't accept NULL for the callback. So I've removed the
gdb_console_out and done:

  static void semihosting_cb(CPUState *cs, target_ulong ret, target_ulong err)
  {
      if (ret == (target_ulong) -1) {
          qemu_log("%s: gdb console output failed (%s)", __func__, 
strerror(-err));
      }
  }

  int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int 
len)
  {
      GString *s = copy_user_string(env, addr, len);
      int out = s->len;

      if (use_gdb_syscalls()) {
          gdb_do_syscall(semihosting_cb, "write,2,%x,%x", addr, s->len);
      } else {
          out = qemu_semihosting_log_out(s->str, s->len);
      }

      g_string_free(s, true);
      return out;
  }

for now.

>> I could just drop the API here and allow the semihosting API to call
>> gdb_do_syscallv directly?
>
> I think we should either make the implementation of the function
> properly synchronous (ie do the post-semaphore-in-callback,
> wait-on-it-after-gdb_do_syscallv thing in the implementation,
> or have an API that lets callers do it.
>
> Perhaps we should just make gdb_do_syscall really be
> synchronous (ie have it do the semaphore stuff)? We
> only actually use it in semihosting, and I think all
> those cases require that the operation completes before
> we can resume execution of the guest CPU. So doing the
> synchronization in one place in the gdb code would be
> simpler than doing it separately in all the callers...

Maybe.. this seems like a bigger clean-up to do that properly. Maybe
that would be worth tackling after the gdbstub refactor stuff goes in?

--
Alex Bennée