Re: [RFC PATCH 1/3] target/openrisc: Add basic support for semihosting

[Richard Henderson]

On 5/27/22 10:27, Stafford Horne wrote:
> +void do_or1k_semihosting(CPUOpenRISCState *env, uint32_t k);
...
> +DEF_HELPER_FLAGS_2(nop, 0, void, env, i32)

Just call the helper "semihosting" and be done with it.
And the helper wants an ifdef for system mode.

> @@ -10,6 +10,7 @@ openrisc_ss.add(files(
>     'fpu_helper.c',
>     'gdbstub.c',
>     'interrupt_helper.c',
> +  'openrisc-semi.c',
>     'sys_helper.c',
>     'translate.c',
>   ))

You want to add the new file for system mode only.
Or, now that I think of it, conditional on CONFIG_SEMIHOSTING itself.

> +static void or1k_semi_return_u32(CPUOpenRISCState *env, uint32_t ret)
> +{
> +    cpu_set_gpr(env, 11, ret);
> +}

Let's drop this until you actually use it.  This appears to be attempting to mirror other, 
more complete semihosting, but missing the third "error" argument.


> +void do_or1k_semihosting(CPUOpenRISCState *env, uint32_t k)
> +{
> +    uint32_t result;
> +
> +    switch (k) {
> +    case HOSTED_EXIT:
> +        gdb_exit(cpu_get_gpr(env, 3));
> +        exit(cpu_get_gpr(env, 3));
> +    case HOSTED_RESET:
> +#ifndef CONFIG_USER_ONLY
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        return;
> +#endif

Do you in fact want to exit to the main loop after asking for reset?
That's the only way that "no return value" makes sense to me...

> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "or1k-semihosting: unsupported "
> +                      "semihosting syscall %d\n", k);

%u.

>   static bool trans_l_nop(DisasContext *dc, arg_l_nop *a)
>   {
> +    if (semihosting_enabled() &&
> +        a->k != 0) {
> +        gen_helper_nop(cpu_env, tcg_constant_i32(a->k));
> +    }

Perhaps cleaner to move the semihosting dispatch switch here, instead of leaving it to the 
runtime?  The reason we have a runtime switch for other guests is that the semihosting 
syscall number is in a register.  This would then be

    if (semihosting_enabled()) {
        switch (a->k) {
        case 0:
            break; /* normal nop */
        case HOSTED_EXIT:
            gen_helper_semihost_exit(cpu_R(dc, 3));
            break;
        case HOSTED_RESET:
            gen_helper_semihost_reset();
            tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);

            dc->base.is_jmp = DISAS_EXIT;
            break;
        ...
        }
    }


r~