Re: [PATCH] riscv: Add semihosting support [v4]

[Peter Maydell]

On Tue, 28 Jan 2020 at 23:34, Keith Packard via <address@hidden> wrote:
>
> Adapt the arm semihosting support code for RISCV. This implementation
> is based on the standard for RISC-V semihosting as documented in
>
>         https://riscv.org/specifications/
>
> Signed-off-by: Keith Packard <address@hidden>
>
> ---

> + *  ARM Semihosting is documented in:
> + *     Semihosting for AArch32 and AArch64 Release 2.0
> + *     https://static.docs.arm.com/100863/0200/semihosting.pdf

True but irrelevant. You need to refer to a proper
risc-v specification for your semihosting.

> +    case TARGET_SYS_EXIT:
> +    case TARGET_SYS_EXIT_EXTENDED:
> +        if (nr == TARGET_SYS_EXIT_EXTENDED || sizeof(target_ulong) == 0) {
> +            /*
> +             * The A64 version of SYS_EXIT takes a parameter block,
> +             * so the application-exit type can return a subcode which
> +             * is the exit status code from the application.
> +             * SYS_EXIT_EXTENDED is an a new-in-v2.0 optional function
> +             * which allows A32/T32 guests to also provide a status code.
> +             */

This code and comment describe Arm semihosting, where we
made this bad decision about the API for 32-bit Arm, fixed
it for 64-bit Arm and then put in an extension to add the more
sensible API to 32-bit as a backwards-compatible new function.
Nobody else should make this API choice from the start.
What does RISC-V want to do here? This should be in
your specification.

> +            GET_ARG(0);
> +            GET_ARG(1);
> +
> +            if (arg0 == ADP_Stopped_ApplicationExit) {
> +                ret = arg1;
> +            } else {
> +                ret = 1;
> +            }
> +        } else {
> +            /*
> +             * The A32/T32 version of SYS_EXIT specifies only
> +             * Stopped_ApplicationExit as normal exit, but does not
> +             * allow the guest to specify the exit status code.
> +             * Everything else is considered an error.
> +             */
> +            ret = (args == ADP_Stopped_ApplicationExit) ? 0 : 1;
> +        }
> +        gdb_exit(env, ret);
> +        exit(ret);
> +    case TARGET_SYS_SYNCCACHE:
> +        /*
> +         * Clean the D-cache and invalidate the I-cache for the specified
> +         * virtual address range. This is a nop for us since we don't
> +         * implement caches. This is only present on A64.
> +         */
> +        if (sizeof(target_ulong) == 8) {
> +            return 0;
> +        }
> +        /* fall through -- invalid for A32/T32 */

Again, this is an Arm-ism, where the old A32 ABI
doesn't have this function but the new A64 one does. What
does RISC-V want to specify here?

> +    default:
> +        fprintf(stderr, "qemu: Unsupported SemiHosting SWI 0x%02x\n", nr);
> +        cpu_dump_state(cs, stderr, 0);
> +        abort();

This is repeating the Arm ABI mistake of allowing implementations
to just crash and burn if they're handed a function call they don't
recognize. Ideally it should be avoided in a new ABI.

> +    }
> +}

thanks
-- PMM