[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] riscv: Add semihosting support [v4]
From: |
Peter Maydell |
Subject: |
Re: [PATCH] riscv: Add semihosting support [v4] |
Date: |
Wed, 29 Jan 2020 15:58:08 +0000 |
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