Re: [PATCH v2 1/3] [sim/riscv] Add basic semi-hosting support

[Andrew Burgess <aburgess@redhat.com>]

Mike Frysinger <vapier@gentoo.org> writes:

> On 12 Dec 2023 17:24, Andrew Burgess wrote:
>> Mike Frysinger <vapier@gentoo.org> writes:
>> > On 30 Oct 2023 13:00, jaydeep.patil@imgtec.com wrote:
>> >> Added support for basic semi-hosting calls OPEN, EXIT and GET_CMDLINE.
>> >
>> > what host environment are you implementing ?  none of the syscalls you've
>> > defined match what have long been in the riscv libgloss port.  those are
>> > the only ones i'd really expect at this point to be wired up.
>> 
>> This would be the RISC-V semihosting target, which is included in
>> newlib (since 2020), but is separate to libgloss.
>
> included where exactly ?  newlib/libc/machine/riscv/ has no syscalls afaict.
> the word "semi" doesn't really appear anywhere in the codebase.

I was referring to newlib the project rather than newlib the libc
library.  Which you figured out once you did a grep ...

> if you're referring to this commit, it's in libgloss, not newlib.
> https://sourceware.org/git/?p=newlib-cygwin.git;a=commit;h=865cd30dcc2f00c81c8b3624a9f3464138cd24a5

Yeah, it's a bit "yuck".  The commit lives in the libgloss directory,
but actually adds a parallel set of build rules that create a
libsemihost.a as a separate thing from libgloss.a, hence my "separate to
libgloss" comment.  Which I'll argue is both correct and incorrect at
the same time (correct: it's a separate library, incorrect: it's within
the libgloss part of the newlib project tree).

>
> looking at libgloss/riscv/machine/syscall.h, i see it defines the two sets
> in parallel -- SYS_xxx and SEMIHOST_xxx.
>
> the question is why does libgloss have both ?  if the riscv libgloss SYS_xxx
> are only used by libgloss, and no one is implementing that (the sim hasn't),
> and SEMIHOST_xxx are used by everyone, why not only use the semihost interface
> and drop the SYS_xxx (and rename SEMIHOST_xxx to SYS_xxx).

I don't know why they are both defined at the same time.  I guess that
could be changed.

I also don't know who implements either the semihosting syscalls, or the
libgloss sys_* syscalls.

>
> where exactly is the riscv semihost standard defined such that people are
> implementing the same API (source files) & ABI (the stub that processes the
> ebreak calls) ?

This is where things get even more iffy.  As best I can tell the RISC-V
semihosting spec is here:

  https://github.com/riscv-software-src/riscv-semihosting/blob/main/riscv-semihosting-spec.adoc

But this looks far from complete to me.  The commit that added
semihosting to newlib (the project) can be found here:

  https://inbox.sourceware.org/newlib/694d497b-bc07-a3ba-2643-a7336927e9a7@embecosm.com/

Comments in that thread seem to indicate that the situation is more of a
ad hoc standard, with (maybe) openocd being a first reference
implementation?  However, it is supported by QEMU with commit
a10b9d93ecea0a8 (though I think there are additional follow up commits
relating to this topic) so there are definitely other simulators
supporting this out there.

>
>> Where libgloss syscalls use ecall, the semihosting uses ebreak with two
>> specific nop instructions (one before, one after the ebreak).
>
> the calling convention doesn't really matter to the sim.  it can do either.
>
> the question is whether we want to support them simultaneously, or only one
> at a time.  what are other stubs doing ?

I haven't looked.  I'll leave that as an exercise for Jaydeep.  I'd be
open to supporting both simultaneously as a first cut ... but I guess if
you feel strongly then I'm not against making it a requirement for this
to be switchable... it feels like that should be simple enough.

>
>> Do you see any reason why we can't support both of these syscall
>> libraries?  Potentially we could have a switch to select between them,
>> but I'm inclined to say we should just support both until someone comes
>> with a use-case where supporting both is a bad idea...  But maybe you
>> have some deeper insights here.
>
> supporting them both isn't a problem.  the port, as written, isn't following
> our existing conventions for integrating with syscalls, but before i went down
> that hole, i wanted to understand at a higher level the diff between the two.
> the patch def needs a lot of work either way.

For my own education; the ECALL handling does make use of sim_syscall.
When you say: "the port, as written, isn't following our existing
conventions for integrating with syscalls", do you mean the new
semihosting port?  Or are you saying the (pre-patch) existing code is
also not using the standard sim syscall mechanism?

The problem I see (at a quick glance) with the semihosting API is that
the syscall arguments are read from memory, while the existing syscall
mechanism seems to assume syscall arguments are in registers.  My
initial thoughts were that getting the semihosting support to use the
existing mechanism might not be straight forward.

Thanks,
Andrew