Re: [PATCH v2 00/15] target/arm: Implement semihosting v2.0

[Peter Maydell]

Ping for code review, please?

thanks
-- PMM

On Mon, 16 Sep 2019 at 15:15, Peter Maydell <address@hidden> wrote:
>
> This patchset implements support in QEMU for v2.0 of the
> Arm semihosting specification:
>  https://developer.arm.com/docs/100863/latest/preface
>
> Specifically, v2.0 has:
>  * a mechanism for detection of optional extra features,
>    which works by allowing the guest to open a magic file
>    named ":semihosting-features" and read some feature
>    flags from it
>  * two defined extensions:
>   - STDOUT_STDERR lets the guest separately open stdout and
>     stderr via the ":tt" magic filename (v1.0 only allowed
>     access to stdout)
>   - EXIT_EXTENDED lets A32/T32 guests exit with a specified
>     exit status (otherwise only available to A64 guests).
>     This is something that people have been complaining
>     about for a long time.
>
> (Technically some of the things we already support, like
> having an A64 semihosting interface at all, are also part of
> the v2.0 spec.)
>
> This patchset:
>  * fixes some bugs relating to errnos in some cases
>  * makes semihosting hand out its own filedescriptors rather
>    than just passing out host fd numbers
>  * abstracts out the fd-related semihosting calls so they
>    indirect via a function table based on the type of the fd
>  * adds a new type of fd representing the magic file
>    ":semihosting-features" which is used for feature-detection
>  * implements both of the extensions defined by the v2.0 spec
>
> I've tested this by improving my semihosting test suite:
>  https://git.linaro.org/people/peter.maydell/semihosting-tests.git/
> (if people have other guest binaries that make much use of
> semihosting then testing would certainly be welcome.)
>
> Changes v1->v2:
>  * Added a patch which corrects misunderstanding in a FIXME
>    comment about the when the callback function is called
>    for arm_gdb_syscall()
>  * in patch 4, if the SYS_open is going via the gdbstub, we
>    must do the associate_guestfd() work in the gdbstub callback
>    function. This is because in softmmu mode the callback will
>    not be called until after do_arm_semihosting() returns.
>    (The v1 series effectively broke SYS_open in the gdbstub
>    + softmmu config)
>  * Pass CPUARMState* to set_swi_errno(), rather than creating
>    an odd local-to-this-file typedef of TaskState for the
>    softmmu compilation
>  * New patch: avoid ifdeffery in gdb callback fns by
>    using set_swi_errno() rather than doing it by-hand
>  * The various 'factor out SYS_foo' patches are basically
>    unchanged, but all the functions no longer need to take
>    a TaskState*. This seemed kind of borderline as to whether
>    to retain Alex's reviewed-by tags, so I dropped them.
>  * Since we need 'env' for set_swi_errno(), we don't need
>    to put the variable declaration inside ifdefs any more
>    in the host_readfn() etc.
>
> I do plan to have a go at fixing the odd FIXME surrounding
> arm_gdb_syscall() which patch 3 clarifies/states in a comment.
> But I thought it better to not tangle that up with this
> patchset, which is already pretty long.
>
> thanks
> -- PMM
>
>
> Peter Maydell (15):
>   target/arm/arm-semi: Capture errno in softmmu version of
>     set_swi_errno()
>   target/arm/arm-semi: Always set some kind of errno for failed calls
>   target/arm/arm-semi: Correct comment about gdb syscall races
>   target/arm/arm-semi: Make semihosting code hand out its own file
>     descriptors
>   target/arm/arm-semi: Restrict use of TaskState*
>   target/arm/arm-semi: Use set_swi_errno() in gdbstub callback functions
>   target/arm/arm-semi: Factor out implementation of SYS_CLOSE
>   target/arm/arm-semi: Factor out implementation of SYS_WRITE
>   target/arm/arm-semi: Factor out implementation of SYS_READ
>   target/arm/arm-semi: Factor out implementation of SYS_ISTTY
>   target/arm/arm-semi: Factor out implementation of SYS_SEEK
>   target/arm/arm-semi: Factor out implementation of SYS_FLEN
>   target/arm/arm-semi: Implement support for semihosting feature
>     detection
>   target/arm/arm-semi: Implement SH_EXT_EXIT_EXTENDED extension
>   target/arm/arm-semi: Implement SH_EXT_STDOUT_STDERR extension
>
>  target/arm/arm-semi.c | 707 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 577 insertions(+), 130 deletions(-)
>
> --
> 2.20.1