From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.gentoo.org (woodpecker.gentoo.org [140.211.166.183]) by sourceware.org (Postfix) with ESMTP id D870A3858C78 for ; Mon, 18 Dec 2023 23:06:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D870A3858C78 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gentoo.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gentoo.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org D870A3858C78 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=140.211.166.183 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702940783; cv=none; b=BNCmtC9cEROawJZvcGMwRT+OyKcCfkYkEv4gu5Po/cMkaZQgzbaNcJ7xBmL8ugBD4HERTZIbbAMB/6sm03tVZWxnC1Sh+a8JZDgKdzMG8y8WtI6KX+LykPYmH6MZSFGEsHt0Nj5IbE+cf1br5bcuOEjW3WSlUopBVkzmmhcfl0o= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702940783; c=relaxed/simple; bh=YDkD+h5Gz474UYe9bZFTVFGqZVB/OP6KgOW/2tjHkKY=; h=Date:From:To:Subject:Message-ID:MIME-Version; b=DY/iiCoaf6zJSSnSXwbk3c8RmZJ+wdGA3lwOsWyVxxuDGt5CMDKpgs4eNmC4l32xY0qC2vXBewk0Ew21Gx7UbdU0sZGAyDzE/9/VaQ2eVKoVRuQEav+nasmTM4Eq80aDFc/ECYWIHSPLhqi/q0yoDlnzVAI4x80UtB4W4dqsLQ0= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by smtp.gentoo.org (Postfix, from userid 559) id 62F6D335D7B; Mon, 18 Dec 2023 23:06:20 +0000 (UTC) Date: Mon, 18 Dec 2023 18:06:19 -0500 From: Mike Frysinger To: Andrew Burgess Cc: jaydeep.patil@imgtec.com, gdb-patches@sourceware.org, joseph.faulls@imgtec.com, bhushan.attarde@imgtec.com Subject: Re: [PATCH v2 1/3] [sim/riscv] Add basic semi-hosting support Message-ID: References: <20231030130042.1472535-1-jaydeep.patil@imgtec.com> <20231030130042.1472535-2-jaydeep.patil@imgtec.com> <87y1dze3m5.fsf@redhat.com> <87sf3z4r4o.fsf@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="J7pmgVR7ZSrobmH0" Content-Disposition: inline In-Reply-To: <87sf3z4r4o.fsf@redhat.com> X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --J7pmgVR7ZSrobmH0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On 18 Dec 2023 12:44, Andrew Burgess wrote: > Mike Frysinger writes: > > On 12 Dec 2023 17:24, Andrew Burgess wrote: > >> Mike Frysinger writes: > >> > On 30 Oct 2023 13:00, jaydeep.patil@imgtec.com wrote: > >> >> Added support for basic semi-hosting calls OPEN, EXIT and GET_CMDLI= NE. > >> > > >> > what host environment are you implementing ? none of the syscalls y= ou'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. > >>=20 > >> 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 af= aict. > > the word "semi" doesn't really appear anywhere in the codebase. >=20 > I was referring to newlib the project rather than newlib the libc > library. Which you figured out once you did a grep ... some ports put their syscalls under newlib/. it was more common in the pas= t, but the point of libgloss was to pull such things out. it's why i was prec= ise when i said "libgloss" and not generically "newlib". new ports should not = be putting things like syscalls into newlib, they should be in libgloss. the exact path is important because we automate importing of syscalls from libgloss & newlib (see sim/common/gennltvals.py), and handcoding the table = in the sim is not what we want at all. > > if you're referring to this commit, it's in libgloss, not newlib. > > https://sourceware.org/git/?p=3Dnewlib-cygwin.git;a=3Dcommit;h=3D865cd3= 0dcc2f00c81c8b3624a9f3464138cd24a5 >=20 > 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). the output of the libgloss/ tree is not standard by any means. while riscv creates a literal "libgloss.a" file, that is not the most common behavior. considering the mess and complete lack of agreement between gcc specs and the libgloss project outputs, better to be precise here. > > where exactly is the riscv semihost standard defined such that people a= re > > implementing the same API (source files) & ABI (the stub that processes= the > > ebreak calls) ? >=20 > This is where things get even more iffy. As best I can tell the RISC-V > semihosting spec is here: >=20 > https://github.com/riscv-software-src/riscv-semihosting/blob/main/riscv= -semihosting-spec.adoc >=20 > But this looks far from complete to me. The commit that added > semihosting to newlib (the project) can be found here: >=20 > https://inbox.sourceware.org/newlib/694d497b-bc07-a3ba-2643-a7336927e9a= 7@embecosm.com/ >=20 > 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. the rsicv-semihosting spec is real rough. ignoring the opening paragraph: > This is a Discussion Document: Assume everything can change. This documen= t is not complete yet and was created only for the purpose of conversation = outside of the document. See http://riscv.org/spec-state for more details. it merely defines part of the calling standard. basically how to trigger t= he semihost. it doesn't cover the actual calling convention (where do extra a= rgs go), the syscall ABI (numbers/arguments/etc...), or how errors are passed b= ack (in the return register ? sep location ? what are the error numbers ?). = or how are non-basic types defined & passed (e.g. is off_t always 32-bit ? how are 64-bit values passed on a 32-bit system ?). padding ? alignment ? if there's disagreements/bugs in implementations, how do we decide which is= the "correct" behavior ? if someone wants to add another syscall, who decides ? the libgloss port seems like "written to work against whatever reference implementation they had available" rather than to/with a spec. the qemu commit also lacks details beyond the bare min spec you linked. it makes it sound like riscv is just reusing the semihost syscall numbers that arm already has ? is it just copying & pasting the entire arm ABI then ? if that's the case, it screams even more "don't put it in riscv/". we have arm & aarch64 ports which could also benefit. to compare, QEMU links to this page for ARM: https://github.com/ARM-software/abi-aa/blob/main/semihosting/semihosting.rst that's an actual spec -- 1500 lines of detailed content. > >> 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 eit= her. > > > > the question is whether we want to support them simultaneously, or only= one > > at a time. what are other stubs doing ? >=20 > 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. if they're both actually developed/used, i'm fine with both. i'm not keen = on supporting the treadmill of "new group wants new feature, designs new thing= in isolation, adds another syscall ABI". we're left holding the bag of dead A= BIs no one cares about. so when someone shows up with an under-specified new t= hing, i'm automatically suspicious. > >> 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 foll= owing > > our existing conventions for integrating with syscalls, but before i we= nt down > > that hole, i wanted to understand at a higher level the diff between th= e two. > > the patch def needs a lot of work either way. >=20 > 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? >=20 > 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. i'm referring to the new semihost code in this thread. the sim_syscall is not perfect by any means, but i'd like to focus on impro= ving the existing core rather than each port & syscall ABI doing it themselves. = we end up with each port working diff depending on the host OS, and having to = copy and paste fixes/improvements between them. i'm a bit guilty of this myself= with the Blackfin port, but in my defense, i wasn't as familiar with the codebas= e. sim_syscall itself is a convenience if you have arguments in registers. if= they are in memory, then you can unpack before calling sim_syscall_multi yoursel= f. other problems at a glance (as i haven't gone too deep yet): * isn't respecting --environment framework (existing code needs fixing too) * hardcoding syscall numbers instead of using our framework to extract * no error handling afaict * casting from target addresses/ints to host uintptr_t looks fundamentally = wrong -mike --J7pmgVR7ZSrobmH0 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEuQK1JxMl+JKsJRrUQWM7n+g39YEFAmWA0GoACgkQQWM7n+g3 9YEnhg//dBMbK7YjRY0nGre0Zm5+/Sj/DoMyYb/17EaCWUoA50KM+x7SKrCmJldB eeFBRSuXg9CnI28bke2AHFXBDSDNKooi+BT43J56CjTZoOVsmbY/Vetv/45L4V/M h0YxSixFKDd8s8hR8386OHuCzZUrtlBQ5plovVw/ejxslQKH7ukm2nJlzMUpyNnP pgpTqh7Krdxj490CwwAnrWOMdV36YCip4tt7UsMLZ1WKRH5FSn3X5N9LvsUwiB9E sboHa6PGkScH3ZQuNYEp2g8DWXHbGLJLoN8P7IuLFe/ssbNqqEu072kLnF3o4IaD G3gqqJ5I8cjCR7C+jC9Xq4kkRmNRYrkZ8RQ7k0ZyI2NDfTfYNIVakirR1nEaED+u AtlYxq9NvL+61lL2B3PJ3p+UvmHOUusQRUhlXS9ulwniQvayuNVv8woOygT+q8E3 gVQiD3rI3FRuQAMv3YHz1jvnvMoIYZfH2Y9G1yHjdXeogkwzMwj0DWZi11ADoijI 12og21rojsqhoYTbcZrszPvjytMS6+e9NXFgMqKZMvTJC4/XL4fGa52iQSZV0yo8 iComwNDPjrpFO0bc00VJsx/5iJDeYHUvHIwKmSaDid+e0TJ+g1k/rhqyUPPbuPd3 xZppmjx92iMfUQyiHUuY9y4v44QFaLjmzHNE1vgxtOtPVpTSeDI= =RJCY -----END PGP SIGNATURE----- --J7pmgVR7ZSrobmH0--