Re: [Qemu-devel] [PATCH v5 2/2] Add Nios II semihosting support.

[Peter Maydell]

On Wed, 13 Feb 2019 at 04:02, Sandra Loosemore <address@hidden> wrote:
>
> This patch adds support for libgloss semihosting to Nios II bare-metal
> emulation.
>
> Signed-off-by: Sandra Loosemore <address@hidden>
> Signed-off-by: Julian Brown <address@hidden>

Hi; here are some more detailed code review comments based
on your spec document. They're all fairly minor; otherwise
the code looks good.

> @@ -169,6 +170,16 @@ void nios2_cpu_do_interrupt(CPUState *cs)
>          break;
>
>      case EXCP_BREAK:
> +        qemu_log_mask(CPU_LOG_INT, "BREAK exception at pc=%x\n",
> +                      env->regs[R_PC]);
> +
> +        if (semihosting_enabled()) {
> +            qemu_log_mask(CPU_LOG_INT, "Entering semihosting\n");
> +            env->regs[R_PC] += 4;
> +            do_nios2_semihosting(env);
> +            break;
> +        }

The spec says that "break 1" instructions are semihosting.
This looks like it's treating any kind of break instruction
as semihosting. Is the check for "only 1" being done elsewhere,
or should it happen here ?

> +static void translate_stat(CPUNios2State *env, target_ulong addr,
> +                           struct stat *s)
> +{
> +    struct nios2_gdb_stat *p;
> +
> +    p = lock_user(VERIFY_WRITE, addr, sizeof(struct nios2_gdb_stat), 0);
> +
> +    if (!p) {
> +        /* FIXME - should this return an error code? */

Should it ? :-) I would suggest yes. (Alternatively if
the semihosting spec for this architecture specifically
forbids it we should comment that here as we do for the
"no way to report errors" cases in nios2_semi_return_u32 and _u64,
and drop the fixme.)

> +        return;
> +    }

> +/* Read the input value from the argument block; fail the semihosting
> + * call if the memory read fails.
> + */
> +#define GET_ARG(n) do {                                 \
> +    if (get_user_ual(arg ## n, args + (n) * 4)) {       \
> +        result = -1;                                    \
> +        errno = EFAULT;                                 \
> +        goto failed;                                    \
> +    }                                                   \
> +} while (0)
> +
> +void do_nios2_semihosting(CPUNios2State *env)
> +{
> +    int nr;
> +    uint32_t args;
> +    target_ulong arg0, arg1, arg2, arg3;
> +    void *p;
> +    void *q;
> +    uint32_t len;
> +    uint32_t result;
> +
> +    nr = env->regs[R_ARG0];
> +    args = env->regs[R_ARG1];
> +    switch (nr) {
> +    case HOSTED_EXIT:
> +        gdb_exit(env, env->regs[R_ARG0]);
> +        exit(env->regs[R_ARG0]);
> +    case HOSTED_OPEN:
> +        GET_ARG(0);
> +        GET_ARG(1);
> +        GET_ARG(2);
> +        GET_ARG(3);
> +        if (use_gdb_syscalls()) {
> +            gdb_do_syscall(nios2_semi_cb, "open,%s,%x,%x", arg0, (int)arg1,
> +                           arg2, arg3);
> +            return;
> +        } else {
> +            p = lock_user_string(arg0);
> +            if (!p) {
> +                /* FIXME - check error code? */
> +                result = -1;

Yes, we should set errno here, right? (and also in the other
FIXME comments below).

> +            } else {
> +                result = open(p, translate_openflags(arg2), arg3);
> +                unlock_user(p, arg0, 0);
> +            }
> +        }
> +        break;
> +    case HOSTED_LSEEK:
> +        {
> +            uint64_t off;
> +            GET_ARG(0);
> +            GET_ARG(1);
> +            GET_ARG(2);
> +            GET_ARG(3);
> +            off = (uint32_t)arg2 | ((uint64_t)arg1 << 32);
> +            if (use_gdb_syscalls()) {
> +                nios2_semi_is_fseek = 1;

Shouldn't this flag be called '...is_lseek' ?

> +                gdb_do_syscall(nios2_semi_cb, "lseek,%x,%lx,%x",
> +                               arg0, off, arg3);
> +            } else {
> +                off = lseek(arg0, off, arg3);
> +                nios2_semi_return_u64(env, off, errno);
> +            }
> +            return;
> +        }

> +    default:
> +        cpu_abort(CPU(nios2_env_get_cpu(env)),
> +                  "Unsupported semihosting syscall %d\n", nr);
> +        result = 0;

We should never cpu_abort() for things the guest can provoke:
we should just log this as LOG_GUEST_ERROR and continue.

> +    }
> +failed:
> +    nios2_semi_return_u32(env, result, errno);

thanks
-- PMM